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.

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

JCW Implementor types "pollute" original package #1132

Open
jonpryor opened this issue Jul 24, 2023 · 4 comments
Open

JCW Implementor types "pollute" original package #1132

jonpryor opened this issue Jul 24, 2023 · 4 comments
Labels
callable-wrappers Issues with Java Callable Wrappers enhancement Proposed change to current functionality

Comments

@jonpryor
Copy link
Member

Consider a xamarin/xamarin-android build, after which the following file may exist:

src/Mono.Android/obj/Debug/net8.0/android-34/jcw/src/android/hardware/Camera_IOnZoomChangeListenerImplementor.java

This comes from the C# binding:

namespace Android.Hardware {
	public partial class Camera : Java.Lang.Object {
		// Note: no [Register]
		internal sealed partial class IOnZoomChangeListenerImplementor : global::Java.Lang.Object, IOnZoomChangeListener {
		}
	}
}

which results in the Java Callable Wrapper:

package android.hardware;

public class Camera_IOnZoomChangeListenerImplementor
  extends java.lang.Object
  implements
    mono.android.IGCUserPeer,
    android.hardware.Camera.OnZoomChangeListener
{
    // …
}

…and therein lies the issue: why is Camera.IOnZoomChangeListenerImplementor resulting in the Java Callable Wrapper of android.hardware.Camera_IOnZoomChangeListenerImplementor, and not crc64….Camera_IOnZoomChangeListenerImplementor? Why are we "polluting" the original package name?

@jpobst jpobst added the callable-wrappers Issues with Java Callable Wrappers label Jul 25, 2023
@jpobst
Copy link
Contributor

jpobst commented Oct 9, 2023

Note that this seems to be a side effect of:
#1105

These implementor classes moved from (ex): mono.android.view to android.view.

We are reverting this in:
xamarin/xamarin-android#8337

It may be desirable to hide them even further into something like crc64..., but if this path is pursued we definitely need to keep the needed Proguard changes in mind as well.

@jonpryor
Copy link
Member Author

jonpryor commented Oct 9, 2023

@jpobst wrote:

we definitely need to keep the needed Proguard changes in mind as well.

Related: we currently have code to create a ProGuard config file based on TypeDefinitions: https://github.com/xamarin/xamarin-android/blob/41aaf3873f702d260eb20b354ffdbb8884c51984/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/GenerateProguardConfiguration.cs#L74-L75

However, the above code doesn't appear to be invoked in xamarin/xamarin-android#8337. If it were invoked, then we'd have a -keep class android.view.View_IOnClickListenerImplementor running around, thus "solving" xamarin/xamarin-android#8337.

For .NET 8, we'll partially revert #1105 so that [Register] is once again emitted, which will "fix" xamarin/xamarin-android#8337, because the [Register] values will allow this line from proguard_xamarin.cfg to apply, ensuring that ProGuard preserves View_IOnClickListenerImplementor:

https://github.com/xamarin/xamarin-android/blob/main/src/Xamarin.Android.Build.Tasks/Resources/proguard_xamarin.cfg#L10

-keep class mono.android.** { *; <init>(...); }

For .NET 9, we should re-introduce #1105 and address #1132 (this issue) and ensure that xamarin-android emits a ProGuard file which preserves e.g. crc64….View_IOnClickListenerImplementor.

@jonpryor
Copy link
Member Author

jonpryor commented Oct 9, 2023

Now, why should we care about [Register] ? Mostly because of https://github.com/xamarin/monodroid/commit/eb04c91c:

[MSBuild] Use an MD5SUM for package name, not namespace name.

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=15205#c1
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=16805#c0
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=16826
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=21147

The problem is that when generating Android Callable Wrappers the
namespace name was assumed to be unique. (I mean, who would share
source code between assemblies, source code that subclasses
Java.Lang.Object?! Azure Mobile Services, that's who!)

For example:

// Lib1.dll
namespace Utilities {
	class Holder<T> : Java.Lang.Object {
		public T Value { get; set; }
	}
}
// Lib2.dll
namespace Utilities {
	class Holder<T> : Java.Lang.Object {
		public T Value { get; set; }
	}
}

In the above example, previous releases would lowercase the namespace
name to create the package name, while the class name is unchanged.
The result is that both types have the same ACW name:
utilities.Holder. This in turn causes a build-time crash, due to the
duplicate names, e.g.

error : Duplicate managed type found! Mappings between managed types and Java types must be unique.
First Type: 'Android.Support.V4.App.FragmentManager/IOnBackStackChangedListenerImplementor, Xamarin.Android.Support.v4-r18, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null';
Second Type: 'Android.Support.V4.App.FragmentManager/IOnBackStackChangedListenerImplementor, Mono.Android.Support.v4, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065'

To remove this problem, we instead use a package name which is the
"md5" + MD5SUM("@namespace@, @AssemblyQualifiedName@"). This ensures
that different assemblies get unique package names, even if their
namespace is the same. (The "md5" prefix is to ensure that the first
character is not a digit, and is thus a valid Java package identifier.)

Rephrased: "infrastructure" types such as IOnClickListenerImplementor "shouldn't be allowed to collide" with identically named types from other assemblies, unless there's a good reason to prevent that. Emitting [Register] on such types can introduce Java-side collisions for otherwise "valid" constructs, and thus @jonpryor would prefer that [Register] not be emitted unless otherwise required (e.g. bindings, or "user-friendly" Java-side activity names for adb shell am start …).

@jonpryor
Copy link
Member Author

jonpryor commented Oct 9, 2023

@jpobst also determined why JCW's were "polluting" the original package:

static bool IsPackageNamePreservedForAssembly (string assemblyName)
{
return assemblyName == "Mono.Android";
}
public static string GetPackageName (Type type)
{
string assemblyName = GetAssemblyName (type.Assembly);
if (IsPackageNamePreservedForAssembly (assemblyName))
return type.Namespace!.ToLowerInvariant ();

If the type does not have [Register] attribute, and the type is within Mono.Android.dll, we lowercase the namespace name to determine the package name for the Java Callable Wrapper.

Which explains why the Java Callable Wrapper for Android.Hardware.Camera.IOnZoomChangeListenerImplementor is android.hardware.Camera_IOnZoomChangeListenerImplementor.

(Though that doesn't quite explain ed63d89 and android.view.View_IOnClickListenerImplementor; shouldn't that be android.views.View_IOnClickListenerImplementor, as the namespace name is Android.Views?)

(See also: https://github.com/xamarin/monodroid/commit/bb245a48c8d8f6578d1aae8ecd6cfa64b29105b4 )

@jpobst jpobst added the enhancement Proposed change to current functionality label Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
callable-wrappers Issues with Java Callable Wrappers enhancement Proposed change to current functionality
Projects
None yet
Development

No branches or pull requests

2 participants