Incremental `./mach build` doesn't properly get packaged into incremental fenix build
Categories
(Firefox Build System :: Android Studio and Gradle Integration, defect, P3)
Tracking
(firefox82 fixed)
Tracking | Status | |
---|---|---|
firefox82 | --- | fixed |
People
(Reporter: kats, Assigned: nalexander)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
- Check out mozilla-mobile/fenix
- Check out mozilla-central
- In mozilla-central, do
./mach build
- In fenix, set
dependencySubstitutions.geckoviewTopsrcdir=/path/to/mozilla-central
inlocal.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.
Assignee | ||
Comment 1•5 years ago
|
||
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.
Reporter | ||
Comment 2•5 years ago
|
||
(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.
Assignee | ||
Comment 3•5 years ago
|
||
(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.
Comment 4•5 years ago
|
||
The priority flag is not set for this bug.
:nalexander, could you have a look please?
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 5•5 years ago
|
||
(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.
Comment 6•5 years ago
|
||
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.)
Assignee | ||
Comment 8•4 years ago
|
||
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.
Assignee | ||
Comment 9•4 years ago
|
||
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!
Comment 10•4 years ago
|
||
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!
Assignee | ||
Comment 11•4 years ago
|
||
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!
Comment 12•4 years ago
|
||
This is phenomenal news, Nick! I'll test the next version of your patch when I get a chance.
Reporter | ||
Comment 13•4 years ago
|
||
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.
Assignee | ||
Comment 14•4 years ago
|
||
Updated•4 years ago
|
Comment 15•4 years ago
|
||
Comment 16•4 years ago
|
||
\o/
Comment 17•4 years ago
|
||
bugherder |
Updated•4 years ago
|
Updated•2 years ago
|
Description
•