Closed Bug 910660 Opened 6 years ago Closed 5 years ago

Packager: Precompile of startup cache fails

Categories

(Firefox Build System :: General, defect, critical)

x86_64
Windows 7
defect
Not set
critical

Tracking

(firefox38 fixed, firefox39 fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: mark, Assigned: glandium)

References

Details

Attachments

(4 files, 8 obsolete files)

19.34 KB, patch
gps
: review+
Details | Diff | Splinter Review
5.18 KB, patch
gps
: review+
Details | Diff | Splinter Review
14.62 KB, patch
gps
: review+
Details | Diff | Splinter Review
9.12 KB, patch
gps
: review+
Details | Diff | Splinter Review
With the overhaul of the packager, trying to build v24 beta on Windows with added components in the package-manifest file I'm running into an issue with an error similar to bug 872439 where precompiling of the startup cache fails.

As far as I have been able to find out, the problem is that app_path is incorrect, and it sets "base" in packager.py not to the app base path, but instead to an arbitrary path from the package-manifest (in my case, I've added files under distribution/bundles, and I include them in package-manifest under [browser chrome files]) -- precompiling fails because this distribution/bundles/* path is what gets passed to precompile_cache with "-a" and not the actual app path to the browser.

Bug 872439 shows a similar error on Linux where the path passed in -a is not the app path but /browser under it, which would also be incorrect, and seems to draw the path from the same [chrome] files subsection?

Possibly this is a result of re-using variables too much and python not remembering the proper value as a result. In any case, packaging fails with a fatal error: "Error: Error while running startup cache precompilation".

Currently this is a blocker for me because I can't package my builds with omnijar - so, kind request alongside this bug: is there a quick workaround for this incorrect path, short of hard-coding it in the packager.py?
Blocks: new-packager
What is the file list under distribution/bundles?
The following additions:

@BINPATH@/distribution/bundles/statusbar@palemoon.org/chrome/*
@BINPATH@/distribution/bundles/statusbar@palemoon.org/components/status4evar.js
@BINPATH@/distribution/bundles/statusbar@palemoon.org/components/status4evar.xpt
@BINPATH@/distribution/bundles/statusbar@palemoon.org/defaults/*
@BINPATH@/distribution/bundles/statusbar@palemoon.org/install.rdf
@BINPATH@/distribution/bundles/statusbar@palemoon.org/chrome.manifest

I've had to split it up like this because of the .xpt in there

The resulting [chrome files] section in package-manifest.in is:

; [Browser Chrome Files]
@BINPATH@/browser/chrome.manifest
@BINPATH@/browser/chrome/browser@JAREXT@
@BINPATH@/browser/chrome/browser.manifest
@BINPATH@/browser/chrome/pdfjs.manifest
@BINPATH@/browser/chrome/pdfjs/*
@BINPATH@/browser/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/install.rdf
@BINPATH@/browser/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/icon.png
; Pale Moon status bar functionality
@BINPATH@/distribution/bundles/statusbar@palemoon.org/chrome/*
@BINPATH@/distribution/bundles/statusbar@palemoon.org/components/status4evar.js
@BINPATH@/distribution/bundles/statusbar@palemoon.org/components/status4evar.xpt
@BINPATH@/distribution/bundles/statusbar@palemoon.org/defaults/*
@BINPATH@/distribution/bundles/statusbar@palemoon.org/install.rdf
@BINPATH@/distribution/bundles/statusbar@palemoon.org/chrome.manifest
@BINPATH@/chrome/toolkit@JAREXT@
@BINPATH@/chrome/toolkit.manifest
@BINPATH@/chrome/recording.manifest
@BINPATH@/chrome/recording/*
#ifdef MOZ_GTK
@BINPATH@/browser/chrome/icons/default/default16.png
@BINPATH@/browser/chrome/icons/default/default32.png
@BINPATH@/browser/chrome/icons/default/default48.png
#endif

; shell icons
#ifdef XP_UNIX
#ifndef XP_MACOSX
; shell icons
@BINPATH@/browser/icons/*.png
#ifdef MOZ_UPDATER
; updater icon
@BINPATH@/icons/updater.png
#endif
#endif
#endif 


The app path passed to the cache precompile script becomes 
@BINPATH@/distribution/bundles/statusbar@palemoon.org/ instead of 
@BINPATH@
Looking at both bugs: does it pick the path of the last occurrence of "chrome.manifest" that gets included?
The packager considers any chrome.manifest not included from another one as the start of a new application. A workaround for you is to pack your bundle as a xpi.
(In reply to Mike Hommey [:glandium] from comment #4)
> A workaround for you is to pack your bundle
> as a xpi.

But distribution/bundles requires xpis to be unpacked, AFAIK, so that won't be possible... Unless that behavior has changed?
Indeed, it looks like they are supposed to be unpacked (which, btw, is suboptimal, but that'd be for another bug)

Another workaround for you is to just place the bundles after packaging.
(In reply to Mike Hommey [:glandium] from comment #6)
> Another workaround for you is to just place the bundles after packaging.

That would be a no-go as well, unless there is a way to build the installer with manually added bundles - which I don't think is possible since it will read the package manifest for that too.

Why is the startup cache pre-built and packaged anyway? Sounds to me like a lot of effort just so the first launch after install would be a bit faster...?
Couldn't I just skip precompiling the cache, instead, since it would get built by the browser anyway when run?
As a workaround, I'll be using jar format chrome packaging instead of omnijar, although with the current setup of the chrome that seems to be a little expensive in terms of file I/O. At least it doesn't seem to have this issue.
See Also: → 913165
I'm getting this issue too when attempting to package an extension in @RESPATH@/extensions/{extension-id}. I don't have the option of packaging it as an xpi since it needs to be unpacked due to binary components and the other workarounds also don't help in my situation. Therefore, I'd like to fix this issue instead.

I haven't quite looked at the best way to fix it, but maybe checking for /extensions/ somehow could help. The other thing is that the precompile cache shouldn't be filled for extension files, and the logging output places the extension files in resource://app/myExtensionFile.js which isn't right either.

Do you have any requirements on how it should be fixed?
Assignee: nobody → philipp
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(mh+mozilla)
An easy way around this is to make the packager look whether there's an install.rdf file in which case it can treat the directory as an extension.
Flags: needinfo?(mh+mozilla)
I'm having a bit trouble with packager.py, spend a few hours on it today. I thought it would just be a matter of checking for the install.rdf in the path and then excluding the whole subdir, but I can't seem to find the right place to check for that. 

I thought I could just do this in packager/__init__.py's add() function, but since the file finder finds both /path/to/ext/chrome.manifest and /path/to/ext/components/something.manifest and there is only an install.rdf in the base directory, I can't seem to figure out how to recursively exclude the extension directory from omni.ja but still package it using the flat formatter. Any hints you could give me?
Attached patch Fix - v1 (obsolete) — Splinter Review
How is this? Requesting feedback while the try run is going, I'll change it to review once everything is green.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f2bd92ff662

Once the patch is in good shape, I'd appreciate if we could get this one fixed and backported to aurora fairly soon. I need this to correctly package Lightning according to our plans for the Thunderbird 38 release. In the worst case I could check if there is a way we could use a copy of the packager with this patch applied, but I assume that will end up being quite messy.
Attachment #8571019 - Flags: feedback?(mh+mozilla)
Attached patch Fix - v2 (obsolete) — Splinter Review
Here is v1 with a python test included.
Attachment #8571019 - Attachment is obsolete: true
Attachment #8571019 - Flags: feedback?(mh+mozilla)
Attachment #8571020 - Flags: feedback?(mh+mozilla)
Unfortunately I am not quite there yet, its still doing the wrong thing w.r.t. l10n-repacks. I'd still appreciate feedback on the initial approach.
Comment on attachment 8571020 [details] [diff] [review]
Fix - v2

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

I think you only need one or two lines around here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.py#385
Attachment #8571020 - Flags: feedback?(mh+mozilla)
I'm not sure that will be enough, but admittedly there is more than one issue for me to fix:

* The files are being added to the precompile cache. This might be fixed with a few lines as you mentioned.
* Extensions are also being added to the omni.ja, which they should not be. 
* The packager is linking all .xpt files, even for extensions. This breaks Lightning because we have two xpt files where either the first or second is loaded dynamically, but both must not be loaded.

I don't quite recall what the l10n repack issue was since I haven't looked in a few days, but it was something about the packager treating the extension files as part of the main product and doing some optimizations that don't apply.

With this extra information, does my previous approach make more sense?
Flags: needinfo?(mh+mozilla)
Resetting needinfo because we discussed this on irc.
Flags: needinfo?(mh+mozilla)
Attached patch Fix - v3 (obsolete) — Splinter Review
Ok, this should do it. It still doesn't catch our special situation where we have a manifest in extdir/components/ that is not referenced from chrome.manifest, but I'm going to fix that using a simple but ugly hack in our code instead.

I decided to unwrap the array comprehension because it wasn't very readable. If you prefer, I can just add one more line to it instead.
Attachment #8571020 - Attachment is obsolete: true
Attachment #8576254 - Flags: review?(mh+mozilla)
Comment on attachment 8576254 [details] [diff] [review]
Fix - v3

Actually, this still fails :-/

The precompile cache is now fixed, but I've noticed that omni.ja files are created in the extension root(s), which contain the interfaces.xpt and chrome.manifest only.
Attachment #8576254 - Attachment is obsolete: true
Attachment #8576254 - Flags: review?(mh+mozilla)
Comment on attachment 8571020 [details] [diff] [review]
Fix - v2

I'm going to go back to suggesting Fix v2. As discussed on IRC, I added those 5 lines to just remove the extension directories from get_bases(). Again this made the precompile cache work, but it ended up renaming the chrome.manifest to {extensionid}.manifest and swallowing all unreferenced manifests in subdirectories.

The Fix v2 also doesn't add extension directories to what get_bases returns, and handles all extension subdirectories as flat directories that are not added to omni.ja.

Given you are planning to rewrite the packager to do the right thing with extensions anyway, I'd suggest to take this patch for now (given there are no major downsides of course).

I will handle any needed changes to l10n repacks in a different issue.
Attachment #8571020 - Attachment is obsolete: false
Attachment #8571020 - Flags: review?(mh+mozilla)
Comment on attachment 8571020 [details] [diff] [review]
Fix - v2

This really ought to be handled at the formatter level. Let me take it from here.
Attachment #8571020 - Flags: review?(mh+mozilla) → review-
Assignee: philipp → mh+mozilla
While I was around, I figured I'd do some cleanup.
Attachment #8571020 - Attachment is obsolete: true
Attachment #8577109 - Flags: review?(gps)
Addons are always formatted with jars for omnijar and jar packaging, and
flat for flat packaging.
Attachment #8577110 - Flags: review?(gps)
Attachment #8577110 - Attachment description: Make package formatters handle addons → part 2 - Make package formatters handle addons
Addons are detected by the presence of an install.rdf file in their base.
Attachment #8577111 - Flags: review?(gps)
Philipp, can you check this works for you?
Flags: needinfo?(philipp)
Comment on attachment 8577110 [details] [diff] [review]
part 2 - Make package formatters handle addons

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

Somehow this is broken on Windows only.
Attachment #8577110 - Flags: review?(gps)
It's kind of sad that make package runs before the make check that did catch the problem...
Attachment #8577110 - Attachment is obsolete: true
Attachment #8577152 - Flags: review?(gps)
That wasn't the right file.
Attachment #8577152 - Attachment is obsolete: true
Attachment #8577152 - Flags: review?(gps)
Attachment #8577153 - Flags: review?(gps)
Error: Addon base manifest (Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calBackendLoader.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calCompositeCalendar.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calDavCalendar.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calDefaultACLManager.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calICSCalendar.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calImportExportModule.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calItemModule.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calItipEmailTransport.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calItipProtocolHandler.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calMemoryCalendar.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calSleepMonitor.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calStorageCalendar.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calTimezoneService.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/calWcapCalendarModule.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/interfaces.manifest, Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/components/lightningTextCalendarConverter.manifest) is included in some other manifest
Traceback (most recent call last):
  File "/Users/kewisch/mozilla/comm-central/mozilla/toolkit/mozapps/installer/packager.py", line 399, in <module>
    main()
  File "/Users/kewisch/mozilla/comm-central/mozilla/toolkit/mozapps/installer/packager.py", line 352, in main
    copier.add(mozpack.path.join(respath, 'removed-files'), removals)
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py", line 24, in __exit__
    self.gen.next()
  File "/Users/kewisch/mozilla/comm-central/mozilla/python/mozbuild/mozpack/errors.py", line 129, in accumulate
    raise AccumulatedErrors()
mozpack.errors.AccumulatedErrors


These manifests are included from the main chrome.manifest.
Flags: needinfo?(philipp)
Comment on attachment 8577153 [details] [diff] [review]
part 2 - Make package formatters handle addons

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

Something else not covered by tests is broken.
Attachment #8577153 - Flags: review?(gps)
Comment on attachment 8577111 [details] [diff] [review]
part 3 - Make the SimplePackager emit a separate base for addons

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

Heh, this one is wrong too.
Attachment #8577111 - Flags: review?(gps)
Blocks: 706103
I've found a nice workaround for the above error and it seems to be working perfectly. Therefore please go ahead with the patches. Thanks again for taking care of this!
Attachment #8577798 - Attachment description: Refactor test_packager_formats.py so that it's easier to follow → part 1 - Refactor test_packager_formats.py so that it's easier to follow
Attachment #8577153 - Attachment is obsolete: true
Attachment #8577800 - Flags: review?(gps)
Attachment #8577111 - Attachment is obsolete: true
Attachment #8577801 - Flags: review?(gps)
Comment on attachment 8577798 [details] [diff] [review]
part 1 - Refactor test_packager_formats.py so that it's easier to follow

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

I wouldn't be surprised if something slipped through review - this change is very difficult to grok as a single change. I do like the new state of the test and it should make futures changes much easier to grok.
Attachment #8577798 - Flags: review?(gps) → review+
Comment on attachment 8577799 [details] [diff] [review]
part 2 - Add a test for the unpacker code

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

::: python/mozbuild/mozpack/test/test_packager_unpack.py
@@ +1,1 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public

This file will need to be added to python/moz.build. Yes, having to explicitly add new Python test files is inefficient.
Attachment #8577799 - Flags: review?(gps) → review+
Comment on attachment 8577800 [details] [diff] [review]
part 3 - Make package formatters handle addons

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

I'd like to hear your responses before granting r+.

::: python/mozbuild/mozpack/packager/formats.py
@@ +78,5 @@
>          # Only allow to add a base directory before calls to _get_base()
>          assert not self._frozen_bases
>          if not base in self._bases:
>              self._bases.append(base)
> +        if addon and not base in self._addons:

and base not in self._addons

Actually, I don't see anywhere where the ordering of self._addons is important. Can we make it a set?

@@ +222,3 @@
>          '''
>          base = self._get_base(path)
> +        use_omnijar = not base in self._addons

base not in self._addons

@@ +224,5 @@
> +        use_omnijar = not base in self._addons
> +        if use_omnijar:
> +            if is_resource is None:
> +                is_resource = self.is_resource(path, base)
> +            use_omnijar = is_resource

use_omnijar = self.is_resource(path, base) if not is_resource else False ?

@@ +236,5 @@
>          return self.omnijars[base], base, mozpack.path.relpath(path, base)
>  
>      def add(self, path, content):
> +        formatter, base, path = self._get_formatter(path)
> +        formatter.add(path, content)

With the rewritten _get_formatter(), we have code paths that return new objects rather than objects attached to this instance. If this API were used on a code path that returned a new object, I'm guessing the results would not be pleasant.

Should there be asserts sprinkled around to ensure we're always interfacing with an appropriate formatter instance?

@@ +241,4 @@
>  
>      def add_manifest(self, entry):
>          if isinstance(entry, ManifestBinaryComponent):
> +            formatter, base = super(OmniJarFormatter, self), ''

Similar concern as last.

@@ +265,5 @@
>          '''
>          Return whether the given path corresponds to a resource to be put in an
>          omnijar archive.
>          '''
> +        if base is None:

Why isn't "if not base" sufficient?
Attachment #8577800 - Flags: review?(gps) → feedback+
Comment on attachment 8577801 [details] [diff] [review]
part 4 - Make package formatters handle addons

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

::: python/mozbuild/mozpack/packager/__init__.py
@@ +303,5 @@
> +                               if mozpack.path.dirname(m) in self._addons)
> +        if broken_addons:
> +            errors.fatal(
> +                'Addon base manifest (%s) is included in some other manifest' %
> +                ', '.join(broken_addons)

Nice.

::: toolkit/mozapps/installer/packager.py
@@ +383,5 @@
>          else:
>              gre_path = None
>          for base in sorted([[p for p in [mozpack.path.join('bin', b), b]
>                              if os.path.exists(os.path.join(args.source, p))][0]
> +                           for b in sink.packager.get_bases(addons=False)]):

This is somewhat difficult to read. Feel free to "unroll" this.
Attachment #8577801 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #39)
> Comment on attachment 8577800 [details] [diff] [review]
> part 3 - Make package formatters handle addons
> 
> Review of attachment 8577800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to hear your responses before granting r+.
> 
> ::: python/mozbuild/mozpack/packager/formats.py
> @@ +78,5 @@
> >          # Only allow to add a base directory before calls to _get_base()
> >          assert not self._frozen_bases
> >          if not base in self._bases:
> >              self._bases.append(base)
> > +        if addon and not base in self._addons:
> 
> and base not in self._addons
> 
> Actually, I don't see anywhere where the ordering of self._addons is
> important. Can we make it a set?

Not that it matters much, but sets are an order of magnitude slower than lists for small number of elements.

> @@ +236,5 @@
> >          return self.omnijars[base], base, mozpack.path.relpath(path, base)
> >  
> >      def add(self, path, content):
> > +        formatter, base, path = self._get_formatter(path)
> > +        formatter.add(path, content)
> 
> With the rewritten _get_formatter(), we have code paths that return new
> objects rather than objects attached to this instance. If this API were used
> on a code path that returned a new object, I'm guessing the results would
> not be pleasant.
> 
> Should there be asserts sprinkled around to ensure we're always interfacing
> with an appropriate formatter instance?

I'm not following what your concern is. The possible values returned by _get_formatter are the same as the combination of what _get_omnijar did, with the other code path from this function, except we're now using super() instead of self itself, because we need a rebind not to call the same method again.

> @@ +265,5 @@
> >          '''
> >          Return whether the given path corresponds to a resource to be put in an
> >          omnijar archive.
> >          '''
> > +        if base is None:
> 
> Why isn't "if not base" sufficient?

Because '' is a valid value.
Attachment #8577800 - Flags: review?(gps)
Attachment #8577800 - Flags: review?(gps) → review+
Does this support building with unpacked extensions that contain .xpt files too?
(In reply to neil@parkwaycc.co.uk from comment #44)
> Does this support building with unpacked extensions that contain .xpt files
> too?

This is the case for Lightning. I'm still piecing things together to make it work, but with this and a few other bugs glandium fixed recently it will work with .xpt files, if you are ok with them being linked together as one.
Comment on attachment 8577798 [details] [diff] [review]
part 1 - Refactor test_packager_formats.py so that it's easier to follow

Approval Request Comment (for all 4 parts)
[Feature/regressing bug #]: Feature
[User impact if declined]: No way to package Lightning with Thunderbird using the build system. We'd like to package Lightning with Thunderbird 38.
[Describe test coverage new/current, TreeHerder]: Green builds on TH, this patch is already down to aurora.
[Risks and why]: Possible risks with release packaging? We'd find out eventually when this merges to beta next cycle, so I don't think that should be a reason to reject approval.
[String/UUID change made/needed]: none
Attachment #8577798 - Flags: approval-mozilla-beta?
Attachment #8577799 - Flags: approval-mozilla-beta?
Attachment #8577800 - Flags: approval-mozilla-beta?
Attachment #8577801 - Flags: approval-mozilla-beta?
Comment on attachment 8577798 [details] [diff] [review]
part 1 - Refactor test_packager_formats.py so that it's easier to follow

Should be low risk for Firefox, we want to help our thunderbird friends, especially when 38 is an ESR.
Should be in 38 beta 2 or 3.
Attachment #8577798 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8577799 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8577800 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8577801 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.