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

[Xamarin.Android.Build.Tasks] <GenerateJavaStubs /> generates a shorter acw-map.txt #1131

Merged
merged 1 commit into from
Dec 21, 2017

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Dec 20, 2017

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=61073
Context: dotnet/java-interop#227

The Java-to-Managed typemaps list types such as:

android/app/Activity
Android.App.Activity, Mono.Android, Version=0.0.0.0, Culture=neutral,
PublicKeyToken=84e04ff9cfb79065

This is found in the intermediate dir after a build in acw-map.txt,
or $(_AcwMapFile).

Let’s assume you have an Android project with the following
assembly-level attribute:

[assembly:AssemblyVersion("1.0.0.*")]

Then on every build, the typemap is invalidated because your version
number has been incremented.

Changes:

  • Bumped Java.Interop to master/429dc2a
  • JNIEnv needs to use the shorter type name when calling
    monodroid_typemap_managed_to_java
  • GenerateJavaStubs should not be writing lines for
    type.GetAssemblyQualifiedName into acw-map.txt
  • JnienvTest needed some updates to use the new type name format
  • Wrote a test using [assembly:AssemblyVersion("1.0.0.*")] that
    checks acw-map.txt contents

@jonathanpeppers jonathanpeppers added the do-not-merge PR should not be merged. label Dec 20, 2017
@jonathanpeppers
Copy link
Member Author

There is probably more to do here, this is the initial PR to see what breaks.

@jonpryor
Copy link
Member

JnienvTest.ManagedToJavaTypeMapping() is failing in the macOS+xbuild PR Build:

  Expected: not 0
   But was:  0
 +++++++++++++++++++
STACK TRACE:
  at Java.InteropTests.JnienvTest.ManagedToJavaTypeMapping () [0x00016] in <6c61e4bbfe054677adac786d9e684c35>:0 
  at (wrapper managed-to-native) System.Reflection.MonoMethod.InternalInvoke(System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00032] in <808e27c157614d6b865c43847235b62d>:0 

The stack trace doesn't quite make sense, but presumably it's due to line 395:

var m = monodroid_typemap_managed_to_java (typeof (Activity).AssemblyQualifiedName);
Assert.AreNotEqual (IntPtr.Zero, m);

The test likely needs to be updated, as we're no longer using Type.AssemblyQualifiedName in the typemap.* files. This presumably will need to manually generate the value:

var m = monodroid_typemap_managed_to_java ($"{typeof (Activity).FullName}, {typeof (Activity).Assembly.GetName().Name}");

...which should be split out into a helper so line 396 can reuse it.

Additionally, we should probably add a comment for why lines 396-397 are supposed to fail, because I forgot while re-reading the test. (sheepish-grin)

The rationale is that typemap.mj only exists to map names between Java & Managed types. JnienvTest does not inherit Java.Lang.Object, and thus should not be in the typemap.mj files.

@jonpryor
Copy link
Member

Comment on the commit message: this commit is actually fixing #61073, so a Context: line isn't appropriate. It should be Fixes: .

@jonathanpeppers jonathanpeppers changed the title [jcw-gen] changes to support shorter typemaps [Xamarin.Android.Build.Tasks] <GenerateJavaStubs /> generates a shorter acw-map.txt Dec 21, 2017
@jonathanpeppers jonathanpeppers removed the do-not-merge PR should not be merged. label Dec 21, 2017
Assert.AreEqual (IntPtr.Zero, m);
var m = monodroid_typemap_managed_to_java (GetTypeName (typeof (Activity)));
Assert.AreNotEqual (IntPtr.Zero, m, "`Activity` subclasses Java.Lang.Object, it should be in the typemap!");
m = monodroid_typemap_managed_to_java (GetTypeName (typeof (JnienvTest));
Copy link
Member

Choose a reason for hiding this comment

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

You're missing a closing ):

Java.Interop/JnienvTest.cs(401,76): error CS1026: ) expected

This prevents it from building.

var m = monodroid_typemap_managed_to_java (GetTypeName (typeof (Activity)));
Assert.AreNotEqual (IntPtr.Zero, m, "`Activity` subclasses Java.Lang.Object, it should be in the typemap!");
m = monodroid_typemap_managed_to_java (GetTypeName (typeof (JnienvTest));
Assert.AreEqual (IntPtr.Zero, m, "`JnienvTest` does *not* subclass Java.Lang.Object, it should *not* be in the typemap!");
Copy link
Member

Choose a reason for hiding this comment

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

Love the new assert messages! 👍

…er acw-map.txt

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=61073
Context: dotnet/java-interop#227

The Java-to-Managed typemaps list types such as:
```
android/app/Activity
Android.App.Activity, Mono.Android, Version=0.0.0.0, Culture=neutral,
PublicKeyToken=84e04ff9cfb79065
```
This is found in the intermediate dir after a  build in `acw-map.txt`,
or `$(_AcwMapFile)`.

Let’s assume you have an Android project with the following
assembly-level attribute:
```
[assembly:AssemblyVersion("1.0.0.*")]
```

Then on *every* build, the typemap is invalidated because your version
number has been incremented.

Changes:
- Bumped Java.Interop to master/429dc2a
- `JNIEnv` needs to use the shorter type name when calling
`monodroid_typemap_managed_to_java`
- `GenerateJavaStubs` should not be writing lines for
`type.GetAssemblyQualifiedName` into `acw-map.txt`
- `JnienvTest` needed some updates to use the new type name format
- Wrote a test using `[assembly:AssemblyVersion("1.0.0.*")]` that
checks `acw-map.txt` contents
@jonpryor jonpryor merged commit e5b1c92 into dotnet:master Dec 21, 2017
@jonathanpeppers jonathanpeppers deleted the jwcgen-assembly-name branch December 21, 2017 20:04
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Mar 12, 2018
…-map.txt file

Fixes dotnet#1373

Since dotnet#1131, the contents of the `acw-map.txt` file have changed. We
have removed the fully qualified type names from the file, and there is
some code in the `<Proguard />` task that relies on a specific number of
lines per C# type.

We need to update the for-loop to skip every third line instead of every
fourth line.

I also updated the loop to only index against the `acwLines` array once
per loop interation, as a small performance improvement. The code is also
a little more readable from this change.

Lastly, I added some code to the proguard test so that it validates the
proguard configuration file contains the appropriate contents. Prior to
this, the test was only making sure the build succeeded.
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Mar 12, 2018
…-map.txt file

Fixes dotnet#1373

Since dotnet#1131, the contents of the `acw-map.txt` file have changed. We
have removed the fully qualified type names from the file, and there is
some code in the `<Proguard />` task that relies on a specific number of
lines per C# type.

We need to update the for-loop to skip every third line instead of every
fourth line.

I also updated the loop to only index against the `acwLines` array once
per loop interation, as a small performance improvement. The code is also
a little more readable from this change.

Lastly, I added some code to the proguard test so that it validates the
proguard configuration file contains the appropriate contents. Prior to
this, the test was only making sure the build succeeded.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 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.

2 participants