dotnet / linker

389 stars 127 forks source link

Do we want to keep PublishTrimmed a publishing option? #2085

Open MichalStrehovsky opened 3 years ago

MichalStrehovsky commented 3 years ago

PublishTrimmed is currently a checkbox in the VS publish dialog. We also have docs and blog articles suggesting people publish by adding /p:PublishTrimmed=true parameter to their dotnet command line.

We set several defaults based on PublishTrimmed in the SDK: https://github.com/dotnet/sdk/blob/9ed756ecfe082b738af11a76cabb74742102a0a7/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets#L31-L44

The problem with that is that if people specify trimming as part of publish, they'll get different behavior after publishing from the behavior they would get with F5 debug or dotnet run due to feature switches being enabled when publishing, but not when building.

We need to shift the recommendation from "specify trimming as part of publish" to "specify trimming in the csproj".

VS UI should also change.

Cc @vitek-karas @sbomer

sbomer commented 3 years ago

See also https://github.com/dotnet/sdk/issues/14562#issuecomment-842502097. I agree we should recommend setting it in the csproj.

It also might be worth renaming the option to reflect that it no longer just affects publish, especially since Xamarin runs the linker during build, but I'm not sure what would be a good name. How about EnableTrimming? Would we want to do the same for PublishSingleFile/PublishReadyToRun?

sbomer commented 3 years ago

@vitek-karas @agocke is renaming the publish options something we should consider for .NET6?

agocke commented 3 years ago

I think the problem with any name change is that it will only take effect during publish outside Xamarin. It might be pretty confusing if you set EnableTrimming in the project file but don't run publish and nothing happens.

MichalStrehovsky commented 3 years ago

The resolution of this could also be that we just remove the checkbox from the VS publish dialog and update all the docs and blog articles to say "put this in your CSPROJ".

PublishTrimmed is a fine name as long as we only do trimming during publish. Now - I heard there has been some effort to unify what "publish" and "build" means. If it ever comes to that, it's shouldn't be a big issue to come up with a new name of the property and adding a fallback to initialize its value from PublishTrimmed.

agocke commented 3 years ago

I agree -- if we unify publish and build then this problem would go away and we should simplify.

I'm still ambivalent on the idea of removing the checkbox from the publish page. I'm worried that we won't be able to add anything to the project pages in time and the only option will be editing the project file manually. This may be acceptable, but would be far less discoverable.

agocke commented 3 years ago

Thinking about this more: I think it's way too late to change the name of the property now. There's not enough time for users to adapt to the change.

The only change I could see us making this late into .NET 6 is removing the publish option entirely from the publish page.

MichalStrehovsky commented 3 years ago

The idea that trimming doesn't change the behavior of the trimmed apps depends on the "emulation" of feature switches that depend on PublishTrimmed being set to true. If we don't see it set to true in F5 debugging, people will have to troubleshoot their apps at the moment they're ready to ship them.

We had such experience in .NET Native (F5 Debug build was running on CoreCLR that didn't attempt to emulate the F5 Release build based on .NET Native). People didn't like it.

agocke commented 3 years ago

Yup, it's an important concern. I think the big question is: is documentation good enough?

agocke commented 3 years ago

@samsp-msft for thoughts

samsp-msft commented 3 years ago

There are a couple of parts wrapped up in this.

My recommendation(s) would be:

If these are msbuild properties, I assume we don't need to change msbuild api, its more SDK & targets work?

samsp-msft commented 3 years ago

There are a couple of parts wrapped up in this.

My suggestions:

jkotas commented 3 years ago

Publishing with Trimming today, not only trims the app, but also sets some feature flags that could affect app behavior

It disables features that are known to be broken with trimming. The primary reason why we are disabling these features with trimming by default is predictabile experience. For example, you will get a nice "built-in COM not support exception" instead of a crash somewhere inside the runtime that only a few interop experts would be able to understand.

I do not think we want a UI checkbox to re-enable features broken with trimming. It is very advanced scenario that should be only used by people who know what their are doing and are capable of dealing with the consequences.

marek-safar commented 3 years ago

I think the problem with any name change is that it will only take effect during publish

That's not entirely true. Trimming during the build will most likely happen for specific workloads for .net6 and I think Uno does that even today with .NET5.

some effort to unify what "publish" and "build" means. If it ever comes to that, it's shouldn't be a big issue to come up with a new name of the property and adding a fallback to initialize its value from PublishTrimmed.

It's true that there is an ongoing effort to make publish to be an alias for dotnet build. There will be some steps made for some workloads in .NET6 but it's gonna take us more than few releases to get there fully.

sbomer commented 3 years ago

Moving the option to set those flags in F5/debug/dotnet build makes sense. The name publishtrimmed however doesn't make sense for that.

I agree, but I am not sure adding a separate build-time-only property is a better experience. Most people will want both the build-time and publish-time behavior, so I think our default recommendation should be to set just one property in the csproj.

PublishTrimmed has the right behavior in that case, just not a perfect name. It sounds like .NET7 would be a better time to rename it, especially if there are plans to unify build and publish. It sounds like for now we should just make sure our docs recommend putting it in the csproj, and maybe remove the VS UI option.

It might be pretty confusing if you set EnableTrimming in the project file but don't run publish and nothing happens.

My point is that something does happen - it toggles the feature settings and enables trim analysis. Maybe EnableTrimming isn't the best name though. I was trying to suggest a name that both declares intent to trim during build, and actually trims during publish.

Trimming during the build will most likely happen for specific workloads for .net6

The ones I'm aware of already hide the non-fitting PublishTrimmed name behind their own options, but it would be nice for this to be more consistent in the future.

agocke commented 3 years ago

I think this issue should mainly be concerned with plain .NET 6 apps. First, if another app model is automatically doing a publish during every build, then this issue doesn't really matter much, since they'll be running the linker on every build anyway, and so the app that they test will be the one with the feature flags set.

For other workloads, they have their own target files where they can appropriately set the right defaults for their workload, including enabling the feature flags. If we need new APIs in the base SDK to make that easier, I'm happy to discuss those, but that doesn't really affect the plain .NET 6 scenario.

For plain .NET 6, we could add a new property just for enabling feature flags, but I don't have any great naming suggestions, and personally I think I would rather have PublishTrimmed just be present in the project file (just because it would personally align with the behavior I want in all scenarios).

samsp-msft commented 3 years ago

For plain .NET 6, we could add a new property just for enabling feature flags, but I don't have any great naming suggestions, and personally I think I would rather have PublishTrimmed just be present in the project file (just because it would personally align with the behavior I want in all scenarios).

If you are suggesting that having the "PublishTrimmed" property will just affect the feature flags for build, then that seems reasonable and documentable.

vitek-karas commented 3 years ago

If I ignore the weird name of the property (nicely described by @sbomer above) the only remaining UX issue is that the Publish UX in VS doesn't look at properties in csproj. The behavior is basically the same with or without PublishTrimmed=true in the .csproj - the publish settings generated by VS will disable trimming by default even if it's enabled in .csproj.

samsp-msft commented 3 years ago

{Github doesn't have an emoji for banging one's head on the table}.

What, if any, relationship does the VS publish dialog have to do with dotnet publish?

vitek-karas commented 3 years ago

The VS publish dialog does:

In a way it's a GUI on top of several properties which get passed to dotnet publish command.