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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sp1ritCS
Copy link
Contributor

@sp1ritCS sp1ritCS commented Feb 5, 2025

I'm unhappy with the error messages, please suggest better ones

The NDK has a jni.h in sysroot/usr/include/, there are no extra native
modules that provide any callable symbols.
@sp1ritCS sp1ritCS requested a review from jpakkane as a code owner February 5, 2025 13:46
Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

@tristan957, any additional input?

Comment on lines +572 to +575
if 'version' in kwargs:
mlog.error('Can\'t set version filter for jni on Android')
self.is_found = False
return
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.

self.is_found = False
return
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants