Port bug 1362617 to C-C [Generalize MOZ_CHROME_MULTILOCALE to work for browser as well]

RESOLVED FIXED in Thunderbird 58.0

Status

Thunderbird
Build Config
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: Paenglab, Assigned: Jorg K (GMT+1))

Tracking

unspecified
Thunderbird 58.0

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

7 months ago
Created attachment 8889038 [details] [diff] [review]
multilocale.patch

Not tested but it should work. The path in

DPKG_LOCALE_MANIFEST=$(topobjdir)/toolkit/locales/locale-manifest.in

looks correct in my objdir.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8889038 - Flags: review?(jorgk)
(Assignee)

Comment 3

7 months ago
Comment on attachment 8889038 [details] [diff] [review]
multilocale.patch

That got built.
Attachment #8889038 - Flags: review?(jorgk) → review+
(Reporter)

Updated

7 months ago
Keywords: checkin-needed
(Reporter)

Comment 4

7 months ago
Bug 1362617 was backed-out. See bug 1362617 comment 21.
Keywords: checkin-needed
(Reporter)

Updated

7 months ago
Depends on: 1362617
(Reporter)

Comment 5

7 months ago
The changes in bug 1362617 are now too complicated for my null makefile knowledge. This can someone do which knows what he does.
Assignee: richard.marti → nobody
Status: ASSIGNED → NEW
(Reporter)

Updated

7 months ago
Attachment #8889038 - Attachment is obsolete: true
Jorg - heads up. We're getting ready to land it.

The two differences I believe for this patch is that we now added `MOZ_CHROME_LOCALE_ENTRIES` to your locales/Makefile.in which has the list of entries for each locale.

Based on the resources I see you removing from package-manifest.in, I think you want to set it to:

```
MOZ_CHROME_LOCALE_ENTRIES=@RESPATH@/chrome/
```

and it'll populate in PKG_LOCALE_MANIFEST.

For `PKG_LOCALE_MANIFEST` instead of pointing it at toolkit, you'll now have it in the objdir matching your locales dir, so sth like:

```
DEFINES += -DPKG_LOCALE_MANIFEST=$(topobjdir)/{mail|suite|im}/locales/locale-manifest.in
```

Let me know if you need more help! I'll be happy to look at the patch
(Assignee)

Comment 7

4 months ago
Created attachment 8921997 [details] [diff] [review]
1383388-multilocale.patch (v2)

Thanks for the heads-up, Zibi. I really have no idea what I'm doing here, so perhaps Tom can take it from here.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8921997 - Flags: review?(mozilla)
Attachment #8921997 - Flags: feedback?(gandalf)
Comment on attachment 8921997 [details] [diff] [review]
1383388-multilocale.patch (v2)

lgtm!
Attachment #8921997 - Flags: feedback?(gandalf) → feedback+
Comment on attachment 8921997 [details] [diff] [review]
1383388-multilocale.patch (v2)

Review of attachment 8921997 [details] [diff] [review]:
-----------------------------------------------------------------

This look reasonable.
Attachment #8921997 - Flags: review?(mozilla) → review+

Comment 10

4 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8d4167a5c1e0
Port bug 1362617 to C-C [Generalize MOZ_CHROME_MULTILOCALE to work for browser as well]. r=zibi,tomprince CLOSED TREE
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
(Assignee)

Comment 11

4 months ago
I had to land this right on top of a whole lot of bustage since bug 1362617 landed.
Target Milestone: --- → Thunderbird 58.0
(Assignee)

Comment 12

4 months ago
That busted the build:

mozbuild.preprocessor.Error: ('/builds/slave/tb-c-cen-lx-000000000000000000/build/mail/installer/package-manifest.in', 66, 'FILE_NOT_FOUND', '/builds/slave/tb-c-cen-lx-000000000000000000/build/objdir-tb/mail/locales/locale-manifest.in')

https://hg.mozilla.org/comm-central/rev/8d4167a5c1e0#l4.13
+DEFINES += -DPKG_LOCALE_MANIFEST=$(topobjdir)/mail/locales/locale-manifest.in

Who builds that?
Looks like M-C now settled for:
https://hg.mozilla.org/mozilla-central/rev/9ded8c9fd0d2#l2.12
+DEFINES += -DPKG_LOCALE_MANIFEST=$(topobjdir)/browser/installer/locale-manifest.in
+MOZ_CHROME_LOCALE_ENTRIES=@RESPATH@/browser/chrome/ @RESPATH@/chrome/

Zibi, any hints?
Flags: needinfo?(gandalf)
(Assignee)

Comment 13

4 months ago
Created attachment 8923075 [details] [diff] [review]
1383388-follow-up.patch (v1)

This makes a little more like M-C:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d6999483fd3ba1b164334adc62c36f57ecdd23ce
Attachment #8923075 - Flags: feedback?(gandalf)
(Assignee)

Comment 14

4 months ago
Created attachment 8923079 [details] [diff] [review]
1383388-follow-up.patch (v2)

Try was successful, here the patch also for IB and SM.
Attachment #8923075 - Attachment is obsolete: true
Attachment #8923075 - Flags: feedback?(gandalf)
Flags: needinfo?(gandalf)

Comment 15

4 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/395acea4a3e7
Follow-up: Adapt to landed M-C version. rs=bustage-fix CLOSED TREE
Oh, my fault. Yes, we had to change a couple things after I reviewed your patch and I failed to update it here. Glad it works now :)
(Assignee)

Comment 17

4 months ago
The only doubt I had was whether
  MOZ_CHROME_LOCALE_ENTRIES=@RESPATH@/chrome/
is enough or we should do it like FF
  MOZ_CHROME_LOCALE_ENTRIES=@RESPATH@/mail/chrome/ @RESPATH@/chrome/

Works with the former.
The only big we moved is the entries in `/mail/installer/package-manifest.in`, which for mail were:

```
-@RESPATH@/chrome/@AB_CD@@JAREXT@
-@RESPATH@/chrome/@AB_CD@.manifest
```

are now generated from python *per locale*. For now it doesn't change much because we're building only single locale builds, but later, we may be able to just do `MOZ_CHROME_MULTILOCALE="fr pl de" ./mach package` and get chrome entries packaged for all of those locales.

So yes, in case of mail|im|suite you only need `MOZ_CHROME_LOCALE_ENTRIES=@RESPATH@/chrome/` :)
Comment hidden (mozreview-request)

Updated

4 months ago
Attachment #8924679 - Flags: review?(mozilla) → review+

Comment 20

4 months ago
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/comm-central/rev/87226f1198cc
Followup to support building in m-c topdir as well; r=me
You need to log in before you can comment on or make changes to this bug.