Closed
Bug 1496190
Opened 6 years ago
Closed 5 years ago
Multi-locale builds of GeckoView
Categories
(GeckoView :: General, enhancement, P1)
GeckoView
General
Tracking
(geckoview62 wontfix, firefox-esr60 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)
RESOLVED
FIXED
mozilla66
People
(Reporter: mbrubeck, Assigned: nalexander)
References
(Depends on 1 open bug)
Details
(Keywords: intl)
Attachments
(4 files, 2 obsolete files)
We are currently shipping GeckoView with resources for the en-US locale only. We should instead ship multi-locale builds and set `pref("intl.locale.requested", "")` to tell the LocalService to get the list of preferred locales from OSPreferences (see bug 1493306). This will allow strings built into Gecko (like the default label for HTML forms "Submit" buttons) to be displayed in the user's preferred locale.
Comment 1•6 years ago
|
||
Nick is this a quick fix in our build system?
Flags: needinfo?(nfitzgerald)
Priority: -- → P1
Reporter | ||
Updated•6 years ago
|
Assignee: nobody → mbrubeck
Comment 2•6 years ago
|
||
Matt says we can use Fennec's localized strings for GV.
status-firefox62:
--- → wontfix
status-firefox63:
--- → fix-optional
status-firefox-esr60:
--- → wontfix
status-geckoview62:
--- → wontfix
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment #4) > Ha! Oops sorry. Nick please see comment 1. I did some initial reading and investigation, and I think this will mostly Just Work. We need to inject an "archive-geckoview" step between https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/testing/mozharness/mozharness/mozilla/l10n/multi_locale_build.py#79-80 and then invoke |mach android archive-geckoview| in that step. As far as I can see, the existing GeckoView packaging infrastructure produces an omni.ja that has all the requested locales in its (Gecko) resources. After that, we'll need to work through whatever Gecko prefs are required to actually make multi-locale work. This is non-trivial in Fennec (but I do not understand it) and even wonky in the build system (Bug 1428099 -- but I can work through that). I'll push a try build sometime soon and somebody else can work through the prefs parts.
Flags: needinfo?(nalexander)
Reporter | ||
Comment 6•6 years ago
|
||
(In reply to Nick Alexander :nalexander [he/him] from comment #5) > I'll push a try build sometime soon and somebody else can work through the > prefs parts. Have you had a chance to do this yet?
Flags: needinfo?(nalexander)
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #6) > (In reply to Nick Alexander :nalexander [he/him] from comment #5) > > I'll push a try build sometime soon and somebody else can work through the > > prefs parts. > > Have you had a chance to do this yet? I did the legwork/investigation but didn't push until just now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=195708d697c9f403b45f763b7c4788fd5681fbf7. gandalf: just putting this on your radar: the instructions at https://searchfox.org/mozilla-central/source/build/docs/locales.rst#76 aren't right -- I think it needs to be `mach package AB_CD=multi`. I packaged those docs up into a little mach command that works locally; see the commits above. (Not ready to land yet, just for whenever I look at this next.)
Flags: needinfo?(nalexander) → needinfo?(gandalf)
Assignee | ||
Comment 8•6 years ago
|
||
mozharness is a constant source of frustration, but the obvious thing "just works": https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=206457188&revision=cdf83a93605bbcd119444f36a2e17f8606635d3e The GeckoView AAR at https://queue.taskcluster.net/v1/task/LOapEuCoTmOk34VDoDHddQ/runs/0/artifacts/public/android/maven/org/mozilla/geckoview/geckoview-nightly-try-armeabi-v7a/64.0.20181018205755/geckoview-nightly-try-armeabi-v7a-64.0.20181018205755.aar has a multi-locale omnijar. mbrubeck, can you pick up futzing with prefs, etc?
Flags: needinfo?(mbrubeck)
Reporter | ||
Comment 9•6 years ago
|
||
(In reply to Nick Alexander :nalexander [he/him] from comment #8) > mbrubeck, can you pick up futzing with prefs, etc? Yes. If you want to land the mozharness patch in this bug, I can use bug 1493306 for the prefs changes.
Flags: needinfo?(mbrubeck)
Reporter | ||
Comment 10•6 years ago
|
||
Note: For some reason the "mobile-l10n.js" prefs file in the AAR from the try build above is the one for en-US, not the one that sets the pref correctly for multi-locale. If we don't plan to support single-locale builds, we just just set the multi-locale pref unconditionally in "geckoview-prefs.js".
Reporter | ||
Comment 11•6 years ago
|
||
Based on my local testing, setting the "intl.locale.requested" pref to "" works fine for both en-US builds and multi-locale builds. Pushed this patch to try to make sure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f264a1c61591eca6f2ef81add915ccf623db6dad
Comment 13•6 years ago
|
||
Nick, do you have any updates on this multi-locale bug? Matt said you had some WIP patches.
status-firefox65:
--- → affected
Flags: needinfo?(nalexander)
Assignee | ||
Comment 14•6 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #13) > Nick, do you have any updates on this multi-locale bug? Matt said you had > some WIP patches. Sorry, this has not been high priority for me. I've got a green try at https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=212326565&revision=4fea8c445acdaf859bdbb370922024084c29325a however, and https://queue.taskcluster.net/v1/task/ZF6tHtZiRmWdUL-WU5mI6A/runs/1/artifacts/public/android/maven/org/mozilla/geckoview/geckoview-nightly-try-armeabi-v7a/65.0.20181116230354/geckoview-nightly-try-armeabi-v7a-65.0.20181116230354.aar has a happy multi-locale looking omni.ja inside of it. I'll shepherd this through review next week.
Flags: needinfo?(nalexander)
Assignee | ||
Comment 15•6 years ago
|
||
All that is really required for this ticket is to invoke |mach android archive-geckoview| after |mach package| in the right place. But it's actively unhelpful to have this magic in mozharness -- especially since the documentation in `locales.rst` is subtly incorrect (the environment variables and Make variables don't quite work as written). So this commit adds a Mach command to do the actual work and replaces most of the mozharness magic with that command. Since the l10n Make targets check out the l10n HG repositories locally, this basically Just Works without the mozharness checkout steps when developing locally.
Reporter | ||
Updated•5 years ago
|
Attachment #9020941 -
Attachment is obsolete: true
Reporter | ||
Updated•5 years ago
|
Assignee: mbrubeck → nalexander
Assignee | ||
Comment 16•5 years ago
|
||
I've updated with reviewer feedback (thanks, review team!) and a fresh try build is percolating at https://treeherder.mozilla.org/#/jobs?repo=try&revision=61d2ec091748bdcfb8a31e1bf0bf79e3afcad401.
Assignee | ||
Updated•5 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 17•5 years ago
|
||
There's a guard to prevent action, but it's confusing to ask for it and then not take the action. Let's not ask. I'm leaving the guard, pointless as it should be, just in case.
Assignee | ||
Comment 18•5 years ago
|
||
This is a frustrating leak of Make's semantics into |mach build|, but we already have such a leak with the Makefile target and dumbmake logic, and I don't have a better solution here, so what's one more? The reason for this approach is that multi-locale Fennec builds (and soon to be GeckoView builds) do a two-part build that looks like |mach build| and then a bunch of multi-locale-specific processing. That processing runs things like |make -C ... target L10NBASEDIR=...|. I want to translate those |make -C ...| invocations to |mach build| as part of this work, but |mach build target| doesn't support the assignments on the command line. I'd prefer to just set L10NBASEDIR and not pass it through, and it can be set with --with-l10n-base in mozconfig... but the directory needs to exist at |mach configure| time. It's not clear how to arrange that just for the two-part multi-locale builds, but it's a good restriction so I don't want to weaken it. So here's the compromise I've landed on: move |make -C ...| to |mach build|, but plumb the Make wart into |mach build| so I don't have to completely rework how multi-locale builds work. One step forward, I think... Depends on D14828
Comment 19•5 years ago
|
||
Instead of handling `mach build FOO=bar` could you instead use `FOO=bar mach build` (or the equivalent, setting `FOO=bar` in the environment when you invoke the mach process)? They're not exactly equivalent to make, but they will both set the `FOO` variable in makefiles.
Assignee | ||
Comment 20•5 years ago
|
||
(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #19) > Instead of handling `mach build FOO=bar` could you instead use `FOO=bar mach > build` (or the equivalent, setting `FOO=bar` in the environment when you > invoke the mach process)? They're not exactly equivalent to make, but they > will both set the `FOO` variable in makefiles. Are you 100% confident that the environment variable is equivalent? If it is, that is my preference (since it doesn't leak this Make detail) -- but I couldn't find documentation saying that they are equivalent. In particular, it wasn't clear what happened if you had: a) FOO=baz in mozconfig b) FOO=bar in the environment c) invoked FOO=zot make ... Which one wins? If it's not c), then b), then a) in greatest to least priority then we need to do something different, I think.
Flags: needinfo?(ted)
Comment 21•5 years ago
|
||
It's not *exactly* that, but: https://www.gnu.org/software/make/manual/html_node/Environment.html#Environment Values from the environment are set as variables, but will be overridden by explicit setting in a Makefile. Values set on the commandline override even explicit setting in a Makefile. One quirk here is that mozconfig evaluation is...odd. `mk_add_options` gets evaluated in client.mk, not regular Makefiles, and other bits get evaluated as part of configure. The other complicating factor is whether the variable in question is set as a subst (AC_SUBST / set_config), in which case the value will be persisted from configure to autoconf.mk, where it is explicitly set in the Makefile context. If *that's* what needs overriding then yes, you probably do need to pass it as a value on the make commandline.
Flags: needinfo?(ted)
Assignee | ||
Comment 22•5 years ago
|
||
(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #21) > It's not *exactly* that, but: > https://www.gnu.org/software/make/manual/html_node/Environment. > html#Environment > > Values from the environment are set as variables, but will be overridden by > explicit setting in a Makefile. Values set on the commandline override even > explicit setting in a Makefile. One quirk here is that mozconfig evaluation > is...odd. `mk_add_options` gets evaluated in client.mk, not regular > Makefiles, and other bits get evaluated as part of configure. > > The other complicating factor is whether the variable in question is set as > a subst (AC_SUBST / set_config), in which case the value will be persisted > from configure to autoconf.mk, where it is explicitly set in the Makefile > context. If *that's* what needs overriding then yes, you probably do need to > pass it as a value on the make commandline. Right. L10NBASEDIR is overriding (a missing) --with-l10n-base=... in mozconfig, which turns into a `set_config`. Since it's missing for this build, it probably would work to set it in the environment. I'm not sure what would happen if it is set, but I don't really care about that situation; if it's set, it should be consistent. I will try this on try. Hehe.
Updated•5 years ago
|
Product: Firefox for Android → GeckoView
Assignee | ||
Comment 23•5 years ago
|
||
(In reply to Nick Alexander :nalexander [he/him] from comment #22) > (In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #21) > > It's not *exactly* that, but: > > https://www.gnu.org/software/make/manual/html_node/Environment. > > html#Environment > > > > Values from the environment are set as variables, but will be overridden by > > explicit setting in a Makefile. Values set on the commandline override even > > explicit setting in a Makefile. One quirk here is that mozconfig evaluation > > is...odd. `mk_add_options` gets evaluated in client.mk, not regular > > Makefiles, and other bits get evaluated as part of configure. > > > > The other complicating factor is whether the variable in question is set as > > a subst (AC_SUBST / set_config), in which case the value will be persisted > > from configure to autoconf.mk, where it is explicitly set in the Makefile > > context. If *that's* what needs overriding then yes, you probably do need to > > pass it as a value on the make commandline. > > Right. L10NBASEDIR is overriding (a missing) --with-l10n-base=... in > mozconfig, which turns into a `set_config`. Since it's missing for this > build, it probably would work to set it in the environment. I'm not sure > what would happen if it is set, but I don't really care about that > situation; if it's set, it should be consistent. > > I will try this on try. Hehe. Per https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ad83dd61f320ae7843deaee3ab8760a0651be6a, this doesn't seem to do what we had hoped. I set `L10NBASEDIR: l10n_base + 'GARBAGE'` and the job was green. ted, can we roll with the awkward Make FOO=BAR approach? I want to time-box this 'cuz this code is a rats nest of assumptions and frailties.
Flags: needinfo?(ted)
Comment 24•5 years ago
|
||
Looking at the build log, it clones the l10n repositories from scratch when doing chrome-an etc. Maybe the garbage stuff did work? Hard to tell what's where in the build log. I triggered a desktop l10n build, which, IIRC, has more data in the log.
Assignee | ||
Comment 25•5 years ago
|
||
> Looking at the build log, it clones the l10n repositories from scratch when > doing chrome-an etc. Maybe the garbage stuff did work? Hard to tell what's > where in the build log. > > I triggered a desktop l10n build, which, IIRC, has more data in the log. Oh? If it's cloning during chrome-an, then it is getting passed through; and if nobody touches the structure, it should work. Tom Prince suggested an alternate approach, in which we checkout the l10n repos before the "build" mozharness step. That makes a lot of sense to me: then we set --with-l10n-base for the build and everything is coherent. There's a WIP patch at https://phabricator.services.mozilla.com/D15146. However, Callek was mildly against this approach. NI to discuss why.
Flags: needinfo?(bugspam.Callek)
Comment 26•5 years ago
|
||
I wonder if we should disable the automatic clone in https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/toolkit/locales/l10n.mk#192-197 for automation builds, to make errors like we're trying to prevent here actual errors? There was something about single-locale fennecs apparently that I worked around by adding that for nightly, maybe that's fixed by now? As for when to set l10n base, it shouldn't matter if we first set that in configure and then check out, or check out and then set it.
Comment 27•5 years ago
|
||
(In reply to Axel Hecht [PTO, back Jan 7][:Pike] from comment #26) > I wonder if we should disable the automatic clone in > https://searchfox.org/mozilla-central/rev/ > 9528360768d81b1fc84258b5fb3601b5d4f40076/toolkit/locales/l10n.mk#192-197 for > automation builds, to make errors like we're trying to prevent here actual > errors? There was something about single-locale fennecs apparently that I > worked around by adding that for nightly, maybe that's fixed by now? > > As for when to set l10n base, it shouldn't matter if we first set that in > configure and then check out, or check out and then set it. This makes sense to me! (Explicit errors is better than implicit errors) (In reply to Nick Alexander :nalexander [he/him] (Away from Bugmail Dec 22-Jan 3) from comment #25) > Tom Prince suggested an alternate approach, in which we checkout the l10n > repos before the "build" mozharness step. That makes a lot of sense to me: > then we set --with-l10n-base for the build and everything is coherent. > There's a WIP patch at https://phabricator.services.mozilla.com/D15146. > However, Callek was mildly against this approach. I should be clear, I was mildly against the approach in the patch, but the concept is sound. * Any build that needs l10n should (if possible) do l10n clones as early as possible, even ideally alongside the cloning for m-c itself. * We should put those clones in a known, safe, spot that lets the build system find them. * The build should know about those clones at the earliest possible configure time, rather than pretending they don't exist and trying to piece things together after the fact. So I think given that tom's code solves a very real problem right now, even though it adds a chunk of complexity that we hope to slowly rip away, I won't block on it unless someone has a better idea. I think we should additionally set the l10n-base in mozconfigs for the multi-l10n style builds. So we have all the data when needed and can avoid the messy FOO=bar make variable headache. My ideal future is that these jobs don't have to invoke a sub-process for mozharness and things can happen as part of the main build script/system, when necessary.
Flags: needinfo?(bugspam.Callek)
Assignee | ||
Comment 28•5 years ago
|
||
I added a commit that does #26 -- the simplest possible thing. A try that should go green is at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc44b8aa2ecde86e292fbb06a609a9ce1445dd9d a try that should go red is at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c56c5b75c028cdeaa69bc93d14f623734e0645be
Assignee | ||
Comment 29•5 years ago
|
||
At some point we made L10NBASEDIR required. That means that env L10NBASEDIR=... make chrome-AB_CD takes the value set by configure. That is different than make chrome-AB_CD L10NBASEDIR=... which uses the value passed on the command line. Rather than making the latter style work with `mach build`, we instead set the "correct" value for L10NBASEDIR in automation. We could remove the --with-l10n-base stanzas from many automation mozconfigs, but there's some small advantage to keeping them explicit. Perhaps eventually we will remove them -- hopefully after standardizing l10n vs l10n-central! Depends on D14828
Assignee | ||
Comment 30•5 years ago
|
||
Depends on D15776
Updated•5 years ago
|
Attachment #9032075 -
Attachment is obsolete: true
Updated•5 years ago
|
Attachment #9026474 -
Attachment is obsolete: true
Updated•5 years ago
|
Attachment #9026474 -
Attachment is obsolete: false
Assignee | ||
Comment 31•5 years ago
|
||
A final try build before landing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aa1464e82d0a2047771527cbd910d39ce4c8f15
Comment 32•5 years ago
|
||
Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/463445e80106 Pre: Don't ask for multi-l10n mozharness step. r=Callek https://hg.mozilla.org/integration/autoland/rev/48b9df6b1284 Pre: Default L10NBASEDIR to correct value in automation. r=Callek https://hg.mozilla.org/integration/autoland/rev/e1f71a0cf95f Pre: Fail automation builds that check out l10n repos. r=Pike,Callek https://hg.mozilla.org/integration/autoland/rev/8c34488a6c8a Add |mach package-multi-locale|; produce multi-locale GeckoView archives. r=Callek,firefox-build-system-reviewers,ted
Comment 33•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/463445e80106
https://hg.mozilla.org/mozilla-central/rev/48b9df6b1284
https://hg.mozilla.org/mozilla-central/rev/e1f71a0cf95f
https://hg.mozilla.org/mozilla-central/rev/8c34488a6c8a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Comment 34•5 years ago
|
||
65=wontfix because I assume we don't need to uplift the multi-locale builds to GV 65 for Focus 9.0.
Updated•5 years ago
|
Flags: needinfo?(ted)
You need to log in
before you can comment on or make changes to this bug.
Description
•