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

Traverse parent DexClassLoaders for native libraries in DelegateClassLoader #14965

Conversation

Bencodes
Copy link
Contributor

@Bencodes Bencodes commented Mar 4, 2022

Fixes #13661 by traversing the class loaders looking for native libraries.

@Bencodes Bencodes requested a review from ahumesky as a code owner March 4, 2022 22:19
@google-cla
Copy link

google-cla bot commented Mar 4, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@sgowroji sgowroji added the team-Android Issues for Android team label Mar 24, 2022
@Bencodes Bencodes force-pushed the traverse-parent-dexclassloaders-for-native-libraries-in-delegateclassloader branch from 88e6744 to 4f29b96 Compare March 24, 2022 17:40
@Bencodes Bencodes requested a review from ted-xie as a code owner March 24, 2022 17:40
@Bencodes
Copy link
Contributor Author

Bencodes commented Apr 5, 2022

cc: @ted-xie @ahumesky

@sgowroji sgowroji added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 21, 2022
@ted-xie
Copy link
Contributor

ted-xie commented May 9, 2022

Thanks for the PR, Ben! LGTM, I will push this through the internal tests.

@Bencodes
Copy link
Contributor Author

@ted-xie I think this PR might be a better option to merge #15361

@bazel-io bazel-io closed this in f5ed7d0 May 10, 2022
@ted-xie
Copy link
Contributor

ted-xie commented May 10, 2022

Oh, I didn't see your comment until now (after I've already merged this PR). I'll discuss with Alex.

@ted-xie
Copy link
Contributor

ted-xie commented May 10, 2022

@Bencodes @oliviernotteghem

Hi Ben,

Is there a particular reason you'd prefer #15361 over this PR (14965) that's already merged? From a quick glance at the code in #15361, there's no evident performance or functionality benefit. Additionally, this PR is much simpler (smaller, doesn't use reflection).

@Bencodes
Copy link
Contributor Author

Oh, I didn't see your comment until now (after I've already merged this PR). I'll discuss with Alex.

This only fixes part of the issue where mobile install can't find native libraries. The pull request that @oliviernotteghem put up also addresses the reverse problem where native libraries need to call back into the JVM.

You can reproduce this using https://github.com/facebook/flipper. It's been a while since i've looked at their code but I believe this is where it will error out https://github.com/facebook/flipper/blob/main/android/src/main/cpp/sonar.cpp#L46-L48

@ted-xie
Copy link
Contributor

ted-xie commented May 12, 2022

Thanks for the information, Ben. I'll propose some next steps internally; will update the bug after that.

@ShreeM01 ShreeM01 removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Android Issues for Android team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mobile install fails to load native libraries provided by AARs
5 participants