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

[trimming] preserve custom views and $(AndroidHttpClientHandlerType) #8954

Merged
merged 6 commits into from
Jun 17, 2024
Merged

[trimming] preserve custom views and $(AndroidHttpClientHandlerType) #8954

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2163693
[trimming] preserve custom views and `$(AndroidHttpClientHandlerType)`
jonathanpeppers May 13, 2024
cebeb8a
Fix apk tests
jonathanpeppers May 16, 2024
ef77b4e
Skip custom views if not IJavaObject
jonathanpeppers May 17, 2024
aef660c
Merge branch 'main' into issue8797take2
jonathanpeppers Jun 6, 2024
7d84df7
https://github.com/xamarin/xamarin-android/issues/9008
jonathanpeppers Jun 6, 2024
02e0514
Put namespace back
jonathanpeppers Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions src/Microsoft.Android.Sdk.ILLink/MarkJavaObjects.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Mono.Linker;
using Mono.Linker.Steps;
using Mono.Tuner;
using Java.Interop.Tools.Cecil;
using Xamarin.Android.Tasks;

namespace MonoDroid.Tuner {
Expand All @@ -16,9 +17,46 @@ public class MarkJavaObjects : BaseMarkHandler
public override void Initialize (LinkContext context, MarkContext markContext)
{
base.Initialize (context, markContext);
context.TryGetCustomData ("AndroidHttpClientHandlerType", out string androidHttpClientHandlerType);
context.TryGetCustomData ("AndroidCustomViewMapFile", out string androidCustomViewMapFile);
var customViewMap = MonoAndroidHelper.LoadCustomViewMapFile (androidCustomViewMapFile);

markContext.RegisterMarkAssemblyAction (assembly => ProcessAssembly (assembly, androidHttpClientHandlerType, customViewMap));
markContext.RegisterMarkTypeAction (type => ProcessType (type));
}

bool IsActiveFor (AssemblyDefinition assembly)
{
return assembly.MainModule.HasTypeReference ("System.Net.Http.HttpMessageHandler") || assembly.MainModule.HasTypeReference ("Android.Util.IAttributeSet");
Copy link
Member Author

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?

https://github.com/xamarin/xamarin-android/blob/14b70ac32b2a9efefae127795e96ef73124c8557/tests/Mono.Android-Tests/Mono.Android-Test.Library/CustomTextView.cs#L9

Copy link
Member

Choose a reason for hiding this comment

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

See:

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.

}

public void ProcessAssembly (AssemblyDefinition assembly, string androidHttpClientHandlerType, Dictionary<string, HashSet<string>> customViewMap)
{
if (!IsActiveFor (assembly))
return;

foreach (var type in assembly.MainModule.Types) {
// Custom HttpMessageHandler
if (!string.IsNullOrEmpty (androidHttpClientHandlerType) &&
androidHttpClientHandlerType.StartsWith (type.Name, StringComparison.Ordinal)) {
var assemblyQualifiedName = type.GetPartialAssemblyQualifiedName (Context);
if (assemblyQualifiedName == androidHttpClientHandlerType) {
Annotations.Mark (type);
PreservePublicParameterlessConstructors (type);
continue;
}
}

// Custom views in Android .xml files
jonathanpeppers marked this conversation as resolved.
Show resolved Hide resolved
if (!type.ImplementsIJavaObject (cache))
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

continue;
if (customViewMap.ContainsKey (type.FullName)) {
Annotations.Mark (type);
PreserveJavaObjectImplementation (type);
}
}
}

public void ProcessType (TypeDefinition type)
{
// If this isn't a JLO or IJavaObject implementer,
Expand Down Expand Up @@ -48,6 +86,21 @@ void PreserveJavaObjectImplementation (TypeDefinition type)
PreserveInterfaces (type);
}

void PreservePublicParameterlessConstructors (TypeDefinition type)
{
if (!type.HasMethods)
return;

foreach (var constructor in type.Methods)
{
if (!constructor.IsConstructor || constructor.IsStatic || !constructor.IsPublic || constructor.HasParameters)
continue;

PreserveMethod (type, constructor);
break; // We can stop when found
}
}

void PreserveAttributeSetConstructor (TypeDefinition type)
{
if (!type.HasMethods)
Expand Down
4 changes: 1 addition & 3 deletions src/Mono.Android/Android.Runtime/AndroidEnvironment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,9 +336,7 @@ static IWebProxy GetDefaultProxy ()
[DynamicDependency (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor, typeof (Xamarin.Android.Net.AndroidMessageHandler))]
static object GetHttpMessageHandler ()
{
// FIXME: https://github.com/xamarin/xamarin-android/issues/8797
// Note that this is a problem for custom $(AndroidHttpClientHandlerType) or $XA_HTTP_CLIENT_HANDLER_TYPE
[UnconditionalSuppressMessage ("Trimming", "IL2057", Justification = "DynamicDependency should preserve AndroidMessageHandler.")]
[UnconditionalSuppressMessage ("Trimming", "IL2057", Justification = "Preserved by the MarkJavaObjects trimmer step.")]
[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
static Type TypeGetType (string typeName) =>
Type.GetType (typeName, throwOnError: false);
Expand Down
1 change: 1 addition & 0 deletions ...uild.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.AssemblyResolution.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ _ResolveAssemblies MSBuild target.
;_OuterIntermediateAssembly=@(IntermediateAssembly)
;_OuterOutputPath=$(OutputPath)
;_OuterIntermediateOutputPath=$(IntermediateOutputPath)
;_OuterCustomViewMapFile=$(_CustomViewMapFile)
</_AdditionalProperties>
<_AndroidBuildRuntimeIdentifiersInParallel Condition=" '$(_AndroidBuildRuntimeIdentifiersInParallel)' == '' ">true</_AndroidBuildRuntimeIdentifiersInParallel>
</PropertyGroup>
Expand Down
2 changes: 2 additions & 0 deletions ...in.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.ILLink.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ This file contains the .NET 5-specific targets to customize ILLink
Used for the <ILLink CustomData="@(_TrimmerCustomData)" /> value:
https://github.com/dotnet/sdk/blob/a5393731b5b7b225692fff121f747fbbc9e8b140/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets#L147
-->
<_TrimmerCustomData Include="AndroidHttpClientHandlerType" Value="$(AndroidHttpClientHandlerType)" />
<_TrimmerCustomData Include="AndroidCustomViewMapFile" Value="$(_OuterCustomViewMapFile)" />
<_TrimmerCustomData Include="XATargetFrameworkDirectories" Value="$(_XATargetFrameworkDirectories)" />
<_TrimmerCustomData
Condition=" '$(_ProguardProjectConfiguration)' != '' "
Expand Down
53 changes: 47 additions & 6 deletions src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/LinkerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,14 +156,53 @@ static AssemblyDefinition CreateFauxMonoAndroidAssembly ()
return assm;
}

private void PreserveCustomHttpClientHandler (string handlerType, string handlerAssembly, string testProjectName, string assemblyPath)
private void PreserveCustomHttpClientHandler (
string handlerType,
string handlerAssembly,
string testProjectName,
string assemblyPath,
TrimMode trimMode)
{
var proj = new XamarinAndroidApplicationProject () { IsRelease = true };
testProjectName += trimMode.ToString ();

var class_library = new XamarinAndroidLibraryProject {
IsRelease = true,
ProjectName = "MyClassLibrary",
Sources = {
new BuildItem.Source ("MyCustomHandler.cs") {
TextContent = () => """
class MyCustomHandler : System.Net.Http.HttpMessageHandler
{
protected override Task <HttpResponseMessage> SendAsync (HttpRequestMessage request, CancellationToken cancellationToken) =>
throw new NotImplementedException ();
}
"""
},
new BuildItem.Source ("Bar.cs") {
TextContent = () => "public class Bar { }",
}
}
};
using (var libBuilder = CreateDllBuilder ($"{testProjectName}/{class_library.ProjectName}")) {
Assert.IsTrue (libBuilder.Build (class_library), $"Build for {class_library.ProjectName} should have succeeded.");
}

var proj = new XamarinAndroidApplicationProject {
ProjectName = "MyApp",
IsRelease = true,
TrimModeRelease = trimMode,
Sources = {
new BuildItem.Source ("Foo.cs") {
TextContent = () => "public class Foo : Bar { }",
}
}
};
proj.AddReference (class_library);
proj.AddReferences ("System.Net.Http");
string handlerTypeFullName = string.IsNullOrEmpty(handlerAssembly) ? handlerType : handlerType + ", " + handlerAssembly;
proj.SetProperty (proj.ActiveConfigurationProperties, "AndroidHttpClientHandlerType", handlerTypeFullName);
proj.MainActivity = proj.DefaultMainActivity.Replace ("base.OnCreate (bundle);", "base.OnCreate (bundle);\nvar client = new System.Net.Http.HttpClient ();");
using (var b = CreateApkBuilder (testProjectName)) {
using (var b = CreateApkBuilder ($"{testProjectName}/{proj.ProjectName}")) {
Assert.IsTrue (b.Build (proj), "Build should have succeeded.");

using (var assembly = AssemblyDefinition.ReadAssembly (Path.Combine (Root, b.ProjectDirectory, proj.IntermediateOutputPath, assemblyPath))) {
Expand All @@ -173,12 +212,14 @@ private void PreserveCustomHttpClientHandler (string handlerType, string handler
}

[Test]
public void PreserveCustomHttpClientHandlers ()
public void PreserveCustomHttpClientHandlers ([Values (TrimMode.Partial, TrimMode.Full)] TrimMode trimMode)
{
PreserveCustomHttpClientHandler ("Xamarin.Android.Net.AndroidMessageHandler", "",
"temp/PreserveAndroidMessageHandler", "android-arm64/linked/Mono.Android.dll");
"temp/PreserveAndroidMessageHandler", "android-arm64/linked/Mono.Android.dll", trimMode);
PreserveCustomHttpClientHandler ("System.Net.Http.SocketsHttpHandler", "System.Net.Http",
"temp/PreserveSocketsHttpHandler", "android-arm64/linked/System.Net.Http.dll");
"temp/PreserveSocketsHttpHandler", "android-arm64/linked/System.Net.Http.dll", trimMode);
PreserveCustomHttpClientHandler ("MyCustomHandler", "MyClassLibrary",
"temp/MyCustomHandler", "android-arm64/linked/MyClassLibrary.dll", trimMode);
}

[Test]
Expand Down
19 changes: 19 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.Linker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,24 @@ public static bool ExistsInFrameworkPath (string assembly)
.Select (p => Path.GetDirectoryName (p.TrimEnd (Path.DirectorySeparatorChar)))
.Any (p => assembly.StartsWith (p, StringComparison.OrdinalIgnoreCase));
}

static readonly char [] CustomViewMapSeparator = [';'];

public static Dictionary<string, HashSet<string>> LoadCustomViewMapFile (string mapFile)
{
var map = new Dictionary<string, HashSet<string>> ();
if (!File.Exists (mapFile))
return map;
foreach (var s in File.ReadLines (mapFile)) {
var items = s.Split (CustomViewMapSeparator, count: 2);
var key = items [0];
var value = items [1];
HashSet<string> set;
if (!map.TryGetValue (key, out set))
map.Add (key, set = new HashSet<string> ());
set.Add (value);
}
return map;
}
}
}
14 changes: 1 addition & 13 deletions src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,19 +408,7 @@ public static Dictionary<string, HashSet<string>> LoadCustomViewMapFile (IBuildE
var cachedMap = engine?.GetRegisteredTaskObjectAssemblyLocal<Dictionary<string, HashSet<string>>> (mapFile, RegisteredTaskObjectLifetime.Build);
if (cachedMap != null)
return cachedMap;
var map = new Dictionary<string, HashSet<string>> ();
if (!File.Exists (mapFile))
return map;
foreach (var s in File.ReadLines (mapFile)) {
var items = s.Split (new char [] { ';' }, count: 2);
var key = items [0];
var value = items [1];
HashSet<string> set;
if (!map.TryGetValue (key, out set))
map.Add (key, set = new HashSet<string> ());
set.Add (value);
}
return map;
return LoadCustomViewMapFile (mapFile);
Copy link
Member

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;

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Member Author

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.

}

public static bool SaveCustomViewMapFile (IBuildEngine4 engine, string mapFile, Dictionary<string, HashSet<string>> map)
Expand Down
13 changes: 7 additions & 6 deletions tests/Mono.Android-Tests/Android.Widget/CustomWidgetTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
using System.Diagnostics.CodeAnalysis;
using Android.App;
using Android.App;
using Android.Content;
using Android.Util;
using Android.Views;
using Android.Widget;
using NUnit.Framework;
using Mono.Android_Test.Library;

namespace Xamarin.Android.RuntimeTests
{
[TestFixture]
public class CustomWidgetTests
{
public CustomWidgetTests()
{
// NOTE: ensure the C# compiler has a reference to the library
new Mono.Android_Test.Library.Foo ();
Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Is this (still?) a problem? #8904 updated things to look for "transitive assembly references" (see also).

If we need to add explicit type references to work around missing transitive assembly references, maybe we need to revisit #8904?

Copy link
Member Author

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.

Copy link
Member Author

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:

}

// https://bugzilla.xamarin.com/show_bug.cgi?id=23880
[Test]
[DynamicDependency (DynamicallyAccessedMemberTypes.All, typeof (CustomTextView))]
public void UpperCaseCustomWidget_ShouldNotThrowInflateException ()
{
Assert.DoesNotThrow (() => {
Expand All @@ -24,7 +27,6 @@ public void UpperCaseCustomWidget_ShouldNotThrowInflateException ()
}

[Test]
[DynamicDependency (DynamicallyAccessedMemberTypes.All, typeof (CustomTextView))]
public void LowerCaseCustomWidget_ShouldNotThrowInflateException ()
{
Assert.DoesNotThrow (() => {
Expand All @@ -34,7 +36,6 @@ public void LowerCaseCustomWidget_ShouldNotThrowInflateException ()
}

[Test]
[DynamicDependency (DynamicallyAccessedMemberTypes.All, typeof (CustomTextView))]
public void UpperAndLowerCaseCustomWidget_FromLibrary_ShouldNotThrowInflateException ()
{
Assert.DoesNotThrow (() => {
Expand Down
4 changes: 4 additions & 0 deletions tests/Mono.Android-Tests/Mono.Android-Test.Library/Foo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
namespace Mono.Android_Test.Library;

// NOTE: Type to be used, so there is a reference to this library
public class Foo { }