dotnet / arcade

Tools that provide common build infrastructure for multiple .NET Foundation projects.
MIT License
664 stars 339 forks source link

project.restore.configFilePaths differs between Visual Studio and command line #2352

Open sharwell opened 5 years ago

sharwell commented 5 years ago

The following appears when running restore on the command line:

  "project": {
    // ...
    "restore": {
      // ...
      "configFilePaths": [
        "C:\\dev\\roslyn\\NuGet.config"
      ],
// ...

The following appears when running restore within Visual Studio 2019 RC 3:

  "project": {
    // ...
    "restore": {
      // ...
      "configFilePaths": [
        "C:\\dev\\roslyn\\NuGet.Config",
        "C:\\Users\\sam\\AppData\\Roaming\\NuGet\\NuGet.Config",
        "C:\\Program Files (x86)\\NuGet\\Config\\Microsoft.VisualStudio.Offline.config"
      ],
// ...
markwilkie commented 5 years ago

Isn't this a VS feature? https://docs.microsoft.com/en-us/nuget/consume-packages/package-restore

sharwell commented 5 years ago

@markwilkie this appears to be an arcade configuration which is being applied in only one of two contexts where it needs to be applied:

https://github.com/dotnet/arcade/blob/002cce7e8e3e043c50acae673741ee3962411e10/src/Microsoft.DotNet.Arcade.Sdk/tools/DefaultVersions.props#L20-L29

The "note" in this text is actually a bug.

tmat commented 5 years ago

AFAIK there is no way how to force NuGet to only use the specified NuGet.config file for design time build :(. I wonder why the paths need to be stored in assets file. @nkolev92

nkolev92 commented 5 years ago

@tmat They need to be stored in the assets file for no-op purposes. A config change dirties the assets file, as the config data can affect the resolution/policies.

tmat commented 5 years ago

I see. Then we need to be able to specify that only the config file in RestoreConfigFile property should be considered during design time build restore. Can we do that?

tmat commented 5 years ago

Also, including additional config files seems generally wrong - it makes the build non-deterministic, depending on what the user who builds has configured in the other config files. We want the build to work the same for all developers who clone the repository.

nkolev92 commented 5 years ago

I see. Then we need to be able to specify that only the config file in RestoreConfigFile property should be considered during design time build restore. Can we do that?

We could consider that. We already pretty much everything else.
Related: https://github.com/NuGet/Home/issues/7533 https://github.com/NuGet/Home/wiki/%5BSpec%5D-NuGet-settings-in-MSBuild https://github.com/NuGet/Home/issues/6746

The problem with all these approaches is that it's difficult to provide a UI. The project level config story is very long and confusing. //cc @anangaur

Also, including additional config files seems generally wrong - it makes the build non-deterministic, depending on what the user who builds has configured in the other config files. We want the build to work the same for all developers who clone the repository.

Unfortunately the stacking of configs has been a staple for a very long time. The issues with the stacking are kind of addressed by the clear tag in the individual sections, but that won't fix the above problem.

We could consider adding some flags that would allow to block the merging of configs beyond a certain point. Created an issue here: https://github.com/NuGet/Home/issues/7944

tmat commented 5 years ago

@nkolev92 Thanks!

markwilkie commented 5 years ago

Thanks again @nkolev92. Closing this issue in favor of NuGet/Home#7944

rrelyea commented 5 years ago

I'd recommend that you consider doing the following. Put nuget.config in the root of your repo (or other similar place). At the topmost one, use <clear /> in packageSources, disabledPackageSources, etc...

This gives you the ability to: 1) work in VS 2) work in command line 3) not have to configure a pointer to one config. 4) avoid having machine wide state affect your repos.

This works now, and has worked for a long time.

Would that work well in this case?

tmat commented 5 years ago

I guess we could remove setting the RestoreConfigFile and instead add a target that validates that the NuGet.config file in the repository root includes all the <clear/> elements it should to make the restore deterministic.

markwilkie commented 5 years ago

Added to post preview 7