Skip to content
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

Merged
merged 3 commits into from
Aug 10, 2023

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Aug 8, 2023

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.

@ghost
Copy link

ghost commented Aug 8, 2023

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

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.

Author: sbomer
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

Copy link
Member

@ViktorHofer ViktorHofer left a 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

<!-- 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>
make things easier?

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@sbomer
Copy link
Member Author

sbomer commented Aug 9, 2023

Unfortunately no - using RuntimeFramework would use the current version for the original illink.runtimeconfig.json, but tests that use live illink need to run on the LKG runtime (from SDK in global.json).

Comment on lines +277 to +280
<_BuildOutputPackageFile Include="$(OutputPath)**"
Exclude="$(OutputPath)publish\**;
$(OutputPath)" />
<TfmSpecificPackageFile Include="@(_BuildOutputPackageFile)"
Copy link
Member

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.

Copy link
Member Author

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.

@sbomer sbomer merged commit c13739b into dotnet:main Aug 10, 2023
180 of 183 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Sep 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.NET 8 preview 7's illink runs with preview 6 if preview 6 is installed
2 participants