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] Update ILCompiler paths to handle PublishAotUsingRuntimePack=true #85996

Closed

Conversation

filipnavara
Copy link
Member

When using PublishAotUsingRuntimePack we need to update the logic to get native libraries from the runtime pack, and not to replace the framework files with the host ILCompiler ones.

This PR is minimal attempt that does the job but we can do better. There's a lot of logic for the framework file substitution that is not necessary when using the runtime pack since we already start with the correct set of files.

cc @akoeplinger

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label May 9, 2023
@ghost
Copy link

ghost commented May 9, 2023

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

Issue Details

When using PublishAotUsingRuntimePack we need to update the logic to get native libraries from the runtime pack, and not to replace the framework files with the host ILCompiler ones.

This PR is minimal attempt that does the job but we can do better. There's a lot of logic for the framework file substitution that is not necessary when using the runtime pack since we already start with the correct set of files.

cc @akoeplinger

Author: filipnavara
Assignees: -
Labels:

community-contribution, area-NativeAOT-coreclr

Milestone: -

@filipnavara filipnavara marked this pull request as ready for review May 16, 2023 15:19
@filipnavara filipnavara requested a review from akoeplinger May 16, 2023 15:24
<ItemGroup Condition="'$(PublishAotUsingRuntimePack)' == 'true'">
<PrivateSdkAssemblies Include="$(IlcFrameworkSdkPath)*.dll"/>
<FrameworkAssemblies Include="@(RuntimePackAsset)" Condition="'%(Extension)' == '.dll'" />
<DefaultFrameworkAssemblies Include="@(FrameworkAssemblies)" />
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't PrivateSdkAssemblies included in DefaultFrameworkAssemblies in this case?

Copy link
Member Author

@filipnavara filipnavara May 19, 2023

Choose a reason for hiding this comment

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

It's not exactly intuitive but RuntimePackAsset already contains the files that end up in PrivateSdkAssemblies, so FrameworkAssemblies already contains the union of the two sets.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, FrameworkAssemblies itself is not used beyond this point, so the additional assemblies included in it have no effect.

Copy link
Member

Choose a reason for hiding this comment

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

Also, FrameworkAssemblies itself is not used beyond this point, so the additional assemblies included in it have no effect.

Sorry, I got confused with this.
Aren't FrameworkAssemblies used through DefaultFrameworkAssemblies to populate IlcReference:

<IlcReference Include="@(DefaultFrameworkAssemblies)" />

Copy link
Member Author

@filipnavara filipnavara May 19, 2023

Choose a reason for hiding this comment

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

DefaultFrameworkAssemblies is used beyond the point and it's expected to contain the union of lib/net8.0/*.dll and native/*.dll from the runtime pack.

In the case of PublishAotUsingRuntimePack=false these come from two different source directories and get merged into one DefaultFrameworkAssemblies item group.

In the case of PublishAotUsingRuntimePack=true all the files in the DefaultFrameworkAssemblies item group come from the RuntimePackAsset item group which already contains both the lib/net8.0/*.dll files and the native/*.dll files.

I was just trying to clarify that FrameworkAssemblies in the PublishAotUsingRuntimePack=true case will also include PrivateSdkAssemblies (native/*.dll), while in the PublishAotUsingRuntimePack=false case it won't include those files. However, FrameworkAssemblies itself is not used beyond this point, so the distinction doesn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can just rewrite it as to make it clearer:

-      <FrameworkAssemblies Include="@(RuntimePackAsset)" Condition="'%(Extension)' == '.dll'" />
-      <DefaultFrameworkAssemblies Include="@(FrameworkAssemblies)" />
+      <DefaultFrameworkAssemblies Include="@(RuntimePackAsset)" Condition="'%(Extension)' == '.dll'" />

Copy link
Member

Choose a reason for hiding this comment

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

Thank you very much for the clarification!

Regarding your last comment:

I can just rewrite it as to make it clearer:

Would it make sense to keep having a single ItemGroup and just distinguish what gets included in DefaultFrameworkAssemblies based on PublishAotUsingRuntimePack value?

<ItemGroup>
<PrivateSdkAssemblies Include="$(IlcSdkPath)*.dll" />
<ItemGroup Condition="'$(PublishAotUsingRuntimePack)' == 'true'">
<PrivateSdkAssemblies Include="$(IlcFrameworkSdkPath)*.dll"/>
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to name this IlcPrivateSdkPath since that's where PrivateSdkAssemblies come from?

Suggested change
<PrivateSdkAssemblies Include="$(IlcFrameworkSdkPath)*.dll"/>
<PrivateSdkAssemblies Include="$(IlcPrivateSdkPath)*.dll"/>

@filipnavara
Copy link
Member Author

Superceded by #86652

@filipnavara filipnavara closed this Jun 5, 2023
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this pull request Jun 6, 2023
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this pull request Jun 6, 2023
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this pull request Jun 7, 2023
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this pull request Jun 9, 2023
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this pull request Jun 13, 2023
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this pull request Jun 15, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 5, 2023
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.

3 participants