Native AOT runtime changes for multiple package conflict #72346

LakshanF · 2022-07-17T22:51:33Z

Fixes dotnet/sdk#26268 where currently adding a ILCompiler package reference creates an application that doesn't work. We want to support this scenario where we expect developers to get the latest ILCompiler package (rather than use the SDK one, which can be a little older) when publishing, for example to pick a fix that is not yet present in the SDK.

Some notes

The fix is going to be in the SDK (Native AOT publish changes to be active after runtime changes sdk#26652) but requires these changes. The SDK PR will be in draft form until these changes are merged and flow into SDK. It is important to validate that the fix works due to the delay in syncing
There are 2 packages that are affected: microsoft.dotnet.ilcompiler deals primarily with setting up properties and targets and is handled similar to the way the linker manages conflicts with package references (see Move ILLink targets to the linker repo sdk#25993 and Move the ILLink targets from the SDK repo to the linker repo linker#2837)

SDK-ONLY: In the SDK, import the ILCompiler package, Sdk.props from Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.props

SDK-ONLY: ILCompiler's Sdk.props will only import Microsoft.DotNet.ILCompiler.props from the ILCompiler package build directory if a sentinel, ILCompilerTargetsPath, is not set. The SDK will also set a flag, AotRuntimePackageLoadedViaSDK, to indicate that the SDK has loaded the ILCompiler package if the sentinel is not previously set.

Both SDK and Nuget: Microsoft.DotNet.ILCompiler.props contains a sentinel, ILCompilerTargetsPath, that sets the path to Microsoft.DotNet.ILCompiler.targets. We assume that if there is an explicit package reference to the ILCompiler package, Nuget will set this sentinel and SDK will honor that. If there is no explicit package reference, then the SDK will set the sentinel

There will be only one path, the sentinel path, that will be used to set necessary properties and targets when there are multiple references to the ILCompiler package with this approach. We want to use the explicit package reference if the developer expresses the intent. Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets will load this sentinel and will honor the explicit package reference path if one is present.

The platform specific package (for example, runtime.win-x64.microsoft.dotnet.ilcompiler for the windows package) that contains ilc tool and runtime assets. The package reference path is now set based on a flag that gets sets if the SDK package is the only one used. Otherwise, the Nuget package one that comes from the explicit package reference in the app will be picked.
The publish AOT mechanism has evolved over the time and needs to be cleaned up. Issue [NativeAOT] Clean up AOT Publishing #72415 tracks this and should be done as a high priority since following the current code is painful

The original code was added to only support explicit package references

SDK support was added for PublishAot with backward compatibility support for the explicit package reference without the SDK path (for a short period)

This feature adds support to adding explicit package reference with SDK PublishAot. We are not planning to support the explicit package reference without the SDK path.

agocke · 2022-07-18T06:59:24Z

src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets

@@ -42,20 +42,20 @@
  <!-- Locate the runtime package according to the current target runtime -->
  <Target Name="ImportRuntimeIlcPackageTarget" Condition="'$(BuildingFrameworkLibrary)' != 'true' AND $(IlcCalledViaPackage) == 'true'" DependsOnTargets="$(ImportRuntimeIlcPackageTargetDependsOn)" BeforeTargets="Publish">
    <!-- This targets file is imported by the SDK when the PublishAot property is set. SDK resolves runtime package paths differently-->
-    <Error Condition="'@(ResolvedILCompilerPack)' == '' AND '$(PublishAot)' == 'true'" Text="The ResolvedILCompilerPack ItemGroup is required for target ImportRuntimeIlcPackageTarget" />
+    <Error Condition="'@(ResolvedILCompilerPack)' == '' AND '$(AotRuntimePackageLoadedViaSDK)' == 'true'" Text="The ResolvedILCompilerPack ItemGroup is required for target ImportRuntimeIlcPackageTarget" />


How is AotRuntimePackageLoadedViaSDK different from IlcCalledViaPackage?

To the best of my knowledge, AotRuntimePackageLoadedViaSDK, is intended to indicate ILCompiler package loaded mechanism and is a new flag. IlcCalledViaPackage seems to used to control the native targets and is used for the ILCompiler runtime package

Created an issue to track this, #72415

If I read this correctly, IlcCalledViaPackage no longer does what it suggests since it's set to true in these targets which get imported on both the SDK and package paths. Can we remove it?

IlcCalledViaPackage was introduced here: dotnet/corert#5123 - all the context we have left is in what's in the pull request/comments.

It was working around some issue around two situations:

The props/targets are included from the NuGet package (IlcCalledViaPackage == true) and

The props/targets are included directly (like we do in this repo when we build tests, or crossgen2)

There were things happening differently in those two scenarios. I'm not aware of us doing anything to make that not needed, but I also don't know much about what the original problem was.

agocke · 2022-07-18T07:01:54Z

src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets



    <!-- This targets file is imported by the SDK when the PublishAot property is set. Use the SDK runtime package resolve property to set  down stream properties -->
-    <PropertyGroup Condition="'$(PublishAot)' == 'true'">
+    <PropertyGroup Condition="'$(AotRuntimePackageLoadedViaSDK)' == 'true'">
      <RuntimePackagePath>@(ResolvedILCompilerPack->'%(PackageDirectory)')</RuntimePackagePath>


Can we somehow reuse this logic for the NuGet package as well? i.e., have the NuGet package set ResolvedILCompilerPack and use the SDK logic for restoring the runtime pack?

Is this suggestion as a best practice suggestion? This approach seems simpler since the Target ResolveFrameworkReferences that processes ResolvedILCompilerPack happens fairly early in the SDK. Setting the ILCompiler runtime package path as was done prior to the SDK support here seems to be working.

Created an issue to track this, #72415

I agree, I would prefer to have one code path that does the package resolution in both cases. Would it work to adjust KnownFrameworkReference to include a reference to the right version?

That would be my hope, but I've noticed that restore is weird and delicate, so it may not work how I would expect.

src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.props

sbomer · 2022-07-18T23:44:05Z

src/installer/pkg/projects/Microsoft.DotNet.ILCompiler/sdk/Sdk.props

  <!-- Only import the build props if the NativeAot package isn't referenced via NuGet. -->
-  <Import Project="$(MSBuildThisFileDirectory)..\build\Microsoft.DotNet.ILCompiler.targets" Condition="'$(PublishAot)' == 'true'"/>
+  <Import Project="$(MSBuildThisFileDirectory)..\build\Microsoft.DotNet.ILCompiler.props" Condition="'$(PublishAot)' == 'true' and '$(ILCompilerTargetsPath)' == ''" />


It looks like PublishAot was originally acting as a kind of sentinel, but presumably it was broken (since it looks like the SDK would always import the included targets when PublishAot was set, even with a nuget package reference). Can we remove the PublishAot condition now?

I wasn't sure if there is any way that the SDK path can be activated before the explicit ILCompiler package reference can set the sentinel since there are non-PublishAOT calls in the SDK that might set the sentinel.

So there are two concerns here,

If explicit ILCompiler package references always set the sentinel first, then its ok to remove the check here

There needs to be a guard for non-PublishAot SDK code paths to not import aot targets since Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.props and Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets might get invoked in non-PublishAot scenarios. I also added a check in the targets file before importing the native AOT target file and if the above concern is not valid, we can remove it from the props file

sbomer · 2022-07-18T23:48:38Z

src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets

@@ -42,20 +42,20 @@
  <!-- Locate the runtime package according to the current target runtime -->
  <Target Name="ImportRuntimeIlcPackageTarget" Condition="'$(BuildingFrameworkLibrary)' != 'true' AND $(IlcCalledViaPackage) == 'true'" DependsOnTargets="$(ImportRuntimeIlcPackageTargetDependsOn)" BeforeTargets="Publish">
    <!-- This targets file is imported by the SDK when the PublishAot property is set. SDK resolves runtime package paths differently-->
-    <Error Condition="'@(ResolvedILCompilerPack)' == '' AND '$(PublishAot)' == 'true'" Text="The ResolvedILCompilerPack ItemGroup is required for target ImportRuntimeIlcPackageTarget" />
+    <Error Condition="'@(ResolvedILCompilerPack)' == '' AND '$(AotRuntimePackageLoadedViaSDK)' == 'true'" Text="The ResolvedILCompilerPack ItemGroup is required for target ImportRuntimeIlcPackageTarget" />


If I read this correctly, IlcCalledViaPackage no longer does what it suggests since it's set to true in these targets which get imported on both the SDK and package paths. Can we remove it?

src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets

sbomer · 2022-07-18T23:53:39Z

src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets

@@ -42,20 +42,20 @@
  <!-- Locate the runtime package according to the current target runtime -->
  <Target Name="ImportRuntimeIlcPackageTarget" Condition="'$(BuildingFrameworkLibrary)' != 'true' AND $(IlcCalledViaPackage) == 'true'" DependsOnTargets="$(ImportRuntimeIlcPackageTargetDependsOn)" BeforeTargets="Publish">


ImportRuntimeIlcPackageTargetDependsOn needs to be updated (it looks like it was originally intended to be defined only when using the nuget package). We shouldn't need to run RunResolvePackageDependencies if we are resolving the compiler package from ResolvedILCompilerPack.

src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets

sbomer · 2022-07-18T23:56:12Z

src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets



    <!-- This targets file is imported by the SDK when the PublishAot property is set. Use the SDK runtime package resolve property to set  down stream properties -->
-    <PropertyGroup Condition="'$(PublishAot)' == 'true'">
+    <PropertyGroup Condition="'$(AotRuntimePackageLoadedViaSDK)' == 'true'">
      <RuntimePackagePath>@(ResolvedILCompilerPack->'%(PackageDirectory)')</RuntimePackagePath>


I agree, I would prefer to have one code path that does the package resolution in both cases. Would it work to adjust KnownFrameworkReference to include a reference to the right version?

sbomer

Functionally LGTM. My personal preference would be to clean up the old properties here, but it also seems fine to do that in a follow-up if you want to unblock this change. Thank you!

kasperk81 · 2022-07-24T01:55:11Z

this is blocking sdk flow dotnet/sdk#26715

and is it really needed by even 1% of the users? it is disconnected from everything else integrated into the sdk. crossgen integration in sdk is shipped without this hook and no one has ever asked or complained about it. we wait for next release of .net for all the new features and improvements together, if someone need to try out the latest dev version once in a while. they download the daily build which takes no more than a few seconds to download than the runtime pack..

jkotas · 2022-07-24T02:59:54Z

@kasperk81 Both linker and Roslyn have similar hooks. Yes, we need this hook to be successful with customers that we are closely working with.

crossgen does not have this hook since it is tightly coupled with the runtime pack and so it does not make sense to have a separate hook for it.

they download the daily build which takes no more than a few seconds to download than the runtime pack.

The SDK version is often controlled in customer build environments. Requiring them to update the SDK to pickup or validate a bug-fix is a non-starter.

Native AOT runtime changes for multiple package conflict

7658fd8

LakshanF requested a review from MichalStrehovsky as a code owner July 17, 2022 22:51

dotnet-issue-labeler bot added the area-NativeAOT-coreclr label Jul 17, 2022

ghost assigned LakshanF Jul 17, 2022

LakshanF requested review from sbomer and agocke July 17, 2022 22:52

agocke reviewed Jul 18, 2022

View reviewed changes

sbomer reviewed Jul 18, 2022

View reviewed changes

LakshanF mentioned this pull request Jul 19, 2022

[NativeAOT] Clean up AOT Publishing #72415

Closed

Partial FB fixes

1b04f5b

sbomer approved these changes Jul 19, 2022

View reviewed changes

LakshanF merged commit 901bbbc into dotnet:main Jul 19, 2022

LakshanF deleted the AotMultiplePkgFix branch July 19, 2022 22:16

ghost locked as resolved and limited conversation to collaborators Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native AOT runtime changes for multiple package conflict #72346

Native AOT runtime changes for multiple package conflict #72346

LakshanF commented Jul 17, 2022 •

edited

Loading

agocke Jul 18, 2022

LakshanF Jul 18, 2022

LakshanF Jul 18, 2022

sbomer Jul 18, 2022

MichalStrehovsky Jul 19, 2022

agocke Jul 18, 2022

LakshanF Jul 18, 2022

LakshanF Jul 18, 2022

sbomer Jul 18, 2022

agocke Jul 19, 2022

sbomer Jul 18, 2022

LakshanF Jul 19, 2022

sbomer Jul 18, 2022

sbomer Jul 18, 2022

sbomer Jul 18, 2022

sbomer left a comment

kasperk81 commented Jul 24, 2022

jkotas commented Jul 24, 2022

		@@ -42,20 +42,20 @@
		<!-- Locate the runtime package according to the current target runtime -->
		<Target Name="ImportRuntimeIlcPackageTarget" Condition="'$(BuildingFrameworkLibrary)' != 'true' AND $(IlcCalledViaPackage) == 'true'" DependsOnTargets="$(ImportRuntimeIlcPackageTargetDependsOn)" BeforeTargets="Publish">

Native AOT runtime changes for multiple package conflict #72346

Native AOT runtime changes for multiple package conflict #72346

Conversation

LakshanF commented Jul 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbomer left a comment

Choose a reason for hiding this comment

kasperk81 commented Jul 24, 2022

jkotas commented Jul 24, 2022

LakshanF commented Jul 17, 2022 •

edited

Loading