1311005 - Update mail/locales/Makefile.in

Status: RESOLVED FIXED

(Thunderbird :: Build Config, defect)

Description

[aleth [:aleth]]

There seem to be a bunch of unported changes here.

Comment 1

[aleth [:aleth]]

Attachment: Port Bug 1223385 - use in-tree compare-locales in Makefiles

Comment 2

[aleth [:aleth]]

Attachment: 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.

Comment 3

[aleth [:aleth]]

Attachment: Port Bug 1223385 - use in-tree compare-locales in Makefiles

We need to specify the l10n-ini location due to a relative path issue.

Comment 4

[aleth [:aleth]]

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

rs+ over IRC

Comment 5

[aleth [:aleth]]

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

[Ian Neal]

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 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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?

Comment 8

[aleth [:aleth]]

(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.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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!

Comment 10

[aleth [:aleth]]

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.)

Comment 11

[aleth [:aleth]]

(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.)

Comment 12

[aleth [:aleth]]

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.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 14

[aleth [:aleth]]

(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 ;)

Comment 15

[aleth [:aleth]]

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.

Comment 16

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/edfe22f3146694cab42ca04046cf8c6dc52e53d5
Bug 1311005 - Add XPI_ROOT_APPID for mail. r=philipp