Skip to content

Native AOT runtime changes for multiple package conflict #72346

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

Merged
merged 2 commits into from
Jul 19, 2022
Merged

Native AOT runtime changes for multiple package conflict #72346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7658fd8
Native AOT runtime changes for multiple package conflict
LakshanF Jul 17, 2022
1b04f5b
Partial FB fixes
LakshanF Jul 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<!--
***********************************************************************************************
Microsoft.DotNet.ILCompiler.props

WARNING: DO NOT MODIFY this file unless you are knowledgeable about MSBuild and have
created a backup copy. Incorrect changes to this file will make it
impossible to load or build your projects from the command-line or the IDE.

Copyright (c) .NET Foundation. All rights reserved.
***********************************************************************************************
-->
<Project>

<PropertyGroup>
<!-- N.B. The ILCompilerTargetsPath is used as a sentinel to indicate a version of this file has already been imported. It will also be the path
used to import the targets later in the SDK. -->
<ILCompilerTargetsPath>$(MSBuildThisFileDirectory)Microsoft.DotNet.ILCompiler.targets</ILCompilerTargetsPath>
</PropertyGroup>
</Project>
14 changes: 7 additions & 7 deletions src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,21 +41,21 @@

<!-- Locate the runtime package according to the current target runtime -->
<Target Name="ImportRuntimeIlcPackageTarget" Condition="'$(BuildingFrameworkLibrary)' != 'true' AND $(IlcCalledViaPackage) == 'true'" DependsOnTargets="$(ImportRuntimeIlcPackageTargetDependsOn)" BeforeTargets="Publish">
Copy link
Member

Choose a reason for hiding this comment

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

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.

<!-- 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" />
<!-- This targets file is imported by the SDK when the AotRuntimePackageLoadedViaSDK property is set. SDK resolves runtime package paths differently-->
<Error Condition="'@(ResolvedILCompilerPack)' == '' AND '$(AotRuntimePackageLoadedViaSDK)' == 'true'" Text="The ResolvedILCompilerPack ItemGroup is required for target ImportRuntimeIlcPackageTarget" />
Copy link
Member

Choose a reason for hiding this comment

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

How is AotRuntimePackageLoadedViaSDK different from IlcCalledViaPackage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue to track this, #72415

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

<!-- NativeAOT runtime pack assemblies need to be defined to avoid the default CoreCLR implementations being set as compiler inputs -->
<Error Condition="'@(PackageDefinitions)' == '' AND '$(PublishAot)' != 'true'" Text="The PackageDefinitions ItemGroup is required for target ImportRuntimeIlcPackageTarget" />
<Error Condition="'@(PackageDefinitions)' == '' AND '$(AotRuntimePackageLoadedViaSDK)' != 'true'" Text="The PackageDefinitions ItemGroup is required for target ImportRuntimeIlcPackageTarget" />


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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue to track this, #72415

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

<IlcHostPackagePath>@(ResolvedILCompilerPack->'%(PackageDirectory)')</IlcHostPackagePath>
<IlcPath>>@(ResolvedILCompilerPack->'%(PackageDirectory)')</IlcPath>
</PropertyGroup>

<!-- Use the non-SDK runtime package resolve property to set down stream properties if SDK is not involved -->
<PropertyGroup Condition="'$(PublishAot)' != 'true'">
<!-- Use the non-SDK runtime package resolve property to set down stream properties if there is an explicit reference in the project -->
<PropertyGroup Condition="'$(AotRuntimePackageLoadedViaSDK)' != 'true'">
<RuntimePackagePath Condition="'%(PackageDefinitions.Name)' == '$(RuntimeIlcPackageName)'">%(PackageDefinitions.ResolvedPath)</RuntimePackagePath>
<IlcHostPackagePath Condition="'%(PackageDefinitions.Name)' == '$(IlcHostPackageName)'">%(PackageDefinitions.ResolvedPath)</IlcHostPackagePath>
</PropertyGroup>
Expand Down
2 changes: 1 addition & 1 deletion src/installer/pkg/projects/Microsoft.DotNet.ILCompiler/Microsoft.DotNet.ILCompiler.pkgproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
<ItemGroup>
<File Include="$(CoreCLRBuildIntegrationDir)*" TargetPath="build" />
<File Include="$(CoreCLRILCompilerDir)netstandard\*" TargetPath="tools/netstandard" />
<File Include="sdk\Sdk.targets" TargetPath="Sdk" />
<File Include="sdk\Sdk.props" TargetPath="Sdk" />
</ItemGroup>
</Target>

Expand Down
9 changes: 7 additions & 2 deletions ...crosoft.DotNet.ILCompiler/sdk/Sdk.targets → ...Microsoft.DotNet.ILCompiler/sdk/Sdk.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<!--
***********************************************************************************************
Sdk.targets
Sdk.props

WARNING: DO NOT MODIFY this file unless you are knowledgeable about MSBuild and have
created a backup copy. Incorrect changes to this file will make it
Expand All @@ -11,7 +11,12 @@ Copyright (c) .NET Foundation. All rights reserved.
-->
<Project>

<!-- Set a flag to indicate that aot packages are loaded via SDK -->
<PropertyGroup>
<AotRuntimePackageLoadedViaSDK Condition="'$(ILCompilerTargetsPath)'== ''">true</AotRuntimePackageLoadedViaSDK>
</PropertyGroup>

<!-- 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)' == ''" />
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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,


</Project>