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

ILCompiler.pkgproj: remove 'runtime.json' when DotNetBuildSourceOnly. #102908

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

tmds
Copy link
Member

@tmds tmds commented May 31, 2024

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 31, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 31, 2024
To work around these errors we remove the 'runtime.json' file completely on the source-built SDK.
'ProcessFrameworkReferences' is responsible for either picking up the bundled pack (when targetting the source-built rid)
or to add a 'runtime.<rid>.Microsoft.DotNet.ILCompiler' package for download. -->
<IncludeRuntimeJson Condition="'$(DotNetBuildSourceOnly)' == 'true'">false</IncludeRuntimeJson>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This depends on dotnet/arcade#14805.

@ViktorHofer
Copy link
Member

@ericstj can you please take a look? (This is about pkgproj infra)

@ericstj
Copy link
Member

ericstj commented May 31, 2024

Can we just remove this completely? We shouldn't be relying on runtime.json anywhere anymore. We removed that from the product in 2.0. Anything that needs support like this should have an SDK doing the package selection.

@dsplaisted
Copy link
Member

Can we just remove this completely? We shouldn't be relying on runtime.json anywhere anymore. We removed that from the product in 2.0. Anything that needs support like this should have an SDK doing the package selection.

Per conversation in dotnet/source-build#1215, it sounds like using a PackageReference to Microsoft.DotNet.ILCompiler is still supported, and for that scenario to work the runtime.json needs to be there to acquire the RID-specific ILCompiler package.

@jkotas jkotas added area-NativeAOT-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 31, 2024
Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@jkotas
Copy link
Member

jkotas commented May 31, 2024

Can we just remove this completely? We shouldn't be relying on runtime.json anywhere anymore. We removed that from the product in 2.0. Anything that needs support like this should have an SDK doing the package selection.

Per conversation in dotnet/source-build#1215, it sounds like using a PackageReference to Microsoft.DotNet.ILCompiler is still supported, and for that scenario to work the runtime.json needs to be there to acquire the RID-specific ILCompiler package.

We need a simple way to override the default version of Microsoft.DotNet.ILCompiler. It is frequently used to trouble shoot issues and to provide workarounds for customers. Microsoft.DotNet.ILCompiler package reference works perfectly for this purpose today.

Can we make it work equally well without runtime.json?

@dsplaisted
Copy link
Member

We need a simple way to override the default version of Microsoft.DotNet.ILCompiler. It is frequently used to trouble shoot issues and to provide workarounds for customers. Microsoft.DotNet.ILCompiler package reference works perfectly for this purpose today.

Can we make it work equally well without runtime.json?

Rather than a PackageReference, I think you could do this:

<ItemGroup>
    <KnownILCompilerPack Update="Microsoft.DotNet.ILCompiler" ILCompilerPackVersion="8.0.5"/>
</ItemGroup>

I think this would work with an ILCompiler package without the runtime.json and the logic we already have in the SDK today.

@jkotas
Copy link
Member

jkotas commented Jun 1, 2024

I dislike that we would be moving from PackageReference to SDK magic. I think PackageReferences are much cleaner and easier to understand from user point of view.

If we agree that the SDK magic is right way to go, I guess it can work. @agocke @MichalStrehovsky Do you see any problems with it? We would have to update the docs and chase down all places that depend on the current behavior. It may be best to do it separately from enabling source build for native AOT toolchain.

We shouldn't be relying on runtime.json anywhere anymore. We removed that from the product in 2.0. Anything that needs support like this should have an SDK doing the package selection.

runtime.json may not be officially documented, but it is the only way we have to provide good user experience for nuget packages with (large) platform-specific binaries. Both our and 3rd party packages depend on it for this reason.

@MichalStrehovsky
Copy link
Member

If we agree that the SDK magic is right way to go, I guess it can work. @agocke @MichalStrehovsky Do you see any problems with it? We would have to update the docs and chase down all places that depend on the current behavior. It may be best to do it separately from enabling source build for native AOT toolchain.

I don't see an issue other than what Jan already wrote. If we do this change, we'll need to consider KnownILCompilerPack and ILCompilerPackVersion basically a "public API" so that we don't have to redo chasing down all places that depend on this or mention this next time it changes. If we're doing a break, it would be ideal if this mechanism is standardized with other tools that ship in NuGets - ILLink and crossgen off the top of my head, so that we know what we have is going to last.

@sbomer is there an existing mechanism when a user needs to pick up a new/specific version of ILLink if they need a different one from the one that ships with the SDK?

@jkotas
Copy link
Member

jkotas commented Jun 3, 2024

it would be ideal if this mechanism is standardized with other tools that ship in NuGets - ILLink and crossgen off the top of my head

There are number of other tools. For example, you can reference a specific Roslyn version too - via a package reference. I think the only meaningful standard are package references. Everything else are bespoke solutions that won't ever be consistent across the ecosystem.

@sbomer
Copy link
Member

sbomer commented Jun 3, 2024

<ItemGroup>
    <KnownILCompilerPack Update="Microsoft.DotNet.ILCompiler" ILCompilerPackVersion="8.0.5"/>
</ItemGroup>

We should be careful with this - it updates the ILC version used for all TFMs. To update it for just one TFM you'd have to do something like this:

<ItemGroup>
    <KnownILCompilerPack
        Update="@(KnownILCompilerPack->WithMetadataValue('TargetFramework', 'net8.0'))"
        ILCompilerPackVersion="8.0.5" />
</ItemGroup>

@sbomer is there an existing mechanism when a user needs to pick up a new/specific version of ILLink if they need a different one from the one that ships with the SDK?

It's the same as for ILCompiler: the user can add a PackageReference, which works but produces a warning:

warning : Delete explicit 'Microsoft.NET.ILLink.Tasks' package reference in your project file to avoid versioning problems.

The same trick (updating KnownILLinkPack) works as well.

I don't think updating the Known*Packs is a good user experience. It works in a pinch, but if we care about the user experience in this scenario I would not recommend relying on this mechanism.

@dsplaisted
Copy link
Member

It may be best to do it separately from enabling source build for native AOT toolchain.

I also think it would be best to treat these two things as separate - ie enabling native AOT on source-built SDKs, and removing the runtime.json from Microsoft.DotNet.ILCompiler (and other similar packages, if there are any).

@ViktorHofer ViktorHofer merged commit 7e977dc into dotnet:main Jun 17, 2024
70 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants