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

Remove Java.Interop.TypeNameMappings project, it is gone. #837

Merged
merged 1 commit into from
Sep 18, 2017

Conversation

atsushieno
Copy link
Contributor

This fixes JavaNativeTypeManager.PackageNamingPolicy initialization issue
that blocks the entire use of package naming policy customization (one in
Xamarin.Android.Build.Tasks.dll is initialized in GenerateJavaStubs, while
the actual use in Java.Interop.Tools.JavaCallableWrappers.dll never gets
initialized).

Added test for PackageNamingPolicy.Lowercase. Deployed apps work fine too.

@atsushieno
Copy link
Contributor Author

Do not merge this until we merge dotnet/java-interop#184 and bump Java.Interop reference.

atsushieno referenced this pull request in atsushieno/xamarin-android-architecture-components-binding Sep 12, 2017
…arin.Android package naming policy is stupid.
@atsushieno atsushieno force-pushed the refactoring-jnitype-2 branch 3 times, most recently from 9fc1276 to 726aab2 Compare September 14, 2017 08:23
@atsushieno
Copy link
Contributor Author

build

@atsushieno
Copy link
Contributor Author

The build failures on Jenkins is not reproducible on my environment (with make leeroy). There is no chance that my changes can result in the missing IMap interface that are reported there.

This fixes JavaNativeTypeManager.PackageNamingPolicy initialization issue
that blocks the entire use of package naming policy customization (one in
Xamarin.Android.Build.Tasks.dll is initialized in GenerateJavaStubs, while
the actual use in Java.Interop.Tools.JavaCallableWrappers.dll never gets
initialized).

Added test for PackageNamingPolicy.Lowercase. Deployed apps work fine too.

The newly added source is positioned at the weird location because
there is a hack around positioning sources:
dotnet@ada479b
@atsushieno atsushieno force-pushed the refactoring-jnitype-2 branch from 726aab2 to 5598976 Compare September 15, 2017 12:48
@jonpryor
Copy link
Member

This feels like half the solution. Should we merge this and do the other half separately, or do both halves at the same time?

The missing half is in Mono.Android.dll, in JNIEnv.GetJniName(Type): If the monodroid_typemap_managed_to_java() codepath fails -- because typemap.mj doesn't contain an entry for the System.Type instance -- then we fallback to using JavaNativeTypeManager.ToJniName(Type). Logically, this should rely on the JavaNativeTypeManager.PackageNamingPolicy property value, but JNIEnv never sets that property.

This in turn means that JavaNativeTypeManager.PackageNamingPolicy will always be PackageNamingPolicy.LowercaseHash ("0") when running on device, which means we could generate the wrong names.

At least, that's the theory. In practice...how would monodroid_typemap_managed_to_java() fail? We'd have to "somehow" have a managed type + corresponding System.Type instance, for a bound type (e.g. not List<T>), and somehow not have this bound type contained within typemap.mj.

I'm not sure how to make that happen that wouldn't involve some form of System.Reflection.Emit/etc. and generating types at runtime. This could very well be a non-issue.

That said, let's call it a nagging concern in the back of my mind. It should probably be addressed.

Good way to test that setting JavaNativeTypeManager.PackageNamingPolicy within JNIEnv works: Update <GenerateJavaStubs/> so that it doesn't generate typemap.mj and typemap.jm. This will force execution of the Reflection-based JNI name generation path, and should quickly let you know if things are working as expected. :-)

@atsushieno
Copy link
Contributor Author

That probably matches my observation that apps never failed for me, especially even that we have such a bogus code path for Mono.Android.dll https://github.com/xamarin/java.interop/blob/master/src/Java.Interop.Tools.TypeNameMappings/Java.Interop.Tools.TypeNameMappings/JavaNativeTypeManager.cs#L192 where lowercasing is not enough (what happens if it is given "Android.Views" ? It should look up [NamespaceMapping]s).

But my general doubt is that package naming doesn't actually matter for run-time lookup. If you think your concern is real, then that should be formed as a unit test. That's what Test First should mean.

@atsushieno
Copy link
Contributor Author

Also note that this is being a blocking concern/issue for bringing Android support 26.1.0 support as Jon Dick reported that it depends on Android Architecture Components and MD5 hashing breaks AAC.

@jonpryor jonpryor merged commit 92984e6 into dotnet:master Sep 18, 2017
jonpryor added a commit that referenced this pull request Sep 18, 2017
jonpryor pushed a commit that referenced this pull request Sep 26, 2017
Bumps to Java.Interop/d15-5/53b91e3f.

This fixes JavaNativeTypeManager.PackageNamingPolicy initialization issue
that blocks the entire use of package naming policy customization (one in
Xamarin.Android.Build.Tasks.dll is initialized in GenerateJavaStubs, while
the actual use in Java.Interop.Tools.JavaCallableWrappers.dll never gets
initialized).

Added test for PackageNamingPolicy.Lowercase. Deployed apps work fine too.

The newly added source is positioned at the weird location because
there is a hack around positioning sources:
ada479b
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 20, 2021
Fixes: dotnet/java-interop#835
Fixes: dotnet#5921

Context: dotnet#5894

Changes: http://github.com/xamarin/Java.Interop/compare/12e670a8560f69581d3a3adf0a9d91e8ce8c9afa...2573dc8c84fd4eb68e75bcae73912c26f4942356

  * dotnet/java-interop@2573dc8c: [Java.Interop.Tools.*] IMetadataResolver not TypeDefinitionCache (dotnet#842)
  * dotnet/java-interop@412e974b: Revert "[generator] Disable [SupportedOSPlatform] until .NET 5/6. (dotnet#781)" (dotnet#841)
  * dotnet/java-interop@23baf0bc: [Java.Interop] Fix NRT warnings introduced by targeting 'net6.0' (dotnet#840)
  * dotnet/java-interop@131c1496: [generator] Fix NRE from return type not consistently set (dotnet#834)
  * dotnet/java-interop@100fffc1: [generator] Ensure "global::" is prepended to generic return casts. (dotnet#838)
  * dotnet/java-interop@9b89e90e: [generator] Ignore types without names (dotnet#837)
  * dotnet/java-interop@0e01fb5d: [Java.Interop.Tools.JavaSource] Merge @return block values (dotnet#836)
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 20, 2021
Fixes: dotnet/java-interop#835
Fixes: dotnet#5921

Context: dotnet#5894

Changes: http://github.com/xamarin/Java.Interop/compare/12e670a8560f69581d3a3adf0a9d91e8ce8c9afa...2573dc8c84fd4eb68e75bcae73912c26f4942356

  * dotnet/java-interop@2573dc8c: [Java.Interop.Tools.*] IMetadataResolver not TypeDefinitionCache (dotnet#842)
  * dotnet/java-interop@412e974b: Revert "[generator] Disable [SupportedOSPlatform] until .NET 5/6. (dotnet#781)" (dotnet#841)
  * dotnet/java-interop@23baf0bc: [Java.Interop] Fix NRT warnings introduced by targeting 'net6.0' (dotnet#840)
  * dotnet/java-interop@131c1496: [generator] Fix NRE from return type not consistently set (dotnet#834)
  * dotnet/java-interop@100fffc1: [generator] Ensure "global::" is prepended to generic return casts. (dotnet#838)
  * dotnet/java-interop@9b89e90e: [generator] Ignore types without names (dotnet#837)
  * dotnet/java-interop@0e01fb5d: [Java.Interop.Tools.JavaSource] Merge @return block values (dotnet#836)
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 21, 2021
Fixes: dotnet/java-interop#835
Fixes: dotnet#5921

Context: dotnet#5894

Changes: http://github.com/xamarin/Java.Interop/compare/12e670a8560f69581d3a3adf0a9d91e8ce8c9afa...a5ed8919fb2ec894cb8144e51ae7c29b4811ee2a

  * dotnet/java-interop@a5ed8919: [Java.Interop.Tools.Cecil] Fix the xamarin-android build
  * dotnet/java-interop@2573dc8c: [Java.Interop.Tools.*] IMetadataResolver not TypeDefinitionCache (dotnet#842)
  * dotnet/java-interop@412e974b: Revert "[generator] Disable [SupportedOSPlatform] until .NET 5/6. (dotnet#781)" (dotnet#841)
  * dotnet/java-interop@23baf0bc: [Java.Interop] Fix NRT warnings introduced by targeting 'net6.0' (dotnet#840)
  * dotnet/java-interop@131c1496: [generator] Fix NRE from return type not consistently set (dotnet#834)
  * dotnet/java-interop@100fffc1: [generator] Ensure "global::" is prepended to generic return casts. (dotnet#838)
  * dotnet/java-interop@9b89e90e: [generator] Ignore types without names (dotnet#837)
  * dotnet/java-interop@0e01fb5d: [Java.Interop.Tools.JavaSource] Merge @return block values (dotnet#836)
jonpryor added a commit that referenced this pull request May 23, 2021
Fixes: dotnet/java-interop#835
Fixes: #5921

Context: #5894

Changes: http://github.com/xamarin/Java.Interop/compare/12e670a8560f69581d3a3adf0a9d91e8ce8c9afa...a5ed8919fb2ec894cb8144e51ae7c29b4811ee2a

  * dotnet/java-interop@a5ed8919: [Java.Interop.Tools.Cecil] Fix the xamarin-android build
  * dotnet/java-interop@2573dc8c: [Java.Interop.Tools.*] IMetadataResolver not TypeDefinitionCache (#842)
  * dotnet/java-interop@412e974b: Revert "[generator] Disable [SupportedOSPlatform] until .NET 5/6. (#781)" (#841)
  * dotnet/java-interop@23baf0bc: [Java.Interop] Fix NRT warnings introduced by targeting 'net6.0' (#840)
  * dotnet/java-interop@131c1496: [generator] Fix NRE from return type not consistently set (#834)
  * dotnet/java-interop@100fffc1: [generator] Ensure "global::" is prepended to generic return casts. (#838)
  * dotnet/java-interop@9b89e90e: [generator] Ignore types without names (#837)
  * dotnet/java-interop@0e01fb5d: [Java.Interop.Tools.JavaSource] Merge @return block values (#836)
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants