Allow specifying csproj files and/or disabling the default .sln glob for projects to build#1567
Conversation
…n's) and add property to disable building .sln files
| Condition="'$(SdkTaskProjects)' == '' and '$(Restore)' == 'true' and '$(_QuietRestore)' != 'true'"/> | ||
| Condition="'$(BuildSolutions)' != 'false' AND '$(SdkTaskProjects)' == '' and '$(Restore)' == 'true' and '$(_QuietRestore)' != 'true'"/> | ||
|
|
||
| <!-- Invoke restore by winvoking a build on NuGet.targets directly. This avoids duplicate calls to RestoreTask and race conditions on writing restore results to disk. --> |
There was a problem hiding this comment.
Invoke restore by winvoking a build on NuGet.targets directly. This avoids duplicate calls to RestoreTask and race conditions on writing restore results to disk. [](start = 9, length = 161)
Seems like a workaround for a NuGet bug. Is there an issue tracking it?
There was a problem hiding this comment.
It seems like this is the intended behavior of NuGet.targets according to my interpretation of the doc comments. Arcade is currently calling <MSBuild Targets="Restore" Projects="@(ProjectToBuild)">, which is basically the same as calling dotnet restore on each project file. If the projects overlap in their <ProjectReference>'s, those projects are restoring multiple times. When done in parallel, NuGet fails because multiple restores are trying to write to the assets file.
This problem likely hasn't occurred for you yet because most Arcade repos I found are using .sln, and the solution-target implementation of /t:restore already solved race condition problem.
There was a problem hiding this comment.
I see. Does this mean that <MSBuild Properties="@(ProjectToBuild)" Targets="Build"/> will have the same problem?
There was a problem hiding this comment.
No, the Build and ResolveProjectReferences targets are implemented in a way that does not run into the same problem.
There was a problem hiding this comment.
I'd prefer this to be handled by NuGet, not us here, that is <MSBuild Projects="a.proj;b.proj;c.sln" Targets="Restore" /> should just work. I'll file a bug on Nuget
There was a problem hiding this comment.
In the meantime, can we merge as is? This is a blocker for aspnetcore.
There was a problem hiding this comment.
I'm not saying that we shouldn't merge this, just that we should follow up with NuGet and have an issue tracking it.
There was a problem hiding this comment.
Here's a NuGet issue for this one NuGet/Home#7648
|
Thanks for figuring the race conditions out. The approach looks good overall. Some comments inline... |
|
Updated - PR checks passing now. |
src/Microsoft.DotNet.Arcade.Sdk.Tests/Microsoft.DotNet.Arcade.Sdk.Tests.csproj
Outdated
Show resolved
Hide resolved
|
Thanks for adding tests. |
tmat
left a comment
tmat
left a comment
There was a problem hiding this comment.
Other than the minor change around BuildSolutions, LGTM.
Thanks for fixing.
|
This caused CoreFX to have a warning when restoring our root project because we don’t specify any. We basically just default to msbuild to pick our project in the working directory which overrides the restore target. The piece causing the warning is calling NuGet.targets I think we should conditionally call restore on that targets file only when ProjectsToBuild is not set. |
| $bl = if ($binaryLog) { "/bl:" + (Join-Path $LogDir "Build.binlog") } else { "" } | ||
|
|
||
| if ($projects) { | ||
| $properties += "/p:Projects=$projects" |
There was a problem hiding this comment.
In line 103 we add the properties to the command with @properties which causes the following output:
C:\Users\vihofer\.nuget\packages\microsoft.dotnet.arcade.sdk\1.0.0-beta.18618.3\tools\Build.proj /p:Configuration=Debug / p : P r o j e c t s = b u i l d . p r o j.
I'm surprised that slipped through testing.
There was a problem hiding this comment.
This is one of those delightful PowerShell bugs. Powershell is changing the type of $properties to char[]. Why? Who knows
We do specify which project to build in corefx: https://github.com/dotnet/corefx/blob/master/eng/Build.props |
| Condition="'$(SdkTaskProjects)' == '' and '$(Restore)' == 'true' and '$(_QuietRestore)' != 'true'"/> | ||
| Condition="'$(_BuildSolutions)' != 'false' and '$(SdkTaskProjects)' == '' and '$(Restore)' == 'true' and '$(_QuietRestore)' != 'true'"/> | ||
|
|
||
| <!-- Invoke restore using NuGet.targets directly. This avoids duplicate calls to RestoreTask and race conditions on writing restore results to disk. --> |
There was a problem hiding this comment.
This is probably the cause of the warning and the fail of the restore in corefx. I believe the _GenerateRestoreProjectPathWalk target is not invoked because our build.proj is not an sdk based project and does not import the common nuget props/targets. I believe that would have been caught early when not using the internal target directly.
Target "_GenerateRestoreProjectPathWalk" skipped. The target does not exist in the project and SkipNonexistentTargets is set to true.
I don't like that we call directly into internals here. Maybe there is a way to avoid the race without calling into the restore target directly?
There was a problem hiding this comment.
just tested, when using the project restore target it works fine.
In response to dotnet/arcade#1567
In response to dotnet/arcade#1567. This uses an empty project to work around the new behavior that requires a project file even for restore operations.
In response to dotnet/arcade#1567. This uses an empty project to work around the new behavior that requires a project file even for restore operations.
In response to dotnet/arcade#1567. This uses an empty project to work around the new behavior that requires a project file even for restore operations. Commit migrated from dotnet/coreclr@c0d7784
5 participants
Addresses #1454 - cc @rynowak
Addresses #1525 - cc @bricelam
Changes: