Closed Bug 802254 Opened 7 years ago Closed 7 years ago

JarMaker.py's manifest handling is wrong, and limited

Categories

(Toolkit Graveyard :: Build Config, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: glandium, Assigned: jimm)

References

Details

Attachments

(3 files, 10 obsolete files)

xpi
602.18 KB, application/x-xpinstall
Details
8.33 KB, patch
Details | Diff | Splinter Review
2.91 KB, patch
Details | Diff | Splinter Review
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.
I'm thinking the latter could be solved by an actual "smart" packager for addons.
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.
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.
Depends on: 809798
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.
(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'.
Assignee: nobody → jmathies
Attached patch starting point patch (obsolete) — Splinter Review
Attached patch wip v.1 (obsolete) — Splinter Review
This addresses the xpi problem for browser builds. I haven't investigated the both-manifests issue yet.
Attachment #707253 - Attachment is obsolete: true
Attachment #707803 - Flags: review?(mh+mozilla)
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)
Attached patch wip v.1 (obsolete) — Splinter Review
- cleaned up some commenting
Attachment #707803 - Attachment is obsolete: true
Attachment #707803 - Flags: review?(mh+mozilla)
Attachment #707805 - Flags: review?(mh+mozilla)
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))
Attachment #707805 - Flags: review?(mh+mozilla)
Attachment #707805 - Flags: review?(l10n)
Attachment #707805 - Flags: review+
(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.
(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.
(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.
Only append the option to MAKE_JARS_FLAGS when XPI_NAME is defined?
Attached patch wip v.2 (obsolete) — Splinter Review
Updates per review comments and irc discussion.
Attachment #707805 - Attachment is obsolete: true
Attachment #707805 - Flags: review?(l10n)
Attachment #708563 - Flags: review?(l10n)
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.
(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
Attached patch mc patch wip (obsolete) — Splinter Review
(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.
(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 :)
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.
(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.
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.
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?
(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.
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.
Comment on attachment 708563 [details] [diff] [review]
wip v.2

Thanks for the follow up.
Attachment #708563 - Flags: review?(l10n)
Comment on attachment 709011 [details] [diff] [review]
mc patch wip

for up front mc landing. the rest can fold into metro-build.
Attachment #709011 - Flags: review?(mh+mozilla)
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.
Attachment #709011 - Flags: review?(mh+mozilla) → review+
> > +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.
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
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.
> 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.
Attached patch mc bits (obsolete) — Splinter Review
https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=447802b21ff9

assuming that last win opt build finishes green, here's the mc patch.
Attachment #708563 - Attachment is obsolete: true
Attachment #709011 - Attachment is obsolete: true
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.
Attachment #710710 - Flags: review+
Attached patch mc bits v.2 (obsolete) — Splinter Review
updated per comments.
Attached patch elm bits (obsolete) — Splinter Review
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?
(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.
Attachment #710727 - Attachment is obsolete: true
hrm, something broke with packaging app dirs. Looking into it.
Attached file xpi
Actually, spoke too soon, I didn't have the patches applied on my l10n repo. Attached is the resulting x-testing xpi, looks good.
Attached patch mc bits (obsolete) — Splinter Review
Attachment #710710 - Attachment is obsolete: true
Attachment #710726 - Attachment is obsolete: true
Attached patch elm bits (obsolete) — Splinter Review
(In reply to Jim Mathies [:jimm] from comment #42)
> Created attachment 710931 [details] [diff] [review]
> mc bits

ugh, wrong patch. :/
Attached patch mc bitsSplinter Review
off the correct network drive this time.
Attachment #710931 - Attachment is obsolete: true
Attachment #710933 - Attachment is obsolete: true
(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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.