dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.68k stars 1.06k forks source link

PublishTrimmed should imply self-contained #30104

Closed eerhardt closed 9 months ago

eerhardt commented 1 year ago

Doing the following steps:

Results in the following error:

C:\Users\eerhardt\.nuget\packages\microsoft.net.illink.tasks\8.0.100-1.22619.1\build\Microsoft.NET.ILLink.targets(196,5): error NETSDK1102: Optimizing assemblies for size is not supported for
the selected publish configuration. Please ensure that you are publishing a self-contained app. [C:\DotNetTest\Net8Console\Net8Console.csproj]

However, doing

Just works without more command line args.

We should make PublishTrimmed=true imply SelfContained=true, just like PublishAot=true and PublishSingleFile=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, but SelfContained=false, those customers can explicitly --self-contained false. This is consistent with the current PublishSingleFile=true behavior. By default it is self-contained. And you can opt into Framework Dependent Deployment with dotnet publish -p:PublishSingleFile=true --self-contained false.

cc @vitek-karas @sbomer @agocke

sbomer commented 1 year ago

For PublishSingleFile the default FDD behavior is OK, it won't fail. And that was the default behavior in NET 6.

I think this will be the case after the breaking change - but it looks like in .NET 6 it would actually fail without an explicit RID (and would be SCD with an explicit RID).

.NET 6:

svbomer@svbomer-ubuntu:~/tmp/scd$ dotnet publish -p:PublishSingleFile=true
...
/home/svbomer/bin/sdk/6.0.405/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Publish.targets(102,5): error NETSDK1097: It is not supported to publish an application to a single-file without specifying a RuntimeIdentifier. You must either specify a RuntimeIdentifier or set PublishSingleFile to false. [/home/svbomer/tmp/scd/scd.csproj]

svbomer@svbomer-ubuntu:~/tmp/scd$ dotnet publish -p:PublishSingleFile=true -r linux-x64
...
/home/svbomer/bin/sdk/6.0.405/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(1120,5): warning NETSDK1179: One of '--self-contained' or '--no-self-contained' options are required when '--runtime' is used. [/home/svbomer/tmp/scd/scd.csproj]
  scd -> /home/svbomer/tmp/scd/bin/Debug/net6.0/linux-x64/scd.dll
  scd -> /home/svbomer/tmp/scd/bin/Debug/net6.0/linux-x64/publish/
svbomer@svbomer-ubuntu:~/tmp/scd$ du -hs bin/Debug/net6.0/linux-x64/publish/
62M     bin/Debug/net6.0/linux-x64/publish/

svbomer@svbomer-ubuntu:~/tmp/scd$ dotnet publish -p:PublishSingleFile=true -r linux-x64 --no-self-contained
...
  scd -> /home/svbomer/tmp/scd/bin/Debug/net6.0/linux-x64/scd.dll
  scd -> /home/svbomer/tmp/scd/bin/Debug/net6.0/linux-x64/publish/
svbomer@svbomer-ubuntu:~/tmp/scd$ du -hs bin/Debug/net6.0/linux-x64/publish/
164K    bin/Debug/net6.0/linux-x64/publish/
nagilson commented 1 year ago

@richlander

I actually thought we implemented PublishSelfContained last version, but I guess not.

It was implemented and made it into 7.0.200 -- works for me. The confusing part is the release page version is called 7.0.2 but it is 7.0.102... which is not 7.0.200... The SDK installation page is based on the runtime version, and does not communicate the SDK version, and we have given them feedback in the past that it should be more clear. Maybe my comment here:

Close this issue [...] As well as one for PublishSelfContained.

Added to the confusion, I meant to say an issue for inference of these properties.

richlander commented 1 year ago

Also, we're never going to build Native AOT FDD

The more I look at it, the more I think the same think about PublishTrimmed.

The two examples are not remotely similar.

agocke commented 1 year ago

They are. The only thing that matters is our likelihood of building them. I don't think the likelihood of building FDD trimming is any higher than AOT. I have even more use cases for AOT FDD than I do for trimming, e.g. plugin scenarios.

richlander commented 1 year ago

Interesting. I had never thought of plug-ins. That said, I still think Native AOT and FDD is a bad idea due to versioning issues and giving up on perf due to a boundary.

I know what the rules are for CoreCLR with plugins in terms of multiple runtime versions. Does Native AOT have the same rules or different? The native plug-in scenario only makes sense to me if the rules are more relaxed.

Contrast that with trimming. It is easy to see how we could provide benefit for FDD apps, particularly the ones that take a dependency on the big cloud SDKs.

agocke commented 1 year ago

Contrast that with trimming. It is easy to see how we could provide benefit for FDD apps, particularly the ones that take a dependency on the big cloud SDKs.

I covered some of the basic problems with it here: https://github.com/dotnet/linker/issues/2609. I don't think it will ever be feasible. We likely couldn't ever statically guarantee safety because we wouldn't know the code paths that could be executed by the dynamically-loaded framework which could end up doing reflection against the target library.

vitek-karas commented 1 year ago

I don't want to use this issue to discuss the feasibility of FDD trimming. That said I don't think the problem of trimming one assembly only is the same as FDD trimming - with FDD trimming we would make an assumption that framework is fully annotated. With that you can do some things - not everything works, but the basic removal of obviously dead code should work just fine - and in a safe manner. (Servicing of the framework is a tricky question though... but that's too much detail right now).

richlander commented 1 year ago

We're going around in circles a bit, in large part because there is no obvious "right answer".

Let's make this a little easier:

Is that fair?

sbomer commented 1 year ago

Fair summary of my position. I don't think Vitek wanted single-file SCD, but I'll let him and Andy speak for themselves.

eerhardt commented 1 year ago

I am very against making PublishAot FDD by default. It would mean our new dotnet new api -aot template would need to put 2 properties in the csproj instead of 1. It just adds clutter/confusion in my mind.

Properties inferring other properties makes total sense. PublishAot implies PublishTrimmed and PublishSingleFile. I could imagine a world where I want PublishAot but don’t trim anything. Is it surprising that adding PublishAot also flips PublishTrimmed? dotnet publish doesn’t trim (just like it isn’t self contained). Why is it OK for PublishTrimmed to get its non-default value when I PublishAot?

The same argument applies to SelfContained. If it isn’t explicitly specified, it can get defaulted to what makes the most sense.

richlander commented 1 year ago

I agree with you, however your "very against" is a bit overplayed in terms of how many properties we have in a project file. I don't think that's an important metric for this decision. My continued claim is that this conversation is primarily a CLI one (for "neutral" projects). We've added more properties over the last couple releases and I'm sure we'll add more.

eerhardt commented 1 year ago

My continued claim is that this conversation is primarily a CLI one (for "neutral" projects).

  1. When it comes to PublishAot, PublishTrimmed, and PublishSingleFile, we recommend devs put these properties in their csproj. For example, see https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/trimming-options?pivots=dotnet-7-0#enable-trimming

  2. I see no reason why the property defaulting behavior would be any different between the command line and the csproj. Passing -p:PublishXXX on the command line would default the same properties as setting <PublishXXX> in the csproj.

richlander commented 1 year ago

I meant something different. Clearly, the AOT template should be configured correctly to provide the intended results. I view near 100% of the discussion on this thread to center around trimming. That clearly wasn't about the AOT template, but about project files that don't have any of these properties in them and what just specifying -p:PublishTrimming=true via the CLR should do. I don't think anyone raised what the property on its own in a project file should do.

eerhardt commented 1 year ago

what just specifying -p:PublishTrimming=true via the CLR should do. I don't think anyone raised what the property on its own in a project file should do.

That’s probably my mistake. My presupposition was that setting a property via the command line and setting a property in the .csproj results in the same behavior. I could have just as easily written the original issue with setting <PublishedTrimmed> in the csproj.

richlander commented 1 year ago

My presupposition was that setting a property via the command line and setting a property in the .csproj results in the same behavior.

They do.

I could have just as easily written the original issue with setting in the csproj.

But that wouldn't be nearly as compelling. Writing settings into a project file is a one and done activity. Typing things via the CLI is every time.

jkotas commented 1 year ago

When it comes to PublishAot, PublishTrimmed, and PublishSingleFile, we recommend devs put these properties in their csproj.

These properties enable analyzers that warn about the form factor incompatibilities in the inner dev loop.

Setting these properties via command line is not a pit of success.

richlander commented 1 year ago

I completely agree with that. It leans us towards making the properties consistent and less focused on their "optimized usability" via the CLI. That leads me to believe that the models proposed by Sven and me are most likely to provide the best long-term results. In fact, those models will encourage people to use project files over the CLI.

sbomer commented 1 year ago

Properties inferring other properties makes total sense. [....] Why is it OK for PublishTrimmed to get its non-default value when I PublishAot?

My 2c: SCD/FDD is a very impactful setting that fundamentally changes the prerequisites required to run the output on a target machine, so it's worth being explicit about it. I think this is also part of the reason the RID -> SCD breaking change is worth making. I am personally more ok with letting one of the publish "characteristics" pick different optimizations. AOT with or without trimming would not have to be deployed to different environments.

agocke commented 1 year ago

You have my position mostly right Rich, but the big reason I think we should make trimmed imply scd is that i strongly believe we'll never do FDD for either aot or trimming, and I find it obnoxious when dev tools have one possible interpretation of your command that will work, and then produce an error instead of doing it.

The risk here is that I'm wrong and we eventually add FDD. But that risk is tempered by the likelihood that even if we add those configurations, they'll probably be a lot less popular.

agocke commented 1 year ago

That said, I agree that adding things to the project file once isn't a huge deal. Tbh I hate "Self Contained" build a lot more than some extra typing. Self contained build is never what I want and it just clutters the output and makes my artifacts directory ~100 MB larger than it should be.

nagilson commented 1 year ago

My presupposition was that setting a property via the command line and setting a property in the .csproj results in the same behavior.

They do.

@richlander @eerhardt This is not always true. For example: -p:Configuration=Release and <Configuration>Release</Configuration>. One works with dotnet build/publish/etc, the later one doesn't.

richlander commented 1 year ago

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.

eerhardt commented 1 year ago

@richlander - can you explain why you are cutting the "Andy and Vitek" option from consideration?

I see significant diversity of opinion for FDD trimming on this thread on feasibility, value, and likelihood.

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.

richlander commented 1 year ago

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.

agocke commented 1 year ago

I'm fine with going forward with FDD-by-default.

richlander commented 1 year ago

BTW: A lot of this is my fault. I pushed implying SCD for some of the Publish* settings in .NET 7. It was clearly not well thought through.

richlander commented 1 year ago

I propose we make that change in Preview 2 and then course correct as needed. Fair?

That means all the Publish* settings will be FDD by default, except PublishSelfContained.

That leaves what to do with SelfContained. I think we should just undocument and call it good. I'm not sure that breaking it is useful. Note that this is separate from the CLI args, which should stay as-is.

agocke commented 1 year ago

I think we should just undocument and call it good

We'll need a new command line argument then, and we'll need to change error messages

agocke commented 1 year ago

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 -p:PublishSelfContained

richlander commented 1 year ago

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 SelfContained alone, that's fine. I was trying to re-raise a suggestion you had raised earlier. Do you have another suggestion on how to approach that?

agocke commented 1 year ago

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.

richlander commented 1 year ago

I see. This change would mean that we are for sure saying that the way to produce final prod apps with .NET is via publish. I think we're mostly there anyway. If no one else wants to raise a concern, I'm good with that. In fact, it might simplify the whole model.

@baronfel @dsplaisted

marcpopMSFT commented 1 year ago

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 dotnet build --sc also produce an error?

+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)?

richlander commented 1 year ago

Mostly "yes" and great questions.

That means introducing errors when aot, trimming, and singlefile correct?

Would dotnet build --sc also produce an error?

With this proposal, yes. We'd move that to dotnet publish only.

Would we need to coordinate changes to the api -aot template

Yes.

This actually aligns quite closely with the -r change now that I think about it. It is just one step further.

DamianEdwards commented 1 year ago

I've read all the reasoning here, and while I don't fault the logic, I find the fact that PublishAot will now simply error without PublishSelfContained also being specified a strange UX for us to default to, especially when we seem to believe it's extremely unlikely we'd ever support AOT with FDD apps.

richlander commented 1 year ago

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.

dsplaisted commented 1 year ago

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 publish. I think we're mostly there anyway. If no one else wants to raise a concern, I'm good with that. In fact, it might simplify the whole model.

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.

agocke commented 1 year ago

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 <ProjectReference Targets="...;Publish" /> instead of a plain ProjectReference. I'd much rather people do that since right now there's a large cliff where if you want just a self-contained build, that can work with no extra configuration, but if you want a single-file build you have to switch to Publish and all the associated configuration.

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.

dsplaisted commented 1 year ago

It works fine though, you just need to pass <ProjectReference Targets="...;Publish" /> instead of a plain ProjectReference.

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.

richlander commented 1 year ago

I don't have a strong viewpoint on this topic. Perhaps the two of you can hash out a proposal?

agocke commented 1 year ago

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.

OptiStrat commented 1 year ago

The more I look at it, the more I think the same think about PublishTrimmed.

I've seen several asks for FDD Trimming over time (TerraFx - if I remember correctly is one example, WinUI is another).

@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?

vitek-karas commented 1 year ago

The issue https://github.com/dotnet/linker/issues/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 https://github.com/dotnet/linker/issues/2139 regardless.

The problems described in https://github.com/dotnet/linker/issues/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. https://github.com/dotnet/linker/issues/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.

sbomer commented 9 months ago

From https://github.com/dotnet/runtime/pull/95496#discussion_r1412655368:

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.

Now that PublishTrimmed implies PublishSelfContained, can we close this issue @eerhardt @nagilson? I opened a separate issue to track the proposal around fixing the SelfContained behavior (https://github.com/dotnet/sdk/issues/37780).

eerhardt commented 9 months ago

Retested with SDK v8.0.200-preview.23620.12 and the repro steps in the original post now work correctly. Closing.