Error out if SelfContained is not specified for Native AOT publish

[MichalStrehovsky]

Fixes a 1st party customer reported issue.

The SDK has logic to specify SelfContained for PublishAot automatically. Except this doesn't apply when we're using msbuild to run the Publish target (_IsPublishing is not set): https://github.com/dotnet/sdk/blob/75184c7e763197fa2971954aa5baf70ffd188799/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets#L64-L83

This can lead to a bad failure mode because SelfContained is the thing that ensure ILC gets all the references to assemblies it needs. It will appear to work most of the time even without SelfContained (because ILCompiler packages carries a framework with it), but it will fail for things like ASP.NET. Not setting it was always a bug, it just didn't break for vanilla apps so it looked like it's unnecessary.

Enable back the logic that just errors out for this. If someone does msbuild /t:publish they'll get an error saying they need to also specify SelfContained instead of some weird failure mode. There was some discussion about this aspect in dotnet/sdk#28714.

Cc @dotnet/ilc-contrib

[ghost]

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes a 1st party customer reported issue.

The SDK has logic to specify SelfContained for PublishAot automatically. Except this doesn't apply when we're using msbuild to run the Publish target (_IsPublishing is not set): https://github.com/dotnet/sdk/blob/75184c7e763197fa2971954aa5baf70ffd188799/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets#L64-L83

This can lead to a bad failure mode because SelfContained is the thing that ensure ILC gets all the references to assemblies it needs. It will appear to work most of the time even without SelfContained (because ILCompiler packages carries a framework with it), but it will fail for things like ASP.NET. Not setting it was always a bug, it just didn't break for vanilla apps so it looked like it's unnecessary.

Enable back the logic that just errors out for this. If someone does msbuild /t:publish they'll get an error saying they need to also specify SelfContained instead of some weird failure mode. There was some discussion about this aspect in dotnet/sdk#28714.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	linkable-framework
Milestone:	-

[ghost]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes a 1st party customer reported issue.

The SDK has logic to specify SelfContained for PublishAot automatically. Except this doesn't apply when we're using msbuild to run the Publish target (_IsPublishing is not set): https://github.com/dotnet/sdk/blob/75184c7e763197fa2971954aa5baf70ffd188799/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets#L64-L83

This can lead to a bad failure mode because SelfContained is the thing that ensure ILC gets all the references to assemblies it needs. It will appear to work most of the time even without SelfContained (because ILCompiler packages carries a framework with it), but it will fail for things like ASP.NET. Not setting it was always a bug, it just didn't break for vanilla apps so it looked like it's unnecessary.

Enable back the logic that just errors out for this. If someone does msbuild /t:publish they'll get an error saying they need to also specify SelfContained instead of some weird failure mode. There was some discussion about this aspect in dotnet/sdk#28714.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	MichalStrehovsky
Labels:	linkable-framework, area-NativeAOT-coreclr
Milestone:	-

[ivanpovazan]

/cc: @rolfbjarne

[eerhardt]

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

[eerhardt]

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.

[MichalStrehovsky]

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

[eerhardt]

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.

[MichalStrehovsky]

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...

[MichalStrehovsky]

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.

[sbomer]

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.

[eerhardt]

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.

[eerhardt]

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.

[sbomer]

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.

[eerhardt]

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]

I think a step back is required:

Except this doesn't apply when we're using msbuild to run the Publish target (_IsPublishing is not set):

Is dotnet build /t:Publishsupposed to work like dotnet publish?

If so, then the _IsPublishing property should go away (if they're identical, then you shouldn't need a property to distinguish them...) - or be set in the -t:Publish case as well (which I believe was deemed unfeasible: dotnet/sdk#32272 (comment))

If not, then we should define what the difference is supposed to be.

IMHO the answer to "dotnet build /t:Publish doesn't work" is "use dotnet publish instead"... or alternatively say that "/t:Publish" requires passing "/p:_IsPublishing=true" as well

[MichalStrehovsky]

Please add an escape hatch, so that the Apple SDKs can opt out of this error:

Could you double check the PrepareForILLink target actually runs in this situation? This error is part of the native compilation targets and I would not expect the target to run when RuntimeIdentifier is empty (which is the condition in the quoted hack).

Is dotnet build /t:Publish supposed to work like dotnet publish?

Everyone commenting on the differences between running the publish target vs dotnet publish is commenting on the wrong place. @eerhardt listed several issues where this was discussed ad nauseam with the actual owners of this experience. This PR will not come to a conclusion because none of the SDK owners are involved in this. They are aware of the current situation based on the issues and that's the most I can do. I agree it's weird. Looks like owners say it's a hard problem. We're not going to fix this in the ILLink targets or this repo. If it's fixed, the line I'm changing can be deleted. I'm not adding new handling for this.

[MichalStrehovsky]

Could you double check the PrepareForILLink target actually runs in this situation? This error is part of the native compilation targets and I would not expect the target to run when RuntimeIdentifier is empty (which is the condition in the quoted hack).

@rolfbjarne could you confirm that the PrepareForILLink target runs when RuntimeIdentifier is empty and we need a hack escape hatch for this? I would rather not add the extra condition to this unless it's really needed - it doesn't look like it is.

[rolfbjarne]

Could you double check the PrepareForILLink target actually runs in this situation? This error is part of the native compilation targets and I would not expect the target to run when RuntimeIdentifier is empty (which is the condition in the quoted hack).

@rolfbjarne could you confirm that the PrepareForILLink target runs when RuntimeIdentifier is empty and we need a hack escape hatch for this? I would rather not add the extra condition to this unless it's really needed - it doesn't look like it is.

I applied this change locally and nothing broke, so it looks like it's good on our side.

[MichalStrehovsky]

I applied this change locally and nothing broke, so it looks like it's good on our side.

Thank you!

[radical]

NativeAOT jobs didn't run on this PR. Maybe they should be triggered for changes in src/tools/illink. We do that for wasm, for example.
cc @kg

[jkotas]

This seems to be causing widespread CI failures: #95712

[MichalStrehovsky]

NativeAOT jobs didn't run on this PR. Maybe they should be triggered for changes in src/tools/illink. We do that for wasm, for example. cc @kg

Yep, native AOT legs absolutely should run. Parts of the native AOT compiler are literally in this directory.

The fix should be in #95718 and I'll add this to triggers as well.

[ghost]

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.