910660 - Packager: Precompile of startup cache fails

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mark Straver]

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?

Comment 1

[Mike Hommey [:glandium]]

What is the file list under distribution/bundles?

Comment 2

[Mark Straver]

The following additions:

@BINPATH@/distribution/bundles/[e-mail]/chrome/*
@BINPATH@/distribution/bundles/[e-mail]/components/status4evar.js
@BINPATH@/distribution/bundles/[e-mail]/components/status4evar.xpt
@BINPATH@/distribution/bundles/[e-mail]/defaults/*
@BINPATH@/distribution/bundles/[e-mail]/install.rdf
@BINPATH@/distribution/bundles/[e-mail]/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/[e-mail]/chrome/*
@BINPATH@/distribution/bundles/[e-mail]/components/status4evar.js
@BINPATH@/distribution/bundles/[e-mail]/components/status4evar.xpt
@BINPATH@/distribution/bundles/[e-mail]/defaults/*
@BINPATH@/distribution/bundles/[e-mail]/install.rdf
@BINPATH@/distribution/bundles/[e-mail]/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/[e-mail]/ instead of 
@BINPATH@

Comment 3

[Mark Straver]

Looking at both bugs: does it pick the path of the last occurrence of "chrome.manifest" that gets included?

Comment 4

[Mike Hommey [:glandium]]

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.

Comment 5

[Mark Straver]

(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?

Comment 6

[Mike Hommey [:glandium]]

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.

Comment 7

[Mark Straver]

(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?

Comment 8

[Mark Straver]

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.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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?

Comment 10

[Mike Hommey [:glandium]]

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.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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?

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

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.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

Here is v1 with a python test included.

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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 15

[Mike Hommey [:glandium]]

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

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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?

Comment 17

[Mike Hommey [:glandium]]

Resetting needinfo because we discussed this on irc.

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v3

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.

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 21

[Mike Hommey [:glandium]]

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.

Comment 22

[Mike Hommey [:glandium]]

Attachment: part 1 - Add an assertContents method to make test_packager_formats.py slightly more readable

While I was around, I figured I'd do some cleanup.

Comment 23

[Mike Hommey [:glandium]]

Attachment: part 2 - Make package formatters handle addons

Addons are always formatted with jars for omnijar and jar packaging, and
flat for flat packaging.

Comment 24

[Mike Hommey [:glandium]]

Attachment: part 3 - Make the SimplePackager emit a separate base for addons

Addons are detected by the presence of an install.rdf file in their base.

Comment 25

[Mike Hommey [:glandium]]

Philipp, can you check this works for you?

Comment 26

[Mike Hommey [:glandium]]

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.

Comment 27

[Mike Hommey [:glandium]]

Attachment: part 2 - Make package formatters handle addons

It's kind of sad that make package runs before the make check that did catch the problem...

Comment 28

[Mike Hommey [:glandium]]

Attachment: part 2 - Make package formatters handle addons

That wasn't the right file.

Comment 29

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 30

[Mike Hommey [:glandium]]

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.

Comment 31

[Mike Hommey [:glandium]]

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.

Comment 32

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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!

Comment 33

[Mike Hommey [:glandium]]

Attachment: part 1 - Refactor test_packager_formats.py so that it's easier to follow

Comment 34

[Mike Hommey [:glandium]]

Attachment: part 2 - Add a test for the unpacker code

Comment 35

[Mike Hommey [:glandium]]

Attachment: part 3 - Make package formatters handle addons

Comment 36

[Mike Hommey [:glandium]]

Attachment: part 4 - Make package formatters handle addons

Comment 37

[Gregory Szorc [: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.

Comment 38

[Gregory Szorc [:gps]]

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.

Comment 39

[Gregory Szorc [:gps]]

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?

Comment 40

[Gregory Szorc [:gps]]

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.

Comment 41

[Mike Hommey [:glandium]]

(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.

Comment 42

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3fe89223c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/31126d044d6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b73fca5142ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/9403517d849c

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8f3fe89223c4
https://hg.mozilla.org/mozilla-central/rev/31126d044d6d
https://hg.mozilla.org/mozilla-central/rev/b73fca5142ce
https://hg.mozilla.org/mozilla-central/rev/9403517d849c

Comment 44

[[e-mail]]

Does this support building with unpacked extensions that contain .xpt files too?

Comment 45

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to [e-mail] 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 46

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

Comment 47

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a141c675b405
https://hg.mozilla.org/releases/mozilla-beta/rev/06cd1b5deb25
https://hg.mozilla.org/releases/mozilla-beta/rev/90925932c88c
https://hg.mozilla.org/releases/mozilla-beta/rev/f4f4979e09b2