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)
Firefox Build System
General
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)
1.67 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
5.34 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
4.44 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
I'll file separate bugs for the more involved cases.
Assignee | ||
Updated•5 years ago
|
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
Assignee | ||
Comment 1•5 years ago
|
||
Attachment #8607866 -
Flags: review?(gps)
Assignee | ||
Comment 2•5 years ago
|
||
Attachment #8607867 -
Flags: review?(gps)
Assignee | ||
Comment 3•5 years ago
|
||
Attachment #8607868 -
Flags: review?(gps)
Comment 4•5 years ago
|
||
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.
Updated•5 years ago
|
Attachment #8607867 -
Flags: review?(gps) → review+
Updated•5 years ago
|
Attachment #8607866 -
Flags: review?(gps) → review+
Comment 5•5 years ago
|
||
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+
Assignee | ||
Comment 6•5 years ago
|
||
(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
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•5 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7a07ddeed713 https://hg.mozilla.org/mozilla-central/rev/021552507526 https://hg.mozilla.org/mozilla-central/rev/9d8b0864153e
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•2 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•