Closed Bug 1362617 Opened 7 years ago Closed 7 years ago

Generalize MOZ_CHROME_MULTILOCALE to work with Desktop as well

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.
59 bytes, text/x-review-board-request
Pike
: review+
mshal
: review+
Details 
For L10nRegistry we need a way to package the information about locales packaged in.

We currently have 

`mk_add_options 'export MOZ_CHROME_MULTILOCALE=en-US pl'` in mozconfig that lists the locales we're building with, but it's Fennec only.

We should generalize it so that we can use `/res/multilocale.json` on desktop as well.
This patch moves the generation of res/multilocale.json to toolkit. In result both android and Firefox will receive it which will allow us to register multiple languages in L10nRegistry (which reads the multilocale.json at startup).
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

https://reviewboard.mozilla.org/r/145156/#review160046

This looks mostly fine, but to get the file packaged you'll also need to duplicate this block into browser/installer/package-manifest.in:
https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/mobile/android/installer/package-manifest.in#569
Attachment #8873747 - Flags: review?(ted) → review+
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

I had to change a few things and I'd like to get another review pass on it: https://reviewboard.mozilla.org/r/145156/diff/3-5/

This allows me to build both single and multilocale builds for both Firefox Desktop and Fennec.

This is not yet everything needed if we'll ever want to release mutli-locale Firefox Desktop because I'm not packaging the "/browser/chrome/@AB_CD@/" entries which are product-specific. I'm only packaging the "/chrome/@AB_CD@/" which are common between Fennec and Firefox.

If we'll ever want to get there, we'd probably need a per-product way of defining per-locale entries to be populated into locale-manifest.in.
For now, this patch generalizes the multi-locale code from Fennec to work both in Fennec and Firefox Desktop.
This is enough for all the new l10n architecture for testing as it gives me the entries and it gives me the multilocale.json file in /res/.
Attachment #8873747 - Flags: review?(catlee)
Attachment #8873747 - Flags: review?(catlee)
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

I'm not the right person to review this kind of change.
Attachment #8873747 - Flags: review?(catlee)
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

Trying :gps :)
Attachment #8873747 - Flags: review?(gps)
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

https://reviewboard.mozilla.org/r/145156/#review161802

I don't fully grok what all is going on with this l10n code. But this refactor seems OK I guess.

The main thing to watch out for is a dependency on locale-manifest.in. I suspect the directory traversal is such that it isn't a problem. I dunno.

If you can wait another day, Ted will be back from PTO and could be an extra set of eyes. But if you are confident with this, I think it is OK to land.

::: toolkit/locales/Makefile.in:44
(Diff revision 6)
> +ifndef MOZ_CHROME_MULTILOCALE
> +MOZ_CHROME_MULTILOCALE:=en-US
> +endif

This pattern can be accomplished by `MOZ_CHROME_MULTILOCALE ?= en-US`.

The `?=` assignment operator assigns a value if the variable isn't defined yet.
Attachment #8873747 - Flags: review?(gps) → review+
:ted said he'll take a final look at this tomorrow.
Flags: needinfo?(ted)
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

https://reviewboard.mozilla.org/r/145156/#review163758

::: toolkit/locales/Makefile.in:54
(Diff revision 7)
> +BASE_PATH:=@BINPATH@
> +else
> +BASE_PATH:=@RESPATH@
> +endif
> +
> +locale-manifest.in: $(GLOBAL_DEPS) FORCE

This probably doesn't *need* the force, although maybe that'd break l10n repacks in some weird way. We're going to rip this all out in the near future anyway.

::: toolkit/locales/Makefile.in:65
(Diff revision 7)
> +	  printf '$(BASE_PATH)/chrome/'"$$LOCALE"'.manifest\n' >> $@; \
> +	done
> +	COMMA=,
> +	#XXX: It would be nice to not duplicate en-US here, but makefile makes it hard.
> +	echo '{"locales": [$(foreach l,$(MOZ_CHROME_MULTILOCALE),"$(l)"$(COMMA)) "en-US"]}' \
> +	  > $(topobjdir)/dist/bin/res/multilocale.json

This is more commonly written as `$(DIST)/bin/...` in Makefiles.
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/55070d5d1a59
Generalize MOZ_CHROME_MULTILOCALE to work for browser as well. r=gps,ted
https://hg.mozilla.org/mozilla-central/rev/55070d5d1a59
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Backed out for breaking OS X and Linux L10n nightlies:

https://hg.mozilla.org/mozilla-central/rev/7ce557b85b611536b69539a7c18d4834ffc92eea

Push with failing nightlies: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=a599289ac64ba1d52a1552e33150342746e3e61b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=116628594&repo=mozilla-central

[task 2017-07-22T11:14:25.937271Z] 11:14:25     INFO -  /home/worker/workspace/build/src/obj-l10n/_virtualenv/bin/python /home/worker/workspace/build/src/config/nsinstall.py -t -m 644 /home/worker/workspace/build/l10n/pa-IN/toolkit/crashreporter/crashreporter.ini ../../dist/xpi-stage/locale-pa-IN
[task 2017-07-22T11:14:25.980403Z] 11:14:25     INFO -  printf '\n[multilocale]\n' > locale-manifest.in
[task 2017-07-22T11:14:25.985279Z] 11:14:25     INFO -  printf '@RESPATH@/res/multilocale.json\n' >> locale-manifest.in
[task 2017-07-22T11:14:25.986195Z] 11:14:25     INFO -  for LOCALE in en-US ;\
[task 2017-07-22T11:14:25.986341Z] 11:14:25     INFO -  do \
[task 2017-07-22T11:14:25.986514Z] 11:14:25     INFO -    printf '@RESPATH@/chrome/'"$LOCALE"'@JAREXT@\n' >> locale-manifest.in; \
[task 2017-07-22T11:14:25.986715Z] 11:14:25     INFO -    printf '@RESPATH@/chrome/'"$LOCALE"'.manifest\n' >> locale-manifest.in; \
[task 2017-07-22T11:14:25.986822Z] 11:14:25     INFO -  done
[task 2017-07-22T11:14:25.987171Z] 11:14:25     INFO -  COMMA=,
[task 2017-07-22T11:14:25.988124Z] 11:14:25     INFO -  #XXX: It would be nice to not duplicate en-US here, but makefile makes it hard.
[task 2017-07-22T11:14:25.989060Z] 11:14:25     INFO -  echo '{"locales": ["en-US", "en-US"]}' \
[task 2017-07-22T11:14:25.989216Z] 11:14:25     INFO -    > ../../dist/bin/res/multilocale.json
[task 2017-07-22T11:14:25.989936Z] 11:14:25     INFO -  /bin/sh: ../../dist/bin/res/multilocale.json: No such file or directory
[task 2017-07-22T11:14:25.990109Z] 11:14:25     INFO -  Makefile:63: recipe for target 'locale-manifest.in' failed
[task 2017-07-22T11:14:25.990270Z] 11:14:25    ERROR -  make[2]: *** [locale-manifest.in] Error 1
[task 2017-07-22T11:14:25.990493Z] 11:14:25     INFO -  make[2]: *** Deleting file 'locale-manifest.in'
[task 2017-07-22T11:14:25.990658Z] 11:14:25     INFO -  make[2]: Leaving directory '/home/worker/workspace/build/src/obj-l10n/toolkit/locales'
[task 2017-07-22T11:14:25.990789Z] 11:14:25     INFO -  Makefile:24: recipe for target 'libs-pa-IN' failed
[task 2017-07-22T11:14:25.990920Z] 11:14:25    ERROR -  make[1]: *** [libs-pa-IN] Error 2
[task 2017-07-22T11:14:25.991125Z] 11:14:25     INFO -  make[1]: Leaving directory '/home/worker/workspace/build/src/obj-l10n/toolkit/locales'
[task 2017-07-22T11:14:25.991254Z] 11:14:25     INFO -  Makefile:105: recipe for target '
Status: RESOLVED → REOPENED
Flags: needinfo?(gandalf)
Resolution: FIXED → ---
And Windows too for good measure.
status-firefox56: fixed → ---
Target Milestone: mozilla56 → ---
Pike, can I get a bit of your help here?

I'm always confused because it seems that treeherder log does not provide any indication as to what command has been issued to trigger it and in result I don't understand what happened here.

I have some faint idea that this is some type of CI build that happens every now and then and that's why it didn't show up right after landing it in autoland and didn't show on try, but I'm not sure how to reproduce it locally.

I have an idea that the cause is probably that in some type of build we don't have the `dist/bin/res/` directory structure, but I'm not sure if I should look to force-create it, move locales-manifest.in to a different location, or remove the whole makefile section from being executed for this type of make command that triggers this error.

Halp? :)
Flags: needinfo?(ted)
Flags: needinfo?(l10n)
Flags: needinfo?(gandalf)
So yeah, the try push has been pretty useless for this patch.

I recommend pushing to try with just opt builds, and no tests. And then you'll need to manually trigger l10n repacks on try, and an Android Nighlty.

I suspect that $(DIST)/bin/res/multilocale.json is wrong for repacks, maybe FINAL_TARGET, which fennec used before your patch? https://dxr.mozilla.org/mozilla-central/source/config/config.mk#84. You'd still need to $(NSINSTALL) the res subdirectory, maybe.

Some parts of this patch would benefit from the IS_LANGUAGE_REPACK and IS_LANGPACK defines I add in bug 1370506, might be good to adapt this patch to that once that lands.
Flags: needinfo?(l10n)
I need some help. I don't understand what is not working.

I followed Axel's recommendation from comment 24 and got this try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b0f158ccdb4

I clearly see that Mac OS L10n Repack is failing. I applied the patch on mac and tried to do a repack:

1) ./mach build
2) ./mach package
3) cd obj-x86_64-apple-darwin16.7.0/browser/locales
4) make merge-pl LOCALE_MERGEDIR=$PWD/mergedir
5) make installers-pl LOCALE_MERGEDIR=$PWD/mergedir

It works and I end up with firefox-56.0a1.pl.mac.dmg

So, what else is the treeherder's l10n repack doing that fails? How do I reproduce the failure locally? What command does the treeherder uses? Maybe :ted or :gps can help?
Flags: needinfo?(ted)
Flags: needinfo?(gps)
The problem is that you already have a build, which is in dist. The repacks don't.

The easiest way to reproduce this is to create a second objdir, and copy your package from one builddir/dist to otherbuidldir/dist.

And then do merge-pl, installers-pl.
Flags: needinfo?(ted)
Flags: needinfo?(gps)
ok. Seems like this patch is not ready to land.

What it needs is to handle the repackaging. In the repackage case, it should unpack `res/multilocale.json` from platform's omni.ja and change it to the new locale, then package back.

I tried adding `res/multilocale.json` to http://searchfox.org/mozilla-central/source/toolkit/mozapps/installer/l10n-repack.py#28 but that doesn't seem to unpack the multilocale.json from the omni.ja in `unpack` step. :(
Ok, I was able to, thanks to help from Pike, get a solution that I think may work: https://reviewboard.mozilla.org/r/145156/diff/11-12/

In short, I separated locale-manifest.in from multilocale.json steps, and I now call the multilocale.json step from installers-% for browser and mobile.

This allows me to generate the multilocale.json with the new locale before installers start packaging.

I'm going to test it on all platforms using the steps from comment 24 and request new review.
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

https://reviewboard.mozilla.org/r/145156/#review167508

::: browser/locales/Makefile.in:168
(Diff revision 12)
>  
>  # This is a generic target that will make a langpack, repack ZIP (+tarball)
>  # builds, and repack an installer if applicable. It is called from the
>  # tinderbox scripts. Alter it with caution.
>  
> -installers-%: clobber-% langpack-% repackage-win32-installer-% repackage-zip-%
> +installers-%: clobber-% multilocale.json-% langpack-% repackage-win32-installer-% repackage-zip-%

sry, but I think that adding more to this list of dependencies is not the right way to do this.

Basically, this list of a mistake, and I intend to remove it once I get to land bug 1370506.

I think this belongs into the libs-% step, probably as a step and not a dependency.
Thanks Axel!

I'm happy to move things around, as long as it works :)
Ok, a new attempt rebased on top of bug 1385227.

It seems to:

1) Work as ./mach build without MOZ_CHROME_MULTILOCALE
2) Work as ./mach build with MOZ_CHROME_MULTILOCALE
3) Work as ./mach -C objdir/browser/locale chrome-%

with setting the proper objdir/toolkit/locales/locale-manifest.in and objdir/dist/bin/res/multilocale.json

I still need to test it against repacks and android builds more
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

https://reviewboard.mozilla.org/r/145156/#review172328

There are a couple of things in this patch that rub me backwards.

The libs and chrome phase are executed per locale, and thus shouldn't have cross-locale steps in them.

I also think that toolkit/locales/Makefile.in shouldn't do any work other than packaging jar.mn. That's more my own personal idea than a design principle, though. It'd be good to turn those ideas into design. It just feels like anything that's non-trivial outside of $(APP)/locales/Makefile.in will make adding more build backends just harder.

Which brings me to my final point, I'm afraid I don't have a precise idea on what you should actually do. We should get more input on that from build folks.
Attachment #8873747 - Flags: review?(l10n) → review-
Michael - as Axel pointed out in his review in comment 39, my current patch make us execute the multilocale.json step per-locale in `libs` and `chrome`.

This is becoming a higher priority for us as we want to enable more rich language fallback chains both in packaging and at runtime.  Can you help me with this patch?
Flags: needinfo?(mshal)
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

No reason to keep the r? on Pike until we get a direction from :mshal :)
Attachment #8873747 - Flags: review?(l10n) → feedback?(mshal)
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

grrrr. Damn you, mozreview, for resetting the reviewer on every upload of a patch even after I removed the r? from the title.
Attachment #8873747 - Flags: review?(l10n)
Here we go...

Axel, I worked with :mshal today on this. Michael's final suggestion was to remove the `FORCE` statement which prevents the step from being executed unless mozconfig changes if I understand correctly.

With this patch I was able to:

1) On Android
 - build a single locale:
    - ./mach build
    - ./mach package
    - ./mach install
 - build a multilocale
    - Add `mk_add_options 'export MOZ_CHROME_MULTILOCALE=pl de fr'` to mozconfig
    - ./mach package
    - `for loc in de fr pl; de ./mach build chrome-$loc; done;`
    - ./mach install

2) On Desktop
 - build en-US build
    - ./mach build
    - ./mach package
 - repack
    - ./mach build
    - ./mach package
    - ./mach build installers-pl

In each case I believe the result multilocale.json to contain the right data (with en-US fallback) and the right multilocale.json which comes either from `dist/bin/res/multilocale.json` (for android multilocale or desktop build) or from xpi-stage/locale-*/res/multilocale.json (for repacks).

I understand that your request was a bit different - to look for an architecture way to only trigger those two steps at build time or on repackaging, but :mshal indicated that there may be no mechanism for that as far as I understand.

Is that something you would consider accepting for landing? We can file a follow up to discuss the architecture refactoring that could be performed during Callek's planned l10n build system cleanup.
Flags: needinfo?(mshal)
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

https://reviewboard.mozilla.org/r/145156/#review197610

Just spot-checked this patch, didn't do a full review.

::: browser/locales/Makefile.in:128
(Diff revision 20)
>  chrome-%: AB_CD=$*
>  chrome-%: IS_LANGUAGE_REPACK=1
>  chrome-%:
>  	$(if $(filter en-US,$(AB_CD)),, @$(MAKE) merge-$*)
>  	@$(MAKE) -C ../../toolkit/locales chrome-$*
> +	@$(MAKE) -C ../../toolkit/locales locale-manifest.in LOCALES=$* MOZ_CHROME_LOCALE_ENTRIES="$(MOZ_CHROME_LOCALE_ENTRIES)"

This looks wrong to me.

For one, I'd hate if we had chrome-% and libs-% diverge again, 'cause right now, they're kind of the same thing, just with XPI_NAME being undefined or defined.

Also, chrome-% is called per locale, and this looks like it would just leave the locale-manifest of the last.
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

https://reviewboard.mozilla.org/r/145156/#review197824

::: browser/locales/Makefile.in:118
(Diff revision 21)
>  	@$(MAKE) libs AB_CD=$* XPI_NAME=locale-$* PREF_DIR=$(PREF_DIR)
>  	@$(MAKE) -C $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales AB_CD=$* XPI_NAME=locale-$*
>  
> +MOZ_CHROME_LOCALE_ENTRIES=@RESPATH@/browser/chrome/ @RESPATH@/chrome/
> +
> +libs::

I would prefer if we could get rid of this custom libs rule, that just invokes the multilocale rule in toolkit/locales. Since it seems like the only reason to do it here is for setting MOZ_CHROME_LOCALE_ENTRIES, I think we should just have another way of setting those per-app entries that toolkit/locales/makefile.in can use.

For example, I'd suggest creating a file called browser/locales/moz-chrome-locale-entries, and then we can do something like this in toolkit/locales/Makefile.in:

\-locale-manifest.in: $(GLOBAL_DEPS) FORCE
\+MOZ_CHROME_LOCALE_ENTRIES = $(topsrcdir)/$(MOZ_BUILD_APP)/locales/moz-chrome-locale-entries
\+locale-manifest.in: $(MOZ_CHROME_LOCALE_ENTRIES) $(GLOBAL_DEPS) FORCE
...
\+         for ENTRY in \`cat $(MOZ_CHROME_LOCALE_ENTRIES)\` ;\\

Then we could remove the libs rules in mobile/android/locales and browser/locales, and just have libs:: multilocale in toolkit/locales.

Pike, what do you think?

::: mobile/android/locales/Makefile.in:45
(Diff revision 21)
>  	@$(MAKE) libs AB_CD=$* XPI_NAME=locale-$* PREF_DIR=defaults/pref
>  ifeq ($(OS_TARGET),Android)
>  	@$(MAKE) -C $(DEPTH)/mobile/android/base/locales AB_CD=$* XPI_NAME=locale-$*
>  endif
>  
> +MOZ_CHROME_LOCALE_ENTRIES=@BINPATH@/chrome/

Similar to browser/locales, I think this libs:: rule should go away and put the @BINPATH@/chrome/ line in a file called moz-chrome-locale-entries

::: toolkit/locales/Makefile.in:54
(Diff revision 21)
> +BASE_PATH:=@BINPATH@
> +else
> +BASE_PATH:=@RESPATH@
> +endif
> +
> +multilocale: $(GLOBAL_DEPS)

Actually, we can probably just do:

libs:: multilocale.json locale-manifest.in

We don't need separate $(MAKE) invocations or a separate 'multilocale' target here.

::: toolkit/locales/Makefile.in:72
(Diff revision 21)
> +	  done \
> +	done
> +
> +# This trick allows us to replace space sign with a comma to generate
> +# the list of locales.
> +EMPTY :=

EMPTY, SPACE, and COMMA should already be defined in config.mk / rules.mk. I don't think we should repeat the definitions here.
Attachment #8873747 - Flags: review?(mshal)
Thanks Michael!

Updated the patch to use the rules.mk variables.

I'll wait for Pike's opinion on the switch from MOZ_CHROME_LOCALE_ENTRIES to a `moz-chrome-locale-entries` file, but that should be an easy one.

Hope the rest of the patch looks sound to you :)
I don't have utterly strong opinions.

I like the idea to have toolkit/locales/Makefile.in just do jar packaging. If there's code that can be shared, I'd prefer to see it in l10n.mk. I'm also generally scared about how easy it is to break things in toolkit for TB.

PS: Does the multilocale.json have quotes around the strings for locale codes?
(In reply to Axel Hecht [:Pike] from comment #52)
> I don't have utterly strong opinions.
> 
> I like the idea to have toolkit/locales/Makefile.in just do jar packaging.
> If there's code that can be shared, I'd prefer to see it in l10n.mk. I'm
> also generally scared about how easy it is to break things in toolkit for TB.

l10n.mk sounds like a good spot to me. We could have the multilocale.json and locale-manifest.in rules in there, and set MOZ_CHROME_LOCALE_ENTRIES in browser/locales/Makefile.in and mobile/android/locales/Makefile.in as before. That way we avoid the extra moz-chrome-locale-entries files, and also avoid the cross-directory make invocations.
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

https://reviewboard.mozilla.org/r/145156/#review197858

Looks great to me! Just a few minor nits if you don't mind :)

::: browser/locales/Makefile.in:116
(Diff revision 23)
>  	@$(MAKE) -C ../../devtools/shim/locales AB_CD=$* XPI_NAME=locale-$* XPI_ROOT_APPID='$(XPI_ROOT_APPID)'
>  	@$(MAKE) -B searchplugins AB_CD=$* XPI_NAME=locale-$*
>  	@$(MAKE) libs AB_CD=$* XPI_NAME=locale-$* PREF_DIR=$(PREF_DIR)
>  	@$(MAKE) -C $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales AB_CD=$* XPI_NAME=locale-$*
>  
> +MOZ_CHROME_LOCALE_ENTRIES=@RESPATH@/browser/chrome/ @RESPATH@/chrome/

This variable kinda gets visually lost in between the large libs & chrome rules. Can we move it up near where DIST_SUBDIRS gets set? And perhaps with a comment that it gets used by l10n.mk

::: mobile/android/locales/Makefile.in:45
(Diff revision 23)
>  	@$(MAKE) libs AB_CD=$* XPI_NAME=locale-$* PREF_DIR=defaults/pref
>  ifeq ($(OS_TARGET),Android)
>  	@$(MAKE) -C $(DEPTH)/mobile/android/base/locales AB_CD=$* XPI_NAME=locale-$*
>  endif
>  
> +MOZ_CHROME_LOCALE_ENTRIES=@BINPATH@/chrome/

Similarly here, just a brief comment that it gets used in l10n.mk and maybe move it up above the l10n.mk include. (Note this doesn't affect the output, just more for human readability factors).

::: toolkit/locales/l10n.mk:296
(Diff revision 23)
> +		  printf "$$ENTRY""$$LOCALE"'@JAREXT@\n' >> $@; \
> +		  printf "$$ENTRY""$$LOCALE"'.manifest\n' >> $@; \
> +	  done \
> +	done
> +
> +# This trick allows us to replace space sign with a comma to generate

I think this comment was leftover from when you set COMMA and SPACE here. It can probably be removed now?

::: toolkit/mozapps/installer/l10n-repack.py:27
(Diff revision 23)
>      'update.locale',
>      'updater.ini',
>      'extensions/langpack-*@*',
>      'distribution/extensions/langpack-*@*',
>      'chrome/**/searchplugins/*.xml',
> +    '**/multilocale.json'

I think just 'multilocale.json' will work here, since the finder is already recursive. If I'm wrong, feel free to disregard.
Attachment #8873747 - Flags: review?(mshal) → review+
Thanks Michael!

I updated the patch to your comments with two nits:

1) I left the `**/multilocale.json` because that's the convention used in the list. Even if it's not needed, it doesn't hurt and I'd prefer not to refactor the list right now.

2) I had to introduce a new variable - MULTILOCALE_DIR, that copies FINAL_TARGET minus SUBDIR.

Basically, the move from toolkit/locales/Makefile.in to toolkit/locales/l10n.mk means that we now operate in a different FINAL_TARGET and on the browser it meant that the multilocale was added to `dist/bin/browser/res/multilocale.json` instead of `dist/bin/res/multilocale.json` and same for langpack.

Let me know if that works for you!
Flags: needinfo?(mshal)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #57)
> Thanks Michael!
> 
> I updated the patch to your comments with two nits:
> 
> 1) I left the `**/multilocale.json` because that's the convention used in
> the list. Even if it's not needed, it doesn't hurt and I'd prefer not to
> refactor the list right now.

Oh, I was looking at entries like 'updater.ini' that have no wildcards. But it seems just 'multilocale.json' doesn't work, it would need to be 'res/multilocale.json'. So feel free to leave it as is, or change it to res/multilocale.json

> 2) I had to introduce a new variable - MULTILOCALE_DIR, that copies
> FINAL_TARGET minus SUBDIR.

Seems fine. I don't have a better suggestion :)
Flags: needinfo?(mshal)
I've triggered a fennec nightly, too, https://treeherder.mozilla.org/#/jobs?repo=try&revision=0478ee6f9fc6&filter-searchStr=night&selectedJob=139634039, and from what I can tell, the multilocale packaging is broken there? Both for multilocale.json and chrome/chrome.manifest.
(In reply to Axel Hecht [:Pike] from comment #52)
> I don't have utterly strong opinions.
> 
> I like the idea to have toolkit/locales/Makefile.in just do jar packaging.
> If there's code that can be shared, I'd prefer to see it in l10n.mk. I'm
> also generally scared about how easy it is to break things in toolkit for TB.
> 
> PS: Does the multilocale.json have quotes around the strings for locale
> codes?

At least before this patch, the output looked like http://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/mobile/android/installer/Makefile.in#91
Axel is right. Thank you for testing this!

The feature works, it just works differently now.

Basically, we moved the step when we're constructing the multilocale.json (and locale-manifest.in) from early in packaging step, to the build step.

The steps automation does are as follows:

1) ./mach build
2) for loc in de fr pl; de ./mach build chrome-$loc; done;
3) MOZ_CHROME_MULTILOCALE="en-US it fr pl" ./mach package

STR:

1) get mozilla-central
2) rm ./objdir/dist/bin/res/multilocale.json
3) ./mach build
4) cat ./objdir/dist/bin/res/multilocale.json // missing
5) for loc in de fr pl; de ./mach build chrome-$loc; done;
6) cat ./objdir/dist/bin/res/multilocale.json // missing
7) MOZ_CHROME_MULTILOCALE="en-US it fr pl" ./mach package
8) cat ./objdir/dist/bin/res/multilocale.json // now it's updated to the list

9) rm ./objdir/dist/bin/res/multilocale.json
10) apply the patch from this bug
11) ./mach build
12) cat ./objdir/dist/bin/res/multilocale.json // now it has just the default ['en-US'] since MOZ_CHROME_MULTILOCALE is not set
13) for loc in de fr pl; de ./mach build chrome-$loc; done;
14) MOZ_CHROME_MULTILOCALE="en-US it fr pl" ./mach package
8) cat ./objdir/dist/bin/res/multilocale.json // still ['en-US'] since the package step did not update the list


====================

We can solve it either by changing the automation to do:
1) add `export MOZ_CHROME_MULTILOCALE="en-US it fr pl"` to mozconfig
2) ./mach build
3) for loc in de fr pl; de ./mach build chrome-$loc; done;
4) ./mach package

or by moving this patch to perform the steps in ./mach package and ./mach build installers-% to align with the current build system.

I sense that the latter is preferred, but I don't know how to hook our steps into the build system so that they're performed at packaging/repackaging and not build.
Flags: needinfo?(mshal)
> At least before this patch, the output looked like http://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/mobile/android/installer/Makefile.in#91

Yep, same with the patch. No changes here. :)
> We can solve it either by changing the automation to do:
> 1) add `export MOZ_CHROME_MULTILOCALE="en-US it fr pl"` to mozconfig
> 2) ./mach build
> 3) for loc in de fr pl; de ./mach build chrome-$loc; done;
> 4) ./mach package
> 
> or by moving this patch to perform the steps in ./mach package and ./mach
> build installers-% to align with the current build system.
> 
> I sense that the latter is preferred, but I don't know how to hook our steps
> into the build system so that they're performed at packaging/repackaging and
> not build.

I would _much prefer_ that we tell the build about MOZ_CHROME_MULTILOCALE in the mozconfig, and then make ./mach build include the ./mach build chrome-$loc steps.  No lying to the build system about what's going to happen -- it's so much better to have all the information up front.
Flags: needinfo?(l10n)
(In reply to Nick Alexander :nalexander from comment #63)
> I would _much prefer_ that we tell the build about MOZ_CHROME_MULTILOCALE in
> the mozconfig, and then make ./mach build include the ./mach build
> chrome-$loc steps.  No lying to the build system about what's going to
> happen -- it's so much better to have all the information up front.

I like this approach as well. This would fit in with the general direction that we've been moving over the years of having automation just call 'mach build', and the rest of the steps being implemented behind that.

However it's possible that you would just need to set the MOZ_CHROME_MULTILOCALE environment variable in the mozharness build step:

https://dxr.mozilla.org/mozilla-central/rev/a97fb1c39d51a7337b46957bde4273bd308b796a/testing/mozharness/mozharness/mozilla/l10n/multi_locale_build.py#131

Similar to how it's done in the package step:

https://dxr.mozilla.org/mozilla-central/rev/a97fb1c39d51a7337b46957bde4273bd308b796a/testing/mozharness/mozharness/mozilla/l10n/multi_locale_build.py#187

That might get you up and running and we could change the mozharness / build system integration points in a followup.
Flags: needinfo?(mshal)
We need to keep in mind Fennec release automation, in addition to Nightly and local builds. Not sure what the status is there.
Flags: needinfo?(l10n)
(In reply to Michael Shal [:mshal] from comment #65)
> However it's possible that you would just need to set the
> MOZ_CHROME_MULTILOCALE environment variable in the mozharness build step:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> a97fb1c39d51a7337b46957bde4273bd308b796a/testing/mozharness/mozharness/
> mozilla/l10n/multi_locale_build.py#131
> 
> Similar to how it's done in the package step:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> a97fb1c39d51a7337b46957bde4273bd308b796a/testing/mozharness/mozharness/
> mozilla/l10n/multi_locale_build.py#187
> 
> That might get you up and running and we could change the mozharness / build
> system integration points in a followup.

Actually, Pike reminded me that we build both the en-US and multi apks from the same job, so doing this will probably mess up the en-US apk. It seems like to fit with the current mozharness structure, we'd have to update multilocale.json during package. Otherwise, we need to change how the mozharness integration and/or task structure works.
Michael, can you advise me on how to hook my steps into the system so that they get triggered at packaging?
Flags: needinfo?(mshal)
(In reply to Michael Shal [:mshal] from comment #67)
> (In reply to Michael Shal [:mshal] from comment #65)
> > However it's possible that you would just need to set the
> > MOZ_CHROME_MULTILOCALE environment variable in the mozharness build step:
> > 
> > https://dxr.mozilla.org/mozilla-central/rev/
> > a97fb1c39d51a7337b46957bde4273bd308b796a/testing/mozharness/mozharness/
> > mozilla/l10n/multi_locale_build.py#131
> > 
> > Similar to how it's done in the package step:
> > 
> > https://dxr.mozilla.org/mozilla-central/rev/
> > a97fb1c39d51a7337b46957bde4273bd308b796a/testing/mozharness/mozharness/
> > mozilla/l10n/multi_locale_build.py#187
> > 
> > That might get you up and running and we could change the mozharness / build
> > system integration points in a followup.
> 
> Actually, Pike reminded me that we build both the en-US and multi apks from
> the same job, so doing this will probably mess up the en-US apk. It seems
> like to fit with the current mozharness structure, we'd have to update
> multilocale.json during package. Otherwise, we need to change how the
> mozharness integration and/or task structure works.

Urgh.  For Android, these en-US APKs are basically thrown away: we only ship multi-locale builds.  But yes, this will be a problem.  Isn't the solution to either:

- not expect one |mach build; mach package| to build different outputs
- or to make |mach build| take additional information about what output to build?

We can't hide information from the build system and expect to be able to move quickly.
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

Here's the latest attempt.

I separated the `multilocale.json-%` as the step used by repackaging to create `multilocale.json` that will be packaged into the repack.

This allows me to not even bother running `locale-manifest.in` on repack, since it will not be used anyway.

`multilocale.json` and `locale-manifest.in` steps are fired on package both for android and desktop.

Here's the try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d526bb30c5fbc8e37c3466eb5d8475b083333145

Michael - can you take another look please?
Attachment #8873747 - Flags: review+ → review?(mshal)
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

ugh, I don't understand. The builds on the try build doesn't package ./res/multilocale.json into omni.ja and don't seem to package the right chrome resources.

The very same steps executed locally give me a fully localized build with the right multilocale.json in toolkit's omni.ja ./res directory.

I'll keep investigating tomorrow :(
Attachment #8873747 - Flags: review?(mshal)
Attachment #8873747 - Flags: review?(l10n)
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

I can't believe that. I spent 6 hours analyzing and freaking out over a try build that didn't package my changes... A try build I launched off of central... 
`./mach try fuzzy` made it so easy.

Here's a new try build that actually is based on this patch. Maybe it'll all make sense somehow.

Sorry for the spam mail :(
Attachment #8873747 - Flags: review?(mshal)
Attachment #8873747 - Flags: review?(l10n)
I've inspected the builds on https://treeherder.mozilla.org/#/jobs?repo=try&revision=c526622118fa44d11746ab7d838e1d1dfd71286e&selectedJob=140129863, which seems to be the same patch as on mozreview now, though probably with a rebase?

(Drive-by clearing the info on mshal as I think that question was answered on irc)
Flags: needinfo?(mshal)
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

https://reviewboard.mozilla.org/r/145156/#review198986

lgtm, and I've glanced at the binaries from langpacks to artifact builds. Real review probably still on Mike.
Attachment #8873747 - Flags: review?(l10n) → review+
Comment on attachment 8873747 [details]
Bug 1362617 - Generalize MOZ_CHROME_MULTILOCALE to work for browser as well.

https://reviewboard.mozilla.org/r/145156/#review199052

::: toolkit/mozapps/installer/packager.mk:204
(Diff revision 27)
> +# This version of the target uses AB_CD to build multilocale.json and places it
> +# in the $(XPI_NAME)/res dir - it should be used when repackaging a build.
> +multilocale.json-%: LOCALES?=$(AB_CD)
> +multilocale.json-%: MULTILOCALE_DIR=$(DIST)/xpi-stage/$(XPI_NAME)/res
> +multilocale.json-%:
> +	@$(MKDIR) -p "$(MULTILOCALE_DIR)"

We could probably find a way to combine these two, but I don't want to make you jump through more hoops, and it would likely break some random build :)

::: toolkit/mozapps/installer/packager.mk:228
(Diff revision 27)
> +		do \
> +		  printf "$$ENTRY""$$LOCALE"'@JAREXT@\n' >> $@; \
> +		  printf "$$ENTRY""$$LOCALE"'.manifest\n' >> $@; \
> +	  done \
> +	done
> +

nit: Stray newline at the end of the file.
Attachment #8873747 - Flags: review?(mshal) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17ef68e368bb
Generalize MOZ_CHROME_MULTILOCALE to work for browser as well. r=gps,mshal,Pike,ted
Thank you Axel and Michael for all the support and mentoring me through the build system!

This was a hell of a journey and I'm almost sad we only stopped at 30 revisions and didn't test what's MozReview limit ;)

> We could probably find a way to combine these two, but I don't want to make you jump through more hoops, and it would likely break some random build :)

I think that that's the part that I failed to figure out and it was causing all kinds of troubles when I tried. 
Basically `installers-%` should pass LOCALES=$(AB_CD) and `package` should pass LOCALES=$(MOZ_CHROME_MULTILOCALE).
I can file a follow-up if you think we can make it work.
Seems like this broke some OSX cross-compile: https://treeherder.mozilla.org/logviewer.html#?job_id=140196849&repo=autoland&lineNumber=68263

It seems that on MacOS, the ./mach package` is looking for res/multilocale.json in `Nightly.app/Contents/Resources/res/multilocale.json` rather than `dist/bin/res/multilocale.json`.

Does it mean we should put the file in Nightly.app/Contents/Resources/res, or make it look for the file in dist/bin/res?
Flags: needinfo?(mshal)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #82)
> Seems like this broke some OSX cross-compile:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=140196849&repo=autoland&lineNumber=68263
> 
> It seems that on MacOS, the ./mach package` is looking for
> res/multilocale.json in
> `Nightly.app/Contents/Resources/res/multilocale.json` rather than
> `dist/bin/res/multilocale.json`.
> 
> Does it mean we should put the file in Nightly.app/Contents/Resources/res,
> or make it look for the file in dist/bin/res?

I think we'd want it to go into the Nightly.app directory, since that's what gets packaged in OSX. The reason it's looking there is because of how RESPATH is set in browser/installer based on whether or not it's OSX. Maybe something like this:

multilocale.json: MULTILOCALE_DIR=$(if $(filter cocoa,$(MOZ_WIDGET_TOOLKIT)),$(DIST)/Nightly.app/Contents/Resources/res,$(DIST)/bin/res)

Or you could move the if-block for setting BASE_PATH up and add MULTILOCALE_DIR there, so that way RESPATH/BINPATH line up more clearly:

ifeq ($(MOZ_BUILD_APP),mobile/android)
BASE_PATH:=@BINPATH@
MULTILOCALE_DIR = $(DIST)/$(BINPATH)/res
else
BASE_PATH:=@RESPATH@
MULTILOCALE_DIR = $(DIST)/$(RESPATH)/res
endif

I'm not sure how that would work with the multilocale.json-% rules though, so you would want to test a repack on OSX as well. Might want to build with -p all and add a repack on every platform to see what other surprises we have.
Flags: needinfo?(mshal)
hg error in cmd: hg pull gecko -r 8e7ba2086ce0cf518f281fe016521b968110f4ee: pulling from https://reviewboard-hg.mozilla.org/gecko
abort: HTTP Error 500: Internal Server Error
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ded8c9fd0d2
Generalize MOZ_CHROME_MULTILOCALE to work for browser as well. r=gps,mshal,Pike,ted
https://hg.mozilla.org/mozilla-central/rev/9ded8c9fd0d2
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: