-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
PublishTrimmed should imply self-contained #30104
Comments
@dotnet/linker-contrib a new issue has been filed in the ILLink area, please triage |
Surprised that The only thing I don't like is that these properties affect |
I think this is happening because of the default RID selection. PublishReadyToRun/PublishSingleFile/PublishAot use the current RID, and having a RID currently implies self-contained. sdk/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets Lines 79 to 92 in 201d719
But @nagilson is working on a breaking change where RID no longer implies self-contained: #30038. I expect that after this change, PublishAot will have an error similar to the current one for PublishTrimmed, and the PublishSingleFile default will become framework-dependent. The latter might violate user expectations, but at least will be consistent with the new defaults. |
@marcpopMSFT IIRC you set up the auto-tagging of @dotnet/linker-contrib based on the area labels. The team name changed to @dotnet/illink-contrib. Would you be able to update the bot config? |
Good topic! We're trying to provide coherent behaviors for some things that are similar and others less so. It's sometimes easier when we group the options such that the answer pops out. Let's see if that happens here. I'm going to use MSBuild syntax, where all properties are intended as Always framework-dependent:
Supports both:
Could support both, but only supports self-contained:
Always self-contained:
There are two ways we could go with this:
There are four groups. The pragmatic choice is make the first two groups fx-dependent by default and the second two are self-contained. Another observation is that
That approach would have multiple benefits:
This approach seems to balance CLI consistency with convenience while also most often providing users with a set of artifacts that we would say are "good". |
It's worth noting that
It seems like the same reasoning would apply to |
@richlander Based on my comment above, I want to retire |
A couple more things, actually.
I'm not sure I understand what you're suggesting here. You mean that if
The compat between self-contained single file and plain self-contained is getting pretty good at this point. It's pretty much just Assembly.Location now. |
That's a good point @sbomer. Let's go with that. I'm thinking about this more. I was trying to get at something with that categorization in my previous post, but I didn't quite know what. I do now. Let's try again. App characteristics (don't auto-switch from FDD -> SCD):
App kind (these auto switch to SCD):
This feels like a more useful categorization, but I still don't find it particularly satisfying. For example, In the ideal case, users set the minimum values in their project file, like setting I think this conversation comes down to specializing a default project file (like what templates produce) via the CLI. I think a solution like this would help: #26249 (comment). It's basically the same thing as specifying the same values in a project file. @agocke -- @agocke -- I meant that Upon further thinking, I think we should go with the categorization I have above and then rely on good project file and CLI gestures to make it easy to adopt the configuration you want. We just added |
Thanks everyone for engaging here + internally 😄 @richlander Should we still take this change: #30038 if it does not have If we have a finalized decision on this, we should close/edit this issue since we've seem to have decided against it, and make one specifically for having |
Right. I'm suggesting that I think all the other cases should not infer a |
Up-leveling a bit. We should be dissatisfied with experiences that are inconvenient. However, addressing those at the lowest levels (like with these properties) is just one option. We should ensure we have all the options on the table for how to address UX. I'm proposing we handle this UX concerns (like |
If we intend to handle UX concerns at a higher level (which as I understand means through project templates, or some other way to "bake in" the chosen options), would it make sense to treat From the internal discussion it also sounds like a FDD Then we can explain everything by saying that SCD is orthogonal to the other publish options. To use your terminology, SCD is an "app kind" and everything else is an "app characteristic" - where not all combinations are supported, but nothing is implicitly set (so no decoder ring is required). |
I don't think Another middling option is to expose another set of CLI flags, like |
The same can be said about PublishAot. (FDD was the default deployment option for .NET Native for UWP.) |
You are right! I forgot about that. However (and correct me if I'm wrong), it seems more likely that we'll enable trimming for FDD than Native Aot for FDD. Or, do we just need to wait on a similar "one motiving scenario"? |
I agree. |
That's a bit ambiguous. There were two things you could agree with. I assume you were agreeing that trimming for FDD was more likely. |
Yep. |
Here's a concrete proposal, since we still have time on our side.
I make no secret that I think this approach would be a good solution for this specific topic area and also have broader utility for even more common scenarios. |
As stated above there's a conflict between consistency and "ease of use". The above discussion makes it clear that we're proposing sacrifying "easy of use on CLI" and want consistent CLI options. (Personally I'm split on this, but if most people think that's the right approach, let's do it). With that, I don't see why we would special case Another way to look at it - if we think we can solve the UX for trimming outside of CLI, I don't see why we could not solve it the same way for AOT as well. And then keep the CLI 100% consistent. As already mentioned, we could introduce CLI-only aliases which solve the UX: |
I don't think making a breaking change for PublishSingleFile to default to framework-dependent is correct. Logically, I want my app in a single file. To me, making the default behavior being "the whole app in a single file" is the expected behavior. For PublishTrimmed, even if/when we support FDD with PublishTrimmed, the fact that we've only had self-contained for 4, going on 5, versions now should tell us what the logical default is. If the scenario isn't important enough to implement, it isn't important enough to be considered the "default". |
I agree that this is one of the biggest downsides to the breaking change (RID no longer implying self-contained). Same applies to vanilla "dotnet publish -r <RID>". However, I would find it really unexpected if "dotnet publish" produced one kind (SCD/FDD) of deployment, but "dotnet publish -p:PublishSingleFile=true" produced a different kind. So I'll throw another idea into the mix: what if we made Pros:
Cons:
I'm not sure how significant the "dotnet publish" break is. Personally I only ever do a RID-specific publish anyway (and almost always want it to be self-contained), but maybe my usage is not representative. |
Thoughts @richlander ? I think this is a good compromise with UX but also on the orthogonal properties perspective. |
I see significant diversity of opinion for FDD trimming on this thread on feasibility, value, and likelihood. I'd like to offer the options that Sven and I offered. Can we get behind those and choose one? FWIW, I'm happy with either so not specifically trying to push the one I suggested. It's all just tradeoffs. |
@richlander - can you explain why you are cutting the "Andy and Vitek" option from consideration?
My opinion is that even if FDD trimming was feasible, valuable, and likely, I would still want PublishAot, PublishTrimmed, and PublishSingleFile to be SCD by default. |
I was acting on Jan's feedback, which is that we shouldn't be optimizing for the CLI but for project files for setting these properties. I agree with that. If we're talking about project files, then a consistent model trumps reducing properties. |
I'm fine with going forward with FDD-by-default. |
BTW: A lot of this is my fault. I pushed implying SCD for some of the |
I propose we make that change in Preview 2 and then course correct as needed. Fair? That means all the That leaves what to do with |
We'll need a new command line argument then, and we'll need to change error messages |
That is, you'll get an error message from AOT and trimming saying you need to enable self-contained. We need to instead say, PublishSelfContained or |
I think we're getting at CLI and project file UX being different. The following seems fine to me: dotnet publish -p:PublishTrimmed --sc This seems like way too much: dotnet publish -p:PublishTrimmed -p:PublishSelfContained If we want to leave |
My preference is that we would change SelfContained to mean PublishSelfContained. If you want a self contained build, you'll need to execute a publish. Basically the same behavior, just behind a different verb. |
I see. This change would mean that we are for sure saying that the way to produce final prod apps with .NET is via |
It sounds like we're settling on a plan that is FDD by default always. That means introducing errors when aot, trimming, and singlefile correct? Would +1 to Andy's suggestion of SelfContained meaning PublishSelfContained Would we need to coordinate changes to the api -aot template to add the PublishSelfContained property? Are there any other templates that would need to be changed (do we have any way of searching the templates for any of the three Publish properties that are affected)? |
Mostly "yes" and great questions.
With this proposal, yes. We'd move that to
Yes. This actually aligns quite closely with the |
I've read all the reasoning here, and while I don't fault the logic, I find the fact that |
You are not wrong. That's why there were both "Sven" and "Rich" options. However, following Jan's logic, this should not be an actual problem. Fixup your project file and it will work. While my proposal is more pragmatic, I think Sven's proposal is perhaps stronger. We get to document that we offer exactly one model and the project files always look the same way. For example, SCD+Trimming look the same as SCD+AOT. That argument falls down a bit since some properties might infer trimming, but I'm choosing to ignore that. |
This seems like a major change breaking change. Maybe publish should be the way to get the final prod app, but I bet there are lots of people that don't do it that way today. One thing that springs to mind is Exe to Exe project references. It's somewhat of a niche scenario, but today you can have a self-contained app reference another self-contained app, and you can get a working copy of both in the output folder. This wouldn't work with publish, since when you publish a project, you don't publish its project references. |
It works fine though, you just need to pass I'd rather we have one, well-documented way of doing this. And since publish is our gesture going forward for app deployment, we should group this all under publish. |
Are you sure that works? There's a protocol for getting the files from a referenced project, and I don't think running the Publish target on the reference would mean that it would return the published files instead of the build output to the referencing project. |
I don't have a strong viewpoint on this topic. Perhaps the two of you can hash out a proposal? |
I don't have a ProjectReference sample handy so I can't test it, but I don't think it matters because I know this works: <MSBuild Projects="$(RepoRoot)src/coreclr/tools/aot/crossgen2/crossgen2.csproj"
Targets="Restore"
Properties="MSBuildRestoreSessionId=$([System.Guid]::NewGuid())
;_IsPublishing=true" />
<MSBuild Projects="$(RepoRoot)src/coreclr/tools/aot/crossgen2/crossgen2.csproj"
Targets="Publish;PublishItemsOutputGroup"
Properties="_IsPublishing=true">
<Output TaskParameter="TargetOutputs"
ItemName="_RawCrossgenPublishFiles" />
</MSBuild> And that's still a better alternative to using SelfContained ProjectReference. If we want to make sure Publish can work with ProjectReference I'd be fine with fixing it, but I'd also be fine with telling people to just use the MSBuild task. Again, one thing that works in all circumstances is better than multiple things that only work some of the time. |
@richlander @agocke @vitek-karas as part of this thread discusses the usefulness of FDD Trimming and the likelihood to see this feature implemented: I have a scenario where I would have to xcopy-deploy a relatively large set of relatively simple unpackaged WinUI apps. The C#/WinRT projections alone add ~20 MB to every single app. With CsWinRT 2.0, these projections are trimmable, and it would be great if FDD could benefit from this. @agocke are the challenges described in linker/issues/2609 applicable to my scenario as well, or do you think it would be easier to support? |
The issue dotnet/linker#2609 was actually motivated by WinUI projections assembly (it just doesn't say so specifically). But it's ultimately somewhat different problem. The expectation is that WinUI assemblies are simple in that they have only references to framework and not user code assemblies - so they act as "leafs" in the analysis in that sense (assuming FDD trimming like picture). So supporting trimming for these would probably be a bit easier, but I would expect it to hit most of the problems described in dotnet/linker#2139 regardless. The problems described in dotnet/linker#2609 are specifically to support a hybrid world where only some assemblies are trimmed AND not everything is analyzed. The trimmer already supports the case where not everything is trimmed, but it does assume that it will analyze everything. dotnet/linker#2609 tries to explore if it's possible to not analyze most of the app if only a small set of assemblies are to be trimmed. That so far proves to be rather difficult. |
From dotnet/runtime#95496 (comment):
Now that |
Retested with SDK v |
Doing the following steps:
dotnet new console
dotnet publish -p:PublishTrimmed=true
Results in the following error:
However, doing
dotnet publish -p:PublishAot=true
dotnet publish -p:PublishSingleFile=true
Just works without more command line args.
We should make
PublishTrimmed=true
implySelfContained=true
, just likePublishAot=true
andPublishSingleFile=true
does. That way users don't need to pass extra args on the command line, just to make this work.If in the future we want to support a mode where
PublishTrimmed=true
, butSelfContained=false
, those customers can explicitly--self-contained false
. This is consistent with the currentPublishSingleFile=true
behavior. By default it is self-contained. And you can opt into Framework Dependent Deployment withdotnet publish -p:PublishSingleFile=true --self-contained false
.cc @vitek-karas @sbomer @agocke
The text was updated successfully, but these errors were encountered: