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.

Sign up for GitHub

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

Jump to bottom

ILC should run after ComputeResolvedFilesToPublishList #108909

Open
sbomer opened this issue Oct 15, 2024 · 4 comments · May be fixed by #111514
Open

ILC should run after ComputeResolvedFilesToPublishList #108909

sbomer opened this issue Oct 15, 2024 · 4 comments · May be fixed by #111514
Assignees
@sbomer
Labels
area-NativeAOT-coreclr in-pr There is an active PR which will close this issue when it is merged
Milestone
8.0.x

Comments

@sbomer
Copy link
Member

sbomer commented Oct 15, 2024 •
edited
Loading

Native AOT hooks into the publish logic using BeforeTargets="ComputeResolvedFilesToPublishList":

runtime/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Publish.targets

Lines 20 to 22 in 44b7ba3

<Target Name="ComputeLinkedFilesToPublish"
BeforeTargets="ComputeResolvedFilesToPublishList"
DependsOnTargets="_ComputeAssembliesToCompileToNative;LinkNative">

However, this target has some conflict resolution that should be done before ILC runs:
https://github.com/dotnet/sdk/blob/456aa42f693f9ff022c7e5f9f7473e0dc5d3b2f4/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L542-L559

I noticed this in a local build where ILLink was picking a different version of an assembly than ILC (because ILLink runs after ComputeResolvedFilesToPublishList:
https://github.com/dotnet/sdk/blob/456aa42f693f9ff022c7e5f9f7473e0dc5d3b2f4/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L453-L456

The ilc.rsp file in my scenario has both:

-r:C:\Users\svbomer\.nuget\packages\microsoft.azure.amqp\2.6.7\lib\netstandard2.0\Microsoft.Azure.Amqp.dll
-r:D:\azure-amqp\bin\Release\Microsoft.Azure.Amqp\net9.0\Microsoft.Azure.Amqp.dll

ILC seems to use the first dll passed on the command-line.

Compare this with the PublishSelfContained output. During ComputeResolvedFilesToPublishList, the conflict is resolved and only the dll from the nuget package is removed, leaving the one from a local build. The local build is passed to ILLink.

Unfortunately I wasn't able to come up with a simple repro, but the project file looked like this:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net9.0</TargetFramework>
    <!-- <PublishTrimmed>true</PublishTrimmed>
    <PublishAot>true</PublishAot> -->
    <PublishSelfContained>true</PublishSelfContained>
    <TrimmerSingleWarn>false</TrimmerSingleWarn>
    <IsTestSupportProject>true</IsTestSupportProject>
    <_TrimmerDumpDependencies>true</_TrimmerDumpDependencies>
  </PropertyGroup>
  <ItemGroup>
    <ProjectReference Include="..\..\..\..\sdk\eventhub\Azure.Messaging.EventHubs.Processor\src\Azure.Messaging.EventHubs.Processor.csproj" />
    <ProjectReference Include="..\..\..\..\sdk\eventhub\Azure.Messaging.EventHubs\src\Azure.Messaging.EventHubs.csproj" />
    <ProjectReference Include="D:\azure-amqp\src\Microsoft.Azure.Amqp.csproj" />
    <TrimmerRootAssembly Include="Azure.Messaging.EventHubs.Processor" />
  </ItemGroup>
  <ItemGroup>
    <!-- Update this dependency to its latest, which has all the annotations -->
    <PackageReference Include="Microsoft.Extensions.Logging.Configuration" Version="8.0.0" />
  </ItemGroup>
</Project>

Binlogs available upon request.
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Oct 15, 2024
@MichalStrehovsky
Copy link
Member

We should ideally redo how the compiler is hooked up into publish in general. The way it's hooked up right now was inherited from CoreRT and never touched since. SDK is now allowed to know about PublishAot and we should use same mechanisms as PublishTrimmed and PublishSingleFile use (that the SDK knows about and special cases). It would fix other problems, like dotnet/sdk#40488.

@MichalStrehovsky MichalStrehovsky added this to the 10.0.0 milestone Oct 16, 2024
@MichalStrehovsky MichalStrehovsky removed the untriaged New issue has not been triaged by the area owner label Oct 16, 2024
@MichalStrehovsky MichalStrehovsky mentioned this issue Nov 2, 2024
Closed
@jkotas jkotas mentioned this issue Nov 15, 2024
Closed
@jamiewinder jamiewinder mentioned this issue Nov 16, 2024
Open
@agocke
Copy link
Member

agocke commented Nov 18, 2024

@sbomer Could you put this on your backlog? I think it's time to cleanup publishing for AOT and get this into a good state.

@agocke agocke modified the milestones: 10.0.0, 9.0.x, 8.0.x Nov 18, 2024
@jkotas
Copy link
Member

jkotas commented Nov 19, 2024 •
edited
Loading

I think it's time to cleanup publishing for AOT and get this into a good state.

For a servicing fix that addresses #109872, we may want to do a targeted change. E.g. take file versions into account in https://github.com/dotnet/runtime/blob/main/src/coreclr/tools/aot/ILCompiler.Build.Tasks/ComputeManagedAssembliesToCompileToNative.cs#L154 and let the file with higher version win.

@sbomer sbomer mentioned this issue Nov 19, 2024
Merged
@sbomer
Copy link
Member Author

sbomer commented Jan 7, 2025

There are at least five places that can potentially do conflict resolution in a native AOT publish:

  • GenerateBuildDependencyFile
  • _HandlePackageFileConflicts
  • _HandlePackageFileConflictsForPublish
  • ILC's conflict resolution (ComputeManagedAssembliesToCompileToNative)
  • ResolveOverlappingItemGroupConflicts (during ComputeResolvedFilesToPublishList)

The goal is for ILC's special handling to go away and to use the built-in conflict resolution. There are multiple issues here:

  1. ILC hooks into publish in a way that bypasses ComputeResolvedFilesToPublishList (this issue). For this we need to change some of the build integration logic.
  2. ILC doesn't get the built-in conflict resolution for runtime packs (IIRC this is done in GenerateBuildDependencyFile). For this we need to switch to using runtime packs. This is already supported, but we might want to make it the default.

@sbomer sbomer linked a pull request Jan 16, 2025 that will close this issue
Open
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sbomer sbomer

Labels
area-NativeAOT-coreclr in-pr There is an active PR which will close this issue when it is merged
Projects
AppModel
Status: No status
Milestone
8.0.x
Development

Successfully merging a pull request may close this issue.

Run ILC after ComputeResolvedFilesToPublishList sbomer/runtime
4 participants
@agocke @sbomer @jkotas @MichalStrehovsky