Closed Bug 1496190 Opened 6 years ago Closed 5 years ago

Multi-locale builds of GeckoView

Categories

(GeckoView :: General, enhancement, P1)

enhancement

Tracking

(geckoview62 wontfix, firefox-esr60 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)

RESOLVED FIXED
mozilla66
Tracking Status
geckoview62 --- wontfix
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed

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.
Nick is this a quick fix in our build system?
Flags: needinfo?(nfitzgerald)
Priority: -- → P1
Assignee: nobody → mbrubeck
Matt says we can use Fennec's localized strings for GV.
I think you flagged the wrong Nick for ni.
Flags: needinfo?(nfitzgerald)
Ha! Oops sorry. Nick please see comment 1.
Flags: needinfo?(nalexander)
(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)
(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)
(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)
(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)
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".
Attached patch patch: use OS prefs by default (obsolete) — Splinter Review
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
that looks good to me
Flags: needinfo?(gandalf)
Nick, do you have any updates on this multi-locale bug? Matt said you had some WIP patches.
(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)
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.
Attachment #9020941 - Attachment is obsolete: true
Assignee: mbrubeck → nalexander
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.
Status: NEW → ASSIGNED
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.
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
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.
(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)
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)
(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.
Product: Firefox for Android → GeckoView
(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)
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.
> 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)
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.
(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)
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
Attachment #9032075 - Attachment is obsolete: true
Attachment #9026474 - Attachment is obsolete: true
Attachment #9026474 - Attachment is obsolete: false
Blocks: 1518228
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

65=wontfix because I assume we don't need to uplift the multi-locale builds to GV 65 for Focus 9.0.

Depends on: 1524332
Flags: needinfo?(ted)
You need to log in before you can comment on or make changes to this bug.