Mike Hommey [:glandium] Reporter
	Description • 12 years ago

When using --both-manifests:
- JarMaker.py creates chrome/jar.manifest with entries for chrome
- JarMaker.py creates chrome.manifest with entries for chrome
(obviously, that's what --both-manifests is expected to do)
but it also adds "manifest chrome/jar.manifest" in chrome.manifest, which basically makes all entries in chrome.manifest redundant with those in chrome/jar.manifest.

Fortunately, when packing langpacks, chrome/jar.manifest is not included, so that leaves the dangling "manifest chrome/jar.manifest" entry.

Either "manifest chrome/jar.manifest" should not be added with doing --both-manifests, or it should be the only entry, which is that the default does, actually.

That being said, there is a related problem with JarMaker for langpacks (and probably the langpack build rules themselves) in the world of multiple applications, and the world of bug 755724. For instance, on metro builds, there are three applications + the gre, and each of them has their own chrome.
So what JarMaker creates there is:
- browser/chrome.manifest
- browser/chrome/chrome.manifest
- chrome.manifest
- chrome/chrome.manifest
- metro/chrome.manifest
- metro/chrome/chrome.manifest
- webapprt/chrome.manifest
- webapprt/chrome/chrome.manifest

And obviously, while that works very well for application packaging, it doesn't work for xpi packaging: the resulting extension only provides toolkit localization, and nothing else.

	Mike Hommey [:glandium] Reporter
	Comment 1 • 12 years ago

I'm thinking the latter could be solved by an actual "smart" packager for addons.

	Axel Hecht [:Pike]
	Comment 2 • 12 years ago

I doubt that we ever defined what multi-app langpack packaging is supposed to do, nor tied what the app chrome does to support that. Are the three apps providing different chrome providers, or are they all claiming to be "browser"? I'd probably consider that to be a bug.

	Mike Hommey [:glandium] Reporter
	Comment 3 • 12 years ago

webapprt doesn't enable the addon manager at the moment, so it's not really a problem, although it will probably become one some day.

metro and browser are two separate apps with different appids.

An easy way to handle these langpacks is probably to add something like:
manifest browser/chrome.manifest application={appid-of-browser}
manifest metro/chrome.manifest application={appid-of-metro}

to the root chrome.manifest in the xpi.

	Jim Mathies [:jimm] Assignee
	Comment 4 • 12 years ago

I've been experimenting around with building lang packs so i can try to tackle this bug. Basically I've followed the steps listed on the wiki for repacking a new testing lang pack here - 

https://developer.mozilla.org/en-US/docs/Creating_a_Language_Pack#L10n_binary_repack

Using |make langpack| in the browser/locales location I'ver created an xpi (which shows up in dist/win32/ on Windows). In this, I see that the package only has toolkit resources. I think this is part of the browser, although I'd appreciate feedback. 

I've been able to address missing browser resources by editing toolkits l10n.mk such that the zip operation also includes the browser sub folder. Currently just a one off fix but I thought I'd post here first to see if I can get some feedback on whether or not I'm on the right track. 

I haven't tested the resulting xpi but will next. I think I'll run into the root chrome manifest problem glandium mentioned as well.

Note I'm not looking at the --both-manifests issue at this point. Searching mc I see no uses of it, so I'm not sure why that would hold up metro-build. But I'm sure broken language packs would be bad (according to glandium on irc) so I've been looking at that first.

	Jim Mathies [:jimm] Assignee
	Comment 5 • 12 years ago

(In reply to Jim Mathies [:jimm] from comment #4)
> Using |make langpack| in the browser/locales location I'ver created an xpi
> (which shows up in dist/win32/ on Windows). In this, I see that the package
> only has toolkit resources. I think this is part of the browser, although
> I'd appreciate feedback. 

part of the *problem*, not 'browser'.

	Jim Mathies [:jimm] Assignee
	Comment 7 • 12 years ago

This addresses the xpi problem for browser builds. I haven't investigated the both-manifests issue yet.

	Jim Mathies [:jimm] Assignee
	Comment 8 • 12 years ago

Comment on attachment 707803 [details] [diff] [review]
wip v.1

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

::: browser/locales/Makefile.in
@@ +12,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  
>  include $(topsrcdir)/config/config.mk
>  
> +MOZ_LANG_TITLE = TEST

(ignore this line, for testing purposes only)

	Jim Mathies [:jimm] Assignee
	Comment 9 • 12 years ago

- cleaned up some commenting

	Mike Hommey [:glandium] Reporter
	Comment 10 • 12 years ago

Comment on attachment 707805 [details] [diff] [review]
wip v.1

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

Hackish, but probably good for now. The JarMaker option looks overlong.

Axel, what do you think?

::: browser/locales/Makefile.in
@@ +83,5 @@
>  
> +# Required for l10n.mk - If we are building both front ends define
> +# a list of app sub dirs that should be included in lang pack xpis.
> +ifdef MOZ_METRO
> +DIST_SUBDIRS = browser metro

You still need browser when not building MOZ_METRO.

@@ +150,5 @@
>  	@$(MAKE) -C ../../services/sync/locales AB_CD=$* XPI_NAME=locale-$*
>  	@$(MAKE) -C ../../extensions/spellcheck/locales AB_CD=$* XPI_NAME=locale-$*
>  	@$(MAKE) -C ../../intl/locales AB_CD=$* XPI_NAME=locale-$*
> +	@$(MAKE) libs AB_CD=$* XPI_NAME=locale-$* PREF_DIR=$(PREF_DIR) \
> +	  XPI_ROOT_APPID={ec8030f7-c20a-464f-9b0e-13a3a9e97384}

MOZ_APP_ID defines this value in browser/confvars.sh, which is then AC_SUBSTed and put in config/autoconf.mk. So you could just use $(MOZ_APP_ID) here.
I think XPI_ROOT_APPID would be better defined in browser/defs.mk and browser/metro/defs.mk.

::: config/JarMaker.py
@@ +158,5 @@
>      if self.useChromeManifest:
>        self.updateManifest(chromeManifest, chromebasepath % 'chrome/',
>                            register)
> +    # If requested, add a root chrome manifest entry (assumed to be in the
> +    # sub directory under chromeManifest) with the application specific id.

assumed to be in the directory above chromeManifest?

::: config/rules.mk
@@ +1440,5 @@
>  $(FINAL_TARGET)/chrome: $(call mkdir_deps,$(FINAL_TARGET)/chrome)
>  
> +# For langpack packaging we may specify that an application
> +# sub-dir should be added to the root chrome manifest with
> +# a specific application id. 

trailing whitespace.

@@ +1441,5 @@
>  
> +# For langpack packaging we may specify that an application
> +# sub-dir should be added to the root chrome manifest with
> +# a specific application id. 
> +ifneq ($(XPI_ROOT_APPID),)

ifdef XPI_ROOT_APPID

::: toolkit/locales/l10n.mk
@@ +160,5 @@
> +else
> +# Defaut behavior: everything we want exists in a single chrome directory.
> +PKG_ZIP_DIRS = chrome
> +endif
> +endif

You can summarize this entire block with:
PKG_ZIP_DIRS = chrome $(or $(DIST_SUBDIRS),$(DIST_SUBDIR))

	Jim Mathies [:jimm] Assignee
	Comment 11 • 12 years ago

(In reply to Mike Hommey [:glandium] from comment #10)
> I think XPI_ROOT_APPID would be better defined in browser/defs.mk and
> browser/metro/defs.mk.

Isn't defs.mk included when we rebuild everything in platform? I can test but I'm worried --root-manifest-entry-appid will get invoked for every jar we (re)build for xpi-stage in a repack.

	Mike Hommey [:glandium] Reporter
	Comment 12 • 12 years ago

(In reply to Jim Mathies [:jimm] from comment #11)
> (In reply to Mike Hommey [:glandium] from comment #10)
> > I think XPI_ROOT_APPID would be better defined in browser/defs.mk and
> > browser/metro/defs.mk.
> 
> Isn't defs.mk included when we rebuild everything in platform?

It's not. At least not for things where we invoke make -C $(DEPTH)/some/path, unless some/path is under browser.

	Jim Mathies [:jimm] Assignee
	Comment 13 • 12 years ago

(In reply to Mike Hommey [:glandium] from comment #12)
> (In reply to Jim Mathies [:jimm] from comment #11)
> > (In reply to Mike Hommey [:glandium] from comment #10)
> > > I think XPI_ROOT_APPID would be better defined in browser/defs.mk and
> > > browser/metro/defs.mk.
> > 
> > Isn't defs.mk included when we rebuild everything in platform?
> 
> It's not. At least not for things where we invoke make -C
> $(DEPTH)/some/path, unless some/path is under browser.

I did a test build just in browser with this. It does add useless entries in dist/bin/chrome.manifest for normal builds. Maybe I can find a way to avoid this. I really only want this feature turned on when generating xpis.

	Mike Hommey [:glandium] Reporter
	Comment 14 • 12 years ago

Only append the option to MAKE_JARS_FLAGS when XPI_NAME is defined?

	Jim Mathies [:jimm] Assignee
	Comment 15 • 12 years ago

Updates per review comments and irc discussion.

	Mike Hommey [:glandium] Reporter
	Comment 16 • 12 years ago

Comment on attachment 708563 [details] [diff] [review]
wip v.2

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

::: browser/locales/Makefile.in
@@ +86,5 @@
> +ifdef MOZ_METRO
> +DIST_SUBDIRS = browser metro
> +else
> +DIST_SUBDIRS = browser
> +endif

Could you also do a version of the patch that can land on m-c before bug 755724 but doesn't need a change to bug 755724?

Something like
DIST_SUBDIRS = $(DIST_SUBDIR)
would probably do.

::: config/rules.mk
@@ +1442,5 @@
> +# For langpack packaging we may specify that an application
> +# sub-dir should be added to the root chrome manifest with
> +# a specific application id.
> +ifdef XPI_NAME
> +MAKE_JARS_FLAGS += --root-manifest-entry-appid=$(XPI_ROOT_APPID)

make it:
MAKE_JARS_FLAGS += $(addprefix --root-manifest-entry-appid=,$(XPI_ROOT_APPID))

That will avoid adding --root-manifest-entry-appid= when XPI_ROOT_APPID is not defined.

	Jim Mathies [:jimm] Assignee
	Comment 17 • 12 years ago

(In reply to Mike Hommey [:glandium] from comment #16)
> Comment on attachment 708563 [details] [diff] [review]
> wip v.2
> 
> Review of attachment 708563 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/locales/Makefile.in
> @@ +86,5 @@
> > +ifdef MOZ_METRO
> > +DIST_SUBDIRS = browser metro
> > +else
> > +DIST_SUBDIRS = browser
> > +endif
> 
> Could you also do a version of the patch that can land on m-c before bug
> 755724 but doesn't need a change to bug 755724?
> 
> Something like
> DIST_SUBDIRS = $(DIST_SUBDIR)
> would probably do.

Not sure how make treats this, wouldn't this define DIST_SUBDIRS with an empty string?

how about 

# Required for l10n.mk - defines a list of app sub dirs that should
# be included in lang pack xpis.
ifdef MOZ_METRO
# metro build, include both app folders
DIST_SUBDIRS = browser metro
else
ifdef DIST_SUBDIR
DIST_SUBDIRS = $(DIST_SUBDIR)
endif
endif

	Mike Hommey [:glandium] Reporter
	Comment 19 • 12 years ago

(In reply to Jim Mathies [:jimm] from comment #17)
> Not sure how make treats this, wouldn't this define DIST_SUBDIRS with an
> empty string?

ifdef actually doesn't care whether the variable is defined or not. However, with delayed expansion, it considers it defined ; but DIST_SUBDIRS := $(DIST_SUBDIR) works.

	Mike Hommey [:glandium] Reporter
	Comment 20 • 12 years ago

(In reply to Mike Hommey [:glandium] from comment #19)
> but DIST_SUBDIRS := $(DIST_SUBDIR) works.

... only if DIST_SUBDIR is defined first, which means, defs.mk included first, which means config.mk included first. Which is the case, but it's better to make things clear :)

	Axel Hecht [:Pike]
	Comment 21 • 12 years ago

I'm afraid I need a reminder what we're trying to do. I'm not sure if that changed over the course of the bug, too. Or extend the tests in config/tests?

Right now, I only have a very abstract idea of things, and not enough to do a review.

	Mike Hommey [:glandium] Reporter
	Comment 22 • 12 years ago

(In reply to Axel Hecht [:Pike] from comment #21)
> I'm afraid I need a reminder what we're trying to do. I'm not sure if that
> changed over the course of the bug, too. Or extend the tests in config/tests?
> 
> Right now, I only have a very abstract idea of things, and not enough to do
> a review.

Essentially, it's comment 3. If you're going to FOSDEM, I'll be there, so we can discuss it there if that helps.

	Jim Mathies [:jimm] Assignee
	Comment 23 • 12 years ago

A more hands-on example - in xpi-stage and the resulting xpi zip, the root chrome manifest has:

manifest chrome/x-testing.manifest
manifest browser/chrome.manifest application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}
manifest webapprt/chrome.manifest application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}
manifest metro/chrome.manifest application={99bceaaa-e3c6-48c1-b981-ef9b46b67d60}

plus the browser, webapprt, and metro folders.

It also has the neat side effect of allowing us to share firefox langpacks between desktop and metro browsers.

	Axel Hecht [:Pike]
	Comment 24 • 12 years ago

I'm slowly getting there, but now I'm coming down with the plague, and I'd rather have my head with me. Is it OK to look at this next week?

	Jim Mathies [:jimm] Assignee
	Comment 25 • 12 years ago

(In reply to Axel Hecht [:Pike] from comment #24)
> I'm slowly getting there, but now I'm coming down with the plague, and I'd
> rather have my head with me. Is it OK to look at this next week?

Is there anyone else that can look at it? This is holding up the final elm merge and metrofx nightlies off mc. We're already a month behind on getting that out.

	Axel Hecht [:Pike]
	Comment 26 • 12 years ago

I defer to glandium if he feels like we should land this, he's probably the best guy to follow up on that. I guess the general direction is probably fine, but I can't really do a review with the head that I have right now, and there's nobody in the l10n team that would understand this better than glandium does himself.

	Jim Mathies [:jimm] Assignee
	Comment 27 • 12 years ago

Comment on attachment 708563 [details] [diff] [review]
wip v.2

Thanks for the follow up.

	Jim Mathies [:jimm] Assignee
	Comment 28 • 12 years ago

Comment on attachment 709011 [details] [diff] [review]
mc patch wip

for up front mc landing. the rest can fold into metro-build.

	Mike Hommey [:glandium] Reporter
	Comment 29 • 12 years ago

Comment on attachment 709011 [details] [diff] [review]
mc patch wip

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

::: browser/locales/Makefile.in
@@ +79,5 @@
> +# Required for l10n.mk - defines a list of app sub dirs that should
> +# be included in lang pack xpis.
> +ifdef MOZ_METRO
> +# metro build, include both app folders
> +DIST_SUBDIRS = browser metro

This part should probably land with the metro landing, not in this specific patch, although i don't care /that/ much.

@@ +83,5 @@
> +DIST_SUBDIRS = browser metro
> +else
> +# only add DIST_SUBDIRS post bug 755724.
> +ifdef DIST_SUBDIR
> +DIST_SUBDIRS = browser

DIST_SUBDIRS = $(DIST_SUBDIR) certainly wouldn't hurt, here :)

::: config/rules.mk
@@ +1442,5 @@
> +# For langpack packaging we may specify that an application
> +# sub-dir should be added to the root chrome manifest with
> +# a specific application id.
> +MAKE_JARS_FLAGS += $(addprefix --root-manifest-entry-appid=,$(XPI_ROOT_APPID))
> +

You need to copy config/rules.mk to js/src/config/

We should probably add something like:
ifneq (,$(DIST_SUBDIR))
ifndef XPI_ROOT_APPID
$(error XPI_ROOT_APPID is not defined)
endif
endif

inside the ifneq (,$(wildcard $(JAR_MANIFEST)))/ifndef NO_DIST_INSTALL section.

	Jim Mathies [:jimm] Assignee
	Comment 30 • 12 years ago

> > +DIST_SUBDIRS = browser metro
> 
> This part should probably land with the metro landing, not in this specific
> patch, although i don't care /that/ much.
> 
> @@ +83,5 @@
> > +DIST_SUBDIRS = browser metro
> > +else
> > +# only add DIST_SUBDIRS post bug 755724.
> > +ifdef DIST_SUBDIR
> > +DIST_SUBDIRS = browser
> 
> DIST_SUBDIRS = $(DIST_SUBDIR) certainly wouldn't hurt, here :)


I've moved this to the metro-build parts, so now this looks a bit odd - 

# Required for l10n.mk - defines a list of app sub dirs that should
# be included in langpack xpis.
ifdef MOZ_METRO
# metro build, include both app folders
DIST_SUBDIRS = browser metro
else
DIST_SUBDIRS = $(DIST_SUBDIR)
endif

just seems like the non-metro build part should be

DIST_SUBDIRS = browser

despite the fact that we would need to update the sub dir in two places if we changed it. Which of course we won't.

	Mike Hommey [:glandium] Reporter
	Comment 31 • 12 years ago

This patch also adds the appid in the non-xpi chrome.manifest: my try build failed with:
Error: /builds/slave/try-lnx64/build/obj-firefox/dist/bin/chrome.manifest:65: Flags are not supported on "manifest" entries
Error: /builds/slave/try-lnx64/build/obj-firefox/dist/bin/chrome.manifest:66: Flags are not supported on "manifest" entries

	Mike Hommey [:glandium] Reporter
	Comment 32 • 12 years ago

Comment on attachment 709011 [details] [diff] [review]
mc patch wip

>+# For langpack packaging we may specify that an application
>+# sub-dir should be added to the root chrome manifest with
>+# a specific application id.
>+MAKE_JARS_FLAGS += $(addprefix --root-manifest-entry-appid=,$(XPI_ROOT_APPID))

This needs to be in a ifdef XPI_NAME block.

	Jim Mathies [:jimm] Assignee
	Comment 33 • 12 years ago

> We should probably add something like:
> ifneq (,$(DIST_SUBDIR))
> ifndef XPI_ROOT_APPID
> $(error XPI_ROOT_APPID is not defined)
> endif
> endif
> 
> inside the ifneq (,$(wildcard $(JAR_MANIFEST)))/ifndef NO_DIST_INSTALL
> section.

Hmm, errored out for webapprt on mc, which defines DIST_SUBDIR - 

http://mxr.mozilla.org/mozilla-central/source/webapprt/Makefile.in#18

(In reply to Mike Hommey [:glandium] from comment #32)
> Comment on attachment 709011 [details] [diff] [review]
> mc patch wip
> 
> >+# For langpack packaging we may specify that an application
> >+# sub-dir should be added to the root chrome manifest with
> >+# a specific application id.
> >+MAKE_JARS_FLAGS += $(addprefix --root-manifest-entry-appid=,$(XPI_ROOT_APPID))
> 
> This needs to be in a ifdef XPI_NAME block.

Maybe this will fix this as well.

	Jim Mathies [:jimm] Assignee
	Comment 34 • 12 years ago

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=447802b21ff9

assuming that last win opt build finishes green, here's the mc patch.

	Mike Hommey [:glandium] Reporter
	Comment 35 • 12 years ago

Comment on attachment 710710 [details] [diff] [review]
mc bits

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

Now I need to check how l10n-repack.py handles these langpacks.

::: config/rules.mk
@@ +1442,5 @@
> +ifdef XPI_NAME
> +# For langpack packaging we may specify that an application
> +# sub-dir should be added to the root chrome manifest with
> +# a specific application id.
> +MAKE_JARS_FLAGS += $(addprefix --root-manifest-entry-appid=,$(XPI_ROOT_APPID))

You could move this in the block below, which would avoid the extra ifdef XPI_NAME.

	Jim Mathies [:jimm] Assignee
	Comment 36 • 12 years ago

updated per comments.

	Mike Hommey [:glandium] Reporter
	Comment 38 • 12 years ago

Comment on attachment 710726 [details] [diff] [review]
mc bits v.2

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

::: config/rules.mk
@@ +1442,5 @@
>  ifneq (,$(wildcard $(JAR_MANIFEST)))
>  ifndef NO_DIST_INSTALL
> +
> +ifdef XPI_NAME
> +# For langpack packaging we may specify that an application

Actually, this applies to any addon, not only langpacks.

@@ +1450,5 @@
> +
> +# if DIST_SUBDIR is defined but XPI_ROOT_APPID is not there's
> +# no way langpacks will get packaged right, so error out.
> +ifneq (,$(DIST_SUBDIR))
> +$(warning $(DIST_SUBDIR))

Can you remove this warning?

	Jim Mathies [:jimm] Assignee
	Comment 39 • 12 years ago

(In reply to Mike Hommey [:glandium] from comment #38)
> Comment on attachment 710726 [details] [diff] [review]
> mc bits v.2
> 
> Review of attachment 710726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/rules.mk
> @@ +1442,5 @@
> >  ifneq (,$(wildcard $(JAR_MANIFEST)))
> >  ifndef NO_DIST_INSTALL
> > +
> > +ifdef XPI_NAME
> > +# For langpack packaging we may specify that an application
> 
> Actually, this applies to any addon, not only langpacks.
> 
> @@ +1450,5 @@
> > +
> > +# if DIST_SUBDIR is defined but XPI_ROOT_APPID is not there's
> > +# no way langpacks will get packaged right, so error out.
> > +ifneq (,$(DIST_SUBDIR))
> > +$(warning $(DIST_SUBDIR))
> 
> Can you remove this warning?

oops, sorry.

	Jim Mathies [:jimm] Assignee
	Comment 40 • 12 years ago

hrm, something broke with packaging app dirs. Looking into it.

	Jim Mathies [:jimm] Assignee
	Comment 41 • 12 years ago

Actually, spoke too soon, I didn't have the patches applied on my l10n repo. Attached is the resulting x-testing xpi, looks good.

	Jim Mathies [:jimm] Assignee
	Comment 44 • 12 years ago

(In reply to Jim Mathies [:jimm] from comment #42)
> Created attachment 710931 [details] [diff] [review]
> mc bits

ugh, wrong patch. :/

	Jim Mathies [:jimm] Assignee
	Comment 45 • 12 years ago

off the correct network drive this time.

	Jim Mathies [:jimm] Assignee
	Comment 47 • 12 years ago

https://hg.mozilla.org/projects/elm/rev/678ec6615e1b
https://hg.mozilla.org/projects/elm/rev/5ada4b9be686

	Jim Mathies [:jimm] Assignee
	Comment 48 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/62b7e0a68a11

	Jim Mathies [:jimm] Assignee
	Comment 49 • 12 years ago

path issue fix landed on elm - 

https://hg.mozilla.org/projects/elm/rev/d2d6de24317e

	Jim Mathies [:jimm] Assignee
	Comment 50 • 12 years ago

(In reply to Jim Mathies [:jimm] from comment #49)
> path issue fix landed on elm - 
> 
> https://hg.mozilla.org/projects/elm/rev/d2d6de24317e

backed out, and a different fix was pushed:

https://hg.mozilla.org/projects/elm/rev/576c1c5251e8

	Jim Mathies [:jimm] Assignee
	Comment 51 • 12 years ago

additional quote changes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e735fee82b4
https://hg.mozilla.org/projects/elm/rev/65f7ae19d948

	Ryan VanderMeulen [:RyanVM]
	Comment 52 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/62b7e0a68a11
https://hg.mozilla.org/mozilla-central/rev/5e735fee82b4