-
Notifications
You must be signed in to change notification settings - Fork 536
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
[One .NET] "greenfield" projects opt into trimming #8805
Conversation
9496e28
to
46df03f
Compare
Context: dotnet/android#8805 When testing new defaults for `TrimMode=full` in xamarin/xamarin-android: dotnet new android dotnet build -c Release -r android-arm64 Results in a single warning: external\Java.Interop\src\Java.Interop\Java.Interop\JniRuntime.JniValueManager.cs(890,4): Trim analysis warning IL2072: Java.Interop.ProxyValueMarshaler.CreateGenericObjectReferenceArgumentState(Object, ParameterAttributes): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.Interfaces' in call to 'Java.Interop.JniRuntime.JniValueManager.GetValueMarshaler(Type)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. This appears to be a case where the trimmer (ILLink's analyzers) can detect a problem where the Roslyn analyzer could not. It is generally possible for the trimmer to discover some new warning, as it has a complete view of the final code. This appears to be similar to some other places we used the justification: [UnconditionalSuppressMessage ("Trimming", "IL2073", Justification = "This code path is not used in Android projects.")]
Context: dotnet/android#8805 When testing new defaults for `$(TrimMode)=full` in dotnet/android#8805: % dotnet new android % dotnet build -c Release -r android-arm64 Results in a single warning: external\Java.Interop\src\Java.Interop\Java.Interop\JniRuntime.JniValueManager.cs(890,4): Trim analysis warning IL2072: Java.Interop.ProxyValueMarshaler.CreateGenericObjectReferenceArgumentState(Object, ParameterAttributes): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.Interfaces' in call to 'Java.Interop.JniRuntime.JniValueManager.GetValueMarshaler(Type)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. This appears to be a case where the trimmer (ILLink's analyzers) can detect a problem where the Roslyn analyzer could not. It is generally possible for the trimmer to discover some new warning, as it has a complete view of the final code. This appears to be similar to some other places we used the justification: [UnconditionalSuppressMessage ("Trimming", "IL2073", Justification = "This code path is not used in Android projects.")]
674d538
to
f677b78
Compare
As we have solved all trimming warnings in the Android workload, we can now go "all in" on trimming. Early in .NET 6 (maybe even 5?) we "hid" many trimming warnings as we did not yet plan to solve them: <SuppressTrimAnalysisWarnings Condition=" '$(SuppressTrimAnalysisWarnings)' == '' ">true</SuppressTrimAnalysisWarnings> These warnings were not *actionable* at the time for customers, as many warnings were in `Mono.Android.dll`, `Java.Interop.dll`, etc. Going forward, let's stop suppressing these warnings if `TrimMode=full`. We can also enable trimming for new projects: * `dotnet new android` * `dotnet new android-wear` These will have the `TrimMode` property set to `Full` by default: <!-- 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 *yet*, as they might have existing code, NuGet packages, etc. where trimming warnings might be present. We can also improve the templates for Android class libraries: * `dotnet new androidlib` * `dotnet new android-bindinglib` <!-- 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 will be "trimmable" by default and be able to react to trimming warnings. We can also use `TrimMode=full` in many of our existing tests: * MSBuild tests that assert 0 warnings can use `TrimMode=full`. * On-device tests can use `TrimMode=full`. ~~ General trimming warnings ~~ This was discovered through `Mono.Android-NET-Tests.csproj`, but there were a few trimmer warnings in the "layout bindings" feature: dotnet\packs\Microsoft.Android.Sdk.Windows\...\tools\LayoutBinding.cs(79,56): warning IL2091: Xamarin.Android.Design.LayoutBinding.<>c__DisplayClass8_0<T>.<FindFragment>b__0(Activity): 'T' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors', 'DynamicallyAccessedMemberTypes.NonPublicConstructors' in 'Android.App.FragmentManager.FindFragmentById<T>(Int32)'. The generic parameter 'T' of 'Xamarin.Android.Design.LayoutBinding.<>c__DisplayClass8_0<T>' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. dotnet\packs\Microsoft.Android.Sdk.Windows\...\tools\LayoutBinding.cs(35,5): warning IL2091: Xamarin.Android.Design.LayoutBinding.FindView<T>(Int32, T&): 'T' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors', 'DynamicallyAccessedMemberTypes.NonPublicConstructors' in 'Android.App.Activity.FindViewById<T>(Int32)'. The generic parameter 'T' of 'Xamarin.Android.Design.LayoutBinding.FindView<T>(Int32, T&)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. dotnet\packs\Microsoft.Android.Sdk.Windows\...\tools\LayoutBinding.cs(37,5): warning IL2091: Xamarin.Android.Design.LayoutBinding.FindView<T>(Int32, T&): 'T' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors', 'DynamicallyAccessedMemberTypes.NonPublicConstructors' in 'Android.Views.View.FindViewById<T>(Int32)'. The generic parameter 'T' of 'Xamarin.Android.Design.LayoutBinding.FindView<T>(Int32, T&)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. We can `[DynamicallyAccessedMembers (Constructors)]` to fix these. ~~ Trimming warnings in tests ~~ Several tests that verify "trimming unsafe features" just specify: [RequiresUnreferencedCode ("Tests trimming unsafe features")] If the test might have an issue under NativeAOT, I used: // FIXME: dotnet#8724 #pragma warning disable IL3050 Places that use `Assembly.GetType()` can use `Type.GetType()` instead: --var JavaProxyThrowable_type = typeof (Java.Lang.Object) -- .Assembly -- .GetType ("Android.Runtime.JavaProxyThrowable"); ++var JavaProxyThrowable_type = Type.GetType ("Android.Runtime.JavaProxyThrowable, Mono.Android"); `AppDomainTest` was just ignored (and had warnings). Updated to just verify `PlatformNotSupportedException` is thrown. ~~ Test failures ~~ `JsonSerializerTest` requires a feature flag: <JsonSerializerIsReflectionEnabledByDefault>true</JsonSerializerIsReflectionEnabledByDefault> Otherwise, an exception is thrown: System.InvalidOperationException : JsonSerializerIsReflectionDisabled `Java.Interop-Tests` were initially not loaded at all. With the log message: 03-13 16:13:59.940 5262 5344 W NUnit : Failed to load tests from assembly 'Java.Interop-Tests, Version=1.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065 If we make `Java.Interop-Tests.dll` a `@(TrimmerRootAssembly)`: <TrimmerRootAssembly Include="Java.Interop-Tests" RootMode="All" /> Then all the tests cases are preserved and can be run, in the same way the "main app assembly" is preserved. `NinePatchTests` failed with: The drawable created from resource tile should be a NinePatchDrawable. Expected: not null But was: null The only usage of `NinePatchDrawable` was: Assert.IsNotNull (d as NinePatchDrawable); `NinePatchDrawable` likely needs its interfaces and constructors preserved for this test to pass. I added an attribute for just `All` for the test to pass: [DynamicDependency (DynamicallyAccessedMemberTypes.All, typeof (NinePatchDrawable))] `CustomWidgetTests` failed with: (Binary XML file line #1 in Mono.Android.NET_Tests:layout/uppercase_custom: Binary XML file line #1 in Mono.Android.NET_Tests:layout/uppercase_custom: Error inflating class Mono.Android_Test.Library.CustomTextView) at Java.Interop.JniEnvironment.InstanceMethods.CallObjectMethod(JniObjectReference , JniMethodInfo , JniArgumentValue* ) at Java.Interop.JniPeerMembers.JniInstanceMethods.InvokeVirtualObjectMethod(String , IJavaPeerable , JniArgumentValue* ) at Android.Views.LayoutInflater.Inflate(Int32 , ViewGroup ) at Xamarin.Android.RuntimeTests.CustomWidgetTests.<>c.<UpperCaseCustomWidget_ShouldNotThrowInflateException>b__0_0() at NUnit.Framework.Constraints.VoidInvocationDescriptor.Invoke() at NUnit.Framework.Constraints.ExceptionInterceptor.Intercept(Object ) --- End of managed Java.Lang.RuntimeException stack trace --- android.view.InflateException: Binary XML file line #1 in Mono.Android.NET_Tests:layout/uppercase_custom: Binary XML file line #1 in Mono.Android.NET_Tests:layout/uppercase_custom: Error inflating class Mono.Android_Test.Library.CustomTextView Caused by: android.view.InflateException: Binary XML file line #1 in Mono.Android.NET_Tests:layout/uppercase_custom: Error inflating class Mono.Android_Test.Library.CustomTextView Caused by: java.lang.ClassNotFoundException: Mono.Android_Test.Library.CustomTextView at java.lang.Class.classForName(Native Method) at java.lang.Class.forName(Class.java:454) at android.view.LayoutInflater.createView(LayoutInflater.java:815) at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:1006) at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:961) at android.view.LayoutInflater.rInflate(LayoutInflater.java:1123) at android.view.LayoutInflater.rInflateChildren(LayoutInflater.java:1084) at android.view.LayoutInflater.inflate(LayoutInflater.java:682) at android.view.LayoutInflater.inflate(LayoutInflater.java:534) at android.view.LayoutInflater.inflate(LayoutInflater.java:481) at crc643df67da7b13bb6b1.TestInstrumentation_1.n_onStart(Native Method) at crc643df67da7b13bb6b1.TestInstrumentation_1.onStart(TestInstrumentation_1.java:32) at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2189) Caused by: java.lang.ClassNotFoundException: Didn't find class "Mono.Android_Test.Library.CustomTextView" on path In this case, `Mono.Android_Test.Library.CustomTextView` was used from an Android layout, but not used anywhere in managed code. To fix, I added: [DynamicDependency (DynamicallyAccessedMemberTypes.All, typeof (Mono.Android_Test.Library.CustomTextView))] I could have also made `Mono.Android_Test.Library` a `@(TrimmerRootAssembly)`.
75c7028
to
b521911
Compare
@@ -25,6 +25,7 @@ public class NinePatchTests | |||
}; | |||
|
|||
[Test, TestCaseSource (nameof (NinePatchDrawables))] | |||
[DynamicDependency (DynamicallyAccessedMemberTypes.All, typeof (NinePatchDrawable))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was failing, where the only usage of NinePatchDrawable
in C# was:
Assert.IsNotNull (d as NinePatchDrawable);
And so the type NinePatchDrawable
existed, but with everything trimmed away.
@@ -12,6 +14,7 @@ public class CustomWidgetTests | |||
{ | |||
// https://bugzilla.xamarin.com/show_bug.cgi?id=23880 | |||
[Test] | |||
[DynamicDependency (DynamicallyAccessedMemberTypes.All, typeof (CustomTextView))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, Mono.Android_Test.Library.CustomTextView
was used from an Android layout, but not used anywhere in managed code. It threw:
(Binary XML file line #1 in Mono.Android.NET_Tests:layout/uppercase_custom: Binary XML file line #1 in Mono.Android.NET_Tests:layout/uppercase_custom: Error inflating class Mono.Android_Test.Library.CustomTextView)
at Java.Interop.JniEnvironment.InstanceMethods.CallObjectMethod(JniObjectReference , JniMethodInfo , JniArgumentValue* )
at Java.Interop.JniPeerMembers.JniInstanceMethods.InvokeVirtualObjectMethod(String , IJavaPeerable , JniArgumentValue* )
at Android.Views.LayoutInflater.Inflate(Int32 , ViewGroup )
at Xamarin.Android.RuntimeTests.CustomWidgetTests.<>c.<UpperCaseCustomWidget_ShouldNotThrowInflateException>b__0_0()
at NUnit.Framework.Constraints.VoidInvocationDescriptor.Invoke()
at NUnit.Framework.Constraints.ExceptionInterceptor.Intercept(Object )
--- End of managed Java.Lang.RuntimeException stack trace ---
android.view.InflateException: Binary XML file line #1 in Mono.Android.NET_Tests:layout/uppercase_custom: Binary XML file line #1 in Mono.Android.NET_Tests:layout/uppercase_custom: Error inflating class Mono.Android_Test.Library.CustomTextView
Caused by: android.view.InflateException: Binary XML file line #1 in Mono.Android.NET_Tests:layout/uppercase_custom: Error inflating class Mono.Android_Test.Library.CustomTextView
Caused by: java.lang.ClassNotFoundException: Mono.Android_Test.Library.CustomTextView
at java.lang.Class.classForName(Native Method)
at java.lang.Class.forName(Class.java:454)
at android.view.LayoutInflater.createView(LayoutInflater.java:815)
at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:1006)
at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:961)
at android.view.LayoutInflater.rInflate(LayoutInflater.java:1123)
at android.view.LayoutInflater.rInflateChildren(LayoutInflater.java:1084)
at android.view.LayoutInflater.inflate(LayoutInflater.java:682)
at android.view.LayoutInflater.inflate(LayoutInflater.java:534)
at android.view.LayoutInflater.inflate(LayoutInflater.java:481)
at crc643df67da7b13bb6b1.TestInstrumentation_1.n_onStart(Native Method)
at crc643df67da7b13bb6b1.TestInstrumentation_1.onStart(TestInstrumentation_1.java:32)
at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2189)
Caused by: java.lang.ClassNotFoundException: Didn't find class "Mono.Android_Test.Library.CustomTextView" on path
<!-- | ||
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the new project template change for apps.
<!-- | ||
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this would be the project template change for Android class libraries, if we think this is a good idea.
<SuppressTrimAnalysisWarnings Condition=" '$(SuppressTrimAnalysisWarnings)' == '' and '$(TrimMode)' == 'full' ">false</SuppressTrimAnalysisWarnings> | ||
<SuppressTrimAnalysisWarnings Condition=" '$(SuppressTrimAnalysisWarnings)' == '' ">true</SuppressTrimAnalysisWarnings> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @rolfbjarne this is the only defaults I changed. So if TrimMode=full
, customers will see all the trimming warnings.
Existing apps, that are TrimMode=partial
. I don't think they should appear when upgrading to .NET 9, as they might have 100s of warnings in NuGet packages?
* main: [ci] Use managed identity for ApiScan (#8823) [Xamarin.Android.Build.Tasks] DTBs should not rm generator output (#8706) [Xamarin.Android.Build.Tasks] Bump to NuGet 6.7.1 (#8833) $(AndroidPackVersionSuffix)=preview.4; net9 is 34.99.0.preview.4 (#8831) Localized file check-in by OneLocBuild Task (#8824) [Xamarin.Android.Build.Tasks] Enable POM verification features. (#8649) [runtime] Optionally disable inlining (#8798) Fix assembly count when satellite assemblies are present (#8790) [One .NET] new "greenfield" projects are trimmed by default (#8805) Localized file check-in by OneLocBuild Task (#8819) LEGO: Merge pull request 8820
* main: [ci] Use managed identity for ApiScan (#8823) [Xamarin.Android.Build.Tasks] DTBs should not rm generator output (#8706) [Xamarin.Android.Build.Tasks] Bump to NuGet 6.7.1 (#8833) $(AndroidPackVersionSuffix)=preview.4; net9 is 34.99.0.preview.4 (#8831) Localized file check-in by OneLocBuild Task (#8824) [Xamarin.Android.Build.Tasks] Enable POM verification features. (#8649) [runtime] Optionally disable inlining (#8798) Fix assembly count when satellite assemblies are present (#8790) [One .NET] new "greenfield" projects are trimmed by default (#8805) Localized file check-in by OneLocBuild Task (#8819) LEGO: Merge pull request 8820 LEGO: Merge pull request 8818 Bump to dotnet/installer@b40c44502d 9.0.100-preview.3.24165.20 (#8817) Bump com.android.tools:r8 from 8.2.47 to 8.3.37 (#8816) [Mono.Android] Prevent NullPointerException in TranslateStackTrace (#8795) Localized file check-in by OneLocBuild Task (#8815) [Xamarin.Android.Build.Tasks] Make all assemblies RID-specific (#8478) Localized file check-in by OneLocBuild Task (#8813)
* main: Bump to dotnet/installer@dc43d363d2 9.0.100-preview.4.24175.5 (#8828) [Xamarin.Android.Build.Tasks] Update to newer ILRepack which supports debug files. (#8839) Bump 'NuGet.*' and 'Newtonsoft.Json' NuGet versions. (#8825) Localized file check-in by OneLocBuild Task (#8844) [LayoutBindings] Fix '[Preserve]' is obsolete warnings (#8529) LEGO: Merge pull request 8837 [ci] Use managed identity for ApiScan (#8823) [Xamarin.Android.Build.Tasks] DTBs should not rm generator output (#8706) [Xamarin.Android.Build.Tasks] Bump to NuGet 6.7.1 (#8833) $(AndroidPackVersionSuffix)=preview.4; net9 is 34.99.0.preview.4 (#8831) Localized file check-in by OneLocBuild Task (#8824) [Xamarin.Android.Build.Tasks] Enable POM verification features. (#8649) [runtime] Optionally disable inlining (#8798) Fix assembly count when satellite assemblies are present (#8790) [One .NET] new "greenfield" projects are trimmed by default (#8805) Localized file check-in by OneLocBuild Task (#8819) LEGO: Merge pull request 8820
As we have solved all trimming warnings in the Android workload, we
can now go "all in" on trimming.
Early in .NET 6 (maybe even 5?) we "hid" many trimming warnings as we
did not yet plan to solve them:
These warnings were not actionable at the time for customers, as
many warnings were in
Mono.Android.dll
,Java.Interop.dll
, etc.Going forward, let's stop suppressing these warnings if
TrimMode=full
.We can also enable trimming for new projects:
dotnet new android
dotnet new android-wear
These will have the
TrimMode
property set toFull
by default:We wouldn't want to do this for existing projects yet, as they might
have existing code, NuGet packages, etc. where trimming warnings might
be present.
We can also improve the templates for Android class libraries:
dotnet new androidlib
dotnet new android-bindinglib
This way, new class libraries will be "trimmable" by default and be
able to react to trimming warnings.
We can also use
TrimMode=full
in many of our existing tests:MSBuild tests that assert 0 warnings can use
TrimMode=full
.On-device tests can use
TrimMode=full
.General trimming warnings
This was discovered through
Mono.Android-NET-Tests.csproj
, but therewere a few trimmer warnings in the "layout bindings" feature:
We can
[DynamicallyAccessedMembers (Constructors)]
to fix these.Trimming warnings in tests
Several tests that verify "trimming unsafe features" just specify:
If the test might have an issue under NativeAOT, I used:
Places that use
Assembly.GetType()
can useType.GetType()
instead:AppDomainTest
was just ignored (and had warnings). Updated to justverify
PlatformNotSupportedException
is thrown.Test failures
JsonSerializerTest
requires a feature flag:Otherwise, an exception is thrown:
Java.Interop-Tests
were initially not loaded at all. With the logmessage:
If we make
Java.Interop-Tests.dll
a@(TrimmerRootAssembly)
:Then all the tests cases are preserved and can be run, in the same way
the "main app assembly" is preserved.
NinePatchTests
failed with:The only usage of
NinePatchDrawable
was:NinePatchDrawable
likely needs its interfaces and constructorspreserved for this test to pass. I added an attribute for just
All
for the test to pass:
CustomWidgetTests
failed with:In this case,
Mono.Android_Test.Library.CustomTextView
was used froman Android layout, but not used anywhere in managed code.
To fix, I added:
I could have also made
Mono.Android_Test.Library
a@(TrimmerRootAssembly)
.