Skip to content

Fix inconsistent NuGet restore generation #5172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 6, 2020
Merged

Fix inconsistent NuGet restore generation #5172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions eng/common/tools.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,10 +488,11 @@ function GetNuGetPackageCachePath() {
if ($env:NUGET_PACKAGES -eq $null) {
# Use local cache on CI to ensure deterministic build,
# use global cache in dev builds to avoid cost of downloading packages.
# For directory normalization, see also: https://github.com/NuGet/Home/issues/7968
if ($useGlobalNuGetCache) {
$env:NUGET_PACKAGES = Join-Path $env:UserProfile '.nuget\packages'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@nkolev92 nkolev92 Apr 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I assume any project would break?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a link.

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

Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@

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

@sharwell sharwell Mar 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nkolev92 Any idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, sorry. :)

I'm cool with @JohnTortugo is cool.

Targets="Restore"
SkipNonexistentTargets="true"
BuildInParallel="%(_ProjectToRestore.RestoreInParallel)"
Expand Down