Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make _TrimmerDefaultAction public #16140

Closed
sbomer opened this issue Mar 1, 2021 · 6 comments · Fixed by dotnet/docs#23766 or #16865
Closed

Make _TrimmerDefaultAction public #16140

sbomer opened this issue Mar 1, 2021 · 6 comments · Fixed by dotnet/docs#23766 or #16865
Labels
Area-ILLink untriaged Request triage from a team member

Comments

@sbomer
Copy link
Member

sbomer commented Mar 1, 2021

In .NET 6, we are introducing [assembly: AssemblyMetadata("IsTrimmable", "True")] as a way to opt a library into trimming. MSBuild metadata can also be used to override this per-assembly.

With this change, the linker has two global knobs to control trimming:

  • --trim-mode: the trimming granularity for assemblies marked IsTrimmable
    • The SDK sets this to copyused by default, but it is configurable via the property TrimMode
  • --action: the trimming granularity for assemblies not marked IsTrimmable
    • The SDK sets this to copy by default (so that assemblies which aren't marked IsTrimmable are kept).

We should expose a public MSBuild option that maps to the global --action (maybe TrimmerDefaultAction), which allows developers to enable member-level trimming for the entire application by setting:

<TrimMode>link</TrimMode>
<TrimmerDefaultAction>link</TrimmerDefaultAction>

When we discussed full-app trimming earlier, we were hesitant to make it too easy (see the notes under TrimAllAssemblies).

On the other hand, the option is already there (introduced in #16094), so the real question is just whether it should be exposed to the user (and if so, what the name should be). Note that xamarin-android already has some user-facing knobs to do exactly this (<AndroidLinkMode>Full</AndroidLinkMode>).

@marek-safar @vitek-karas @samsp-msft @eerhardt

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label Mar 1, 2021
@eerhardt
Copy link
Member

eerhardt commented Mar 1, 2021

Why introduce a new property and force the user to understand 2 things? Why wouldn't we just introduce a new TrimMode option?

  • copy (currently available)
  • link (currently available)
  • full (newly proposed behavior)

That way I would only need to set 2 properties in my publish script instead of 3:

dotnet publish -c Release -p:PublishTrimmed=true -p:TrimMode=full

vs.

dotnet publish -c Release -p:PublishTrimmed=true -p:TrimMode=link TrimmerDefaultAction=link

@marek-safar
Copy link
Contributor

Why introduce a new property and force the user to understand 2 things?

We don't expect seasonal developers to touch these settings at all. It's something that SDKs will most likely set if they need to. The settings like

<TrimMode>link</TrimMode> 
<TrimmerDefaultAction>copy</TrimmerDefaultAction>

is also valid.

@sbomer
Copy link
Member Author

sbomer commented Mar 1, 2021

I agree it would be a nicer UX to have a single property to make it trim everything aggressively, but I don't think there's agreement on this across my team. The TrimmerDefaultAction might be just enough of a barrier - it's a bit weird to explain why we need this on top of TrimMode, but for someone who knows what they're doing it's still easy enough to set.


That said, I still wanted to explore what it might look like to simplify this. At the implementation level, I don't think we should reuse TrimMode/--trim-mode at least in the current form, since this would give it multiple meanings:

  • copy/link affects the granularity of trimming
  • full affects the granularity of trimming and which assemblies get trimmed

You can set copy/link per assembly to override the global settings, but it doesn't make sense for full to work the same way.

If anything I would push for this to be supported in the command-line. Otherwise we tend to get hidden complexity in the MSBuild logic, which can simplify some user scenarios, but makes it hard to understand how the options interact. Here are two options that I could see working:

  1. Instead of a default action, have an option that controls which assemblies get trimmed. To do full trimming you still need two properties, but arguably they are easier to explain than the above:
<TrimMode>link</TrimMode>
<TrimAssemblies>all</TrimAssemblies> <!-- or could be TrimAllAssemblies bool -->
  • --trim-mode is unchanged - accepts copy/copyused/link.
  • --trim-assemblies:
    • explicit: only trims IsTrimmable assemblies or those with an explicit --action
    • all: trims everything, regardless of IsTrimmable attribute. Explicit action --action copy Assembly can still disable it per-assembly.
  • There is no global TrimmerDefaultAction/--action for non-trimmable assemblies. The setting is subsumed by the combination of --trim-assemblies and --trim-mode
    • You can no longer express combinations like <TrimMode>link</TrimMode>, <TrimmerDefaultAction>copyused</TrimmerDefaultAction>, but I'm not sure it's useful in the first place.
  1. Combine the options at the level of the command-line. You only need one setting in the project file:
<TrimMode>full</TrimMode>
  • Per-assembly --action is unchanged - accepts copy/copyused/link
  • TrimMode/--trim-mode accepts conservative/member/full
    • conservative means: copyused for trimmable assemblies, otherwise copy
    • member means: link for trimmable assemblies, otherwise copy
    • full means: link for trimmable and non-trimmable assemblies
    • I'm sure we can find better names. But the idea is that this option affects the granularity and which assemblies get trimmed, so the names should reflect this to avoid confusion with --action (which only affects granularity)
    • We would support copyused/link for back-compat, but make it a warning to encourage people to use the renamed options.
  • There is no global TrimmerDefaultAction/--action

@samsp-msft
Copy link
Member

samsp-msft commented Mar 2, 2021

Why introduce a new property and force the user to understand 2 things? Why wouldn't we just introduce a new TrimMode option?

  • copy (currently available)
  • link (currently available)
  • full (newly proposed behavior)

...

dotnet publish -c Release -p:PublishTrimmed=true -p:TrimMode=full

I would suggest LinkAll' or ForceLinkAll` as the options rather than full - as its really about trimming on assemblies that are not marked as suitable for linking.

I don't know how often users will want to tweak this - but encouraging it to be done at the assembly level through project file action specification is preferably to a big switch that is likely to need exceptions.

@marek-safar
Copy link
Contributor

I'm still not convinced the "linkall" is a good idea. Are we expecting people to actually set that so we go the extra length to make it more convenient? If we think so I'd suggest staying from simple wording like linkall because even in .net6 in some configurations there could be no difference between link and linkall and that gap will be possibly reduced further in the .NET7 release and we'll be stuck with two options meaning the exact same thing for most apps.

sbomer added a commit to sbomer/sdk that referenced this issue Apr 13, 2021
sbomer added a commit that referenced this issue Apr 19, 2021
* Make TrimmerDefaultAction public

Fixes #16140
Relevant for dotnet/docs#23766

* Don't suppress trim warnings when default action is link

* Move SuppressTrimAnalysisWarnings to targets

So that the condition takes into account properties set in the project file.
sbomer added a commit to sbomer/sdk that referenced this issue Apr 19, 2021
sbomer added a commit that referenced this issue Apr 20, 2021
* Make TrimmerDefaultAction public

Fixes #16140
Relevant for dotnet/docs#23766

* Don't suppress trim warnings when default action is link

* Move SuppressTrimAnalysisWarnings to targets

So that the condition takes into account properties set in the project file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-ILLink untriaged Request triage from a team member
Projects
None yet
4 participants