-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Use current runtime version in ILLink.Tasks package #90197
Conversation
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue DetailsFixes #90015 The ILLink.Tasks package should have a runtimeconfig.json that matches the runtime version bundled with the SDK that references it. This used to be done by stomping the runtime version in the dotnet/sdk build, but ILLink.Tasks is no longer bundled with the SDK so we need to do this ourselves. Tests that use live ILLink bits need to continue working with the SDK specified in global.json, so this preserves the existing illink.runtimeconfig.json in the build output, but defines an additional illink.runtimeconfig.pack.json file that uses the current runtime version. The pack.json file is included in the package (with the name illink.runtimeconfig.json) instead of the original. This includes a fix for #88929 - we weren't actually using live illink for tests due to an ordering issue that I introduced in response to feedback on the PR. @ViktorHofer the Directory.Build.props is imported too early (before the nuget props that this is trying to override) so I think we need to import Microsoft.NET.ILLink.props directly from the project file - but let me know if you can see a better solution.
|
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 using something similar to
runtime/eng/targetingpacks.targets
Lines 167 to 174 in 943af3a
<!-- Update the local targeting pack's version as it's written into the runtimeconfig.json file to select the right framework. --> | |
<Target Name="UpdateRuntimeFrameworkVersion" | |
Condition="'$(UseLocalTargetingRuntimePack)' == 'true'" | |
AfterTargets="ResolveTargetingPackAssets"> | |
<ItemGroup> | |
<RuntimeFramework Version="$(ProductVersion)" | |
Condition="'%(RuntimeFramework.FrameworkName)' == '$(LocalFrameworkOverrideName)'" /> | |
</ItemGroup> |
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Unfortunately no - using |
<_BuildOutputPackageFile Include="$(OutputPath)**" | ||
Exclude="$(OutputPath)publish\**; | ||
$(OutputPath)" /> | ||
<TfmSpecificPackageFile Include="@(_BuildOutputPackageFile)" |
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.
Can you please explain this change? I'm not sure what it is doing.
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.
Before this change, the PackagePath
was just tools/$(TargetFramework)/
- the %(RecursiveDir)%(FileName)%(Extension)
part was empty because the itemgroup hadn't been defined, and the build output was showing:
packaging.targets(280,30): message : MSB4120: Item 'TfmSpecificPackageFile' definition within target references itself via (qualified or unqualified) metadatum 'RecursiveDir'. This can lead to unintended expansion and cross-applying of pre-existing items. More info: https://aka.ms/msbuild/metadata-self-ref [/home/svbomer/src/runtime/src/tools/illink/src/ILLink.Tasks/ILLink.Tasks.csproj::TargetFramework=net8.0]
I needed the filename to be included to make it easier to filter later.
Fixes #90015
The ILLink.Tasks package should have a runtimeconfig.json that matches the runtime version bundled with the SDK that references it. This used to be done by stomping the runtime version in the dotnet/sdk build, but ILLink.Tasks is no longer bundled with the SDK so we need to do this ourselves.
Tests that use live ILLink bits need to continue working with the SDK specified in global.json, so this preserves the existing illink.runtimeconfig.json in the build output, but defines an additional illink.runtimeconfig.pack.json file that uses the current runtime version. The pack.json file is included in the package (with the name illink.runtimeconfig.json) instead of the original.
This includes a fix for #88929 - we weren't actually using live illink for tests due to an ordering issue that I introduced in response to feedback on the PR. @ViktorHofer the Directory.Build.props is imported too early (before the nuget props that this is trying to override) so I think we need to import Microsoft.NET.ILLink.props directly from the project file - but let me know if you can see a better solution.