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 runtime packs for NativeAOT on .NET 10 or higher #37872

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Jan 10, 2024

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-WebSDK untriaged Request triage from a team member labels Jan 10, 2024
@filipnavara
Copy link
Member Author

I am not confident on how to validate this end-to-end aside from modifying my local SDK and hoping for the best. Any other ideas? /cc @akoeplinger

@am11
Copy link
Member

am11 commented Jan 10, 2024

One easy way is to use this script: https://gist.github.com/am11/afc2787bdecb9592663f4eae405d9116 (download at ~/update-daily-build.sh) apply sdk patches in ~/.dotnet9/sdk and then use dotnet9 publish etc. for testing. To refresh, run ~/update-daily-build.sh again. :)

@filipnavara
Copy link
Member Author

@am11 That's essentially what I do. :-)

@am11
Copy link
Member

am11 commented Jan 11, 2024

I think this testing is enough and we can move this forward if local testing passed. After @jtschuster's dotnet/runtime#96858 goes in, source build will be able to validate more interesting runtimepack scenarios (for linux and freebsd etc.)

@filipnavara
Copy link
Member Author

I'm hesitant to call it "ready" unless someone else confirms that it actually works on their machine (tm). It requires a .NET 9 SDK setup like instrumented above, and then running dotnet publish for any AOT app.

@am11
Copy link
Member

am11 commented Jan 11, 2024

Just tested on osx-arm64 (host), linux-musl-arm64 (container) and win-x64 (host); seems to work. 👍

@filipnavara filipnavara marked this pull request as ready for review January 11, 2024 20:55
@am11
Copy link
Member

am11 commented Jan 11, 2024

@filipnavara, a separate / existing but related issue: I just noticed that from runtime pack, we expect it to be present in the installation directory, such as ~/.dotnet9/packs, right? That's not happening for ilcompiler or crossgen2 because both seem to be missing entries corresponding to the corehost ones: https://github.com/dotnet/installer/blob/main/src/redist/targets/GenerateLayout.targets#L159 in that file. Otherwise, nuget just downloads it as a package under the ~/.nuget directory, i.e no special treatment which defeats the purpose of known packs.

@filipnavara
Copy link
Member Author

Otherwise, nuget just downloads it as a package under the ~/.nuget directory, i.e no special treatment which defeats the purpose of known packs.

Wasn't it always working this way? I assumed it did for NativeAOT cross-compilation on .NET 8, and for macOS platform. Similarly for other platforms the ILCompiler package was not part of the bundle and that's where the "runtime pack" files were located.

@am11
Copy link
Member

am11 commented Jan 11, 2024

Wasn't it always working this way?

Pretty sure it was. What I understand from known packs is that it'd be available as a preloaded packages in the binary bundle (installer, zip, tar, whatever installer produces); otherwise the first time it will download the workload, it will store those packs to the same location where apphost packs are. But it does not seem to be the case for crossgen2 (for a longer time) and ilc (relatively recently). I checked with ls /usr/local/share/dotnet/packs on macOS, and there seems to be all kinds of packs available (maui, apphost etc.), except for crossgen2/ilc. Unless this is intentional, it seems like we are missing something for crossgen2 and ilc. I grep'd the sources in sdk and installer repos, and GenerateLayout.targets seems to be the relevant place.

e.g. if we clear the nuget caches dotnet nuget locals all --clear and republish the app for host architecture with PublishReadyToRun or PublishAOT flags, it will re-download the packages, which does not happen for the real known packs in $DOTNET_ROOT/packs directory.

@akoeplinger
Copy link
Member

akoeplinger commented Jan 12, 2024

Known packs do not need to ship with the SDK, e.g. the normal desktop runtime packs aren't there either and will be fetched from nuget the first time they're used. If you install workloads it'll download packages into the packs folder, but that is not a requirement, just an optimization (e.g. we needed to be able to use workloads offline).

@am11
Copy link
Member

am11 commented Jan 12, 2024

that is not a requirement, just an optimization (e.g. we needed to be able to use workloads offline)

That's exactly what I was hoping to achieve from crossgen2/ilc known packs. Otherwise downloading these nugets separately in CI container builds is same pain as before, so I'll continue to mount ~/.nuget directory there.. :)

@akoeplinger
Copy link
Member

@MichalStrehovsky any concerns from your side?

@MichalStrehovsky
Copy link
Member

I don't think this will work in a .props file. This is so early that we don't know the project has PublishAot in it. It is so early that we probably don't even have _TargetFrameworkVersionWithoutV yet.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

I don't think this will work in a .props file. This is so early that we don't know the project has PublishAot in it. It is so early that we probably don't even have _TargetFrameworkVersionWithoutV yet.

Tried this locally and it only does something if I do dotnet publish -bl -p:PublishAot=true -p:_TargetFrameworkVersionWithoutV=9.0. And the thing it does is not good:

    D:\test\test.csproj : error NU1101: Unable to find package Microsoft.NETCore.App.Runtime.NativeAOT.win-x64. No packages exist with this id in source(s): dotnet9, Microsoft Visual Studio Offline Packages, NET8, nuget.org [D:\test\test.csproj]

We don't seem to have the Windows packages published yet: https://dnceng.visualstudio.com/public/_artifacts/feed/dotnet9

@filipnavara
Copy link
Member Author

This is so early that we don't know the project has PublishAot in it. It is so early that we probably don't even have _TargetFrameworkVersionWithoutV yet.

Bummer. I agree about PublishAot, it can probably be left out of the condition. _TargetFrameworkVersionWithoutV is used in the same file quite extensively though.

@MichalStrehovsky
Copy link
Member

_TargetFrameworkVersionWithoutV is used in the same file quite extensively though.

I would be surprised if they work. When I tried this locally, it didn't try to use the runtime pack unless I also specified a value for _TargetFrameworkVersionWithoutV. Either something is wrong with the logic in this PR, or this is simply not available here. I only see it defined in .targets.

@MichalStrehovsky
Copy link
Member

We don't seem to have the Windows packages published yet: https://dnceng.visualstudio.com/public/_artifacts/feed/dotnet9

Fix in dotnet/runtime#97023

@akoeplinger
Copy link
Member

akoeplinger commented Jan 16, 2024

All the existing usages of _TargetFrameworkVersionWithoutV are in Conditions on Item/ItemGroup so maybe the msbuild evaluation order is different for those. You can try moving the checks into the condition on the FrameworkReference.

@filipnavara
Copy link
Member Author

I'm sorry I didn't get back to this. I may find some free cycles to try again once .NET 9 Preview 1 ships with the necessary packages.

@filipnavara
Copy link
Member Author

filipnavara commented Feb 5, 2024

Side note: We have incorrect runtime.json in Microsoft.DotNet.ILCompiler that is missing entries for new platforms (and linux-bionic).

Nevermind, that's actually fine.

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Feb 9, 2024

I'm playing with alternatives here. Below is an alternative approach. We'd delete all the PublishAotUsingRuntimePack handling in the runtime repo before doing the change here (only leave the PublishAotUsingRuntimePack==true codepaths in runtime repo). Once it comes over here, apply this diff.

One significant issue with this general approach is that FrameworkReference is conditioned on '$(_IsPublishing)' == 'true'. It's conditioned like that for good reasons. But this totally breaks everyone who does dotnet build /t:Publish (people who do this want to watch the world burn I guess). We'll need to make this error out with a sensible message. I don't know if the sensible message should include advice to set _IsPublishing to true. I think we have a bunch of native aot testing in this very repo that does /t:Publish :/.

diff --git a/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.DefaultItems.props b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.DefaultItems.props
index 038026fdf5..05f0b41f13 100644
--- a/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.DefaultItems.props
+++ b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.DefaultItems.props
@@ -83,10 +83,14 @@ Copyright (c) .NET Foundation. All rights reserved.
                         RuntimePackLabels="Mono"
                         Condition="'$(UseMonoRuntime)' == 'true' And ('$(_TargetFrameworkVersionWithoutV)' != '') And ('$(_TargetFrameworkVersionWithoutV)' >= '6.0')" />
 
-    <!-- Allow opt-in to NativeAOT runtime pack for .NET 8.0 or higher -->
+    <!-- Allow opt-in to NativeAOT runtime pack for .NET 8.0 or higher and default to it for .NET 9.0 or higher -->
     <FrameworkReference Update="Microsoft.NETCore.App"
                         RuntimePackLabels="NativeAOT"
-                        Condition="'$(_IsPublishing)' == 'true' and '$(PublishAotUsingRuntimePack)' == 'true' And ('$(_TargetFrameworkVersionWithoutV)' != '') And ('$(_TargetFrameworkVersionWithoutV)' >= '8.0')" />
+                        Condition="'$(PublishAot)' == 'true' And '$(_IsPublishing)' == 'true' And '$(_TargetFrameworkVersionWithoutV)' != ''
+                                   And (
+                                     ('$(PublishAotUsingRuntimePack)' == 'true' And '$(_TargetFrameworkVersionWithoutV)' >= '8.0')
+                                     Or
+                                     ('$(_TargetFrameworkVersionWithoutV)' >= '9.0'))" />
 
   </ItemGroup>
 
diff --git a/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets
index 63be500baa..e0fd0be66d 100644
--- a/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets
+++ b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets
@@ -77,6 +77,7 @@ Copyright (c) .NET Foundation. All rights reserved.
     <PropertyGroup>
       <EnableTargetingPackDownload Condition="'$(EnableTargetingPackDownload)' == ''">true</EnableTargetingPackDownload>
       <EnableRuntimePackDownload Condition="'$(EnableRuntimePackDownload)' == ''">true</EnableRuntimePackDownload>
+      <_UsingNativeAotRuntimePack Condition="'%(FrameworkReference.Identity)' ==  'Microsoft.NETCore.App' And '%(FrameworkReference.RuntimePackLabels)' == 'NativeAOT'">true</_UsingNativeAotRuntimePack>
     </PropertyGroup>
 
     <PropertyGroup>
@@ -116,7 +117,7 @@ Copyright (c) .NET Foundation. All rights reserved.
                                 FirstTargetFrameworkVersionToSupportSingleFileAnalyzer="$(_FirstTargetFrameworkVersionToSupportSingleFileAnalyzer)"
                                 SilenceEnableSingleFileAnalyzerUnsupportedWarning="$(_SilenceEnableSingleFileAnalyzerUnsupportedWarning)"
                                 MinNonEolTargetFrameworkForSingleFile="$(_MinNonEolTargetFrameworkForSingleFile)"
-                                AotUseKnownRuntimePackForTarget="$(PublishAotUsingRuntimePack)"
+                                AotUseKnownRuntimePackForTarget="$(_UsingNativeAotRuntimePack)"
                                 RuntimeIdentifier="$(RuntimeIdentifier)"
                                 RuntimeIdentifiers="$(RuntimeIdentifiers)"
                                 RuntimeFrameworkVersion="$(RuntimeFrameworkVersion)"

@sbomer
Copy link
Member

sbomer commented Jan 17, 2025

Rather than adding yet another path through this code, I'd like to focus on making dotnet build /t:Publish without _IsPublishing unsupported (at least for native AOT, if not for other SDK scenarios).

I suggest we default PublishAotUsingRuntimePack to true and take the breaking change in dotnet build /t:Publish (it will produce the error added in #46070). We could keep the option around for at least a couple previews as an opt-out. Which scenarios would be broken by this?

@ivanpovazan
Copy link
Member

As #46070 got merged in and as far as this PR is concerned we should bump the condition: '$(_TargetFrameworkVersionWithoutV)' >= '9.0' to 10.0.

Apart from that minor change, what are the follow-up actions to move this PR forward?

@MichalStrehovsky
Copy link
Member

Rather than adding yet another path through this code, I'd like to focus on making dotnet build /t:Publish without _IsPublishing unsupported (at least for native AOT, if not for other SDK scenarios).

Cc @Sergio0694 on WinAppSDK experiences around this.

I think this is going to put us into a situation where we need to "support" people setting underscored properties. Granted, the underscored property is already semi-supported (Github is full of hits of people setting this, like https://github.com/baronfel/sdk-container-demo/blob/530356ee6891b317b316d1c98199e6fcc50341ef/src/solution-level-example/Directory.Solution.targets#L17-L25), but this would be a first time when we'd need to direct people to set the underscored property in an error message to make the breaking change self-serviceable. The project I'm linking to is one of the examples where people just want to invoke the Publish target and running dotnet publish might not be desirable.

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Jan 20, 2025

The way out I've been thinking about for a while would be to:

  • Add <IsPublishing Condition="$(_IsPublishing) != '' and $(IsPublishing) == ''">$(_IsPublishing)</IsPublishing> somewhere very early in SDK props files. This is for backcompat with everyone who is setting the de-facto supported underscored version. Because it's very early in the props, it will only work if they invoked MSBuild with the property; people setting _IsPublishing somewhere late will be broken, I don't think we need to care.
  • Use IsPublishing instead of _IsPublishing through the SDK
  • Error out in the Publish target if IsPublishing is not true telling people to set the property when invoking the target or just running dotnet publish.

This would address all of the subtle weird issues like "dotnet publish with an PublishTrimmed app succeeds, but dotnet build /t:Publish fails telling me the app is not SelfContained" (the piece that solves the puzzle is that we default SelfContained for PublishTrimmed apps if _IsPublishing is specified to true - people solve this by adding SelfContained=true to their csproj but then their non-publish build is also unnecessarily SelfContained).

@Sergio0694
Copy link
Contributor

For the _IsPublishing part, this would affect both UWP and WinUI 3. The former can only build with msbuild (as it needs stuff that's in Visual Studio), and the latter needs either that, or dotnet msbuild. In practice, there's no scenario where dotnet publish will work for either framework. Which, if I understand correctly, means that _IsPublishing wouldn't be set at all (like it is the case today). So if that suddenly became unsupported and stopped working, I think it would put the two in an awkward situation 😅

@sbomer
Copy link
Member

sbomer commented Jan 21, 2025

direct people to set the underscored property in an error message

I definitely wouldn't mention the underscored property in the message. Until it's made public the error message will be suboptimal (not self-service for cases like the one you linked), but it seems better than hitting some downstream ILC error. I think it accurately reflects the current SDK position that 'dotnet msbuild /t:Publish' is not supported.

I agree with your plan #37872 (comment). I am hoping that putting in errors in the right spots will help get traction on making this public, and I don't think we should block switching to runtime packs on it.

For the _IsPublishing part, this would affect both UWP and WinUI 3

If I understand correctly, the fix for UWP and WinUI 3 will be to require users to invoke msbuild /t:Publish -p:SomeProperty=true, right? If we default to using runtime packs I think it will force the issue - UWP and WinUI 3 folks can own the decision about the appropriate gesture. It could be PublishUWP=true that implies _IsPublishing, or it could be to work with SDK folks to try to make IsPublishing supported more generally.

@baronfel baronfel added the needs team triage Requires a full team discussion label Jan 21, 2025
@filipnavara filipnavara changed the title Use runtime packs for NativeAOT on .NET 9.0 or higher Use runtime packs for NativeAOT on .NET 10 or higher Jan 21, 2025
@Sergio0694
Copy link
Contributor

@MichalStrehovsky @sbomer by the way, we're seeing issues related to the general problem of "how to correctly detect whether you're publishing or not" coming up all the time, and I'm wondering whether this could be a good occasion to work together and see if we can actually find a solution that works well for this across both .NET, Windows, etc.

For instance, in the MSX tooling there's a few scenarios where we're trying to detect whether we're actually publishing or not, because if that's the case then we know the ILC targets will also run, which means we need to account for the output binary being native and not having other separate managed .dll-s etc., which is relevant in several cases (eg. specifying the implementation .dll for a WinRT component in your APPX manifest). What we're doing right now is having targets that do something like DependsOnTargets="IlcCompile" (or some other publish-only target), but this is not really convenient. Also this doesn't work at all if we need to do work earlier than those targets, such as before the actual build.

If there were some property that we could consistently and reliably depend upon to just know "are we going to be published or not", then that solve all of these issues. Perhaps we should set up some time to chat and see if there's a proper solution we could come up with? 🙂

cc. @manodasanW @jkoritzinsky @jevansaks

@MichalStrehovsky
Copy link
Member

If I understand correctly, the fix for UWP and WinUI 3 will be to require users to invoke msbuild /t:Publish -p:SomeProperty=true, right? If we default to using runtime packs I think it will force the issue - UWP and WinUI 3 folks can own the decision about the appropriate gesture. It could be PublishUWP=true that implies _IsPublishing, or it could be to work with SDK folks to try to make IsPublishing supported more generally.

The moment this merges, _IsPublishing will be needed for WinUI/UWP to work. That puts a bit of a pressure on things. I think we'll just end up with _IsPublishing becoming really public and official, it already de facto is because the SDK gave people no choice.

I definitely wouldn't mention the underscored property in the message. Until it's made public the error message will be suboptimal (not self-service for cases like the one you linked), but it seems better than hitting some downstream ILC error. I think it accurately reflects the current SDK position that 'dotnet msbuild /t:Publish' is not supported.

Here's my thoughts around this:

  1. Do we document somewhere that running the Publish target directly is unsupported?
  2. Lots of people do that, some of them even pass _IsPublishing but most of them just do some variation of "run the Publish target". Here is a popular pattern (note these are all pretty high profile).
  3. Executing the Publish target works right now. It even works if you don't pass _IsPublishing because _IsPublishing is just a shorthand for a couple other public properties. After this PR, _IsPublishing is going to be mandatory for PublishAot.
  4. Since this is a breaking change, we need to have guidelines in place for people to unbreak themselves. I don't know what guidelines can we offer to the people doing the "popular pattern" with this, for example. At best we can say "what you were doing no longer works, there's no alternative within MSBuild land, sorry".

I kind of wish Publish was a build configuration not a Target. I'm sure it would have other problems but it would solve a lot of problems here (since _IsPublishing affects how we build things too, not just what we do in the publish target).

I spent a lot of time yesterday thinking about this and even after sleeping on it I don't see good answers. I wonder if in dotnet/runtime#109988 we solved the worst problem with the current approach and whether we should just keep shipping the framework in the ILCompiler package and swap it out before we start ILCompiler (and drop the idea of runtime packs, even on iOS/Bionic - we can just have a ILCompiler package without the compiler for those...).

We can certainly try the approach in the PR too, but I'm starting to be concerned we'll be forced to roll it back because we don't have answers for the people invoking the Publish target today.

@sbomer
Copy link
Member

sbomer commented Jan 22, 2025

this could be a good occasion to work together and see if we can actually find a solution that works well for this across both .NET, Windows, etc.

#26324 tracks this. We could wait for a solution there, but my preference is to take the breaking change earlier rather than later, and adjust the error message once there's a better supported solution. (I do think this is a good occasion to find a broader solution, and introducing a clear error should help it get traction where it hasn't so far.)

Do we document somewhere that running the Publish target directly is unsupported?

To my knowledge it's not documented, just broken in ways that are hard to detect or diagnose (that's what #46070, #46171, and dotnet/runtime#95496 are supposed to help with).

It even works if you don't pass _IsPublishing because _IsPublishing is just a shorthand for a couple other public properties. After this PR, _IsPublishing is going to be mandatory for PublishAot.

Note that PublishAotUsingRuntimePack=false will still be a workaround similar to passing SelfContained. We could leave that as the breaking change guidance until #26324 is resolved.

whether we should just keep shipping the framework in the ILCompiler package and swap it out before we start ILCompiler (and drop the idea of runtime packs, even on iOS/Bionic - we can just have a ILCompiler package without the compiler for those...).

It's certainly an option, but it feels to me like going against the grain of the SDK publishing design. From all the conversations I've had about _IsPublishing, the only solution I can see is a property that needs to be passed as a global property (in that respect it's actually similar to a Configuration). I think _IsPublishing has pretty much the right behavior, but should be made public.

@MichalStrehovsky
Copy link
Member

Note that PublishAotUsingRuntimePack=false will still be a workaround similar to passing SelfContained. We could leave that as the breaking change guidance until #26324 is resolved.

That resolves my biggest concern. I'm still not sure if this will actually work out, but we can try. I assume we wouldn't ship .NET 10 RTM in shape where we have both a framework in a runtime pack and in the ILCompiler package.

@ivanpovazan
Copy link
Member

@akoeplinger do we need someone from dotnet/sdk for official sign off?

@akoeplinger akoeplinger added Area-NativeAOT Native AOT compilation and removed Area-WebSDK labels Jan 24, 2025
@akoeplinger
Copy link
Member

@baronfel I saw you added "needs team triage" label so I assume you want to take a look?

@baronfel
Copy link
Member

@akoeplinger I marked it so the SDK team would discuss this this week. We've done so and are working on some plans for 10 (chatting with @agocke and stuff) but have no specific feedback on this PR at this time.

@baronfel baronfel removed the needs team triage Requires a full team discussion label Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-NativeAOT Native AOT compilation untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants