-
Notifications
You must be signed in to change notification settings - Fork 370
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
Better error message if supported ABI's cannot be found inside the APK. #65
Conversation
relinker/src/main/java/com/getkeepsafe/relinker/ApkLibraryInstaller.java
Show resolved
Hide resolved
Hi there, Any idea when this PR will be merged/released? Cheers |
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.
Hi @cmelchior, Thank you for this PR. I apologize for not getting into this sooner. Please see my comments and request another review once they have been addressed.
I will take care of merging this PR and releasing a new binary with these changes. Tentative release would be around the 2nd week of January.
relinker/src/main/java/com/getkeepsafe/relinker/MissingLibraryException.java
Outdated
Show resolved
Hide resolved
relinker/src/test/java/com/getkeepsafe/relinker/ApkLibraryInstallerTest.java
Outdated
Show resolved
Hide resolved
relinker/src/main/java/com/getkeepsafe/relinker/ApkLibraryInstaller.java
Outdated
Show resolved
Hide resolved
relinker/src/main/java/com/getkeepsafe/relinker/ApkLibraryInstaller.java
Outdated
Show resolved
Hide resolved
relinker/src/main/java/com/getkeepsafe/relinker/ApkLibraryInstaller.java
Outdated
Show resolved
Hide resolved
relinker/src/main/java/com/getkeepsafe/relinker/ApkLibraryInstaller.java
Outdated
Show resolved
Hide resolved
relinker/src/main/java/com/getkeepsafe/relinker/ApkLibraryInstaller.java
Show resolved
Hide resolved
Please see: #65 (review) for tentative release with these changes. |
65fa198
to
d7b220b
Compare
Ready for 2nd round @emarc-m |
Thank you for addressing the comments. |
@cmelchior (cc: @Tweener) version |
Awesome. Thanks 👏 |
This PR adds a better error message if an ABI cannot be successfully located.
This is helpful as some app users are side-loading APKs outside the Play Store. If those apps have been built using App Bundle or APK Split, it could result in users loading an APK that doesn't support their device, which results in a rather unhelpful error message like
com.getkeepsafe.relinker.MissingLibraryException: libtest.so
. This isn't particularly helpful for an app developer tracking their crashes. Especially if they are using a 3rd party library that includes native code. In that case, it could look like it is a bug in the 3rd party library.With this PR, the error message now tries to be more descriptive about what has been tried and what is actually supported by the device, so the error message will now look like this:
Could not find 'libtest.so'. Looking for: [armeabi-v7a], but only [x86, arm64-v8a] are supported.