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.

Sign up for GitHub

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

Jump to bottom

[dotnet] Embrace trimming. #20354

Merged
merged 4 commits into from
Apr 22, 2024
Merged

[dotnet] Embrace trimming. #20354

merged 4 commits into from
Apr 22, 2024

Conversation

rolfbjarne
Copy link
Member

As we have solved all trimming warnings in the our workloads, we can now go
all in on trimming.

Early in .NET 6 we hid many trimming warnings as we did not yet plan to solve
them:

<SuppressTrimAnalysisWarnings Condition=" '$(SuppressTrimAnalysisWarnings)' == '' ">true</SuppressTrimAnalysisWarnings>

Ref: #10405

These warnings were not actionable at the time for customers, as
many warnings were from our code.

Going forward, let's stop suppressing these warnings if TrimMode=full.

We can also enable trimming for new projects:

  • dotnet new ios|tvos|macos|maccatalyst

These will have the TrimMode property set to Full by default for Release
builds:

<!--
  Enable full trimming in Release mode.
  To learn more, see: https://learn.microsoft.com/dotnet/core/deploying/trimming/trimming-options#trimming-granularity
-->
<PropertyGroup Condition="'$(Configuration)' == 'Release'">
  <TrimMode>full</TrimMode>
</PropertyGroup>

We wouldn't want to do this for existing projects, as they might have existing
code, NuGet packages, etc. where trimming warnings might be present.

We can also improve the templates for class libraries and binding libraries:

  • dotnet new ioslib|iosbinding|tvoslib|tvosbinding|...
<!--
  Enable trim analyzers for Android class libraries.
  To learn more, see: https://learn.microsoft.com/dotnet/core/deploying/trimming/prepare-libraries-for-trimming
-->
<IsTrimmable>true</IsTrimmable>

This way, new class libraries and binding libraries will be trimmable by
default and be able to react to trimming warnings.

Fixes #10405.

Ref: dotnet/android#8805

@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@rolfbjarne
Copy link
Member Author

Test failures are unrelated (#20264).

@rolfbjarne
[dotnet] Make 'TrimMode=full' the default for release builds in our t…
8e63b07 
…emplates.
@rolfbjarne
[dotnet] Change all library templates to set 'IsTrimmable=true'.
3b82a7d 
This way the trimming analyzers will be enabled for library projects, which
will help towards getting all libraries trimmer safe.
@rolfbjarne
[dotnet] Don't suppress trimmer warnings anymore when 'TrimMode=full'…
ef26cb8 
… is set.
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@rolfbjarne
[dotnet] Disable COM object descriptor support.
ce98e3f 
This is off by default in .NET, but it's turned off in a property group
conditioned on "PublishTrimmed = true", which we only set in a target later in
the build, thus we don't pick up the default.

Fixes:

> Trim analysis error IL2026: System.ComponentModel.TypeDescriptor.NodeFor(Object, Boolean): Using member 'System.ComponentModel.TypeDescriptor.ComObjectType.get' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. COM type descriptors are not trim-compatible.
@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Windows Integration Tests passed 💻

✅ All Windows Integration Tests passed.

Pipeline on Agent
Hash: ce98e3f07fb38fd2bd39d6bef9023f7ecaa7ab7f [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent
Hash: [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS X64 - Mac Sonoma (14) passed 💻

✅ All tests on macOS X64 - Mac Sonoma (14) passed.

Pipeline on Agent
Hash: [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS M1 - Mac Monterey (12) passed 💻

✅ All tests on macOS M1 - Mac Monterey (12) passed.

Pipeline on Agent
Hash: [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)
  • iOS (no change detected)
  • tvOS (no change detected)
  • watchOS (no change detected)
  • macOS (no change detected)
NET (empty diffs)
  • iOS: (empty diff detected)
  • tvOS: (empty diff detected)
  • MacCatalyst: (empty diff detected)
  • macOS: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)
.NET (No breaking changes)
Legacy Xamarin (stable) vs .NET

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: ce98e3f07fb38fd2bd39d6bef9023f7ecaa7ab7f [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: test results.

🎉 All 172 tests passed 🎉

Tests counts

✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (iOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (MacCatalyst): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (macOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (Multiple platforms): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (tvOS): All 1 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ install-source: All 1 tests passed. Html Report (VSDrops) Download
✅ interdependent-binding-projects: All 7 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac-binding-project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 6 tests passed. Html Report (VSDrops) Download
✅ monotouch (iOS): All 11 tests passed. Html Report (VSDrops) Download
✅ monotouch (MacCatalyst): All 8 tests passed. Html Report (VSDrops) Download
✅ monotouch (macOS): All 9 tests passed. Html Report (VSDrops) Download
✅ monotouch (tvOS): All 11 tests passed. Html Report (VSDrops) Download
✅ monotouch (watchOS): All 4 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: ce98e3f07fb38fd2bd39d6bef9023f7ecaa7ab7f [PR build]

@rolfbjarne rolfbjarne marked this pull request as ready for review April 18, 2024 11:05
@rolfbjarne
Copy link
Member Author

CC @jonathanpeppers

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS M1 - Mac Ventura (13) passed 💻

✅ All tests on macOS M1 - Mac Ventura (13) passed.

Pipeline on Agent
Hash: [PR build]

jonathanpeppers
jonathanpeppers approved these changes Apr 18, 2024
View reviewed changes
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just go with what we have here for now. Maybe there is no need for TrimMode=link to work and everyone uses TrimMode=full.

dotnet/targets/Xamarin.Shared.Sdk.props
<_OriginalSuppressTrimAnalysisWarnings>$(SuppressTrimAnalysisWarnings)</_OriginalSuppressTrimAnalysisWarnings>
<SuppressTrimAnalysisWarnings Condition="'$(_UseNativeAot)' == 'true'">true</SuppressTrimAnalysisWarnings>
<!-- Otherwise suppress trimmer warnings unless we're trimming all assemblies -->
<SuppressTrimAnalysisWarnings Condition="'$(SuppressTrimAnalysisWarnings)' == '' And '$(TrimMode)' == 'full'">false</SuppressTrimAnalysisWarnings>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically what is in the Android workload right now, but I noticed you can also use TrimMode=link:

image

I wonder if this TrimMode=link setting is actually wrong? I can't find this in the docs:

And not finding anything in .NET like: <TrimMode Condition="'$(TrimMode)'=='link'">full</TrimMode>

Copy link
Member Author

@rolfbjarne rolfbjarne Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shows up if you select .NET 6 at the top of the page: https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/trimming-options?pivots=dotnet-6-0#trimming-granularity

@rolfbjarne rolfbjarne merged commit 78139df into xamarin:net9.0 Apr 22, 2024
63 checks passed
@rolfbjarne rolfbjarne deleted the TrimModeFull branch April 22, 2024 16:31
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request May 7, 2024
@jonathanpeppers
[build] make TrimMode=full function like NativeAOT
e44a2c3 
Context: dotnet/android#8805
Context: xamarin/xamarin-macios#20354

In .NET 9, we want .NET MAUI applications to be able to use the
`TrimMode=full` option to remove unused code from the application:

    <PropertyGroup Condition="'$(Configuration)' == 'Release'">
      <TrimMode>full</TrimMode>
    </PropertyGroup>

With all the trimming work done to support NativeAOT, we should toggle
the same options when `TrimMode=full` is used:

* `MauiStrictXamlCompilation=true`
* `MauiXamlRuntimeParsingSupport=false`
* `MauiShellSearchResultsRendererDisplayMemberNameSupported=false`
* `MauiQueryPropertyAttributeSupport=false`
* `MauiImplicitCastOperatorsUsageViaReflectionSupport=false`

With these set, the `dotnet new maui` project template *should* have
zero trimmer warnings when `TrimMode=full` is used. Developers can also
adjust these settings and respond to trimmer warnings in their own code.

I also updated `RunOnAndroid` and `RunOniOS` tests to verify that
project templates launch successfully with `TrimMode=full`.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request May 7, 2024
@jonathanpeppers
[build] make TrimMode=full function like NativeAOT
4559dfc 
Context: dotnet/android#8805
Context: xamarin/xamarin-macios#20354

In .NET 9, we want .NET MAUI applications to be able to use the
`TrimMode=full` option to remove unused code from the application:

    <PropertyGroup Condition="'$(Configuration)' == 'Release'">
      <TrimMode>full</TrimMode>
    </PropertyGroup>

With all the trimming work done to support NativeAOT, we should toggle
the same options when `TrimMode=full` is used:

* `MauiStrictXamlCompilation=true`
* `MauiXamlRuntimeParsingSupport=false`
* `MauiShellSearchResultsRendererDisplayMemberNameSupported=false`
* `MauiQueryPropertyAttributeSupport=false`
* `MauiImplicitCastOperatorsUsageViaReflectionSupport=false`

With these set, the `dotnet new maui` project template *should* have
zero trimmer warnings when `TrimMode=full` is used. Developers can also
adjust these settings and respond to trimmer warnings in their own code.

I also updated `RunOnAndroid` and `RunOniOS` tests to verify that
project templates launch successfully with `TrimMode=full`.
@jonathanpeppers jonathanpeppers mentioned this pull request May 7, 2024
Merged
jonathanpeppers added a commit to dotnet/maui that referenced this pull request May 10, 2024
@jonathanpeppers
[build] make TrimMode=full function like NativeAOT (#22282)
a45f702 
Context: dotnet/android#8805
Context: xamarin/xamarin-macios#20354

In .NET 9, we want .NET MAUI applications to be able to use the
`TrimMode=full` option to remove unused code from the application:

    <PropertyGroup Condition="'$(Configuration)' == 'Release'">
      <TrimMode>full</TrimMode>
    </PropertyGroup>

With all the trimming work done to support NativeAOT, we should toggle
the same options when `TrimMode=full` is used:

* `MauiStrictXamlCompilation=true`
* `MauiXamlRuntimeParsingSupport=false`
* `MauiShellSearchResultsRendererDisplayMemberNameSupported=false`
* `MauiQueryPropertyAttributeSupport=false`
* `MauiImplicitCastOperatorsUsageViaReflectionSupport=false`

With these set, the `dotnet new maui` project template *should* have
zero trimmer warnings when `TrimMode=full` is used. Developers can also
adjust these settings and respond to trimmer warnings in their own code.

I also updated `RunOnAndroid` and `RunOniOS` tests to verify that
project templates launch successfully with `TrimMode=full`.

Note:

* Skip `maui-blazor` on iOS for now, as it contains trimmer warnings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathanpeppers jonathanpeppers jonathanpeppers approved these changes

@mandel-macaque mandel-macaque mandel-macaque approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rolfbjarne @vs-mobiletools-engineering-service2 @jonathanpeppers @mandel-macaque