dotnet / android

.NET for Android provides open-source bindings of the Android SDK for use with .NET managed languages such as C#
MIT License
1.93k stars 532 forks source link

[trimmer] pass `--disable-opt unusedtypechecks` #9435

Closed jonathanpeppers closed 1 month ago

jonathanpeppers commented 1 month ago

Fixes: https://github.com/dotnet/android/issues/9399 An alternative to: https://github.com/dotnet/android/pull/9411

The .NET trimmer will inline cast statements such as:

var d = Application.Context.Resources.GetDrawable (resId);
Assert.IsNotNull (d as NinePatchDrawable);

If NinePatchDrawable is not "instantiated" anywhere via new(), inlined to:

var d = Application.Context.Resources.GetDrawable (resId);
Assert.IsNotNull (null); // BOOM!

We can solve this by passing --disable-opt unusedtypechecks, with $(_AndroidEnableUnusedTypeChecks) as a private MSBuild property to opt out if needed.

Since Java can create these objects, this feels like the reasonable fix for .NET 9 GA.

I also fixed the $(RuntimeIdentifiers) in TestApks.targets to check if someone passed -r android-arm64, for example.

jonathanpeppers commented 1 month ago

The trimmer issue looks OK:

image

As well as the apk size regression test:

image

vitek-karas commented 1 month ago

@sbomer FYI

I must admit I'm not a fan of this solution. It basically says that we know there are trimming holes in the system and we're going to try to avoid them by disabling certain optimizations in the trimmer. The problem is that trimmer may make similar assumptions elsewhere and we didn't catch it yet - or it will start to do so in the next release for example. I'd say as a stop-gap solution for .NET 9, this is OKish, but we should really fix this as soon as possible. Especially in light of the full trim mode "support", which will likely make this problem just worse.

jonpryor commented 1 month ago

Linux tests keep failing for "bizarre" reasons:

Workload installation failed: Version 35.0.1-ci.pr.gh9435.57 of package microsoft.android.sdk.linux is not found in NuGet feeds /mnt/vss/_work/1/s/bin/BuildRelease/nuget-unsigned/;https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-android-df9aaf29-1/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8-transport/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9-transport/nuget/v3/index.json;https://pkgs.dev.azure.com/xamarin/public/_packaging/Xamarin.Android/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-emsdk-91b783ed/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-runtime-ef07c4f2/nuget/v3/index.json.
##[error]build-tools/create-packs/Directory.Build.targets(133,5): Error MSB3073: The command ""/mnt/vss/_work/1/s/bin/Release/dotnet/dotnet" workload install android --skip-manifest-update --skip-sign-check --verbosity diag --source "/mnt/vss/_work/1/s/bin/BuildRelease/nuget-unsigned/" --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-android-df9aaf29-1/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-android-df9aaf29-1/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8-transport/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8-transport/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9-transport/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9-transport/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/xamarin/public/_packaging/Xamarin.Android/nuget/v3/index.json"](https://pkgs.dev.azure.com/xamarin/public/_packaging/Xamarin.Android/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-emsdk-91b783ed/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-emsdk-91b783ed/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-runtime-ef07c4f2/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-runtime-ef07c4f2/nuget/v3/index.json%22) --temp-dir "obj/Release/.xa-workload-temp-r4whol04.5k5"" exited with code 1.

Retrying the whole thing.

jonpryor commented 1 month ago

/azp run

azure-pipelines[bot] commented 1 month ago
Azure Pipelines successfully started running 1 pipeline(s).
jonathanpeppers commented 1 month ago

@sbomer FYI

I must admit I'm not a fan of this solution. It basically says that we know there are trimming holes in the system and we're going to try to avoid them by disabling certain optimizations in the trimmer. The problem is that trimmer may make similar assumptions elsewhere and we didn't catch it yet - or it will start to do so in the next release for example. I'd say as a stop-gap solution for .NET 9, this is OKish, but we should really fix this as soon as possible. Especially in light of the full trim mode "support", which will likely make this problem just worse.

@vitek-karas yes, @sbomer and I chose this as a better idea than https://github.com/dotnet/android/pull/9411. We would like to find a better solution later, especially if there is NativeAOT support for Android. Perhaps .NET 10?

sbomer commented 1 month ago

I don't think this is a reliable long-term solution, but my understanding from conversation with @jonathanpeppers is that we don't have a way to know ahead of time which types may be created from java.

For future reference, this fix is effectively adding a heuristic that says "if there is a type check for a type (and it implements IJavaObject), then assume it may be created from the java side". If we had a source of truth that could tell us which IJavaObject implementors need to be kept, we could rely on that instead and the type check removal optimization wouldn't cause problems.