-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Clean up AOT publish process #73416
Clean up AOT publish process #73416
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I noticed one thing I missed in #72346 - I think file Microsoft.DotNet.ILCompiler.targets should be renamed to not match the package name. Otherwise with a packagereference, those targets will be imported twice, once via nuget, and once by the SDK.
On the SDK side, I think the PublishAot condition should also be removed here: https://github.com/dotnet/sdk/blob/e8dc19097ac6b20ba1c8fc87500251e815e5e984/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L1166
src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets
Outdated
Show resolved
Hide resolved
…iler.targets Co-authored-by: Sven Boemer <sbomer@gmail.com>
…iler.targets Co-authored-by: Sven Boemer <sbomer@gmail.com>
src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets
Outdated
Show resolved
Hide resolved
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.SingleEntry.targets
Outdated
Show resolved
Hide resolved
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
This reverts commit 06339dc.
<PropertyGroup> | ||
<!-- Set the publishAot property to true if not set--> | ||
<PublishAot Condition="'$(PublishAot)' == ''">true</PublishAot> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand it correctly, this was needed because we do the ".targets included from a .props file" antipattern - I think we discussed this in one of the past pull request - to do the real fix, we likely need to break up Microsoft.DotNet.ILCompiler.targets into Microsoft.DotNet.ILCompiler.targets and Microsoft.DotNet.ILCompiler.props as needed.
(The convention is that .props are included before the user project, .targets get included after the user provided stuff - we would see PublishAot being set there if the user wants it.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the stuff that is already in .targets should be in .targets. We wouldn't probably have much (if anything?) in the props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's because we are importing targets from props (if we are, that should probably be fixed) - this was needed to preserve the OOB package behavior where the packagereference was all that was needed to turn on AOT publish (you didn't need to set PublishAot explicitly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right. I saw .targets in a .props and I thought it's an Import
, but it's just storing it in a property for later.
Fixes #72415, the plan is in the comments.