1763770 - After #1749693 merging external .aar is not possible anymore

Status: NEW

(GeckoView :: General, defect, P3)

Description

[Pier Angelo Vendrame]

To build Tor Browser for Android, we perform one build for each architecture (armv7, aarch64, x86, x86_64).

After that, we disable the compilation environment, we define MOZ_ANDROID_FAT_AAR_ARCHITECTURES=armeabi-v7a,arm64-v8a,x86,x86_64 and the various MOZ_ANDROID_FAT_AAR_xxx (one for each architecture) to build the final .aar with a final mach build.

It worked until v96. Then we switched directly to v99, but the final .aar produced in this way is missing the jni directory (and possibly more).

We found that hasCompileArtifacts defined to fix #1749693 does not take this case into account.
Just adding || mozconfig.substs.MOZ_ANDROID_FAT_AAR_ARCHITECTURES to the function does not work, unless it is added also to the first if in syncLibsFromDistDir.onlyIf {.

As a temporary workaround, we changed the calls to hasCompileArtifacts in our fork with boolean values, and we could generate the fat .aar as we did before.

Therefore, this procedure could still work, and we would totally love if you could modify with_gecko_binaries.gradle to work also in our scenario. From some quick searches, I found that some CI scripts might use similar strategies to us.
I am available for any test you may need, just let me know.

Thank you very much.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1749693

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

:agi, since you are the author of the regressor, bug 1749693, could you take a look?
For more information, please visit auto_nag documentation.

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Pier Angelo Vendrame from comment #0)

Hi Pier Angelo! Thanks for the bug report.

After that, we disable the compilation environment,

Interesting! Yeah I think our fat-aar build job does an actual build which we then throw out, and that would explain why this works on our end but not for you. We're definitely open to change that if you would like to contribute!

We found that hasCompileArtifacts defined to fix #1749693 does not take this case into account.
Just adding || mozconfig.substs.MOZ_ANDROID_FAT_AAR_ARCHITECTURES to the function does not work, unless it is added also to the first if in syncLibsFromDistDir.onlyIf {.

As a temporary workaround, we changed the calls to hasCompileArtifacts in our fork with boolean values, and we could generate the fat .aar as we did before.

I'm a little bit confused by this statement. The first if in syncLibsFromDistDir.onlyIf is for hasCompileArtifacts so how would adding MOZ_ANDROID_FAT_AAR_ARCHITECTURES do anything? or do you mean adding it only there?

Comment 4

[Nick Alexander :nalexander [he/him]]

I was able to produce fat AARs that looked healthy using artifact (i.e., DISABLE_COMPILE_ENVIRONMENT) builds when I wrote down some things about this process over in Bug 1759727.

We found that hasCompileArtifacts defined to fix #1749693 does not take this case into account.
Just adding || mozconfig.substs.MOZ_ANDROID_FAT_AAR_ARCHITECTURES to the function does not work, unless it is added also to the first if in syncLibsFromDistDir.onlyIf {.

As a temporary workaround, we changed the calls to hasCompileArtifacts in our fork with boolean values, and we could generate the fat .aar as we did before.

I'm a little bit confused by this statement. The first if in syncLibsFromDistDir.onlyIf is for hasCompileArtifacts so how would adding MOZ_ANDROID_FAT_AAR_ARCHITECTURES do anything? or do you mean adding it only there?

This is as intended. (But it could be changed.) We use !COMPILE_ENVIRONMENT && !MOZ_ARTIFACT_BUILDS as the signal that we shouldn't manage JNI libraries: i.e., for lints, etc. (The comment is, in this fact, correct.) Pier is trying to use that configuration to do things with JNI libraries.

The simplest thing to do is to perform an artifact build for these fat AAR productions. Otherwise, we'll need to add Yet Another Flag to distinguish this use case from all of the others.

Comment 5

[Pier Angelo Vendrame]

(In reply to Agi Sferro | :agi | [slow ni? rn sorry] | ⏰ PST | he/him from comment #3)

(In reply to Pier Angelo Vendrame from comment #0)

Hi Pier Angelo! Thanks for the bug report.

After that, we disable the compilation environment,

Interesting! Yeah I think our fat-aar build job does an actual build which we then throw out, and that would explain why this works on our end but not for you. We're definitely open to change that if you would like to contribute!

Thanks!

We found that hasCompileArtifacts defined to fix #1749693 does not take this case into account.
Just adding || mozconfig.substs.MOZ_ANDROID_FAT_AAR_ARCHITECTURES to the function does not work, unless it is added also to the first if in syncLibsFromDistDir.onlyIf {.

As a temporary workaround, we changed the calls to hasCompileArtifacts in our fork with boolean values, and we could generate the fat .aar as we did before.

I'm a little bit confused by this statement. The first if in syncLibsFromDistDir.onlyIf is for hasCompileArtifacts so how would adding MOZ_ANDROID_FAT_AAR_ARCHITECTURES do anything? or do you mean adding it only there?

Changing hasCompileArtifacts in this way:

def hasCompileArtifacts() {
    return project.mozconfig.substs.COMPILE_ENVIRONMENT
        || project.mozconfig.substs.MOZ_ARTIFACT_BUILDS
        || mozconfig.substs.MOZ_ANDROID_FAT_AAR_ARCHITECTURES
}

makes the build fail because of the exception thrown at line 263.

Since we pass the various .aars through the MOZ_ANDROID_FAT_AAR_arch environment variables, if I understand correctly, skipping syncLibsFromDistDir makes sense for our case.

So, one way to solve the problem could be modifying hasCompileArtifacts() as I wrote here, but use another function in syncLibsFromDistDir.onlyIf to make it return true also when the "external" .aar are passed (e.g., one that checks only for !COMPILE_ENVIRONMENT && !MOZ_ARTIFACT_BUILDS).

(In reply to Nick Alexander :nalexander [he/him] from comment #4)

I was able to produce fat AARs that looked healthy using artifact (i.e., DISABLE_COMPILE_ENVIRONMENT) builds when I wrote down some things about this process over in Bug 1759727.

Thanks for this information!
I have just started looking at the Android build system, and I am sure this documentation will help me 😊️.

We found that hasCompileArtifacts defined to fix #1749693 does not take this case into account.
Just adding || mozconfig.substs.MOZ_ANDROID_FAT_AAR_ARCHITECTURES to the function does not work, unless it is added also to the first if in syncLibsFromDistDir.onlyIf {.

As a temporary workaround, we changed the calls to hasCompileArtifacts in our fork with boolean values, and we could generate the fat .aar as we did before.

I'm a little bit confused by this statement. The first if in syncLibsFromDistDir.onlyIf is for hasCompileArtifacts so how would adding MOZ_ANDROID_FAT_AAR_ARCHITECTURES do anything? or do you mean adding it only there?

This is as intended. (But it could be changed.) We use !COMPILE_ENVIRONMENT && !MOZ_ARTIFACT_BUILDS as the signal that we shouldn't manage JNI libraries: i.e., for lints, etc. (The comment is, in this fact, correct.) Pier is trying to use that configuration to do things with JNI libraries.

The simplest thing to do is to perform an artifact build for these fat AAR productions. Otherwise, we'll need to add Yet Another Flag to distinguish this use case from all of the others.

I am not familiar with the artifact build system, so I should read some docs first.
It could work for us if we can inject our local files without using the network (which we disable, for reproducibility's sake).

Anyway, thank you both!

Comment 6

[Nick Alexander :nalexander [he/him]]

I would be happy reviewing a patch that makes this work based on the environment variables, handling the different cases as required.

I am not familiar with the artifact build system, so I should read some docs first.
It could work for us if we can inject our local files without using the network (which we disable, for reproducibility's sake).

You can: see https://bugzilla.mozilla.org/show_bug.cgi?id=1750109. So I still think just doing an artifact build will be the path of least resistance :)

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:amoya, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #7)

The severity field is not set for this bug.
:amoya, could you have a look please?

For more information, please visit auto_nag documentation.

This is very low severity: it impacts only developers, only very specific ones at that, and has a well understood workaround.