Add -CentralizedRestore to avoid NuGet parallel restore race conditions #65069
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,73 @@ | ||||||
| <Project> | ||||||
| <!-- | ||||||
| Centralized restore project that restores ALL projects in a single MSBuild invocation. | ||||||
| This avoids the NuGet race condition (https://github.com/NuGet/Home/issues/7648) where | ||||||
| parallel restore of the same project from multiple callers causes "Cannot create a file | ||||||
| when that file already exists" errors. | ||||||
|
|
||||||
| Usage in CI: | ||||||
| ./eng/build.cmd -ci -CentralizedRestore ... | ||||||
|
|
||||||
| Or manually: | ||||||
| dotnet restore AspNetCore.slnx | ||||||
| dotnet msbuild eng/CentralizedRestore.proj /t:RestoreDelayedOnly /p:Configuration=Release | ||||||
| ./eng/build.cmd -ci -NoRestore /p:RestoreForTestAssets=false /p:RestoreForDelayedProjects=false ... | ||||||
| --> | ||||||
|
|
||||||
| <Import Project="RequiresDelayedBuildProjects.props" /> | ||||||
|
|
||||||
| <PropertyGroup> | ||||||
| <RepoRoot Condition="'$(RepoRoot)' == ''">$([MSBuild]::NormalizeDirectory('$(MSBuildThisFileDirectory)', '..'))</RepoRoot> | ||||||
| </PropertyGroup> | ||||||
|
|
||||||
| <!-- | ||||||
| Restore only delayed build projects and test assets (projects not in the main solution). | ||||||
| Called after `dotnet restore AspNetCore.slnx` to complete the restore. | ||||||
| --> | ||||||
| <Target Name="RestoreDelayedOnly"> | ||||||
| <Message Importance="High" Text="======================================" /> | ||||||
| <Message Importance="High" Text="Restoring delayed build projects and test assets" /> | ||||||
| <Message Importance="High" Text="======================================" /> | ||||||
|
|
||||||
| <!-- Filter to only restorable projects (not .proj or .nodeproj) --> | ||||||
| <ItemGroup> | ||||||
| <_DelayedProjectsToRestore Include="@(RequiresDelayedBuild)" | ||||||
| Condition="'%(Extension)' != '.nodeproj' AND '%(Extension)' != '.proj'" /> | ||||||
| </ItemGroup> | ||||||
|
|
||||||
| <MSBuild Projects="@(_DelayedProjectsToRestore)" | ||||||
| Targets="Restore" | ||||||
| BuildInParallel="false" | ||||||
| Properties="MSBuildRestoreSessionId=$([System.Guid]::NewGuid())" /> | ||||||
|
|
||||||
| <Message Importance="High" Text="======================================" /> | ||||||
| <Message Importance="High" Text="Delayed projects restore complete" /> | ||||||
| <Message Importance="High" Text="======================================" /> | ||||||
| </Target> | ||||||
|
|
||||||
| <!-- | ||||||
| Full restore of everything - alternative to solution-based restore. | ||||||
| Slower but more comprehensive. | ||||||
| --> | ||||||
| <Target Name="RestoreAll"> | ||||||
| <Message Importance="High" Text="======================================" /> | ||||||
| <Message Importance="High" Text="Centralized Restore - Restoring all projects" /> | ||||||
| <Message Importance="High" Text="======================================" /> | ||||||
|
|
||||||
| <Exec Command="dotnet restore "$(RepoRoot)AspNetCore.slnx"" /> | ||||||
|
There was a problem hiding this comment. The dotnet restore command should include the --disable-parallel flag to avoid NuGet's internal race conditions, consistent with the implementation in build.ps1 line 503. The whole purpose of centralized restore is to avoid parallel restore issues. Change to: dotnet restore "$(RepoRoot)AspNetCore.slnx" --disable-parallel
Suggested change
|
||||||
|
|
||||||
| <ItemGroup> | ||||||
| <_DelayedProjectsToRestore Include="@(RequiresDelayedBuild)" | ||||||
| Condition="'%(Extension)' != '.nodeproj' AND '%(Extension)' != '.proj'" /> | ||||||
| </ItemGroup> | ||||||
|
|
||||||
| <MSBuild Projects="@(_DelayedProjectsToRestore)" | ||||||
| Targets="Restore" | ||||||
| BuildInParallel="false" | ||||||
| Properties="MSBuildRestoreSessionId=$([System.Guid]::NewGuid())" /> | ||||||
|
|
||||||
| <Message Importance="High" Text="======================================" /> | ||||||
| <Message Importance="High" Text="Centralized Restore Complete" /> | ||||||
| <Message Importance="High" Text="======================================" /> | ||||||
| </Target> | ||||||
| </Project> | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -206,6 +206,10 @@ param( | |
| [Alias('pb')] | ||
| [switch]$ProductBuild, | ||
|
|
||
| # Use centralized restore to avoid NuGet parallel restore race conditions (NuGet/Home#7648) | ||
| # When set, performs a single upfront restore then builds without restore | ||
| [switch]$CentralizedRestore, | ||
|
|
||
| # Intentionally lowercase as tools.ps1 depends on it | ||
| [switch]$fromVMR, | ||
|
|
||
|
|
@@ -271,7 +275,9 @@ $RunBuild = if ($NoBuild) { $false } else { $true } | |
|
|
||
| # Run restore by default unless -NoRestore is set. | ||
| # -NoBuild implies -NoRestore, unless -Restore is explicitly set (as in restore.cmd) | ||
| # -CentralizedRestore performs a single upfront restore then disables per-project restore | ||
| $RunRestore = if ($NoRestore) { $false } | ||
| elseif ($CentralizedRestore) { $false } # Will do centralized restore separately | ||
|
There was a problem hiding this comment. The CentralizedRestore property needs to be passed to MSBuild so that the condition checks in BuildAfterTargetingPack.csproj and FunctionalTestWithAssets.targets can work correctly. Without this property being set, the sequential restore logic in BuildAfterTargetingPack.csproj (line 12-13) and the skip restore condition in FunctionalTestWithAssets.targets (line 46) will not activate, defeating the purpose of the centralized restore feature. Add a line after line 280 like: if ($CentralizedRestore) { $MSBuildArguments += "/p:CentralizedRestore=true" } |
||
| elseif ($Restore) { $true } | ||
| elseif ($NoBuild) { $false } | ||
| else { $true } | ||
|
|
@@ -484,6 +490,44 @@ try { | |
| $global:VerbosePreference = 'Continue' | ||
| } | ||
|
|
||
| # Perform centralized restore if requested - this restores everything upfront in a single | ||
| # coordinated operation to avoid NuGet race conditions (https://github.com/NuGet/Home/issues/7648) | ||
| if ($CentralizedRestore) { | ||
| Write-Host | ||
| Write-Host "===== Centralized Restore =====" -ForegroundColor Cyan | ||
| Write-Host "Restoring all projects upfront to avoid NuGet parallel restore race conditions" -ForegroundColor Cyan | ||
|
|
||
| # Restore via the solution file with --disable-parallel to avoid NuGet internal race conditions | ||
| # NuGet's RestoreTask is not thread-safe when same project is restored in parallel | ||
| Write-Host "Restoring AspNetCore.slnx (sequential)..." -ForegroundColor Yellow | ||
| & dotnet restore "$RepoRoot\AspNetCore.slnx" --disable-parallel --verbosity $Verbosity | ||
| if ($LASTEXITCODE -ne 0) { | ||
| Write-Error "Solution restore failed" | ||
| exit $LASTEXITCODE | ||
| } | ||
|
|
||
| # Generate files that other projects depend on (normally done AfterTargets="Restore" in Tools.props) | ||
| # Must be done BEFORE any other project evaluation since they import these generated files | ||
| Write-Host "Generating build files..." -ForegroundColor Yellow | ||
| & dotnet restore "$RepoRoot\eng\tools\GenerateFiles\GenerateFiles.csproj" --verbosity $Verbosity | ||
| if ($LASTEXITCODE -ne 0) { | ||
| Write-Error "GenerateFiles restore failed" | ||
| exit $LASTEXITCODE | ||
| } | ||
| & dotnet msbuild "$RepoRoot\eng\tools\GenerateFiles\GenerateFiles.csproj" /t:GenerateDirectoryBuildFiles /p:Configuration=$Configuration /verbosity:$Verbosity | ||
| if ($LASTEXITCODE -ne 0) { | ||
| Write-Error "GenerateFiles build failed" | ||
| exit $LASTEXITCODE | ||
| } | ||
|
|
||
| # Note: Delayed build projects (RequiresDelayedBuild) are NOT restored here because they | ||
| # require the targeting pack to be built first. | ||
| # They will be restored later by BuildAfterTargetingPack.csproj with sequential restore. | ||
|
|
||
| Write-Host "===== Centralized Restore Complete =====" -ForegroundColor Cyan | ||
| Write-Host | ||
| } | ||
|
|
||
| if (-not $NoBuildRepoTasks) { | ||
| Write-Host | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,13 @@ | |
| <IsUnitTestProject Condition=" '$(SkipTestBuild)' == 'true' ">false</IsUnitTestProject> | ||
| <IsUnitTestProject Condition=" '$(SkipTestBuild)' != 'true' ">true</IsUnitTestProject> | ||
| <IsTestProject>$(IsUnitTestProject)</IsTestProject> | ||
| <!-- | ||
| Use sequential restore when CentralizedRestore is enabled to avoid NuGet race condition where | ||
| parallel restore of the same project causes "Cannot create a file when that file already exists" errors. | ||
| See https://github.com/NuGet/Home/issues/7648 | ||
| --> | ||
| <_RestoreInParallel Condition="'$(CentralizedRestore)' != 'true'">true</_RestoreInParallel> | ||
| <_RestoreInParallel Condition="'$(CentralizedRestore)' == 'true'">false</_RestoreInParallel> | ||
| </PropertyGroup> | ||
|
|
||
| <Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" /> | ||
|
|
@@ -45,7 +52,7 @@ | |
| Condition=" '$(DotNetBuildSourceOnly)' != 'true' AND ('$(TargetOsName)' == 'win' AND '$(TargetArchitecture)' == 'x64') " | ||
| Returns="@(TargetPathWithTargetPlatformMoniker)"> | ||
| <MSBuild Projects="@(RequiresDelayedBuild)" | ||
| BuildInParallel="$(_RestoreInParallel)" | ||
|
Member
There was a problem hiding this comment. This project is the source of most of the issues - I think the rest of the custom stuff here may be overkill, but I'd bet that just setting this to
Member
There was a problem hiding this comment. Actually, maybe we just pass
Member
Author
There was a problem hiding this comment. Thank you for ideas. I was not aware of the other PR, let's maybe first wait for a while and see how good it works, then I can come back here if needed and try to work on improving this attempt. |
||
| Properties="MSBuildRestoreSessionId=$([System.Guid]::NewGuid())" | ||
| Targets="Restore" /> | ||
| <MSBuild Projects="@(RequiresDelayedBuild)" BuildInParallel="$(BuildInParallel)" Targets="Build"> | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation references properties RestoreForTestAssets and RestoreForDelayedProjects that do not appear to exist in the codebase. This example command would not work as documented. Consider updating the documentation to reflect the actual properties used, which is CentralizedRestore based on the implementation in build.ps1 and the condition checks in BuildAfterTargetingPack.csproj and FunctionalTestWithAssets.targets.