-
Notifications
You must be signed in to change notification settings - Fork 532
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
[trimming] preserve custom views and $(AndroidHttpClientHandlerType)
#8954
Conversation
Fixes: dotnet#8797 Here are two cases `TrimMode=full` can break applications: * `$(AndroidHttpClientHandlerType)` set to a custom type * Custom views (Android `.xml`) that are not referenced in C# code In the `MarkJavaObjects` trimmer step we can preserve both of these cases by: * Passing in `$(AndroidHttpClientHandlerType)`, preserve the public, parameterless constructor of the type * Pass in `$(_CustomViewMapFile)`, preserve `IJavaObject` types if they are found in the map file
markContext.RegisterMarkTypeAction (type => ProcessType (type)); | ||
} | ||
|
||
bool IsActiveFor (AssemblyDefinition assembly) | ||
{ | ||
return assembly.MainModule.HasTypeReference ("System.Net.Http.HttpMessageHandler") || assembly.MainModule.HasTypeReference ("Android.Util.IAttributeSet"); |
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.
The point of this, is we don't want to iterate over every type in every assembly.
Is Android.Util.IAttributeSet
required for any custom view?
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.
See:
- https://stackoverflow.com/a/36342089/83444
- https://ataulm.com/2019/10/28/resolving-view-attributes.html
IAttributeSet
is used so that the View can obtain any XML attributes specified on the view. The above stack overflow answer has a good example of this:
<com.anjithsasindran.RectangleView
app:radiusDimen="5dp"
app:rectangleBackground="@color/yellow"
app:circleBackground="@color/green" />
The AttributeSet
is how the RectangleView
constructor would lookup the values for @app:radiusDimen
, @app:rectangleBackground
, and @app:circleBackground
.
I believe, but cannot quickly verify, that the (Context, IAttributeSet)
constructor is always used/preferred by Android.
// NOTE: ensure the C# compiler has a reference to the library | ||
new Mono.Android_Test.Library.Foo (); |
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 was another problem, when we have a @(ProjectReference)
to the class library containing Mono.Android_Test.Library
-- there was no C# code using types from the library. And so the C# compiler would omit the assembly reference, and the trimmer did not even receive it as input.
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.
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.
#8904 is unrelated.
The problem here is if you add a reference to a class library not using any types from it, and one of the AndroidResource
.xml
files uses a custom view. The C# compiler will ignore the assembly, but the AndroidResource
files would still make it to the app.
I'm not sure if this would happen in practice/real customer projects.
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.
I filed this for the future, and put it in comments:
@@ -48,6 +48,8 @@ public void ProcessAssembly (AssemblyDefinition assembly, string androidHttpClie | |||
} | |||
|
|||
// Custom views in Android .xml files | |||
if (!type.ImplementsIJavaObject (cache)) |
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.
👍
set.Add (value); | ||
} | ||
return map; | ||
return LoadCustomViewMapFile (mapFile); |
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.
Shouldn't we "register" mapFile
? (And why didn't we do this before, given that the beginning of this method calls .GetRegisteredTaskObjectAssemblyLocal<…>(…)
?)
var map = LoadCustomViewMapFile (mapFile);
if (map != null) {
engine.RegisterTaskObject (mapFile, map, RegisteredTaskObjectLifetime.Build, allowEarlyCollection: false);
}
return map;
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.
The RegisterTaskObjectAssemblyLocall call happens inside LoadCustomViewMapFile
see https://github.com/xamarin/xamarin-android/blob/a9706b6ef0429250ecaf1e500d77cd19e94e2eb5/src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs#L408
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.
Oh the code changed... ah cos of the linker can't use RegisterTaskObject
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.
Sorry just actually read your comment (its early). The Registration of the file happens in SaveCustomViewMapFile
.
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.
Yeah, the new method that doesn't support RegisterTaskObject
is for the linker/trimmer step assembly. It doesn't have access to MSBuild APIs.
* main: (26 commits) Make APK and shared library alignment configurable (#9046) [r8] update proguard rule to keep .NET runtime classes (#9044) Explicitly align to 4k (#9041) [trimming] preserve custom views and `$(AndroidHttpClientHandlerType)` (#8954) Ignore split configs when bundle config moves shared libraries to base.apk (#8987) Bump to dotnet/android-tools@1c09dcc (#9026) Bump to dotnet/java-interop@ccafbe6 (#9025) [Mono.Android-Tests] Fix repo URL in redirect tests (#9035) [ci] Update checkout path for nightly build (#9028) [ci] Fix android source path for MAUI test job (#9030) Link Code of Conduct (#9034) [ci] Update sdk-insertions trigger to manual only (#9029) Update java-interop and android-tools submodule mentions (#9023) LEGO: Merge pull request 9022 [Xamarin.Android.Build.Tasks] fastdev works with aab files (#8990) Use new binutils URL (#9019) Localized file check-in by OneLocBuild Task: Build definition ID 17928: Build ID 9686669 (#9011) LEGO: Merge pull request 9015 [api-merge] Update "constant" values to mirror latest API levels (#9004) [Mono.Android] Fix wrong value for `ApplicationExitInfoReason.Other` (#9003) ...
Fixes: #8797
Here are two cases
TrimMode=full
can break applications:$(AndroidHttpClientHandlerType)
set to a custom typeCustom views (Android
.xml
) that are not referenced in C# codeIn the
MarkJavaObjects
trimmer step we can preserve both of these cases by:Passing in
$(AndroidHttpClientHandlerType)
, preserve the public, parameterless constructor of the typePass in
$(_CustomViewMapFile)
, preserveIJavaObject
types if they are found in the map file