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 NativeAOT runtime pack build for iOS/tvOS/Catalyst #85047

Merged
merged 2 commits into from
Apr 21, 2023

Conversation

akoeplinger
Copy link
Member

Contributes to #81024

This is the first part that adds runtime packs to the build.

@akoeplinger akoeplinger force-pushed the nativeaot-runtimepack branch from a7626ef to fb651ae Compare April 19, 2023 14:04
@jkotas
Copy link
Member

jkotas commented Apr 19, 2023

What is going to be the name of this runtime pack?

@steveisok
Copy link
Member

steveisok commented Apr 19, 2023

What is going to be the name of this runtime pack?

Similar to how we include mono in the runtime packs. Microsoft.NETCore.App.Runtime.NativeAOT.<os>-<arch>

@steveisok
Copy link
Member

Copy link
Member

@steveisok steveisok left a comment

Choose a reason for hiding this comment

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

Looks good!

@jkotas
Copy link
Member

jkotas commented Apr 19, 2023

Similar to how we include mono in the runtime packs. Microsoft.NETCore.App.Runtime.NativeAOT.-

Is the AOT compiler going to stay in Microsoft.DotNet.ILCompiler?

I am wondering whether it would make sense to do the same change for all hosts and targets.

Comment on lines +187 to +190
<PlatformManifestFileEntry Include="System.Private.DisabledReflection.dll" />
<PlatformManifestFileEntry Include="System.Private.Reflection.Execution.dll" />
<PlatformManifestFileEntry Include="System.Private.StackTraceMetadata.dll" />
<PlatformManifestFileEntry Include="System.Private.TypeLoader.dll" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these needed? I thought NativeAOT ships as native binary?
Is System.Private.Corelib.dll missing in the list?

Copy link
Member

Choose a reason for hiding this comment

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

These are things that could be in CoreLib but since it doesn't cost anything to have them out of CoreLib, we keep them out of CoreLib. We folded some into CoreLib in the past. We definitely need CoreLib too and it's already include above.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see so there is a circular dependency between these and SPC?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is System.Private.Corelib.dll missing in the list?

SPC is already included at line 74.

@steveisok
Copy link
Member

I am wondering whether it would make sense to do the same change for all hosts and targets.

I think it makes sense. Should we also change the name of the mono aot compiler packs? Right now it's Microsoft.NETCore.App.Runtime.AOT.<os>-<arch>.

@akoeplinger akoeplinger marked this pull request as ready for review April 21, 2023 12:18
@akoeplinger
Copy link
Member Author

Is the AOT compiler going to stay in Microsoft.DotNet.ILCompiler?
I am wondering whether it would make sense to do the same change for all hosts and targets.

@jkotas the AOT compiler is going to stay in Microsoft.DotNet.ILCompiler, or at least I don't see a reason why we should move it.

We can make the runtime pack change for all hosts/targets, I just started with the new platforms so the risk is lower.

@akoeplinger akoeplinger merged commit 54741bb into dotnet:main Apr 21, 2023
@akoeplinger akoeplinger deleted the nativeaot-runtimepack branch April 21, 2023 18:53
@ghost ghost locked as resolved and limited conversation to collaborators May 21, 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.

6 participants