-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
NuGet static graph restore can cause subsequent builds to fail to expand globs in .NET SDK 6.0.100-preview6 #6602
Comments
I don't repro in a simple project with the same MSBuild/SDK (or My first thought is that this may be an instance of #406, but there's no recursion in the pattern and the paths aren't all that long. It could conceivably be a different exception causing the same symptom but I don't know what that would be. The source as currently checked in doesn't have any interesting files or patterns: https://github.com/dotnet/runtime/tree/213600c17635bf25f812a6a7e6ab53d4fa875883/src/libraries/System.IO.Compression/src cc @ladipro since this is maybe related to your glob change #6151. |
I have a local repro with SDK 6.0.100-preview.6.21321.7 and the dotnet/runtime repo as in the OP. |
Been looking at this this morning. It looks like there may be a race condition in persistent state in worker nodes somehow? I can get into a state where it fails consistently, but I've also seen states where it consistently passes, for instance building with Naturally those are the ones that are easier to debug, so I haven't chased down the problem yet. |
In the bad state, I see an entry in That's causing this to return true msbuild/src/Build/Utilities/EngineFileUtilities.cs Lines 227 to 230 in bbeb701
which bubbles up to msbuild/src/Build/Utilities/EngineFileUtilities.cs Lines 136 to 141 in bbeb701
I don't see any place where that could be written to other than here: msbuild/src/Build/Utilities/EngineFileUtilities.cs Lines 202 to 222 in bbeb701
But that env var doesn't seem to be set anywhere. |
I finally found AHA! State is leaking from NuGet setting this for restore: I think this may be new behavior related to dotnet/sdk#18263 (cc @rokonec): before, NuGet's limit of 1 worker node would have been in-proc; now it's forced out by Long term, I think we should teach NuGet to unset This suggests a workaround: set the environment variable |
Other possible workarounds:
|
@rainersigwald Static graph restore happens entirely out of proc, how is it leaking over to builds? |
@jeffkl well, we made it more out of proc and now it's too out of proc :) Before:
after
Where that third process ( |
@rainersigwald so you're saying the out of proc evaluations launch a re-usable msbuild.exe node now? Would that be a problem for any app that does evaluations and creates polluted instances? |
It's not the evaluations that are spawning the node but the target executions that collect the output. |
So is the fix to set EnableNodeReuse to false on the BuildParameters? https://source.dot.net/#Microsoft.Build/BackEnd/BuildManager/BuildParameters.cs,396 |
Either that, or explicitly unset Just setting |
We don't set |
No, it's being set by the CLI because it's been observed to improve performance: dotnet/sdk#17916. We should consider changing that mechanism so it's less "infectious", like promoting it to a command-line argument or doing it at the API level somehow. @rokonec thoughts/feelings/preferences? |
(I still think NuGet should make itself robust to the situation since a user might have set it in some rare circumstances.) |
A low-cost fix may be to do this via a different environment variable that MSBuild unsets after reading. Or even just unset |
I would also argue that |
FWIW, I think I'm running into this issue in some variation fairly often in my zig-msbuild-sdk project. |
@alexrp What .NET SDK version are you using? The condition that caused this error shouldn't have ever shipped in a formal preview, just the daily builds for a while. (The underlying bugs are there but shouldn't have bitten anyone until various things combined.) |
|
Here's an example:
The SDK is doing: <Compile Include="**/*.cxx"
Excludes="$(DefaultItemExcludes); $(DefaultExcludesInProjectFolder)"
Condition="'$(CompilerMode)' == 'Cxx'" /> |
Issue Description
The globbing of items in msbuild isn't expanded. These files are passed to the compiler as
AdditionalFiles
items. This was originally reported by @stephentoub.Steps to Reproduce
This should be easily reproducible by doing the same globbing as in the link below in a hello world app. Note that this seems to be dependent on the exact version of msbuild being used.
Expected Behavior
The items specified in https://github.com/dotnet/arcade/blob/28a6403ee97077256fcdc60f599f0ad9e38e3cfa/src/Microsoft.DotNet.CodeAnalysis/build/Microsoft.DotNet.CodeAnalysis.targets#L21-L25 should be expanded correctly. The wildcard should be expanded and not hardcoded as a literal
*
.Actual Behavior
The wildcard globbing isn't expanded and the wildcard
*
is written literally.Analysis
Versions & Configurations
MSBuild version = "17.0.0-preview-21317-11+c579afe9c"
Attach a binlog
Internal link for MSFT employees:
https://microsofteur-my.sharepoint.com/:u:/g/personal/vihofer_microsoft_com/EftKjjkbyK5JvpaijHLKfjABIeIYvPIfQk_fIslqqn-rQA?e=suROf8
The text was updated successfully, but these errors were encountered: