[NativeAOT] Update ILCompiler paths to handle PublishAotUsingRuntimePack=true #85996
Conversation
ghost
added
area-NativeAOT-coreclr
community-contribution
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsWhen using 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
|
| <ItemGroup Condition="'$(PublishAotUsingRuntimePack)' == 'true'"> | ||
| <PrivateSdkAssemblies Include="$(IlcFrameworkSdkPath)*.dll"/> | ||
| <FrameworkAssemblies Include="@(RuntimePackAsset)" Condition="'%(Extension)' == '.dll'" /> | ||
| <DefaultFrameworkAssemblies Include="@(FrameworkAssemblies)" /> |
There was a problem hiding this comment.
Why aren't PrivateSdkAssemblies included in DefaultFrameworkAssemblies in this case?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, FrameworkAssemblies itself is not used beyond this point, so the additional assemblies included in it have no effect.
There was a problem hiding this comment.
Also,
FrameworkAssembliesitself 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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'" />There was a problem hiding this comment.
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"/> |
There was a problem hiding this comment.
Would it make sense to name this IlcPrivateSdkPath since that's where PrivateSdkAssemblies come from?
| <PrivateSdkAssemblies Include="$(IlcFrameworkSdkPath)*.dll"/> | |
| <PrivateSdkAssemblies Include="$(IlcPrivateSdkPath)*.dll"/> |
|
Superceded by #86652 |
ghost
locked as resolved and limited conversation to collaborators
3 participants
When using
PublishAotUsingRuntimePackwe 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