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

Error out if SelfContained is not specified for Native AOT publish #95496

Merged
merged 1 commit into from
Dec 6, 2023
Merged

Error out if SelfContained is not specified for Native AOT publish #95496

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 2 additions & 3 deletions src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,8 @@ Copyright (c) .NET Foundation. All rights reserved.
<!-- The defaults currently root non-framework assemblies, which
is a no-op for portable apps. If we later support more ways
to customize the behavior we can allow linking portable apps
in some cases. If we're not running ILLink because e.g. this
is a NativeAOT app, value of SelfContained doesn't matter. -->
<NETSdkError Condition="'$(RunILLink)' != 'false' And '$(SelfContained)' != 'true'" ResourceName="ILLinkNotSupportedError" />
in some cases. -->
<NETSdkError Condition="'$(SelfContained)' != 'true'" ResourceName="ILLinkNotSupportedError" />
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change, right? A command that didn't fail before will now.

Copy link
Member

@eerhardt eerhardt Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This can lead to a bad failure mode because SelfContained is the thing that ensure ILC gets all the references to assemblies it needs.

Can we instead base this off of SelfContained OR PublishSelfContained? If either of those are set, ensure ILC gets all the references to assemblies it needs.

Or just completely deprecate SelfContained since there is so much confusion and broken things here. See also

dotnet/sdk#32272
dotnet/sdk#32277

Re-reading those issues again, I don't think we should be making this change (doubling down on the problematic behavior I listed in dotnet/sdk#32272). I really think we should be respecting PublishSelfContained as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a breaking change, right? A command that didn't fail before will now

It's breaking for people who run the publish target without _Is Publishing being specified and they only use the vanilla framework. If they use e.g. Asp.net they are already broken today, but the failure mode is obscure and it takes multiple rounds of email to troubleshoot (because the customer will insist they "publish" when in fact they msbuild the publish target).

I really don't care what the fix is, but I don't want to troubleshoot this again. If someone can remove this behavior in the sdk instead, we can just fully delete this line. I don't understand how this works in the sdk.

Cc @nagilson @dsplaisted

Copy link
Member

Choose a reason for hiding this comment

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

I really don't care what the fix is

Why not take my suggested fix then?

Check both SelfContained and PublishSelfContained to ensure ILC gets all the references to assemblies it needs.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean update all places in the sdk that check self contained to also check publishselfcontained? I don't even know why we have two in the first place...

Copy link
Member Author

Choose a reason for hiding this comment

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

Read those 2 issues. I don't know why either.

It looks like a hard fix that someone who understands the sdk will need to fix. This aligns aot with trimmed and we're no worse off. The error cmessage ould be improved but it's correct in principle.

Copy link
Member

Choose a reason for hiding this comment

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

This aligns aot with trimmed and we're no worse off. The error message could be improved but it's correct in principle.

Agreed. I also agree in principle that checking SelfContained and PublishSelfContained would be great, but that would be a larger fix in the SDK for the issues @eerhardt linked, and isn't specific to AOT. I think this change moves us in the right direction.

Copy link
Member

Choose a reason for hiding this comment

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

and we're no worse off

That's not true for someone who is using the current behavior of dotnet msbuild /t:Publish.

It looks like a hard fix that someone who understands the sdk will need to fix.

Why? I don't understand what is hard about the fix. In the place that says if (SelfContained) { AddAllAssemblies(); }, change it to be if (SelfContained || PublishSelfContained) { AddAllAssemblies(); }. The problem on those issues is that the conversation escalated to the larger "why do we have both?" conversation, which never got satisified answers.

Copy link
Member

Choose a reason for hiding this comment

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

I think this change moves us in the right direction.

I understand that this change makes PublishAot consistent with PublishTrimmed. But it makes it consistent with the wrong behavior. It is pointless to tell the user "Did you set SelfContained?" when SelfContained should be the default when setting PublishAot. The UX is bad here. If you want to be consistent, we should also fail if the user doesn't manually set the RID. And if they don't manually set PublishTrimmed and PublishSingleFile when setting PublishAot.

As a user, I should just need to set a single property: PublishAot, and all these other things (SelfContained, RID, PublishTrimmed, PublishSingleFile, etc) are all defaulted to a value that works. I shouldn't have to set 2 properties. We had this conversation last year and came to that agreement. From @nagilson's emailed meeting notes of that meeting:

Here is the conclusion from the meeting.
For PublishReadyToRun, the default will not be SelfContained. This is a breaking change and will be conditioned on the new .NET 8 TFM.
For PublishSingleFile, PublishTrimmed, and PublishAot: PublishSelfContained will be the default. There will be no warning saying you should set SelfContained explicitly.

That meeting was motivated by this even larger discussion: dotnet/sdk#30104.

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 with you that since PublishAot implies PublishSelfContained, this scenario should be made to work out of the box. PublishSelfContained should work with msbuild /t:Publish.

I understand that this change makes PublishAot consistent with PublishTrimmed.

It's not just about consistency - given the current behavior, where SelfContained is what controls the publish behavior of the SDK, I think it is correct to fail here when SelfContained isn't set. This turns an unpredictable failure mode into a predictable one.

I would point to this as another instance of the "wrong behavior" caused by dotnet/sdk#32272, and use this to push for a fix. But I don't think it should block fixing this bug.

Copy link
Member

Choose a reason for hiding this comment

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

If we insist on making this change, the error message could use some work for this new scenario. Currently it reads:

Optimizing assemblies for size is not supported for the selected publish configuration

It may not be readily apparent to users that "Optimizing assemblies for size" means PublishAot in this case.

rolfbjarne marked this conversation as resolved.
Show resolved Hide resolved

<Warning Condition="'$(SuppressILLinkExplicitPackageReferenceWarning)' != 'true' And
'%(PackageReference.Identity)' == 'Microsoft.NET.ILLink.Tasks' And '%(PackageReference.IsImplicitlyDefined)' != 'true'"
Expand Down