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.

Sign up for GitHub

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

dependencies/jni: Properly resolve jni dependency on android #14225

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+12 −0
Open

dependencies/jni: Properly resolve jni dependency on android #14225

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions mesonbuild/dependencies/dev.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,18 @@ def __init__(self, environment: 'Environment', kwargs: JNISystemDependencyKW):

m = self.env.machines[self.for_machine]

if m.is_android():
if 'version' in kwargs:
mlog.error('Can\'t set version filter for jni on Android')
self.is_found = False
return
Comment on lines +572 to +575
Copy link
Member

Choose a reason for hiding this comment

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

I think these need to be if kwargs.get('version'): because they may get set to None or [] in some paths.

I guess the version can't be found without actually compiling something here, since jni.h doesn't have any versions in it.

I think for version I'd prefer a warning rather than an exception, since this prevents someone from having code that runs on Android and !Android without doing extra work. So instead of users having to deal with that, just doing the:

mlog.debug("Android does not provide a mechanism to test the version of JNI, version restrictions are ignored")

Is a better solution. Also just debug for the debug log, because it's not an actionable item.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean jni.h kinda has version constants (the one you pass to AttachThread to specify what JNI interface version you want), its just that this doesn't accurately reflect the java version the version field currently uses (By just querying javac --version).

This obv. won't work for Android, given that you first won't know the java version of the host device and second that they don't use the JVM altogether but instead their ART which just defines the same jni interface)

You could in theory map the different java versions to the latest jni version constant it introduced and check if it is available, but I'm not sure if it is worth it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that all makes sense (and I expected this to be an ART issue), so I think I still like just ignoring it and hoping that you've done the work ahead of time to make sure everything works out is really the best we can do. Especially since someone could really want to target both android and something else in the same code base.

if 'modules' in kwargs:
mlog.error('Can\'t depend on jni modules on Android')
Copy link
Member

Choose a reason for hiding this comment

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

maybe: "Android does not provide JNI modules"

I'm also not sure that this is a good place for an error. I'm trying to decide if I think warning or debug is better

self.is_found = False
return
self.is_found = self.clib_compiler.has_header('jni.h', '', environment, dependencies=[self])
return

if 'java' not in environment.coredata.compilers[self.for_machine]:
detect_compiler(self.name, environment, self.for_machine, 'java')
self.javac = environment.coredata.compilers[self.for_machine]['java']
Expand Down
Loading