Fix inconsistent NuGet restore generation #5172

sharwell · 2020-03-31T21:50:57Z

This change corrects cases where command line restore operations using
the arcade tools failed to produce the same NuGet restore result that
Visual Studio produces for the same project. This failure led to
unnecessary regeneration of these files each time the IDE was opened or
a command line build was performed, thus leading to reduced productivity
and, in some cases, reduced reliability.

sharwell · 2020-03-31T21:52:05Z

src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj

    <MSBuild Projects="@(_ProjectToRestore)"
             Properties="@(_SolutionBuildProps);__BuildPhase=SolutionRestore;_NETCORE_ENGINEERING_TELEMETRY=Restore;MSBuildRestoreSessionId=$([System.Guid]::NewGuid())"
-             RemoveProperties="$(_RemoveProps)"
+             RemoveProperties="$(_RemoveProps);TreatWarningsAsErrors"


📝 The TreatWarningsAsErrors property gets put in the dgspec file if it's defined for the restore target. This must not occur on developer machines, because it's not the same behavior that restore operations inside Visual Studio produce.

The way you proposed the change won't it affect every kind of build?

Seems like a bug in NuGet. Is there an issue tracking it?

The way you proposed the change won't it affect every kind of build?

Only the /t:Restore operation ignores warnings as errors. The flag will still get passed everywhere else.

Seems like a bug in NuGet. Is there an issue tracking it?

I have no idea if this a bug or by-design in NuGet.

@nkolev92 Any idea?

@nkolev92 The problem occurs for projects with this configuration:

<PropertyGroup Condition="'$(BuildingInsideVisualStudio)' != 'true'"> <TreatWarningsAsErrors>true</TreatWarningsAsErrors> </PropertyGroup>

This is a de-facto standard approach to warnings in managed code.

I see.
I'd say this is by design then.
NuGet has coded warnings and respects things like WarningsAsErrors and TreatWarningsAsErrors.

This is a de-facto standard approach to warnings in managed code.

Wasn't familiar with this. A bunch of the dotnet/* projects have warnings as errors on by default, maybe NuGet's handling is one reason why :)

In our sln of 80 projects, restore would go from ~2s, to 9s if we did this.

Will this change end up being disruptive? Any insight into how many builds will suddenly fall over when warnings are now treated as errors?

This is going the other way around. We are forcing TreatWarningsAsErrors to false for /t:Restore.

Ooops, sorry. :)

I'm cool with @JohnTortugo is cool.

markwilkie · 2020-03-31T23:01:46Z

cc/ @JohnTortugo

tmat · 2020-03-31T23:56:10Z

eng/common/tools.ps1

-      $env:NUGET_PACKAGES = Join-Path $env:UserProfile '.nuget\packages'
+      $env:NUGET_PACKAGES = Join-Path $env:UserProfile '.nuget\packages\'
    } else {
      $env:NUGET_PACKAGES = Join-Path $RepoRoot '.packages'


This line should be updated as well

As should be the equivalent code in tools.sh

These lines don't run on the machine with Visual Studio, so it wouldn't cause the problem that this is fixing. If there is a separate issue here, can you be more specific about what the change needs to be?

I understand that you're fixing VS experience, but we should be consistent about whether paths are expected to end with a separator or not.

What is the specific change to make here? And is this going to be \ or /?

I changed it for now; hopefully it's correct.

tmat · 2020-03-31T23:57:44Z

eng/common/tools.ps1

    # Use local cache on CI to ensure deterministic build,
    # use global cache in dev builds to avoid cost of downloading packages.
    if ($useGlobalNuGetCache) {
-      $env:NUGET_PACKAGES = Join-Path $env:UserProfile '.nuget\packages'


Is this also a NuGet bug? Shouldn't it normalize the paths that it's writing into files?

Both ways work. If the separator isn't here, then it gets added to the generated files. The problem is even though the semantics are the same, the change itself produces different hashes and the regeneration occurs.

Right, but if NuGet always normalized the path separator we wouldn't need to make these changes (and most importantly, maintain them). It is entirely non-obvious that not adding the terminating separator makes a difference. On that note, let's add a comment ...

/cc @nkolev92

We're tracking this in NuGet/Home#7968.
I'll look into it this sprint.

I assume any project would break?

I think so (not really break, but not reuse the restore data). @sharwell could you add a comment to the code with a link to NuGet/Home#7968?

I think so (not really break, but not reuse the restore data)

yeah, I guess that's what I meant by break.
Defeat the incremental check when it shouldn't :)

Added a link.

This change corrects cases where command line restore operations using the arcade tools failed to produce the same NuGet restore result that Visual Studio produces for the same project. This failure led to unnecessary regeneration of these files each time the IDE was opened or a command line build was performed, thus leading to reduced productivity and, in some cases, reduced reliability.

sharwell · 2020-04-04T01:05:09Z

What's the status of this? It's a significant workflow disruption for me in Roslyn.

tmat · 2020-04-04T01:47:55Z

@markwilkie This will also need to be ported to 3.x.

sharwell · 2020-04-05T04:28:45Z

@tmat what's the review and merge policy for this repository?

tmat · 2020-04-06T01:48:01Z

@JohnTortugo OK to merge?

tmat · 2020-04-06T22:37:53Z

@markwilkie I believe we will need to port this to 3.x.

markwilkie · 2020-04-06T22:41:26Z

Should be fine. @sharwell - could you fill out the template please?

Also, if we have the time, it'd be great to let this "bake" before porting.

sharwell · 2020-04-06T22:43:42Z

I'm not sure what it means to port this or what the template is. Can you provide more information about how this works?

markwilkie · 2020-04-06T22:44:52Z

Sure, just click the link above (template) and it should be more clear.

sharwell · 2020-04-06T23:43:39Z

Description

This change updates the -restore behavior for command line builds in arcade projects to produce the same (binary equal) NuGet dependency outputs that a restore within Visual Studio has. When this occurs, a restore in one tool will cause the other tool to fast-path the restore process (no changes) for improved overall developer efficiency.

Customer Impact

This change improves developer efficiency on projects using Arcade by reducing the number of significant NuGet restore operations that occur during normal workflows.

Regression

No

Risk

This is a low risk change. The primary risk centers around an unknown question of whether an outer MSBuild with warnings treated as errors will elevate a warning produced by an inner build that did not have the flag set. If it does not work as expected, it is possible that what used to be a build error will now be a build warning.

Workarounds

The only current workaround is restricting NuGet to one tool. This means users must choose to work either inside Visual Studio or on the command line. Mixing the two could cause Visual Studio to crash when it tries to apply large changes to the NuGet graph.

sharwell commented Mar 31, 2020

View reviewed changes

tmat reviewed Mar 31, 2020

View reviewed changes

nkolev92 mentioned this pull request Apr 1, 2020

NuGet should normalize the trailing slashes for the global packages folder, fallback folder and the like NuGet/Home#7968

Open

sharwell force-pushed the consistent-generation branch from 82a5e68 to c5eb5d8 Compare April 2, 2020 00:55

tmat approved these changes Apr 4, 2020

View reviewed changes

JohnTortugo approved these changes Apr 6, 2020

View reviewed changes

tmat merged commit 6c4dea1 into dotnet:master Apr 6, 2020

sharwell deleted the consistent-generation branch April 6, 2020 22:37

jmarolf mentioned this pull request May 7, 2020

always restore project in build dotnet/roslyn#44034

Closed

This was referenced May 22, 2020

Disable NuGet restore within Visual Studio dotnet/roslyn#44523

Closed

Restore.cmd does not produce the same outputs as NuGet Restore in Visual Studio #5550

Closed

ViktorHofer mentioned this pull request Nov 16, 2020

Network issues hit during build restore step are silently ignored dotnet/runtime#44742

Closed

tmds mentioned this pull request Jun 2, 2025

Build.proj restore filters out TreatWarningsAsErrors #15872

Closed

Fix inconsistent NuGet restore generation #5172

Fix inconsistent NuGet restore generation #5172

Uh oh!

Conversation

sharwell commented Mar 31, 2020

Uh oh!

sharwell Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markwilkie commented Mar 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkolev92 Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Apr 4, 2020

Uh oh!

tmat commented Apr 4, 2020

Uh oh!

sharwell commented Apr 5, 2020

Uh oh!

tmat commented Apr 6, 2020

Uh oh!

tmat commented Apr 6, 2020

Uh oh!

markwilkie commented Apr 6, 2020

Uh oh!

sharwell commented Apr 6, 2020

Uh oh!

markwilkie commented Apr 6, 2020

Uh oh!

sharwell commented Apr 6, 2020

Description

Customer Impact

Regression

Risk

Workarounds

sharwell Mar 31, 2020 •

edited

Loading

nkolev92 Apr 1, 2020 •

edited

Loading