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

NativeAOT broken for Apple TFMs in .NET 10 #46790

Closed
rolfbjarne opened this issue Feb 12, 2025 · 12 comments · Fixed by #46839
Closed

NativeAOT broken for Apple TFMs in .NET 10 #46790

rolfbjarne opened this issue Feb 12, 2025 · 12 comments · Fixed by #46839
Labels
Area-NativeAOT Native AOT compilation untriaged Request triage from a team member

Comments

@rolfbjarne
Copy link
Member

NativeAOT is broken for Apple TFMs after #46070.

Now the following fails:

dotnet build

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net10.0-ios</TargetFramework>
    <PublishAot>true</PublishAot>
    <PublishAotUsingRuntimePack>true</PublishAotUsingRuntimePack>
  </PropertyGroup>
</Project>

with:

error NETSDK1225: Native compilation is not supported when invoking the Publish target directly. Try running dotnet publish.

and this fails:

dotnet build

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net10.0-ios</TargetFramework>
    <PublishAot>true</PublishAot>
    <PublishAotUsingRuntimePack>false</PublishAotUsingRuntimePack>
  </PropertyGroup>
</Project>

with:

error NETSDK1203: Ahead-of-time compilation is not supported for the target runtime identifier 'iossimulator-arm64'.

In the past we've automatically set PublishAotUsingRuntimePack=true (always, not only for publish), which I believe we're still supposed to do.

CC @sbomer @MichalStrehovsky

Note: this effectively breaks NativeAOT support in .NET 10 Preview 1 for us.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-NativeAOT Native AOT compilation untriaged Request triage from a team member labels Feb 12, 2025
rolfbjarne added a commit to xamarin/xamarin-macios that referenced this issue Feb 12, 2025
…tually publishing."

This reverts commit b1c05cc.

It's a dotnet/sdk issue: dotnet/sdk#46790
@sbomer
Copy link
Member

sbomer commented Feb 13, 2025

In the past we've automatically set PublishAotUsingRuntimePack=true (always, not only for publish), which I believe we're still supposed to do.

From what I can tell, it's set always because we want to avoid downloading the target ILCompiler pack during build.

How does ios avoid the situation where we run ILC over the coreclr runtime pack? I think by setting NativeCompilationDuringPublish=false, it avoids that issue. So maybe we should fix this by adding a condition on NativeCompilationDuringPublish.

@ivanpovazan
Copy link
Member

First of all, my apologies for not trying out Sven's change with NativeAOT iOS builds.

Which got me wondering, how did we catch this so late - the SDK change was a month ago?

Based on our discussions around NativeAOT runtime packs and running build vs publish targets, could we fix this by adding a condition '$(_IsPublishing)' == 'true' on:
https://github.com/xamarin/xamarin-macios/blob/a50292c2cad1cffc51ef02d4fe8c3d66ef19c5b4/dotnet/targets/Xamarin.Shared.Sdk.props#L192

@sbomer
Copy link
Member

sbomer commented Feb 13, 2025

Based on our discussions around NativeAOT runtime packs and running build vs publish targets, could we fix this by adding a condition '$(_IsPublishing)' == 'true' on:

I had the same thought, but found this comment: xamarin/xamarin-macios@b4975e2#diff-2e24f1b6f44df7580244d00f106fa1a9e2257536fbca9d7bbdc74571549324a9R479-R480. I interpreted this to mean that we want to set it to true even when not publishing, to avoid downloading the target ILCompiler pack during build. Maybe @filipnavara can comment on it.

@filipnavara
Copy link
Member

filipnavara commented Feb 13, 2025

Oh, wow, that's an old workaround, so my memory is a bit fuzzy on that one.

The problem was that specifying just PublishAot=true failed very early in the restore phase when NuGet/SDK/MSBuild downloads the runtime packs. There are no ILCompiler packs for iOS hosts which is what would the SDK normally look for when targeting ios-arm64/iossimulator-arm64 RIDs (and the download logic was not conditioned on _IsPublishing=true). To workaround that we had to unconditionally set PublishAotUsingRuntimePack=true to make the build work at all (changes the logic to look for target runtime packs instead of target ILCompiler packs). It wasn't meant to avoid downloading additional assets, it was meant to avoid attempt to download assets that never exist.

Perhaps the workaround is not needed anymore (ie. SDK would not try to download non-existent ILCompiler when _IsPublishing!=true).

@sbomer
Copy link
Member

sbomer commented Feb 13, 2025

Thanks for the context! I believe SDK will still try to download the non-existing target ILC pack unless PublishAotUsingRuntimePack is true. So I think we have to keep it set for build as well.

Why does ios invoke the publish targets during build? Could they be changed to avoid depending on the SDK's built-in publish targets? That would avoid this issue entirely - building for ios is running publish targets in a non-standard way that breaks here.

If that's not feasible, I can see two other options:

@sbomer
Copy link
Member

sbomer commented Feb 13, 2025

Why does ios invoke the publish targets during build? Could they be changed to avoid depending on the SDK's built-in publish targets? That would avoid this issue entirely - building for ios is running publish targets in a non-standard way that breaks here.

Copying @rolfbjarne's answer from another conversation:

PrepareForPublish runs because ComputeFilesToPublish depends on it, and we call ComputeFilesToPublish during the build to figure out which files we need to copy to the final app bundle

I believe these SDK targets were never designed to be used this way (when not actually publishing). There's a good chance we'll run into similar issues if the SDK adds more consistency checks to related targets. But it looks like fixing this would be a larger change, so I suggest we take #46839 for now. @ivanpovazan would you be able to help validate the fix?

@agocke
Copy link
Member

agocke commented Feb 13, 2025

From what I can tell, it's set always because we want to avoid downloading the target ILCompiler pack during build.

Can someone elaborate on this? The design of the SDK is that anything that needs to be used should be downloaded during initial restore. In the future I expect a lot of CI builds will enforce this -- network access will be cut off after doing a restore. Shouldn't we always download all the runtime packs we might need later? Even if they're not used immediately?

@sbomer
Copy link
Member

sbomer commented Feb 13, 2025

avoid downloading the target ILCompiler pack during build.

This is about the target ILCompiler pack specifically, not the nativeaot runtime pack. That one should never be downloaded when targeting ios. (I think the nativeaot runtime pack should get downloaded during dotnet build as usual.)

@rolfbjarne
Copy link
Member Author

Which got me wondering, how did we catch this so late - the SDK change was a month ago?

I've been on vacation for most of that time, so nobody was around to see it.

Based on our discussions around NativeAOT runtime packs and running build vs publish targets, could we fix this by adding a condition '$(_IsPublishing)' == 'true' on:
xamarin/xamarin-macios@a50292c/dotnet/targets/Xamarin.Shared.Sdk.props#L192

No, that makes dotnet build fail with (when PublishAot=true is set in the csproj):

error NETSDK1203: Ahead-of-time compilation is not supported for the target runtime identifier 'iossimulator-arm64'.

Regarding this part:

PrepareForPublish runs because ComputeFilesToPublish depends on it, and we call ComputeFilesToPublish during the build to figure out which files we need to copy to the final app bundle

I believe these SDK targets were never designed to be used this way (when not actually publishing). There's a good chance we'll run into similar issues if the SDK adds more consistency checks to related targets. But it looks like fixing this would be a larger change, so I suggest we take #46839 for now. @ivanpovazan would you be able to help validate the fix?

This comes from here: https://github.com/xamarin/xamarin-macios/blob/676fe9f33bb288a80e960a251345e05b5e78bcee/dotnet/targets/Xamarin.Shared.Sdk.targets#L251-L271

This is from the beginning of implementing iOS support in .NET 5, so it's been there for quite a while.

IIRC the problem here is that we need "dotnet build" to build something self-contained, which afaik "dotnet build" isn't designed to do - but "dotnet publish" is. So when I implemented .NET support for Apple platforms in .NET 5, I ended up picking parts of "dotnet publish" (such as ComputeFilesToPublish) that would do what we needed.

FWIW for us there's no real difference in the build logic between "dotnet build" and "dotnet publish" (until NativeAOT came along at least) - "dotnet publish" is literally just "dotnet build" + zipping up the results to create an .ipa.

@ivanpovazan
Copy link
Member

On a slightly different but related note, and regarding:

No, that makes dotnet build fail with (when PublishAot=true is set in the csproj):

error NETSDK1203: Ahead-of-time compilation is not supported for the target runtime identifier 'iossimulator-arm64'.

This means that the current state of #37872 wouldn't really help as it sets a default for PublishAotUsingRuntimePack only when '$(_IsPublishing)' == 'true'?

@filipnavara
Copy link
Member

This means that the current state of #37872 wouldn't really help as it sets a default for PublishAotUsingRuntimePack only when '$(_IsPublishing)' == 'true'?

The point of that PR was primarily to get other platforms on the same plan, ie. separate ILCompiler pack on host and target runtime pack. The endgame plan was to remove the runtime bits from the ILCompiler packs because now we ship them at two places.

There was some earlier conversation on whether _IsPublishing should be part of the condition or not, but that got lost in time. There's also dotnet/runtime#111876 which seems to go in the same direction but from a different angle.

@rolfbjarne
Copy link
Member Author

On a slightly different but related note, and regarding:

No, that makes dotnet build fail with (when PublishAot=true is set in the csproj):

error NETSDK1203: Ahead-of-time compilation is not supported for the target runtime identifier 'iossimulator-arm64'.

This means that the current state of #37872 wouldn't really help as it sets a default for PublishAotUsingRuntimePack only when '$(_IsPublishing)' == 'true'?

Looks that way, yes.

rolfbjarne added a commit to xamarin/xamarin-macios that referenced this issue Feb 14, 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 a pull request may close this issue.

5 participants