-
Notifications
You must be signed in to change notification settings - Fork 52
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-gen] use Assembly Name instead of FullName for typemaps #227
Conversation
In |
@@ -202,6 +202,15 @@ static void WriteBinaryMapping (Stream o, Dictionary<string, string> mapping) | |||
o.WriteByte (0x0); | |||
} | |||
|
|||
/// <summary> | |||
/// We need the equivalent of GetAssemblyQualifiedName, but without the Assembly's FullName | |||
/// NOTE: Cecil also uses / instead of + for nested types, see Java.Interop.Tools.Cecil.TypeDefinitionRocks.GetAssemblyQualifiedName |
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 have to wonder: should TypeDefinitionRocks.GetAssemblyQualifiedName()
be updated to do this instead of having it emit "proper" assembly-qualified names? (If we did this, we'd also want to rename it to e.g. .GetAbbreviatedAssemblyQualifiedName()
?)
Looking at the callers of TypeDefinitionRocks.GetAssemblyQualifiedName()
, a worrying one is in JavaCallableWrapperGenerator.cs
, for the static constructor, e.g. from our unit tests:
mono.android.Runtime.register (
"Xamarin.Android.ToolsTests.ExportsMembers, Java.Interop.Tools.JavaCallableWrappers-Tests, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null",
ExportsMembers.class,
__md_methods);
All "full" Assembly-qualified name instances need to be updated, including (especially including) the Java Callable Wrappers, and this PR doesn't touch them.
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.
Ah I just found TypeDefinitionRocks.GetPartialAssemblyQualifiedName
, which just needs a minor fix and we should use it.
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.
What minor fix does it need?
TypeDefinitionRocks.GetPartialAssemblyQualifiedName()
is used in xamarin-android, in src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
, so be careful about semantics.
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=61073 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 ``` 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. The fix here is to use the assembly’s short name via `GetName ().Name` or Mono.Cecil’s equivalent `AssemblyDefinition.Name.Name`. So the above typemap would only be `Android.App.Activity, Mono.Android`. These changes needed to happen in both `JavaNativeTypeManager` and `TypeNameMapGenerator`. The final fix is to find *every* instance of `TypeDefinitionRocks.GetAssemblyQualifiedName` and use `TypeDefinitionRocks.GetPartialAssemblyQualifiedName` in its place. The latter also had the issue of needing to replace the `/` character with `+`, so both methods return valid nested type names.
e45ad1e
to
364b645
Compare
return string.Format ("{0}, {1}", | ||
// Cecil likes to use '/' as the nested type separator, while | ||
// Reflection uses '+' as the nested type separator. Use Reflection. | ||
type.FullName.Replace ('/', '+'), |
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.
Don't forget about the previous question:
TypeDefinitionRocks.GetPartialAssemblyQualifiedName()
is used in xamarin-android, in src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
, so be careful about semantics.
The <GenerateJavaStubs/>
code is writing into obj/$(Configuration)/acw-map.txt
, which is a file which is also used by the IDEs.
Will using +
instead of /
confuse things? ¯_(ツ)_/¯.
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.
@sgmunn? What does Visual Studio for Mac do with acw-map.txt
around nested types? Will using +
instead of /
-- e.g. System.Environment+SpecialFolder
instead of System.Environment/SpecialFolder
-- break anything? Is that even knowable?
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.
It looks like this file would contain two different characters for nested classes right now:
https://github.com/xamarin/xamarin-android/blob/595186928ae4d79fab396dc05e7dc2750e808544/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs#L165-L166
GetPartialAssemblyQualifiedName
returnsSystem.Environment/SpecialFolder
for the type portionGetAssemblyQualifiedName
returnsSystem.Environment+SpecialFolder
for the type portion
Since xamarin-android
will use Type.GetType
at runtime, we need the +
symbol in this PR: https://msdn.microsoft.com/en-us/library/w3f99sx1(v=vs.110).aspx#Remarks
But I bet the IDE might be using this in combination with something else that expects a /
, like Mono.Cecil...
Maybe it is best to add an optional nestedClassDelimiter
parameter that would keep the acw-map.txt
file the same?
Context: 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 ``` 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` - Wrote a test using `[assembly:AssemblyVersion("1.0.0.*")]` that checks `acw-map.txt` contents
…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
…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
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=61073
The Java-to-Managed typemaps list types such as:
Let’s assume you have an Android project with the following
assembly-level attribute:
Then on every build, the typemap is invalidated because your version
number has been incremented.
The fix here is to use the assembly’s short name via
GetName ().Name
or Mono.Cecil’s equivalent
AssemblyDefinition.Name.Name
. So the abovetypemap would only be
Android.App.Activity, Mono.Android
. Thesechanges needed to happen in both
JavaNativeTypeManager
andTypeNameMapGenerator
.The final fix is to find every instance of
TypeDefinitionRocks.GetAssemblyQualifiedName
and useTypeDefinitionRocks.GetPartialAssemblyQualifiedName
in its place.The latter also had the issue of needing to replace the
/
characterwith
+
, so both methods return valid nested type names.