-
Notifications
You must be signed in to change notification settings - Fork 29
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
[WIP] changes for Android JVM integration #144
base: main
Are you sure you want to change the base?
Conversation
Let us know when ready to review; I see this included the |
New to Android and Kotlin, so still figuring out what is ignorable :) Shall fix. |
This is great! The changes to the common code are quite narrow, which is quite encouraging. |
f5ccb57
to
7dc5372
Compare
@@ -34,7 +34,7 @@ static JavaRuntime_GetCreatedJavaVMs_fn JavaRuntime_GetCreatedJavaVMs; | |||
static void *JavaRuntime_dlhandle; | |||
|
|||
__attribute__((constructor)) static void JavaRuntime_init(void) { | |||
JavaRuntime_dlhandle = dlopen("libnativehelper.so", RTLD_NOW | RTLD_GLOBAL); | |||
JavaRuntime_dlhandle = dlopen("libnativehelper.so", RTLD_NOW | RTLD_LOCAL); |
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.
Question about this! Should we link the JavaRuntime target to libnativehelper.so
when building for Android rather than doing the dlopen
here?
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.
Hmm, I'd assumed this wasn't possible because the Android NDK only provides the .so
, and typically on Linux ld
will only find a versioned .so
. But maybe this isn't the case on Android. I'll check and get back.
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.
Actually I may have that around the wrong way, you need the unversioned .so
at ld
time, and the versioned one at runtime. So yes, checking I can link against it now.
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, this is possible, but requires setting the NDK target to >=31:
.unsafeFlags(["--target=aarch64-unknown-linux-android31"], .when(platforms: [.android])),
Obviously I don't want to encode the target architecture, do you know offhand if this is surfaced?
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.
- Actually this is a bit harder than it seems, for reasons I'll explain in a comment later.
0c75fa9
to
416b7f3
Compare
Regarding linking rather than dynamically loading |
05481c7
to
44752ac
Compare
b2c9792
to
75e4f11
Compare
75e4f11
to
3dd9326
Compare
This is not a real fix, I suspect the real fix is to remove javaEnvironment as an instance variable on all platforms and always use the ambient environment (as the JNI specification clearly states the environment cannot be shared between threads). Works around: swiftlang#157
3dd9326
to
661a6e6
Compare
Created this PR to track changes for Android JVM integration. WIP only, not ready for review.