1799002 - Android multi-locale packaging broken by upgrade to Android-Gradle plugin 7.3.0

Status: RESOLVED FIXED

(GeckoView :: Core, defect, P2)

Description

[Nick Alexander :nalexander [he/him]]

Some internal details changed in the Android-Gradle plugin 7.3.0, and the modified omnijar produced by mach package-multi-locale is no longer witnessed by Gradle. Local testing suggests that this is related to the Android-Gradle plugin determining its assets source sets earlier, before we've modified them.

The push before Bug 1791878 has an AAR with omni.ja containing:

      250  01-01-2010 00:00   chrome/an/locale/branding/brand.dtd
      250  01-01-2010 00:00   chrome/ar/locale/branding/brand.dtd
...

The push with Bug 1791878 has an AAR with omni.ja containing only:

      250  01-01-2010 00:00   chrome/en-US/locale/branding/brand.dtd

Comment 1

[Nick Alexander :nalexander [he/him]]

Now, how to address this? I see two broad approaches:

1. Back out Bug 1791878. Presumably whatever lint-related failures motivated that ticket would return.
2. Address the problem. This requires some effort on my (nalexander) part and has other risks.

Let me speak to 2. Historically, GeckoView and Fennec had a very complicated arrangement where Fennec included the omnijar and native libraries rather than GeckoView. As GeckoView grew into a real Gradle project, that arrangement was mostly normalized, but the existing system still retains the flexibility of the original approach, and thus the attendant complexity. This is why we have configureVariantWithGeckoBinaries; for the purpose of this ticket, it adds overhead to any solution.

In addition, historically there was a delicate arrangement where-in both the omnijar and the native libraries were Android assets. Again, as GeckoView (and Android itself!) matured, the arrangement was normalized: now native libraries are Android libs. This is why we have levels of indirection around syncOmnijarFromDistDir and syncLibsFromDistDir: we needed fine-grained mappings from locations to locations. These delicate mappings are no longer needed and the whole process can, I think, be simplified. But, for the purpose of this ticket, that adds time and risk.

Comment 2

[Nick Alexander :nalexander [he/him]]

Something like the tests envisioned by Bug 1570400 might help us prevent such silent failures in the future.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1799002 - Downgrade Kotlin compiler to 1.6.21 and android-gradle-plugin to 7.2.2 for 107.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

We needed to update AGP to version 7.3.0 to avoid crashes with Kotlin 1.7.x like the ones shown below:

FAILURE: Build completed with 2 failures.
1: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':geckoview:extractWithGeckoBinariesDebugAnnotations'.
> A failure occurred while executing com.android.build.gradle.internal.lint.AndroidLintWorkAction
   > Internal error: unexpected lint return value -1
* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
==============================================================================
2: Task failed with an exception.
-----------
* What went wrong:
java.lang.StackOverflowError (no error message)
* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
==============================================================================

https://treeherder.mozilla.org/logviewer?job_id=395447544&repo=try&lineNumber=4863

So to downgrade AGP back to 7.2.2, we'll also need to downgrade Kotlin back to 1.6.21. That looks green on Try:
https://treeherder.mozilla.org/jobs?repo=try&revision=37f70e2b568e1095cbbfa3babfa0d7d52eef9016

It does seem pretty risky to be downgrading our Kotlin compiler right before RC week, but I guess it's either that or trying to diagnose why Kotlin 1.7.x + AGP 7.2.2 is hitting that lint issue. I'll defer to your judgement on that one, Nick.

Comment 6

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

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9301889 [details]
Bug 1799002 - Downgrade Kotlin compiler to 1.6.21 and android-gradle-plugin to 7.2.2 for 107.

Beta/Release Uplift Approval Request

• User impact if declined: Broken GeckoView multi-locale builds
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The Kotlin change only affects test code and the AGP change just reverts us back to the previous known-good version. It's green on Try which seems like a decent indication of the safety in taking this.
• String changes made/needed:
• Is Android affected?: Yes

Comment 8

[Donal Meehan [:dmeehan]]

Comment on attachment 9301889 [details]
Bug 1799002 - Downgrade Kotlin compiler to 1.6.21 and android-gradle-plugin to 7.2.2 for 107.

Approved for 107.0RC1

Comment 9

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/69570cf0d617

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Donal Meehan [:dmeehan] from comment #9)

https://hg.mozilla.org/releases/mozilla-beta/rev/69570cf0d617

I can confirm that the GVE (and thus the AAR) at https://treeherder.mozilla.org/jobs?repo=mozilla-beta&revision=69570cf0d617eb61e853ed02f309dd005e722e25&searchStr=android%2Cshippable&selectedTaskRun=NnKanx_3QmyvRm3HuP7TEA.0 has sensible multi-locale entries, including (for de):

    110:      124  01-01-2010 00:00   localization/de/branding/brand.ftl
    111:     1021  01-01-2010 00:00   localization/de/crashreporter/aboutcrashes.ftl
    112:      696  01-01-2010 00:00   localization/de/mobile/android/aboutConfig.ftl
    113:     6378  01-01-2010 00:00   localization/de/toolkit/about/aboutWebrtc.ftl
   2115:       54  01-01-2010 00:00   chrome/de/locale/branding/brand.properties
   2116:     4005  01-01-2010 00:00   chrome/de/locale/de/browser/browser.properties
   2117:     3401  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/AccessFu.properties
   2118:     1597  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/commonDialogs.properties
   2119:    41767  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/dom/dom.properties
   2120:      212  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/intl.css
   2121:      174  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/intl.properties
   2122:      470  01-01-2010 00:00   chrome/de/locale/de/browser/passwordmgr.properties

So the downgrade is almost certainly going to address the two issues, about:config and addons being localized.

Comment 11

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

The bug is marked as tracked for firefox108 (nightly). However, the bug still has low severity.

:cpeterson, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit auto_nag documentation.

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1799002 - Part 1: Fix Android builds after Android-Gradle plugin version 7.3.0.

This commit removes a good deal of complexity around how
assets/omni.ja and libs/* are packaged into the Android GeckoView
project and ultimately AAR and APK files.

Historically there was a delicate arrangement where-in both the
omnijar and the native libraries were in Android assets, the latter so
they could be compressed and read by Gecko's own loader
implementation. As GeckoView (and Android itself!) matured, the
arrangement was normalized: now Gecko libraries are regular Android
libraries with no compression and special loading. This is why we had
levels of indirection around syncOmnijarFromDistDir and
syncLibsFromDistDir: we needed fine-grained mappings from locations
to locations. These delicate mappings are no longer needed and the
whole process can be simplified in the manner of this patch.

By declaring the Android sourcesets (close to) statically, the updated
Android-Gradle plugin version 7.3.0 no longer "misses" content changes
in the relevant directories.

We continue to need withGeckoBinaries to support non-builds like
linting and generating documentation.

Depends on D161509

Comment 13

[Nick Alexander :nalexander [he/him]]

Ryan: is there any reason to not land the reversion to Nightly while we work on fixing the build system? If so, please land it; if not, why not?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Our typical practice in these situations is to revert on the branch if a fix for the underlying issue is coming in the near future in order to avoid extra churn on m-c. But if you're feeling that this fix isn't going to be ready to land soon, then yeah, we can revert.

Comment 15

[Nick Alexander :nalexander [he/him]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)

Our typical practice in these situations is to revert on the branch if a fix for the underlying issue is coming in the near future in order to avoid extra churn on m-c. But if you're feeling that this fix isn't going to be ready to land soon, then yeah, we can revert.

I'm about to head out for the long weekend, so I'm not going to be able to address this properly. Let's land the reversion into Nightly so that we don't break Beta at merge-time. NI to RyanVM to coordinate the landing -- sorry that you are about to have a long weekend as well. We'll get this sorted on Monday.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out.

Comment 17

[Jonathan Almeida [:jonalmeida]]

Not sure how, but I mistakenly changed the status-firefox-esr102 flag and I'm unsure how to reset it back to it's previous value.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Don't worry about it. The flag was disabled for this component recently.

Comment 19

[Chris Peterson [:cpeterson]]

Waiting to hear in bug 1791878 comment 9 whether that bug (and this regression) will be fixed in 110.

Comment 20

[Chris Peterson [:cpeterson]]

109 and 110 are not affected because bug 1791878 was backed out. Leaving this bug open because this issue needs to be addressed before bug 1791878 can re-land.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Any chance we'll be able to get this landed, Nick?

Comment 22

[Nick Alexander :nalexander [he/him]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)

Any chance we'll be able to get this landed, Nick?

Sorry for the radio silence; this is not very high on my list.

I think I have the actual code kicking around; what I was hoping to do as well and ground out on was some tests for details of the build system. I think we can land just the first bit, and I'll try to get that up for somebody else to test manually shortly. Leaving NI for now.

Comment 23

[Nick Alexander :nalexander [he/him]]

Attachment: WIP: Bug 1799002 - Part 2: Remove Android workarounds for Bug 1627796. r?#build

The underlying Android-Gradle plugin bug has been addressed, so we no
longer see the issue, and therefore we don't need this complicated
"library set generation ID" workaround at all!

Depends on D161510

Comment 24

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1799002 - Part 2: Remove Android workarounds for Bug 1627796. r?#build

The underlying Android-Gradle plugin bug has been addressed, so we no
longer see the issue, and therefore we don't need this complicated
"library set generation ID" workaround at all!

Depends on D161510

Comment 25

[Nick Alexander :nalexander [he/him]]

With the patch stack applied locally, I find:

$ ./mach package-multi-locale --locales en-US de es-ES
...
$ unzip -l ../objdirs/objdir-android-multilocale/dist/geckoview/assets/omni.ja | grep ‘/es-ES/\\|/de/’
      122  01-01-2010 00:00   localization/de/branding/brand.ftl
     1021  01-01-2010 00:00   localization/de/crashreporter/aboutcrashes.ftl
      696  01-01-2010 00:00   localization/de/mobile/android/aboutConfig.ftl
     6378  01-01-2010 00:00   localization/de/toolkit/about/aboutWebrtc.ftl
      122  01-01-2010 00:00   localization/es-ES/branding/brand.ftl
      981  01-01-2010 00:00   localization/es-ES/crashreporter/aboutcrashes.ftl
      671  01-01-2010 00:00   localization/es-ES/mobile/android/aboutConfig.ftl
     6637  01-01-2010 00:00   localization/es-ES/toolkit/about/aboutWebrtc.ftl
       52  01-01-2010 00:00   chrome/de/locale/branding/brand.properties
     4086  01-01-2010 00:00   chrome/de/locale/de/browser/browser.properties
     3401  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/AccessFu.properties
     1597  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/commonDialogs.properties
    42617  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/dom/dom.properties
      212  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/intl.css
      174  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/intl.properties
      470  01-01-2010 00:00   chrome/de/locale/de/browser/passwordmgr.properties
       52  01-01-2010 00:00   chrome/es-ES/locale/branding/brand.properties
     4153  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/browser.properties
     3580  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/overrides/AccessFu.properties
     1567  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/overrides/commonDialogs.properties
    40142  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/overrides/dom/dom.properties
      365  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/overrides/intl.css
      191  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/overrides/intl.properties
      378  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/passwordmgr.properties

I think that's correct.

Comment 26

[Nick Alexander :nalexander [he/him]]

For the record, I had a typo in the fat AAR branch. With that fixed, I have a sensible fat AAR from this try build. E.g., https://firefoxci.taskcluster-artifacts.net/FSrYmyfnQJ2mSDwthQJmEA/0/public/build/maven/org/mozilla/geckoview/geckoview-nightly-try-omni/113.0.20230412021025/geckoview-nightly-try-omni-113.0.20230412021025.aar yields:

$ unzip ~/Downloads/geckoview-nightly-try-omni-113.0.20230412021025.aar assets/omni.ja
$ unzip -p assets/omni.ja res/multilocale.txt
an,ar,ast,az,be,bg,bn,br,bs,ca,cak,cs,cy,da,de,dsb,el,en-CA,en-GB,en-US,eo,es-AR,es-CL,es-ES,es-MX,et,eu,fa,ff,fi,fr,fy-NL,ga-IE,gd,gl,gn,gu-IN,he,hi-IN,hr,hsb,hu,hy-AM,id,is,it,ja,ka,kab,kk,kn,ko,lij,lo,lt,lv,ml,mr,ms,my,nb-NO,ne-NP,nl,nn-NO,oc,pa-IN,pl,pt-BR,pt-PT,rm,ro,ru,sk,sl,son,sq,sr,sv-SE,ta,te,th,tr,trs,uk,ur,uz,vi,wo,xh,zam,zh-CN,zh-TW
$ unzip -l assets/omni.ja | grep '/de/\|/es-ES/'
      124  01-01-2010 00:00   localization/de/branding/brand.ftl
     1021  01-01-2010 00:00   localization/de/crashreporter/aboutcrashes.ftl
      696  01-01-2010 00:00   localization/de/mobile/android/aboutConfig.ftl
     6573  01-01-2010 00:00   localization/de/toolkit/about/aboutWebrtc.ftl
      124  01-01-2010 00:00   localization/es-ES/branding/brand.ftl
      981  01-01-2010 00:00   localization/es-ES/crashreporter/aboutcrashes.ftl
      671  01-01-2010 00:00   localization/es-ES/mobile/android/aboutConfig.ftl
     6637  01-01-2010 00:00   localization/es-ES/toolkit/about/aboutWebrtc.ftl
       54  01-01-2010 00:00   chrome/de/locale/branding/brand.properties
     4086  01-01-2010 00:00   chrome/de/locale/de/browser/browser.properties
     3401  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/AccessFu.properties
     1597  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/commonDialogs.properties
    42856  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/dom/dom.properties
      212  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/intl.css
      174  01-01-2010 00:00   chrome/de/locale/de/browser/overrides/intl.properties
      470  01-01-2010 00:00   chrome/de/locale/de/browser/passwordmgr.properties
       54  01-01-2010 00:00   chrome/es-ES/locale/branding/brand.properties
     4153  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/browser.properties
     3580  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/overrides/AccessFu.properties
     1567  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/overrides/commonDialogs.properties
    40142  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/overrides/dom/dom.properties
      365  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/overrides/intl.css
      191  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/overrides/intl.properties
      378  01-01-2010 00:00   chrome/es-ES/locale/es-ES/browser/passwordmgr.properties

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/968d39196c25
Part 1: Fix Android builds after Android-Gradle plugin version 7.3.0. r=geckoview-reviewers,m_kato
https://hg.mozilla.org/integration/autoland/rev/1cab203fd470
Part 2: Remove Android workarounds for Bug 1627796. r=geckoview-reviewers,firefox-build-system-reviewers,glandium,m_kato

Comment 28

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/968d39196c25
https://hg.mozilla.org/mozilla-central/rev/1cab203fd470