Closed Bug 1166538 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

I'll file separate bugs for the more involved cases.
Summary: Use mozpath.jar-based zip tool instead of $(ZIP) for simple cases → Use mozbuild.jar-based zip tool instead of $(ZIP) for simple cases
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.
Attachment #8607867 - Flags: review?(gps) → review+
Attachment #8607866 - Flags: review?(gps) → review+
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?
Attachment #8607868 - Flags: review?(gps) → review+
(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
Blocks: 1168525
Blocks: 1168532
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.