1166538 - Use mozbuild.jar-based zip tool instead of $(ZIP) for simple cases

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

I'll file separate bugs for the more involved cases.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Make dozip.py a py_action

Comment 2

[Mike Hommey [:glandium]]

Attachment: Make it more straightforward, path-wise, to change $(ZIP) uses to the zip py_action

Comment 3

[Mike Hommey [:glandium]]

Attachment: Use zip py_action in a few places

Comment 4

[(not currently active) Ted Mielczarek]

This is good stuff! I considered doing some of this with dozip.py when I first wrote it but I lacked the motivation. catlee was looking into this at some point to try to make the XPIs we generated for addons tests not be different for every build (because of timestamps), but he ran into some problem that I can't remember.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8607868 [details] [diff] [review]
Use zip py_action in a few places

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

LGTM modulo the question about file size on language packs.

::: config/rules.mk
@@ +1304,5 @@
>  			-exec $(STRIP) $(STRIP_FLAGS) {} >/dev/null 2>&1 \;
>  endif
>  endif
>  	@echo 'Packaging $(XPI_PKGNAME).xpi...'
> +	cd $(FINAL_TARGET) && $(call py_action,zip,../$(XPI_PKGNAME).xpi *)

I guess this is preferred to using -C and $(wildcard)? Not that it matters. I just thought it strange to see the `cd` here after you added -C.

::: toolkit/locales/l10n.mk
@@ -170,5 @@
>  	$(NSINSTALL) -D $(DIST)/$(PKG_LANGPACK_PATH)
>  	$(call py_action,preprocessor,$(DEFINES) $(ACDEFINES) \
>  	  -I$(TK_DEFINES) -I$(APP_DEFINES) $(srcdir)/generic/install.rdf -o $(DIST)/xpi-stage/$(XPI_NAME)/install.rdf)
> -	cd $(DIST)/xpi-stage/locale-$(AB_CD) && \
> -	  $(ZIP) -r9D $(LANGPACK_FILE) install.rdf $(PKG_ZIP_DIRS) chrome.manifest

Here and below we're dropping -9. I'm guessing new Python zip implementation uses the default of -6? This will increase file size, no? If so, I'm guessing some people will care about that. Although for language packs, maybe not so much.

::: toolkit/mozapps/installer/upload-files.mk
@@ -157,5 @@
>  PKG_SUFFIX	= .zip
>  INNER_MAKE_PACKAGE	= $(ZIP) -r9D $(PACKAGE) $(MOZ_PKG_DIR) \
>    -x \*/.mkdir.done
>  INNER_UNMAKE_PACKAGE	= $(UNZIP) $(UNPACKAGE)
> -MAKE_SDK = $(ZIP) -r9D $(SDK) $(MOZ_APP_NAME)-sdk

Ditto regarding -9. Although for the sdk, do we care?

Comment 6

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #5)
> Comment on attachment 8607868 [details] [diff] [review]
> Use zip py_action in a few places
> 
> Review of attachment 8607868 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM modulo the question about file size on language packs.
> 
> ::: config/rules.mk
> @@ +1304,5 @@
> >  			-exec $(STRIP) $(STRIP_FLAGS) {} >/dev/null 2>&1 \;
> >  endif
> >  endif
> >  	@echo 'Packaging $(XPI_PKGNAME).xpi...'
> > +	cd $(FINAL_TARGET) && $(call py_action,zip,../$(XPI_PKGNAME).xpi *)
> 
> I guess this is preferred to using -C and $(wildcard)? Not that it matters.
> I just thought it strange to see the `cd` here after you added -C.

My thinking was that the wildcard wouldn't work... but in fact, it should work if it is quoted. The finder in zip.py would happily do wildcard expansion instead of the shell.
 
> ::: toolkit/locales/l10n.mk
> @@ -170,5 @@
> >  	$(NSINSTALL) -D $(DIST)/$(PKG_LANGPACK_PATH)
> >  	$(call py_action,preprocessor,$(DEFINES) $(ACDEFINES) \
> >  	  -I$(TK_DEFINES) -I$(APP_DEFINES) $(srcdir)/generic/install.rdf -o $(DIST)/xpi-stage/$(XPI_NAME)/install.rdf)
> > -	cd $(DIST)/xpi-stage/locale-$(AB_CD) && \
> > -	  $(ZIP) -r9D $(LANGPACK_FILE) install.rdf $(PKG_ZIP_DIRS) chrome.manifest
> 
> Here and below we're dropping -9. I'm guessing new Python zip implementation
> uses the default of -6? This will increase file size, no? If so, I'm
> guessing some people will care about that. Although for language packs,
> maybe not so much.

The mozjar module always uses -9.
http://hg.mozilla.org/mozilla-central/file/74b944315521/python/mozbuild/mozpack/mozjar.py#l659

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a07ddeed713
https://hg.mozilla.org/integration/mozilla-inbound/rev/021552507526
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8b0864153e

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7a07ddeed713
https://hg.mozilla.org/mozilla-central/rev/021552507526
https://hg.mozilla.org/mozilla-central/rev/9d8b0864153e