Add more trimming options to TrimMode and change defaults #2856

agocke · 2022-06-22T03:04:22Z

The new default for trimming is equivalent to TrimmerDefaultAction=link.
Since we're trying to stay away from the term link and TrimmerDefaultAction
isn't a very clear name, this change repurposes TrimMode.

The two new TrimModes are 'full' and 'partial'. The new default is equivalent
to 'TrimMode=full'. These new modes are also recognized for earlier TFMs, but
the defaults are only changed for net7.0+

The new default for trimming is equivalent to TrimmerDefaultAction=link. Since we're trying to stay away from the term link and TrimmerDefaultAction isn't a very clear name, this change repurposes TrimMode. The two new TrimModes are 'full' and 'partial'. The new default is equivalent to 'TrimMode=full'. These new modes are also recognized for earlier TFMs, but the defaults are only changed for net7.0+

agocke · 2022-06-22T03:05:29Z

I pushed a lot of stuff in this change into the task for two reasons: 1) it was getting really complicated to do this in MSBuild properties, and 2) it was much easier to test.

@MichalStrehovsky this will probably look different for NativeAOT, but I haven't looked at the code yet to see how.

This should root all non-trimmable assemblies, same as TrimmerDefaultAction=copy. We still support consuming ILCompiler through NuGet when targeting .NET 6, so recognize the old thing too. We might want to cut the .NET 6 support close to RC and at that time TrimmerDefaultAction handling can go away. Related: dotnet/linker#2856.

MichalStrehovsky

Looks good to me but I would like Sven to see this too.

Happy to see the TrimMode/TrimmerDefaultAction confusion going away. I kept telling people to forget TrimMode exists. It's now getting a new/better life.

I assume we'll make sure ASP.NET doesn't get insta-broken by this by adding a <TrimMode>partial</> somewhere in their SDK.

src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets

vitek-karas · 2022-06-22T08:00:39Z

src/ILLink.Tasks/LinkTask.cs

+		/// Roughly maps to the TargetFrameworkVersion.
+		/// </summary>
+		[Required]
+		public int LinkVersion { get; set; }


Works - but I would prefer if we simply passed something like TargetFrameworkVersion - could be the same numerical value, but make it clear that it's the TFM version - I don't want to introduce yet another version number to think about.

I'd prefer not to introduce any version to this task. The task should aim to be a straightforward mapping to the command-line arguments. Introducing version-based logic here increases the probability that we set version-specific defaults in the task that can't be overridden by the user.

I tried to make that work, but that means we have to do all of the mapping logic in MSBuild properties. That seems much harder to get right, and also very hard to test

We should have end-to-end tests that exercise the targets+task either way.

It is unfortunately harder to get right. I personally think that is a price worth paying:

to make sure we don't break the ability to override defaults, and

to keep the task a thin wrapper for the tool (unless we want to introduce TFM-versioned logic to the tool itself).

vitek-karas · 2022-06-22T08:04:36Z

src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets

-    <!-- Suppress for WPF/WinForms (unless linking everything) -->
-    <SuppressTrimAnalysisWarnings Condition="'$(TrimmerDefaultAction)' != 'link' And ('$(UseWpf)' == 'true' Or '$(UseWindowsForms)' == 'true')">true</SuppressTrimAnalysisWarnings>
+    <!-- Suppress for WPF/WinForms -->
+    <SuppressTrimAnalysisWarnings Condition="'$(UseWpf)' == 'true' Or '$(UseWindowsForms)' == 'true'">true</SuppressTrimAnalysisWarnings>


Why? If I trim WinForms app and ask for TrimMode=full, I should get all the warnings. Similarly I would expect this behavior in all other places, basically if I say TrimMode=full - I should get warnings by default (since I asked for the "hard " mode).

It's currently blocked off completely, which is why we hide the warnings at all. I think we should either show the warnings or disallow trimming.

OK - if we rely on the error for this, then I agree. We should at least add a comment here (why it's OK to suppress). Ideally if the error is disabled we would reenable the warnings (this should really only affect analyzer anyway, the error will trigger before we get to run the linker anyway)

vitek-karas · 2022-06-22T08:05:36Z

src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets

+    <!-- In .NET 7, TrimmerDefaultAction is deprecated. TrimMode can be used for the supported configurations. -->
+    <Warning Condition="'$(TrimmerDefaultAction)' != '' And $([MSBuild]::VersionGreaterThan('$(TargetFrameworkVersion)', '6.0'))"
+             Text="Property 'TrimmerDefaultAction' is deprecated in .NET 7." />


Please make the warning actionable - maybe just add "Please use TrimMode instead" or something like that.

src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets

akoeplinger · 2022-06-22T10:04:01Z

This will also need a change in xamarin-android since they set the TrimmerDefaultAction: https://github.com/xamarin/xamarin-android/blob/2ca2a103d4e473eeeceecd7689961038bb374cdb/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets#L67

/cc @jonathanpeppers

src/ILLink.Tasks/LinkTask.cs

marek-safar · 2022-06-22T10:21:01Z

src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets

@@ -212,6 +221,10 @@ Copyright (c) .NET Foundation. All rights reserved.
         potentially problematic when warnings are suppressed. -->
    <NETSdkInformation Condition="'$(PublishTrimmed)' == 'true' And '$(SuppressTrimAnalysisWarnings)' == 'true'" ResourceName="ILLinkOptimizedAssemblies" />

+    <!-- In .NET 7, TrimmerDefaultAction is deprecated. TrimMode can be used for the supported configurations. -->
+    <Warning Condition="'$(TrimmerDefaultAction)' != '' And $([MSBuild]::VersionGreaterThan('$(TargetFrameworkVersion)', '6.0'))"


This change will need to be propagated to all SDKs like https://github.com/xamarin/xamarin-macios/blob/main/msbuild/Xamarin.iOS.Tasks.Windows/Xamarin.iOS.Common.After.targets#L333

jkotas · 2022-06-22T13:46:44Z

This should get breaking change doc: https://github.com/dotnet/docs/issues/new?assignees=gewarren&labels=breaking-change%2CPri1%2Cdoc-idea&template=breaking-change.yml&title=%5BBreaking+change%5D%3A+

sbomer

It might be worth running the SDK tests locally against this change to catch any breaks before insertion.

sbomer · 2022-06-22T17:04:23Z

src/ILLink.Tasks/LinkTask.cs

+		/// Roughly maps to the TargetFrameworkVersion.
+		/// </summary>
+		[Required]
+		public int LinkVersion { get; set; }


I'd prefer not to introduce any version to this task. The task should aim to be a straightforward mapping to the command-line arguments. Introducing version-based logic here increases the probability that we set version-specific defaults in the task that can't be overridden by the user.

src/ILLink.Tasks/LinkTask.cs

agocke · 2022-06-22T21:46:55Z

In the process of switching over to setting defaults in the targets, but fixing things up in the tasks (with the intent of eventually passing things all the way through to the linker). I still have to set up SDK testing for this.

src/ILLink.Tasks/LinkTask.cs

Companion to dotnet/linker#2856

rolfbjarne · 2022-07-01T14:53:42Z

What's the actual semantics between these modes?

I'm guessing that:

full: trims every assembly unless an assembly has opted out.
partial: trims every assembly that has opted in.

Is this correct?

agocke · 2022-07-01T16:47:29Z

Yup that’s right

…opy'

sbomer

LGTM, thanks a lot!

This should root all non-trimmable assemblies, same as TrimmerDefaultAction=copy. We still support consuming ILCompiler through NuGet when targeting .NET 6, so recognize the old thing too. We might want to cut the .NET 6 support close to RC and at that time TrimmerDefaultAction handling can go away. Related: dotnet/linker#2856.

Companion to dotnet/linker#2856 * Update src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets * Remove more usages of TrimMode * Update linker versions manually Co-authored-by: Jonathan Peppers <[email protected]>

Changes: dotnet/installer@85a0482...a9c056c Changes: dotnet/linker@ef2d0f2...d27ff61 Changes: dotnet/runtime@206dccb...072eda8 Changes: dotnet/emsdk@40e7c62...11a9acf Updates: * Microsoft.Dotnet.Sdk.Internal: from 7.0.100-preview.7.22361.1 to 7.0.100-rc.1.22368.2 * Microsoft.NET.ILLink.Tasks: from 7.0.100-1.22354.1 to 7.0.100-1.22365.1 * Microsoft.NETCore.App.Ref: from 7.0.0-preview.6.22356.1 to 7.0.0-rc.1.22366.5 * Microsoft.NET.Workload.Emscripten.Manifest-7.0.100: from 7.0.0-preview.7.22330.1 to 7.0.0-rc.1.22362.2 ~~ Set `$(TrimMode)` `partial` by default (#7132) ~~ Companion to dotnet/linker#2856 * Update src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets * Remove usages of `%(TrimMode)` for `$(AndroidLinkMode)` of `Full` * Update .apkdesc files ~~ Set `$(LangVersion)` to 10 in Mono.Android.csproj ~~ There appears to be a C# 11 IL size regression in: dotnet/roslyn#62832 We can use C# 10 for now to avoid this. * Fixed `Mono.Android.dll` size in `.apkdesc` files Co-authored-by: Andy Gocke <[email protected]> Co-authored-by: Jonathan Peppers <[email protected]>

Changes: dotnet/installer@d2fff6d...dbdda95 Changes: dotnet/linker@ef2d0f2...33a76b8 Changes: dotnet/runtime@206dccb...db5d4df Changes: dotnet/emsdk@40e7c62...7d27778 Updates: * Microsoft.Dotnet.Sdk.Internal: from 7.0.100-preview.7.22362.31 to 7.0.100-preview.7.22370.3 * Microsoft.NET.ILLink.Tasks: from 7.0.100-1.22354.1 to 7.0.100-1.22362.1 * Microsoft.NETCore.App.Ref: from 7.0.0-preview.6.22356.1 to 7.0.0-preview.7.22369.4 * Microsoft.NET.Workload.Emscripten.Manifest-7.0.100: from 7.0.0-preview.7.22330.1 to 7.0.0-preview.7.22361.2 ~~ Set `$(TrimMode)` `partial` by default (#7132) ~~ Companion to dotnet/linker#2856 * Update src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets * Remove usages of `%(TrimMode)` for `$(AndroidLinkMode)` of `Full` * Update .apkdesc files ~~ Set `$(LangVersion)` to 10 in Mono.Android.csproj ~~ There appears to be a C# 11 IL size regression in: dotnet/roslyn#62832 We can use C# 10 for now to avoid this. * Fixed `Mono.Android.dll` size in `.apkdesc` files Co-authored-by: Andy Gocke <[email protected]> Co-authored-by: Jonathan Peppers <[email protected]>

…ker#2856) The new default for trimming is equivalent to TrimmerDefaultAction=link. Since we're trying to stay away from the term link and TrimmerDefaultAction isn't a very clear name, this change repurposes TrimMode. The two new TrimModes are 'full' and 'partial'. The new default is equivalent to 'TrimMode=full'. These new modes are also recognized for earlier TFMs, but the defaults are only changed for net7.0+. Commit migrated from dotnet/linker@b6f1fb4

agocke requested review from vitek-karas and MichalStrehovsky June 22, 2022 03:04

agocke requested review from marek-safar and sbomer as code owners June 22, 2022 03:04

agocke added 2 commits June 22, 2022 00:42

Fix test and cleanup

d284611

Lint

67a526a

MichalStrehovsky mentioned this pull request Jun 22, 2022

Recognize TrimMode=partial in ILCompiler targets dotnet/runtime#71117

Merged

MichalStrehovsky approved these changes Jun 22, 2022

View reviewed changes

src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets Outdated Show resolved Hide resolved

vitek-karas reviewed Jun 22, 2022

View reviewed changes

marek-safar reviewed Jun 22, 2022

View reviewed changes

sbomer reviewed Jun 22, 2022

View reviewed changes

agocke added 2 commits June 22, 2022 11:32

Respond to PR comments

6e5faa3

WIP

381e390

agocke added 5 commits June 23, 2022 22:54

Keep defaults in the targets and move full/partial logic to the task

92f1b11

Remove extra argument

ea1ea46

Remove _LinkFramework

06fcb73

Duplicate condition

17055e1

Set nothing for assemblies with blank IsTrimmable

46518c5

agocke mentioned this pull request Jun 25, 2022

Change linker to trim everything by default in 7.0 dotnet/sdk#26246

Merged

sbomer reviewed Jun 27, 2022

View reviewed changes

src/ILLink.Tasks/LinkTask.cs Show resolved Hide resolved

src/ILLink.Tasks/LinkTask.cs Outdated Show resolved Hide resolved

agocke added a commit to agocke/xamarin-android that referenced this pull request Jun 28, 2022

Set TrimMode partial by default

e95be39

Companion to dotnet/linker#2856

agocke mentioned this pull request Jun 28, 2022

Set TrimMode partial by default dotnet/android#7132

Merged

agocke added a commit to agocke/xamarin-macios that referenced this pull request Jun 28, 2022

Set TrimMode to partial if not set

d67523d

Companion to dotnet/linker#2856

agocke mentioned this pull request Jun 28, 2022

Set TrimMode to partial if not set xamarin/xamarin-macios#15368

Merged

agocke added 3 commits June 30, 2022 22:59

Fix after change to actions

c7f5e88

Lint

11e5cd3

Remove unused parameter

83620fa

jonathanpeppers mentioned this pull request Jul 1, 2022

.NET: Unify the MtouchLink and LinkMode MSBuild properties xamarin/xamarin-macios#12796

Closed

Add more assembly metadata testing and change IsTrimmable=false to 'c…

1d93fe4

…opy'

sbomer approved these changes Jul 7, 2022

View reviewed changes

agocke merged commit b6f1fb4 into dotnet:main Jul 7, 2022

agocke deleted the default-trim-action branch July 7, 2022 18:55

MichalStrehovsky mentioned this pull request Jul 20, 2022

Cleanup trimming options and document TrimMode=full,partial dotnet/docs#30279

Merged

rolfbjarne mentioned this pull request Aug 18, 2022

How to link only specific assemblies? #2981

Closed

sbomer mentioned this pull request Aug 22, 2022

Define behavior of AssemblyMetadata("IsTrimmable", "False") #2991

Open

adirh3 mentioned this pull request Sep 16, 2022

CopyUsed with new TrimMode #3039

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more trimming options to TrimMode and change defaults #2856

Add more trimming options to TrimMode and change defaults #2856

agocke commented Jun 22, 2022

agocke commented Jun 22, 2022

MichalStrehovsky left a comment

vitek-karas Jun 22, 2022

sbomer Jun 22, 2022

agocke Jun 22, 2022

sbomer Jun 22, 2022

vitek-karas Jun 22, 2022

agocke Jun 22, 2022

vitek-karas Jun 27, 2022

vitek-karas Jun 22, 2022

akoeplinger commented Jun 22, 2022

marek-safar Jun 22, 2022

jkotas commented Jun 22, 2022

sbomer left a comment

sbomer Jun 22, 2022

agocke commented Jun 22, 2022

rolfbjarne commented Jul 1, 2022 •

edited

Loading

agocke commented Jul 1, 2022

sbomer left a comment

Add more trimming options to TrimMode and change defaults #2856

Add more trimming options to TrimMode and change defaults #2856

Conversation

agocke commented Jun 22, 2022

agocke commented Jun 22, 2022

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akoeplinger commented Jun 22, 2022

Choose a reason for hiding this comment

jkotas commented Jun 22, 2022

sbomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke commented Jun 22, 2022

rolfbjarne commented Jul 1, 2022 • edited Loading

agocke commented Jul 1, 2022

sbomer left a comment

Choose a reason for hiding this comment

rolfbjarne commented Jul 1, 2022 •

edited

Loading