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

Search library also in split APKs. #46

Merged
merged 1 commit into from Aug 27, 2018
Merged

Conversation

michalsrb
Copy link
Contributor

The library may be placed in a split APK. This happens for example when using
the dynamic delivery.

Fixes #44.

@michalsrb
Copy link
Contributor Author

Note that appInfo.splitSourceDirs may be null on some devices, the documentation does not mention it.

@StainlessStlRat
Copy link

Will this be merged soon?

@chriswiesner
Copy link

any update?

@michalsrb
Copy link
Contributor Author

The travis build failed because of ">" character inside comment. The comment was there already.

@philippb
Copy link
Contributor

philippb commented Jul 25, 2018

@michalsrb thanks for the PR. Sorry I just saw this. We will look at this by the end of this week or beginning of next one the latest.

Looking at it briefly:

  • could you please fix the existing tests and add new tests for the new behavior.
  • is there documentation that we can link in here the describes the designed behavior of multi APK apps?

How does it look in production for you?

@philippb
Copy link
Contributor

philippb commented Aug 3, 2018

@emarc-m Did you have a chance to look at this?

@michalsrb
Copy link
Contributor Author

could you please fix the existing tests and add new tests for the new behavior.

The ApkLibraryInstallerTest was failing because apparently the android.util.Pair intentionally behaves badly in unit tests. (stackoverflow)
So I replaced it with custom private class and the test passes now.

is there documentation that we can link in here the describes the designed behavior of multi APK apps?

Here is documentation that describes a bit how Google Play splits the APKs:
https://developer.android.com/guide/app-bundle/configure

I don't know if there is something better.

How does it look in production for you?

It works well. I am getting small amount of failures, but I have been getting those before as well. I suspect those may be people who install APKs from 3rd party sources with wrong ABI.

@sdeff
Copy link

sdeff commented Aug 9, 2018

Does anyone know how long it might take until this PR will be merged?

Copy link
Contributor

@emarc-m emarc-m left a comment

Choose a reason for hiding this comment

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

Thanks for the work! Minor comments.

@michalsrb do you think it's possible to include a unit test for #sourceDirectories(...) wherein the first if branch will be executed?

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP &&
appInfo.splitSourceDirs != null &&
appInfo.splitSourceDirs.length != 0)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

please move { to the previous line such that appInfo.splitSourceDirs.length != 0) {

@@ -70,25 +87,46 @@ public void installLibrary(final Context context,
for (final String abi : abis) {
jniNameInApk = "lib" + File.separatorChar + abi + File.separatorChar
+ mappedLibraryName;

if (jniNameInApk != null) instance.log("Looking for %s in APK %s...", jniNameInApk, sourceDir);
Copy link
Contributor

Choose a reason for hiding this comment

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

jniNameInApk will not be null

@amatsegor
Copy link

@michalsrb your PR is so waited by lots of developers
Could you please fix these 2 tiny tips from @emarc-m so he can accept the PR ?

The library may be placed in a split APK. This happens for example when using
the dynamic delivery.
@michalsrb
Copy link
Contributor Author

I made the small changes. I don't know how to mock the Build.VERSION.SDK_INT to make the unit test.

@philippb philippb merged commit 9bc6d17 into KeepSafe:master Aug 27, 2018
@philippb
Copy link
Contributor

philippb commented Aug 27, 2018

I merged this PR and I'm cutting a new version.

I suggest for the unit testing portion that we create something like an SDK manager class that has a meshing like minSdkVersion() that returns Build.VERSION.SDK_INT. Then we can mock the entire class for testing purpose.

Unit tests should be in a separate PR to this.
cc @emarc-m @michalsrb

@amatsegor
Copy link

Awesome, thanks! So is there any nightly builds of Relinker or when should we expect a new release?

@philippb
Copy link
Contributor

Made a new release: 1.3.0 with split APK support.
Thank you everyone!

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.

None yet

7 participants