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

Add win-x86 to known ILCompiler and NativeAOT runtime packs #19166

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

filipnavara
Copy link
Member

@ViktorHofer
Copy link
Member

If I'm not mistaken, this change has the potential to break our own repositories that list win-x86 in any project's RuntimeIdentifiers section if an SDK with this change is used without the ilcompiler/runtime packages being published first. Unsure if we have any of these cases.

@MichalStrehovsky
Copy link
Member

I wasn't sure about that either that's why I ended up not merging yet. In the past we always started producing these first.

@MichalStrehovsky
Copy link
Member

(However, Net90NativeAOTRuntimePackRids does contain the wasi/wasm packs that are only produced in a separate nuget feed from the runtimelab repo and that one doesn't cause problems.)

@ViktorHofer
Copy link
Member

However, Net90NativeAOTRuntimePackRids does contain the wasi/wasm packs that are only produced in a separate nuget feed from the runtimelab repo

Interesting, I thought that we never ship artifacts from runtimelab. Did that change? To which feed do we publish runtimelab artifacts?

that one doesn't cause problems.

Probably because it only affects projects that specify a RuntimeIdentifier. Maybe none of our projects specify the wasi/wasm RID and enable NativeAOT?

@MichalStrehovsky
Copy link
Member

Interesting, I thought that we never ship artifacts from runtimelab. Did that change? To which feed do we publish runtimelab artifacts?

We've been doing that single the runtimelab beginnings - they should go to the dotnet-experimental feed.

If I'm not mistaken, this change has the potential to break our own repositories that list win-x86 in any project's RuntimeIdentifiers section if an SDK with this change is used without the ilcompiler/runtime packages being published first. Unsure if we have any of these cases.

Mmm, I did a small experiment with a console hello world app that has <RuntimeIdentifiers>wasi-wasm;win-x86;win-x64</RuntimeIdentifiers> and indeed, publish doesn't work even if I try to publish for win-x64 (error NETSDK1082: There was no runtime pack for Microsoft.NETCore.App available for the specified RuntimeIdentifier 'wasi-wasm'). It works if I drop the wasi-wasm part.

This was discussed in #18121 (comment) but looks like this aspect was missed. Cc @akoeplinger

@filipnavara
Copy link
Member Author

It’s fine to wait for the official packs to be built first. There’s a whole chain of PRs that is currently blocked on a 3-line change in the JIT but I decided to preemptively open all the PRs to avoid any further delays once that one issue/PR is resolved.

This should not really break anything, as stated in some of the previous comments about WASI/WASM. The failure mode for KnownRuntimePacks is already catastrophic as it is. If there’s a RID listed without a corresponding NuGet you will get an error about resolving NuGet package which is somewhat searchable/actionable. It only happens when trying to publish for the newly added RID. Conversely, if a RID is missing in the list, the runtime pack resolution doesn’t produce an error but it also doesn’t populate ItemGroups and properties that are expected by the NativeAOT publishing. The end result is that you get cryptic error that must be traced back through BinLogs.

@MichalStrehovsky
Copy link
Member

Mmm, I did a small experiment with a console hello world app that has <RuntimeIdentifiers>wasi-wasm;win-x86;win-x64</RuntimeIdentifiers> and indeed, publish doesn't work even if I try to publish for win-x64 (error NETSDK1082: There was no runtime pack for Microsoft.NETCore.App available for the specified RuntimeIdentifier 'wasi-wasm'). It works if I drop the wasi-wasm part.

Scratch that, it fails with this error even if I drop PublishAot and add --self-contained, so maybe that's unrelated.

@ViktorHofer
Copy link
Member

We've been doing that single the runtimelab beginnings - they should go to the dotnet-experimental feed.

Right, but I was surprised that we add support for artifacts produced by runtimelab into the SDK (in this case known RID items). cc @jkotas

@akoeplinger
Copy link
Member

Right, but I was surprised that we add support for artifacts produced by runtimelab into the SDK (in this case known RID items)

The SDK doesn't care where the runtime packs come from, e.g. we also have linux-s390x or linux-loongarch64 in the lists even though we don't build them on the Microsoft side. Having them in the list just makes it much easier for third parties to provide them.

We can wait with merging this until we have at least one 9.0 preview version on nuget.org with the win-x86 package to be on the safe side.

@jkotas
Copy link
Member

jkotas commented Mar 25, 2024

I was surprised that we add support for artifacts produced by runtimelab into the SDK (in this case known RID items). cc @jkotas

I see the hardcoded RID lists in the SDK as a broken design. Ideally, the SDK would not care about the RID that you are publishing for. It would just take the RID and try to download the package for it.

Having them in the list just makes it much easier for third parties to provide them.

+1

@MichalStrehovsky
Copy link
Member

We now have packages but they are non-functional. Fix is in dotnet/runtime#100512.

@MichalStrehovsky
Copy link
Member

Validated we now produce good packages and the update got ingested into installer. Thank you for your work on this Filip!

@MichalStrehovsky MichalStrehovsky merged commit 144a405 into dotnet:main Apr 5, 2024
22 checks passed
@filipnavara filipnavara deleted the patch-3 branch April 5, 2024 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants