Closed Bug 1216371 Opened 4 years ago Closed 4 years ago

Package addons as XPIs

Categories

(Firefox Build System :: General, defect)

defect
Not set

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.
Blocks: 1186172
Blocks: 1223573
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.
And fix OmniJarFormatter.contains after bug 910660.
Attachment #8685906 - Flags: review?(gps)
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)
This is a followup to bug 1176703, to take in charge the default theme
packaged as an XPI.
Attachment #8685915 - Flags: review?(benjamin)
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)
Here we are.
Attachment #8685922 - Flags: review?(gps)
Attachment #8685916 - Attachment is obsolete: true
With some update to packager.py as well.
Attachment #8685911 - Attachment is obsolete: true
Attachment #8685911 - Flags: review?(gps)
Attachment #8685989 - Flags: review?(gps)
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)
This should be the one.
Attachment #8685922 - Attachment is obsolete: true
Attachment #8686065 - Flags: review?(gps)
Attachment #8685903 - Flags: review?(gps) → review+
Attachment #8685904 - Flags: review?(gps) → review+
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+
Attachment #8685906 - Flags: review?(gps) → review+
Attachment #8685908 - Flags: review?(gps) → review+
Attachment #8685910 - Flags: review?(gps) → review+
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 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+
Attachment #8686065 - Flags: review?(gps) → review+
(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.
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)
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)
Carrying over r+, this is the same patch, only with context changes.
Attachment #8685905 - Attachment is obsolete: true
Attachment #8686470 - Flags: review+
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+
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)
Attachment #8685903 - Attachment is obsolete: true
Carrying over r+. This is the same as previous part 2.
Attachment #8685904 - Attachment is obsolete: true
Attachment #8686476 - Flags: review+
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)
Carrying over r+. Exactly the same as previous iteration's part 6.
Attachment #8685910 - Attachment is obsolete: true
Attachment #8686478 - Flags: review+
Carrying over r+ assuming comment 20 works for you (() instead of [])
Attachment #8685989 - Attachment is obsolete: true
Attachment #8686479 - Flags: review+
Carrying over r+. No change form previous iteration's part 8.
Attachment #8686480 - Flags: review+
Attachment #8685912 - Attachment is obsolete: true
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)
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 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 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+
Attachment #8686468 - Flags: review?(gps) → review+
Attachment #8686474 - Flags: review?(gps) → review+
Attachment #8686477 - Flags: review?(gps) → review+
Attachment #8686482 - Flags: review?(gps) → review+
(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 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+
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)
(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)
Blocks: 1226822
(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)
(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)
Flags: needinfo?(philipp)
Flags: needinfo?(philipp)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.