-
Notifications
You must be signed in to change notification settings - Fork 528
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
Use java-interop sources in monodroid runtime libraries #1717
Use java-interop sources in monodroid runtime libraries #1717
Conversation
Context: https://github.com/xamarin/xamarin-android/projects/1 Bumps to Java.Interop/master/5c35bcb0 Begin merging the `Java.Interop/src/java-interop` library code into `src/monodroid` so that we can eventually unify the GC Bridge logic between the two repos. Add `MonoThread` and `mono_thread_current()` to `dylib-mono.h`. Fixed `monodroid_mono_class_get_field_from_name_fptr()` signature, where we were missing `const` for 2nd argument. Used relevant types in the `monodroid_mono_class_from_mono_type_fptr()` signature.
src/monodroid/jni/Android.mk
Outdated
@@ -1,5 +1,7 @@ | |||
LOCAL_PATH := $(call my-dir) | |||
|
|||
JAVA_INTEROP_PATH=../../external/Java.Interop |
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.
...shouldn’t this be ../../../
? $(CWD)
appears to be src/monodroid/jni
...
src/monodroid/jni/Android.mk
Outdated
debug.c \ | ||
timezones.c \ | ||
zip/ioapi.c \ | ||
zip/unzip.c \ | ||
xamarin_getifaddrs.c \ | ||
cpu-arch-detect.c \ | ||
monodroid-networkinfo.c | ||
monodroid-networkinfo.c \ | ||
../$(JAVA_INTEROP_PATH)/src/java-interop/java-interop-logger.c |
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.
The fact that you had to prefix $(JAVA_INTEROP_PATH)
with ../
strongly suggests that $(JAVA_INTEROP_PATH)
has the wrong value. :-)
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.
This line should also be at the top, before all the other $(MONO_PATH)
values.
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.
$(JAVA_INTEROP_PATH)
is used twice in that file and the related tools are run in different directories.
It might be cleaner to introduce 2 variables for each use. I think I will add $(JAVA_INTEROP_INCLUDES)
to avoid prefixing it with ../
here.
7c5af67
to
2085137
Compare
Updated |
src/monodroid/jni/Android.mk
Outdated
@@ -1,5 +1,8 @@ | |||
LOCAL_PATH := $(call my-dir) | |||
|
|||
JAVA_INTEROP_INCLUDES=../../external/Java.Interop |
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'm still confused as to how this works: relative to src/monodroid/jni
, ../../external
doesn't exist.
This strongly suggests that $(CWD)
isn't necessarily what we think it is. This further suggests that we should instead use a $(LOCAL_PATH)
-relative value, or $(JAVA_INTEROP_PATH)
should be defined at the ndk-build
invocation, just like MONO_PATH
is:
<_NdkBuildArgs>CONFIGURATION=$(Configuration) SGEN_BRIDGE_VERSION=$(MonoSgenBridgeVersion) JAVA_INTEROP_PATH="$(JavaInteropFullPath)" MONO_PATH="$(MonoSourceFullPath)"</_NdkBuildArgs>
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.
OK, let use the $(LOCAL_PATH)
then. It is set to jni
during the build.
These were moved to Java.Interop, use the JI's ones.
2085137
to
30852ab
Compare
This PR contains patch from #1675 which had to be reverted due to problem in win hosts compilation.
It contains the original patch plus updates to build with newer Java.Interop.
It needs to be rebased once dotnet/java-interop#323 is merged.