Closed
Bug 1216371
Opened 9 years ago
Closed 9 years ago
Package addons as XPIs
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox45 fixed)
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(12 files, 13 obsolete files)
15.80 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
4.08 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
6.13 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
4.51 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
9.90 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
7.85 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
9.57 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
4.99 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
9.52 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
12.51 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
1.95 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
8.68 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
Currently, when there is an addon detected by the packager code (through the existence of an install.rdf file), it is packaged with the JarFormatter, which means its chrome is jarized. This is fine for the current use cases, which are the default theme addon (only contains install.rdf and a chrome.manifest) and the lightning addon for thunderbird (which contains a binary component, so needs to stay "unpacked"). However, for system/feature addons, we would rather have them packaged as .XPIs.
Assignee | ||
Comment 1•9 years ago
|
||
I went full throttle on this and seriously refactored the package formatter code. While this makes for a big patch series, the result is totally worth it, as it makes the code simpler overall, made me find a bug that I introduced in bug 910660, and made the total amount of changes required to actually implement XPI packaging incredibly small.
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8685903 -
Flags: review?(gps)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8685904 -
Flags: review?(gps)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8685905 -
Flags: review?(gps)
Assignee | ||
Comment 5•9 years ago
|
||
And fix OmniJarFormatter.contains after bug 910660.
Attachment #8685906 -
Flags: review?(gps)
Assignee | ||
Comment 6•9 years ago
|
||
FlatFormatter, JarFormatter and OmniJarFormatter all, in some way, deal with different pieces of the package being handled differently. Instead of each of them dealing with their different pieces in some subtly different way, instead, introduce a new base package formatter class that will handle it for all of them. Use this new PiecemealFormatter for the FlatFormatter.
Attachment #8685908 -
Flags: review?(gps)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8685910 -
Flags: review?(gps)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8685911 -
Flags: review?(gps)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8685912 -
Flags: review?(gps)
Assignee | ||
Comment 10•9 years ago
|
||
This is a followup to bug 1176703, to take in charge the default theme packaged as an XPI.
Attachment #8685915 -
Flags: review?(benjamin)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8685916 -
Flags: review?(gps)
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8685916 [details] [diff] [review] part 10 - Pack addons that can be packed as XPIs This lacks the necessary changes to the unpacker to handle the xpi files. The 9 other parts are still good to review, though.
Attachment #8685916 -
Flags: review?(gps)
Assignee | ||
Updated•9 years ago
|
Attachment #8685916 -
Attachment is obsolete: true
Assignee | ||
Comment 14•9 years ago
|
||
With some update to packager.py as well.
Attachment #8685911 -
Attachment is obsolete: true
Attachment #8685911 -
Flags: review?(gps)
Attachment #8685989 -
Flags: review?(gps)
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8685922 [details] [diff] [review] part 10 - Pack addons that can be packed as XPIs The unpacking code is not complete and still breaks l10n. Maybe I should write a python test instead of failing during l10n-check after the build...
Attachment #8685922 -
Flags: review?(gps)
Assignee | ||
Comment 16•9 years ago
|
||
This should be the one.
Attachment #8685922 -
Attachment is obsolete: true
Attachment #8686065 -
Flags: review?(gps)
Updated•9 years ago
|
Attachment #8685903 -
Flags: review?(gps) → review+
Updated•9 years ago
|
Attachment #8685904 -
Flags: review?(gps) → review+
Comment 17•9 years ago
|
||
Comment on attachment 8685905 [details] [diff] [review] part 3 - Don't rely on OmniJarFormatter.is_resource for test_omnijar_is_resource Review of attachment 8685905 [details] [diff] [review]: ----------------------------------------------------------------- A commit message explaining the reasoning for this would have helped. I trust you are doing this for a good reason.
Attachment #8685905 -
Flags: review?(gps) → review+
Updated•9 years ago
|
Attachment #8685906 -
Flags: review?(gps) → review+
Updated•9 years ago
|
Attachment #8685908 -
Flags: review?(gps) → review+
Updated•9 years ago
|
Attachment #8685910 -
Flags: review?(gps) → review+
Comment 18•9 years ago
|
||
Comment on attachment 8685912 [details] [diff] [review] part 8 - Distinguish between addons that can be packed and those that cannot in the packager Review of attachment 8685912 [details] [diff] [review]: ----------------------------------------------------------------- It seems a bit weird to have values of True/False/'unpacked'. But Python doesn't have proper enums in 2.x, so meh.
Attachment #8685912 -
Flags: review?(gps) → review+
Comment 19•9 years ago
|
||
Comment on attachment 8685989 [details] [diff] [review] part 7 - Use the PiecemealFormatter for the OmniJarFormatter Review of attachment 8685989 [details] [diff] [review]: ----------------------------------------------------------------- The PiecemealFormatter is really starting to show its worth! ::: python/mozbuild/mozpack/packager/formats.py @@ +242,5 @@ > + def _add_base(self, base, addon=False): > + if addon: > + JarFormatter._add_base(self, base, addon) > + else: > + self._sub_formatter[base] = OmniJarSubFormatter( Do you think it is appropriate to assert base not in self._sub_formatter? @@ +254,5 @@ > + that dispatches between a FlatSubFormatter for the resources data and > + another FlatSubFormatter for the other files. > + ''' > + def __init__(self, copier, omnijar_name, compress=True, optimize=True, > + non_resources=[]): [] should not be used as default arguments because the value passed in the first time the function is called will get reused forever. Oh, Python. Best to default to None and do "non_resources = non_resources or []"
Attachment #8685989 -
Flags: review?(gps) → review+
Updated•9 years ago
|
Attachment #8686065 -
Flags: review?(gps) → review+
Assignee | ||
Comment 20•9 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #19) > Comment on attachment 8685989 [details] [diff] [review] > part 7 - Use the PiecemealFormatter for the OmniJarFormatter > > Review of attachment 8685989 [details] [diff] [review]: > ----------------------------------------------------------------- > > The PiecemealFormatter is really starting to show its worth! > > ::: python/mozbuild/mozpack/packager/formats.py > @@ +242,5 @@ > > + def _add_base(self, base, addon=False): > > + if addon: > > + JarFormatter._add_base(self, base, addon) > > + else: > > + self._sub_formatter[base] = OmniJarSubFormatter( > > Do you think it is appropriate to assert base not in self._sub_formatter? Fair enough. > @@ +254,5 @@ > > + that dispatches between a FlatSubFormatter for the resources data and > > + another FlatSubFormatter for the other files. > > + ''' > > + def __init__(self, copier, omnijar_name, compress=True, optimize=True, > > + non_resources=[]): > > [] should not be used as default arguments because the value passed in the > first time the function is called will get reused forever. Oh, Python. > > Best to default to None and do "non_resources = non_resources or []" How about defaulting to (), which has the only expected properly of []: it's iterable, but it's also immutable, so your concern would go away. '' would work too but would be awkward. Note the [] default is already there in the current code.
Assignee | ||
Comment 21•9 years ago
|
||
There is a lot of repetition across its various tests, and we're going to add some more in a subsequent change, so it is desirable to make it a less repetitive task.
Attachment #8686466 -
Flags: review?(gps)
Assignee | ||
Comment 22•9 years ago
|
||
Only directories containing chrome manifests are given as base to formatters, but there can still be files given outside the bases, like, on mac builds, all files in Content/MacOS, or Content/Info.plist, whereas chrome manifests are under Content/Resources.
Attachment #8686468 -
Flags: review?(gps)
Assignee | ||
Comment 23•9 years ago
|
||
Carrying over r+, this is the same patch, only with context changes.
Attachment #8685905 -
Attachment is obsolete: true
Attachment #8686470 -
Flags: review+
Assignee | ||
Comment 24•9 years ago
|
||
Carrying over r+. This is the same patch, with trivial adjustments for changes from (new) part 1 and 2.
Attachment #8685906 -
Attachment is obsolete: true
Attachment #8686472 -
Flags: review+
Assignee | ||
Comment 25•9 years ago
|
||
Roughly the same as the previous part 1, except it fixes a regression that is caught by the tests added in this iteration (and that build failures caught on try, but it's better to have actual tests)
Attachment #8686474 -
Flags: review?(gps)
Assignee | ||
Updated•9 years ago
|
Attachment #8685903 -
Attachment is obsolete: true
Assignee | ||
Comment 26•9 years ago
|
||
Carrying over r+. This is the same as previous part 2.
Attachment #8685904 -
Attachment is obsolete: true
Attachment #8686476 -
Flags: review+
Assignee | ||
Comment 27•9 years ago
|
||
With trivial changes to the PiecemealFormatter to fix a regression found on try and that the new tests expose. It also adds the assert you asked in comment 19.
Attachment #8685908 -
Attachment is obsolete: true
Attachment #8686477 -
Flags: review?(gps)
Assignee | ||
Comment 28•9 years ago
|
||
Carrying over r+. Exactly the same as previous iteration's part 6.
Attachment #8685910 -
Attachment is obsolete: true
Attachment #8686478 -
Flags: review+
Assignee | ||
Comment 29•9 years ago
|
||
Carrying over r+ assuming comment 20 works for you (() instead of [])
Attachment #8685989 -
Attachment is obsolete: true
Attachment #8686479 -
Flags: review+
Assignee | ||
Comment 30•9 years ago
|
||
Carrying over r+. No change form previous iteration's part 8.
Attachment #8686480 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8685912 -
Attachment is obsolete: true
Assignee | ||
Comment 31•9 years ago
|
||
Patch is unchanged, just its number in the series changed.
Attachment #8685915 -
Attachment is obsolete: true
Attachment #8685915 -
Flags: review?(benjamin)
Attachment #8686481 -
Flags: review?(benjamin)
Assignee | ||
Comment 32•9 years ago
|
||
Only the test changed to fit the changes to how the test works.
Attachment #8686065 -
Attachment is obsolete: true
Attachment #8686482 -
Flags: review?(gps)
Comment 33•9 years ago
|
||
Comment on attachment 8686481 [details] [diff] [review] part 11 - Load default theme from XPI in safe mode Do we need the !exists codepath? I don't want to do that mainthread/startup I/O if I don't have to.
Flags: needinfo?(mh+mozilla)
Comment 34•9 years ago
|
||
Comment on attachment 8686466 [details] [diff] [review] part 1 - Rearrange test_packager_formats.py Review of attachment 8686466 [details] [diff] [review]: ----------------------------------------------------------------- I didn't go through this line by line. But looks good enough.
Attachment #8686466 -
Flags: review?(gps) → review+
Updated•9 years ago
|
Attachment #8686468 -
Flags: review?(gps) → review+
Updated•9 years ago
|
Attachment #8686474 -
Flags: review?(gps) → review+
Updated•9 years ago
|
Attachment #8686477 -
Flags: review?(gps) → review+
Updated•9 years ago
|
Attachment #8686482 -
Flags: review?(gps) → review+
Assignee | ||
Comment 35•9 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #33) > Comment on attachment 8686481 [details] [diff] [review] > part 11 - Load default theme from XPI in safe mode > > Do we need the !exists codepath? I don't want to do that mainthread/startup > I/O if I don't have to. We do, if we care about safe mode working the same way on non packaged builds. Note this code path only happens on safe mode startup.
Flags: needinfo?(mh+mozilla)
Comment 36•9 years ago
|
||
Comment on attachment 8686481 [details] [diff] [review] part 11 - Load default theme from XPI in safe mode Yes ok, missed the "safe mode only" bit. Hopefully we can get rid of this with the rest of heavyweight themes later.
Attachment #8686481 -
Flags: review?(benjamin) → review+
Comment 37•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f07476dfe18b https://hg.mozilla.org/integration/mozilla-inbound/rev/3b4bdcc61b66 https://hg.mozilla.org/integration/mozilla-inbound/rev/6eceea1e4c17 https://hg.mozilla.org/integration/mozilla-inbound/rev/ccce34b98686 https://hg.mozilla.org/integration/mozilla-inbound/rev/65484dcda94e https://hg.mozilla.org/integration/mozilla-inbound/rev/9bdecf73e643 https://hg.mozilla.org/integration/mozilla-inbound/rev/9d4fa9c9f6ff https://hg.mozilla.org/integration/mozilla-inbound/rev/8361648d9079 https://hg.mozilla.org/integration/mozilla-inbound/rev/02c5693414ea https://hg.mozilla.org/integration/mozilla-inbound/rev/28d19f4f8724 https://hg.mozilla.org/integration/mozilla-inbound/rev/8ce4cdc77f88 https://hg.mozilla.org/integration/mozilla-inbound/rev/cad6d8407311
Comment 38•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f07476dfe18b https://hg.mozilla.org/mozilla-central/rev/3b4bdcc61b66 https://hg.mozilla.org/mozilla-central/rev/6eceea1e4c17 https://hg.mozilla.org/mozilla-central/rev/ccce34b98686 https://hg.mozilla.org/mozilla-central/rev/65484dcda94e https://hg.mozilla.org/mozilla-central/rev/9bdecf73e643 https://hg.mozilla.org/mozilla-central/rev/9d4fa9c9f6ff https://hg.mozilla.org/mozilla-central/rev/8361648d9079 https://hg.mozilla.org/mozilla-central/rev/02c5693414ea https://hg.mozilla.org/mozilla-central/rev/28d19f4f8724 https://hg.mozilla.org/mozilla-central/rev/8ce4cdc77f88 https://hg.mozilla.org/mozilla-central/rev/cad6d8407311
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 39•9 years ago
|
||
Might this have broken c-c OSX opt universal build packaging? It fits in the regression window. Traceback (most recent call last): File "/builds/slave/tb-c-cen-m64-00000000000000000/build/mozilla/toolkit/mozapps/installer/packager.py", line 406, in <module> main() File "/builds/slave/tb-c-cen-m64-00000000000000000/build/mozilla/toolkit/mozapps/installer/packager.py", line 402, in main copier.copy(args.destination) File "/builds/slave/tb-c-cen-m64-00000000000000000/build/mozilla/python/mozbuild/mozpack/copier.py", line 375, in copy if f.copy(destfile, skip_if_older): File "/builds/slave/tb-c-cen-m64-00000000000000000/build/mozilla/python/mozbuild/mozpack/copier.py", line 535, in copy file.copy(deflater, skip_if_older) File "/builds/slave/tb-c-cen-m64-00000000000000000/build/mozilla/python/mozbuild/mozpack/unify.py", line 75, in copy assert isinstance(dest, basestring) AssertionError make[3]: *** [stage-package] Error 1 If you have any pointers about what needs to be changed, that would be very helpful, thanks!
Flags: needinfo?(mh+mozilla)
Comment 40•9 years ago
|
||
(In reply to aleth [:aleth] from comment #39) > Might this have broken c-c OSX opt universal build packaging? It fits in the > regression window. > > Traceback (most recent call last): > File "/builds/slave/tb-c-cen-m64-00000000000000000/build/mozilla/toolkit/mozapps/ > installer/packager.py", line 406, in <module> main() ... > If you have any pointers about what needs to be changed, that would be very > helpful, thanks! More lines of scrollback would help
Flags: needinfo?(aleth)
Comment 41•9 years ago
|
||
(In reply to Philip Chee from comment #40) > More lines of scrollback would help https://treeherder.mozilla.org/logviewer.html#?job_id=26622&repo=comm-central
Flags: needinfo?(aleth)
Assignee | ||
Comment 42•9 years ago
|
||
(In reply to aleth [:aleth] from comment #41) > (In reply to Philip Chee from comment #40) > > More lines of scrollback would help > > https://treeherder.mozilla.org/logviewer.html#?job_id=26622&repo=comm-central This is likely the same root cause as bug 1226884. That is, it's probably failing because lightning contains binaries in a XPI, and unifying that fails.
Flags: needinfo?(mh+mozilla)
Updated•9 years ago
|
Flags: needinfo?(philipp)
Updated•9 years ago
|
Flags: needinfo?(philipp)
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•