-
Notifications
You must be signed in to change notification settings - Fork 514
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
[msbuild] Take the .NET version into account when computing the illink.dll location. #15876
Conversation
…k.dll location. We use illink.dll from the executing .NET version: if we're building with .NET 7, then we're using illink from .NET 7. This means we can't hard-code 'net6.0' when computing the path to illink.dll for remote builds. Also add a check to verify that the computed path exists, because otherwise the error we get can be somewhat confusing. Fixes https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/1611403.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…d the file in question.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
<!-- We need to use net5.0 because when building from VS it sets MSBuildRuntimeType to Core and will pick net472 (which doesn't contain the illink assembly) --> | ||
<_RemoteILLinkPath>$(ILLinkTasksAssembly.Replace('$(NetCoreRoot)', '$(_DotNetRootRemoteDirectory)').Replace('net472', 'net6.0').Replace('$([System.IO.Path]::GetFileName('$(ILLinkTasksAssembly)'))', 'illink.dll'))</_RemoteILLinkPath> | ||
<!-- We need to use netX.Y because when building from VS it sets MSBuildRuntimeType to Core and will pick net472 (which doesn't contain the illink assembly) --> | ||
<_RemoteILLinkPath>$(ILLinkTasksAssembly.Replace('$(NetCoreRoot)', '$(_DotNetRootRemoteDirectory)').Replace('net472', 'net$(BundledNETCoreAppTargetFrameworkVersion)').Replace('$([System.IO.Path]::GetFileName('$(ILLinkTasksAssembly)'))', 'illink.dll'))</_RemoteILLinkPath> |
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.
Would this work in a multitargeting scenario? When targeting .NET 6 but building with .NET 7, the .NET 6 folder won't exist
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.
BundledNETCoreAppTargetFrameworkVersion is "7.0" when building with .NET 7
🔥 [PR Build] Build failed 🔥Build failed for the job 'Detect API changes' Pipeline on Agent |
🔥 Unable to find the contents for the comment: D:\a\1\s\change-detection\results\gh-comment.md does not exist :fire Pipeline on Agent |
💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻✅ All tests on macOS Mac Catalina (10.15) passed. Pipeline on Agent |
❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌Failed tests are:
Pipeline on Agent |
🔥 [CI Build] Test results 🔥Test results❌ Tests failed on VSTS: simulator tests 0 tests crashed, 8 tests failed, 212 tests passed. Failures❌ linker tests
Html Report (VSDrops) Download Successes✅ bcl: All 69 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
Test failures are unrelated (https://github.com/xamarin/maccore/issues/1083). |
We use illink.dll from the executing .NET version: if we're building with .NET
7, then we're using illink from .NET 7. This means we can't hard-code 'net6.0'
when computing the path to illink.dll for remote builds.
Fixes https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/1611403.