-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Use Microsoft.IO.Redist in XMake.cs #10705
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
Conversation
This will fix dotnet#10540--it looks like M.IO.Redist has the same fix that .NET 9 has that avoids the failed root enumeration. As a bonus it's generally higher performance/less allocatey (but I don't know that that has a specific positive impact in this file).
I'm going to solve the test failure in I did, however, fix the error reporting at the end of the build to match the (better) core behavior: ❯ dotnet msbuild /profileevaluation:"|||" .\src\Framework\ -t:asdf
Microsoft.Build.Framework failed with 1 error(s) (0.0s)
Q:\src\msbuild\src\Framework\Microsoft.Build.Framework.csproj : error MSB4057: The target "asdf" does not exist in the project.
Build failed with 1 error(s) in 0.4s
Writing profiler report to '|||'...
MSBUILD : error MSB4239: Error writing profiling report. The filename, directory name, or volume label syntax is incorrect. : 'Q:\src\msbuild\|||'.
❯ msbuild /profileevaluation:"|||" .\src\Framework\ -t:asdf
MSBuild version 17.13.0-preview-24504-04+c4d51a11b for .NET Framework
MSBUILD : error MSB1053: Provided filename is not valid. Illegal characters in path.
Switch: |||
For switch syntax, type "MSBuild -help"
❯ artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\amd64\MSBuild.exe /profileevaluation:"|||" .\src\Framework\ -t:asdf
MSBuild version 17.12.0-dev-24507-01+71d7380fc for .NET Framework
Q:\src\msbuild\src\Framework\Microsoft.Build.Framework.csproj : error MSB4057: The target "asdf" does not exist in the project.
Build FAILED.
Q:\src\msbuild\src\Framework\Microsoft.Build.Framework.csproj : error MSB4057: The target "asdf" does not exist in the project.
0 Warning(s)
1 Error(s)
Time Elapsed 00:00:00.93
Writing profiler report to '|||'...
MSBUILD : error MSB4239: Error writing profiling report. Illegal characters in path. |
This was repeated but can be simplified.
This was thrown from GetExtension with an invalid path on .NET Framework.
This tested behavior of System.IO that only appled on .NET Framework and was removed by dotnet#10705. Since evaluation profiling is rarely used, I prefer to just drop the test.
A restore problem/race? https://dev.azure.com/dnceng-public/public/_build/results?buildId=830018&view=logs&j=1522e9b9-b859-5e5f-ec86-a68fc9508baf&t=411bab51-4a66-5244-5496-e7735c534dac&l=234 And a crash in tests on macOS? https://dev.azure.com/dnceng-public/public/_build/results?buildId=830018&view=logs&j=0ddb6181-8b1d-5386-35d2-ca21a772cde8&t=a36a8187-bb52-5aa3-80a2-ccc76a822779&l=330 Retrying for both. |
@JanKrivanek @maridematte I changed this since you reviewed, can you give it a once-over and holler if you hate it? |
I'm all good with the changes here (including the removed unit test) - feel free to merge! |
This will fix #10540--it looks like M.IO.Redist has the same fix that .NET 9 has that avoids the failed root enumeration. As a bonus it's generally higher performance/less allocatey (but I don't know that that has a specific positive impact in this file).