Update mail/locales/Makefile.in

RESOLVED FIXED in Thunderbird 52.0

Status

Thunderbird
Build Config
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

unspecified
Thunderbird 52.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
There seem to be a bunch of unported changes here.
(Assignee)

Comment 1

a year ago
Created attachment 8802105 [details] [diff] [review]
Port Bug 1223385 - use in-tree compare-locales in Makefiles
Attachment #8802105 - Flags: review?(philipp)
(Assignee)

Updated

a year ago
Assignee: nobody → aleth
Status: NEW → ASSIGNED
(Assignee)

Comment 2

a year ago
Created attachment 8802106 [details] [diff] [review]
Add XPI_ROOT_APPID for mail

There may be a reason TB doesn't have this (it's not a recent change), but if so, I don't know what it is.
Attachment #8802106 - Flags: review?(philipp)
(Assignee)

Comment 3

a year ago
Created attachment 8802205 [details] [diff] [review]
Port Bug 1223385 - use in-tree compare-locales in Makefiles

We need to specify the l10n-ini location due to a relative path issue.
Attachment #8802205 - Flags: review?(madperson)
(Assignee)

Comment 4

a year ago
Comment on attachment 8802205 [details] [diff] [review]
Port Bug 1223385 - use in-tree compare-locales in Makefiles

rs+ over IRC
Attachment #8802205 - Flags: review?(madperson) → review+
(Assignee)

Updated

a year ago
Attachment #8802105 - Flags: review?(philipp)
(Assignee)

Updated

a year ago
Attachment #8802105 - Attachment is obsolete: true
(Assignee)

Comment 5

a year ago
https://hg.mozilla.org/comm-central/rev/b165ad0f6019cf5ee7320c8909e99fcb4ffd06a0
Bug 1311005 - Port Bug 1223385 - use in-tree compare-locales in Makefiles. r=philipp DONTBUILD

Comment 6

a year ago
Comment on attachment 8802106 [details] [diff] [review]
Add XPI_ROOT_APPID for mail

If you look at https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/l10n.mk#164 onwards, it uses DIST_SUBDIR if DIST_SUBDIRS is not set anyway but no harm in setting it I guess.
Comment on attachment 8802106 [details] [diff] [review]
Add XPI_ROOT_APPID for mail

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

Do you know what XPI_ROOT_APPID is for? Would be good to know this before we push a patch that includes it.

::: mail/defs.mk
@@ +1,1 @@
> +XPI_ROOT_APPID=$(MOZ_APP_ID)

defs.mk looks pretty bare. Maybe add the license header? I also don't see it included anywhere?
Attachment #8802106 - Flags: review?(philipp) → review+
(Assignee)

Comment 8

a year ago
(In reply to Philipp Kewisch [:Fallen] from comment #7)
> Comment on attachment 8802106 [details] [diff] [review]
> Add XPI_ROOT_APPID for mail
> 
> Review of attachment 8802106 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you know what XPI_ROOT_APPID is for? Would be good to know this before we
> push a patch that includes it.

I don't know for sure, hence comment 2, as I thought you'd be the expert on addon-related things ;) I agree we should find out!

The only in-code documentation is http://searchfox.org/mozilla-central/source/config/rules.mk#1153. It's possible (looking at bug 802254) that this is obsolete and was only added for metro.

> ::: mail/defs.mk
> @@ +1,1 @@
> > +XPI_ROOT_APPID=$(MOZ_APP_ID)
> 
> defs.mk looks pretty bare. Maybe add the license header? I also don't see it
> included anywhere?

It's identical to browser/defs.mk. defs.mk files are included from config.mk.
See also http://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/jar.py#287

Maybe you can check if we need that kind of thing and compare the differences before/after. Note that manifest files are often created with the buildlist action, so to see differences you may need to clobber some files.

Even if I am normally for parity with Firefox, I think we should add some information to mail/defs.mk explaining what it is for. Otherwise people may go about adding various variables there without knowing exactly what for. Just a simple comment like:

# The variables defined here will be set for all files within the mail/ 
# subdirectory

Looks like defs.mk is pretty handy, I might use it for the gdata-provider and calendar too!
(Assignee)

Comment 10

a year ago
The difference appears to be that in a packaged install, the startup error

Could not read chrome manifest 'file:///Applications/Thunderbird.app/Contents/Resources/chrome.manifest'.

is gone, because that file (of zero length) is now created. It's hard to see how a zero length file could have any other impact though, so I must be missing something. (It's zero length for Firefox too, so that's not a bug.)
(Assignee)

Comment 11

a year ago
(Note the stated purpose of XPI_ROOT_APPID is that xpi repacks have access to the strings from the app as well as the strings from toolkit.)
(Assignee)

Comment 12

a year ago
The other chrome.manifest differences produced are consistent with comment 11:
dist/xpi-stage/chrome.manifest:+manifest dbgserver/chrome.manifest application={3550f703-e582-4d05-9a08-453d09bdfdc6}
dist/xpi-stage/chrome.manifest:+manifest locale-en-US/chrome.manifest application={3550f703-e582-4d05-9a08-453d09bdfdc6}

I'm still not sure what these are good for in practice.
(Assignee)

Updated

a year ago
Flags: needinfo?(philipp)
These would be including those manifest files, which may contain further instructions. Does dbgserver/chrome.manifest and locale-en-US/chrome.manifest exist in dist/xpi-stage/ ? Are these manifest files also included from other manifests?

That aside, is it really in dist/xpi-stage directly? I would have assumed a subdirectory thereof, because it doesn't make much sense where it is, nor will it be packaged anywhere (afaik).

Maybe it would make sense to needinfo someone who knows what this is for, possibly the patch author or someone from #build.
Flags: needinfo?(philipp)
(Assignee)

Comment 14

a year ago
(In reply to Philipp Kewisch [:Fallen] from comment #13)
> These would be including those manifest files, which may contain further
> instructions. Does dbgserver/chrome.manifest and
> locale-en-US/chrome.manifest exist in dist/xpi-stage/ ? Are these manifest
> files also included from other manifests?

dist/xpi-stage/dbgserver/chrome.manifest and dist/xpi-stage/locale-en-US/chrome.manifest both exist independently of this patch.
Neither of these are included from any other chrome.manifest file.

> That aside, is it really in dist/xpi-stage directly? I would have assumed a
> subdirectory thereof, because it doesn't make much sense where it is, nor
> will it be packaged anywhere (afaik).

Well, like the Thunderbird.app/Contents/Resources/chrome.manifest above, these are root manifests, so I'm not surprised to find them in what are effectively root dirs. That still doesn't explain what they are good for of course ;)
(Assignee)

Comment 15

a year ago
OK, so the story seems to be that Fx platform and app resources were split in bug 755724 for metro. Since then browser/ has been a DIST_SUBDIR with its own omni.jar. So while the ability to restrict repacks to a particular app dir strictly speaking isn't needed any more since metro is gone, Fx l10n would break if XPI_ROOT_APPID wasn't set. According to glandium, the references to addons/xpis in the code are just references to langpacks.

Thunderbird wasn't affected because it never followed the app/platform split - DIST_SUBDIR := mail is commented out in mail/app/Makefile.in.
(Assignee)

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
You need to log in before you can comment on or make changes to this bug.