Closed Bug 1627796 Opened 1 year ago Closed 8 months ago

Incremental `./mach build` doesn't properly get packaged into incremental fenix build

Categories

(Firefox Build System :: Android Studio and Gradle Integration, defect, P3)

defect

Tracking

(firefox82 fixed)

RESOLVED FIXED
82 Branch
Tracking Status
firefox82 --- fixed

People

(Reporter: kats, Assigned: nalexander, NeedInfo)

References

Details

Attachments

(1 file)

  • Check out mozilla-mobile/fenix
  • Check out mozilla-central
  • In mozilla-central, do ./mach build
  • In fenix, set dependencySubstitutions.geckoviewTopsrcdir=/path/to/mozilla-central in local.properties
  • In fenix, run ./gradlew assembleDebug

This works fine and produces an APK that can be installed and run. Then:

  • touch some .cpp file in mozilla-central (e.g. APZCTreeManager.cpp)
  • Do again ./mach build to produce an incremental build
  • In fenix do again ./gradlew assembleDebug

This produces a busted APK that crashes on startup, because libmozglue.so isn't in the APK. I have to do ./gradlew clean && ./gradlew assembleDebug for it to produce a usable build again, which is somewhat time consuming.

This is https://github.com/mozilla-mobile/fenix/issues/9030. I dug really deep into this but I couldn't find a fix. My preference is to avoid the Gradle snapshot machinery for a time, and just bump the build ID version number. That avoids all of this, AFAICT (at some cost). This is basically https://bugzilla.mozilla.org/show_bug.cgi?id=1557215: if we had such a build ID, we'd use it for this purpose.

Sorry that this is broken so hard -- sometimes it feels like we can't have nice things.

(Quoting from the fenix issue)

What happens is that the merged_native_libs directory gets (correctly, I think) purged after the SNAPSHOT version updates, but the merger only recognizes the changed-as-in-content-hash-differs as needing to be written.

So I guess this means just doing a touch *.so after rebuilding geckoview isn't sufficient to make gradle pick up the new .so files, because the hash will still be the same. Maybe I'll see if there's some random useless bits in the .so header that I can flip to change the hash. At least as a local hack that might work.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)

(Quoting from the fenix issue)

What happens is that the merged_native_libs directory gets (correctly, I think) purged after the SNAPSHOT version updates, but the merger only recognizes the changed-as-in-content-hash-differs as needing to be written.

So I guess this means just doing a touch *.so after rebuilding geckoview isn't sufficient to make gradle pick up the new .so files, because the hash will still be the same. Maybe I'll see if there's some random useless bits in the .so header that I can flip to change the hash. At least as a local hack that might work.

I thought of this, and hope you can achieve it. (My testing was binary editing some meaningless string.) If you look at how the build ID gets baked into libxul.so these days, and did that for every library, that would solve this particular problem: the approach we're using now only bumps the build ID when there's an actual change, and doing it with one .o for every library would avoid the Gradle ticket. If there's some way to bump say the ELF build ID, or add some garbage section that we can swizzle, we could do that from Gradle -- but it would be hard to do it only when required, which is what the existing build ID approach achieves.

The priority flag is not set for this bug.
:nalexander, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(nalexander)

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #4)

The priority flag is not set for this bug.
:nalexander, could you have a look please?

For more information, please visit auto_nag documentation.

I'll say this is P3. I think for those impacted, it's P1 -- it completely breaks the flow -- but that's more of a severity. In terms of fixing it, I don't think we really can, so if somebody can figure something out, good on them; but otherwise, we just have to hope it gets better in the Android-Gradle plugin.

Flags: needinfo?(nalexander)
Priority: -- → P3

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Severity: normal → S3
Duplicate of this bug: 1657190

I happened to be looking at related things so I answered the following question: is the underlying Android-Gradle plugin bug impacted by using SNAPSHOT builds? The answer is no: if we do our own snapshotting by manually setting the version number to something that increments, the underlying bug remains. Further, I traced "autoPublish" logic of Fenix/a-s/a-c and it is essentially what SNAPSHOT is supposed to achieve but done manually. (I don't know why they don't use SNAPSHOT: this is what it is for.) But even done manually their approach won't address this issue, for which we really do need to bake a build ID into each library.

So I conclude that the thing to do really is something like #c3, although I see now that even that isn't sufficient! We need to bake a single build ID into every library produced so that changing any library changes every library. Nasty.

Crud, I attached the patch to the wrong ticket number :(

I'd like some people to test https://phabricator.services.mozilla.com/D87551, please -- perhaps one or two of kats, mstange, and jnicol would be willing?

Apply that patch and then use the "local substitution" flow with a consumer (Fenix, Reference Browser), exactly as documented at https://firefox-source-docs.mozilla.org/mobile/android/geckoview/contributor/geckoview-quick-start.html#dependency-substiting-your-local-geckoview-into-a-mozilla-project. Verify that you don't see startup crashes. Report back. Thanks!

Flags: needinfo?(mstange.moz)
Flags: needinfo?(kats)
Flags: needinfo?(jnicol)

Thanks for looking at this Nick!

When I try to ./mach build gecko with your patch applied I get this error:

 0:01.34  0:00.34 /home/jamie/src/gecko/gradlew geckoview:generateJNIWrappersForGeneratedWithGeckoBinariesDebug -x lint
 0:01.34 Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=utf-8
 0:02.16 FAILURE: Build failed with an exception.
 0:02.16 * Where:
 0:02.16 Script '/home/jamie/src/gecko/mobile/android/gradle/with_gecko_binaries.gradle' line: 39
 0:02.16 * What went wrong:
 0:02.16 A problem occurred configuring project ':geckoview'.
 0:02.16 > Could not create task ':geckoview:syncLibsFromDistDirForWithGeckoBinariesDebug'.
 0:02.16    > Could not create task of type 'SyncLibsAndUpdateGeneration'.
 0:02.17       > /home/jamie/src/gecko/null does not exist.

It seems like project.android.ndkDirectory is null. If I hardcode the path instead the gecko build is successful, and an incremental Fenix build does not crash at startup!

Flags: needinfo?(jnicol)

It seems like project.android.ndkDirectory is null. If I hardcode the path instead the gecko build is successful, and an incremental Fenix build does not crash at startup!

Thanks for this testing! I wasn't certain of whether this was globally set or a function of local.properties, but it looks like it's the latter. I'll bump it to be fished from configure. Thanks again!

This is phenomenal news, Nick! I'll test the next version of your patch when I get a chance.

This is great, it works for me as well! Incremental ./gradlew assembleGeckoNightlyDebug produces a working APK for fenix using the latest version of your patch.

Flags: needinfo?(kats)
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a6b5b4f530d9
Work around Android-Gradle plugin bug causing startup crash when substituting GeckoView. r=agi,geckoview-reviewers

\o/

Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
Depends on: 1533465
Depends on: 1667948
No longer depends on: 1667948
Regressions: 1667948
You need to log in before you can comment on or make changes to this bug.