Support attribute trimming opt-in #1839

sbomer · 2021-02-17T02:54:31Z

This implements the linker side of the attribute opt-in described at https://github.com/mono/linker/blob/main/docs/design/trimmed-assemblies.md#assemblymetadataistrimmable-true.

docs/error-codes.md

docs/illink-options.md

src/linker/Linker/LinkContext.cs

marek-safar · 2021-02-17T09:20:19Z

src/linker/Linker/Driver.cs

+			Console.WriteLine ("  --default-action ACTION   Sets action for assemblies that have no trimmability annotation. Defaults to 'link'");
+			Console.WriteLine ("  --action ACTION ASM       Overrides the default action for specific assembly name");


Do we need two options? What about simpler

-action ACTION Sets action for all assemblies that have no IsTrimmable attribute. Defaults to 'link' -action ACTION ASM Overrides the default action for the specific assembly name

I took your naming suggestions. It's a little odd because --action ACTION ASM will override not just the --action ACTION for unattributed assemblies, but also the --trim-mode ACTION for ones with IsTrimmable. In other words, this might give the impression that there's only one "default" action, when really there are two - the --trim-mode and the --action. That's why I initially picked the names --trim-action and --default-action.

However I do like the consistency of your suggestion. Either way we will need to think carefully about the names on the SDK side. The SDK currently uses TrimMode for both the global --trim-mode and the per-assembly --action.

src/linker/Linker/Driver.cs

marek-safar · 2021-02-17T09:27:02Z

src/linker/Linker.Steps/RootNonTrimmableAssemblies.cs

+
+namespace Mono.Linker.Steps
+{
+	public class RootNonTrimmableAssemblies : BaseStep


I don't understand why this step is needed?

It's needed for example if someone passes -reference UnreferencedAssembly --action copy UnreferencedAssembly - then we need to make sure the unreferenced assembly is kept. This could happen if someone has an unused nuget package dependency (or one that's used via non-understood reflection).

Why would we want to do that? It would keep unused assembly which someone redundantly passed as an argument. We have -a option which does the same thing and should be used if it's intentional.

Maybe a better example is -reference UnreferencedAssembly --action copy (setting the default action to copy instead). In this case we would need to root the assembly if it isn't attributed as IsTrimmable.

If someone publishes an app with a (statically unreferenced) nuget dependency, without trimming, they would expect to see it in the output. Any non-IsTrimmable assemblies should similarly be kept with PublishTrimmed.

We don't want to pass -a for such cases because it would prevent trimming for dependencies that are IsTrimmable.

Then I would do the same with the per-assembly copy action for consistency (this is the first suggestion I made in #1800 (comment)).

src/linker/Linker/AssemblyResolver.cs

src/linker/Linker/LinkContext.cs

marek-safar · 2021-02-17T09:32:46Z

@sbomer it'd be useful to add the attribute to the runtime build before this change goes in to test it

on Windows

- Don't warn on duplicate attributes - Remove comment - Fix typos and wording - Use static array - Rename CheckIsTrimmable -> IsTrimmable

- --trim-action -> --trim-mode -- --default-action -> --action

sbomer · 2021-02-17T23:11:19Z

Currently the tests pass --action skip (or whatever action is specified by SetupLinkerTrimMode in the test) for the framework assemblies, which simulates the attribute. (Skip to avoid copying and analyzing assemblies that are irrelevant to most testcases). Once the attributed framework assemblies flow into the linker we can get rid of this.

src/linker/Linker.Steps/RootNonTrimmableAssemblies.cs

- Add plenty of comments - RootNonTrimmableAssemblies -> ProcessReferencesStep - Make -reference order stable using List instead of HashSet

src/linker/Linker.Steps/ProcessReferencesStep.cs

marek-safar · 2021-02-19T12:21:55Z

src/linker/Linker.Steps/ProcessReferencesStep.cs

+			}
+		}
+
+		public static bool IsFullyPreservedAction (AssemblyAction action)


nit: should be private

This one is used also from MarkStep

marek-safar

Please hold the merge after .NET6 P2 SDK is snapped

Make GetInputAssemblyPaths private

This implements the SDK side of the behavior described at https://github.com/mono/linker/blob/main/docs/design/trimmed-assemblies.md#net-6. This includes a linker update with dotnet/linker#1839.

* Enable additional ways to opt in to trimming This implements the SDK side of the behavior described at https://github.com/mono/linker/blob/main/docs/design/trimmed-assemblies.md#net-6. This includes a linker update with dotnet/linker#1839. * PR feedback - use JoinItems task :) - always set _TrimmerDefaultAction

* Support attribute opt-in * Update docs * Rename test attributes to match * Don't pass native SDK assemblies to tests on Windows * Update LinkTask * PR feedback - Don't warn on duplicate attributes - Remove comment - Fix typos and wording - Use static array - Rename CheckIsTrimmable -> IsTrimmable * PR feedback: rename options - --trim-action -> --trim-mode -- --default-action -> --action * Fix ILLink.Tasks test * PR feedback - Add plenty of comments - RootNonTrimmableAssemblies -> ProcessReferencesStep - Make -reference order stable using List instead of HashSet * PR feedback Make GetInputAssemblyPaths private Commit migrated from dotnet/linker@44907d9

sbomer added 3 commits February 16, 2021 18:44

Support attribute opt-in

d1a6b3d

Update docs

6011ad3

Rename test attributes to match

73b2c4a

sbomer requested a review from marek-safar as a code owner February 17, 2021 02:54

sbomer changed the title ~~Support attribute trimming opt-in~~ [WIP] Support attribute trimming opt-in Feb 17, 2021

marek-safar reviewed Feb 17, 2021

View reviewed changes

sbomer added 5 commits February 17, 2021 11:23

Don't pass native SDK assemblies to tests

6a72f8d

on Windows

Update LinkTask

7126322

PR feedback

308fe09

- Don't warn on duplicate attributes - Remove comment - Fix typos and wording - Use static array - Rename CheckIsTrimmable -> IsTrimmable

PR feedback: rename options

0348c04

- --trim-action -> --trim-mode -- --default-action -> --action

Fix ILLink.Tasks test

7e793f4

sbomer mentioned this pull request Feb 17, 2021

Annotate framework assemblies as trimmable dotnet/runtime#48428

Merged

sbomer changed the title ~~[WIP] Support attribute trimming opt-in~~ Support attribute trimming opt-in Feb 17, 2021

sbomer requested a review from vitek-karas February 17, 2021 23:12

sbomer commented Feb 18, 2021

View reviewed changes

src/linker/Linker.Steps/RootNonTrimmableAssemblies.cs Outdated Show resolved Hide resolved

PR feedback

f4fab05

- Add plenty of comments - RootNonTrimmableAssemblies -> ProcessReferencesStep - Make -reference order stable using List instead of HashSet

marek-safar reviewed Feb 19, 2021

View reviewed changes

src/linker/Linker.Steps/ProcessReferencesStep.cs Outdated Show resolved Hide resolved

marek-safar reviewed Feb 19, 2021

View reviewed changes

marek-safar approved these changes Feb 19, 2021

View reviewed changes

PR feedback

bbb7b68

Make GetInputAssemblyPaths private

eerhardt mentioned this pull request Feb 22, 2021

Add infrastructure to test WASM linking dotnet/aspnetcore#29900

Merged

4 tasks

sbomer merged commit 44907d9 into dotnet:main Feb 25, 2021

sbomer mentioned this pull request Feb 26, 2021

Enable additional ways to opt in to trimming dotnet/sdk#16094

Merged

eerhardt mentioned this pull request Mar 23, 2021

Blazor WASM app leaves untrimmed System.IO.Pipelines assembly by default dotnet/aspnetcore#27782

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support attribute trimming opt-in #1839

Support attribute trimming opt-in #1839

sbomer commented Feb 17, 2021

marek-safar Feb 17, 2021

sbomer Feb 17, 2021 •

edited

Loading

marek-safar Feb 17, 2021

sbomer Feb 17, 2021

marek-safar Feb 18, 2021

sbomer Feb 18, 2021

marek-safar commented Feb 17, 2021

sbomer commented Feb 17, 2021 •

edited

Loading

marek-safar Feb 19, 2021

sbomer Feb 19, 2021

marek-safar left a comment

		Console.WriteLine (" --default-action ACTION Sets action for assemblies that have no trimmability annotation. Defaults to 'link'");
		Console.WriteLine (" --action ACTION ASM Overrides the default action for specific assembly name");

Support attribute trimming opt-in #1839

Support attribute trimming opt-in #1839

Conversation

sbomer commented Feb 17, 2021

marek-safar Feb 17, 2021

Choose a reason for hiding this comment

sbomer Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

marek-safar Feb 17, 2021

Choose a reason for hiding this comment

sbomer Feb 17, 2021

Choose a reason for hiding this comment

marek-safar Feb 18, 2021

Choose a reason for hiding this comment

sbomer Feb 18, 2021

Choose a reason for hiding this comment

marek-safar commented Feb 17, 2021

sbomer commented Feb 17, 2021 • edited Loading

marek-safar Feb 19, 2021

Choose a reason for hiding this comment

sbomer Feb 19, 2021

Choose a reason for hiding this comment

marek-safar left a comment

Choose a reason for hiding this comment

sbomer Feb 17, 2021 •

edited

Loading

sbomer commented Feb 17, 2021 •

edited

Loading