Error out if SelfContained is not specified for Native AOT publish #95496
Conversation
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. 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.
ghost
added
the
linkable-framework
|
Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar Issue DetailsFixes 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 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 Cc @dotnet/ilc-contrib
|
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsFixes 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 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 Cc @dotnet/ilc-contrib
|
|
/cc: @rolfbjarne |
| 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" /> |
There was a problem hiding this comment.
This is a breaking change, right? A command that didn't fail before will now.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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" /> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Could you double check the
Everyone commenting on the differences between running the publish target vs |
@rolfbjarne could you confirm that the |
I applied this change locally and nothing broke, so it looks like it's good on our side. |
Thank you! |
|
NativeAOT jobs didn't run on this PR. Maybe they should be triggered for changes in |
|
This seems to be causing widespread CI failures: #95712 |
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. |
eerhardt
added
the
breaking-change
ghost
added
the
needs-breaking-change-doc-created
|
Added When you commit this breaking change:
Tagging @dotnet/compat for awareness of the breaking change. |
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
Publishtarget (_IsPublishingis not set): https://github.com/dotnet/sdk/blob/75184c7e763197fa2971954aa5baf70ffd188799/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets#L64-L83This 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:publishthey'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