1147207 - l10n repacks don't work with packaged addons

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

Running the following command (taken mostly from the build logs):

MOZ_MAKE_COMPLETE_MAR=1 COMM_REV=THUNDERBIRD_37_0b1_RELEASE MOZ_OBJDIR=objdir-l10n MOZILLA_REV=THUNDERBIRD_37_0b1_RELEASE MOZ_PKG_PRETTYNAMES=1 MOZ_PKG_VERSION=37.0b1 AB_CD=de LOCALE_MERGEDIR=merged ZIP_IN=~/mozilla/comm-beta/thunderbird-37.0b1.tar.bz2 make installers-tr

Results in this error:


/home/kewisch/mozilla/comm-beta/objdir-l10n/_virtualenv/bin/python /home/kewisch/mozilla/comm-beta/mozilla/toolkit/mozapps/installer/l10n-repack.py /home/kewisch/mozilla/comm-beta/objdir-l10n/dist/l10n-stage/thunderbird ../../dist/xpi-stage/locale-tr \
                --non-resource defaults/messenger/mailViews.dat
Traceback (most recent call last):
  File "/home/kewisch/mozilla/comm-beta/mozilla/toolkit/mozapps/installer/l10n-repack.py", line 48, in <module>
    main()
  File "/home/kewisch/mozilla/comm-beta/mozilla/toolkit/mozapps/installer/l10n-repack.py", line 44, in main
    l10n.repack(args.build, args.l10n, args.non_resource, NON_CHROME)
  File "/home/kewisch/mozilla/comm-beta/mozilla/python/mozbuild/mozpack/packager/l10n.py", line 172, in repack
    _repack(app_finder, l10n_finder, copier, formatter, non_chrome)
  File "/home/kewisch/mozilla/comm-beta/mozilla/python/mozbuild/mozpack/packager/l10n.py", line 83, in _repack
    for e in app.entries
  File "/home/kewisch/mozilla/comm-beta/mozilla/python/mozbuild/mozpack/packager/l10n.py", line 84, in <genexpr>
    if isinstance(e, ManifestEntryWithRelPath))
KeyError: 'extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}'
make[1]: *** [repackage-zip] Error 1
make[1]: Leaving directory `/home/kewisch/mozilla/comm-beta/objdir-l10n/mail/locales'
make: *** [repackage-zip-tr] Error 2


After some investigation, the first problem is in LocaleManifestFinder which is in python/mozbuild/mozpack/packager/l10n.py. It reads all manifest files, even those in the unpacked extension directory but fails to find the base in l10n_paths.

I tried to add in some code to exempt the addons from LocaleManifestFinder, but it seems they are not being copied over at all if I do that. Once the copier runs I get an error because all .xpt files are missing from l10n-stage. But then again I probably just did something wrong.

Given that the extension is localized separately, extensions should likely be exempted from the LocaleManifestFinder and the rest of the repack process.

I am not quite decided what the result should be, i.e. if it should copy over the en-US files or just ignore that directory completely and have the rest of the installers-tr target handle it. In any case, I will be hooking in the extension repack process after langpack-$(AB_CD) and before repackage-zip-$(AB_CD).

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Philipp Kewisch [:Fallen] from comment #0)
> I tried to add in some code to exempt the addons from LocaleManifestFinder,
> but it seems they are not being copied over at all if I do that. Once the
> copier runs I get an error because all .xpt files are missing from
> l10n-stage. But then again I probably just did something wrong.
Probably not important but I wanted to clear this up. They are actually being copied, but some files are missing. I've identified at least the .xpt and .manifest files. Probably all things the formatters special-case for.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Add a ComposedFinder class that acts like a FileFinder proxy over multiple FileFinders

Comment 3

[Mike Hommey [:glandium]]

Attachment: Use SimplePackager code to find manifest entries and base directories during l10n repack

This avoids duplicating the logic from SimplePackager to find base
directories, and fixes some cases where the l10n repack code wouldn't
find them properly.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Improve SimplePackager manifest consistency check

Bug 910660 added a consistency check that rejects cases where a manifest
inside a directory detected as being the base of an addon is included from
outside the addon. Unfortunately, this triggered false positives when
a manifest is included from within the addon, but just happens to be at
the top-level of that addon.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Allow to give extra l10n directories to l10n-repack.py

This allows to use separate l10n staging directories for e.g. addons.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8583613 [details] [diff] [review]
Add a ComposedFinder class that acts like a FileFinder proxy over multiple FileFinders

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

::: python/mozbuild/mozpack/files.py
@@ +904,5 @@
> +        self.files = FileRegistry()
> +
> +        for base, finder in sorted(finders.iteritems()):
> +            if self.files.contains(base):
> +                self.files.remove(base)

I understand why this is here. But I'm worried about unexpected surprises.

Without seeing consumers of this code, I question why we aren't passing in an ordered dict and relying on stacking and last-write-wins to perform composition.

::: python/mozbuild/mozpack/test/test_files.py
@@ +989,5 @@
> +
> +    def test_composed_finder(self):
> +        self.prepare_match_test()
> +        # Also add files in $tmp/a/foo/qux because ComposedFinder is
> +        # expected to mask foo/qux entirely with content from $tmp/b.

So you are relying on sorted order for this masking. I believe this reinforces my suspicion that the finders given to ComposedFinder need to be ordered.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8583615 [details] [diff] [review]
Improve SimplePackager manifest consistency check

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

::: python/mozbuild/mozpack/packager/__init__.py
@@ +309,5 @@
> +        broken_bases = sorted(
> +            m for m, includer in self._included_manifests.iteritems()
> +            if mozpath.basedir(m, bases) != mozpath.basedir(includer, bases))
> +        if broken_bases:
> +            for m in broken_bases:

You don't need the if here.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8583616 [details] [diff] [review]
Allow to give extra l10n directories to l10n-repack.py

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

Looks good. Please re-request final review once ComposedFinder is refactored.

::: python/mozbuild/mozpack/packager/l10n.py
@@ +177,5 @@
>          assert isinstance(copier[path], Jarrer)
>          copier[path].preload([l.replace(locale, l10n_locale) for l in log])
>  
>  
> +def repack(source, l10n, extra_l10n={}, non_resources=[], non_chrome=set()):

Adding a docstring here might be nice.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8583613 [details] [diff] [review]
Add a ComposedFinder class that acts like a FileFinder proxy over multiple FileFinders

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

(In reply to Gregory Szorc [:gps] from comment #6)
> So you are relying on sorted order for this masking. I believe this
> reinforces my suspicion that the finders given to ComposedFinder need to be
> ordered.

I don't understand your concern. They /are/ ordered by base path, through the sorted(finders.iteritems()) in the loop, such that '' (root) is before 'foo/qux', which would be before 'foo/qux/hoge' if there was one, and ensures the documented behavior.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Hey Mike, thanks for the patches. I'm having a little trouble running it. Could you elaborate on how it should be called? I tried this, triggered by adding the second line to l10n.mk through a variable:

python /path/to/l10n-repack.py dist/l10n-stage/thunderbird dist/xpi-stage/locale-tr \
  --non-resource defaults/messenger/mailViews.dat \
  extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/=../../dist/xpi-stage/lightning-tr

But am getting this error (twice):

Error: Locale doesn't contain extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/

I tried it on beta since thats where the locale was somewhere near working, but had some conflicts applying some of the patches. Either I am missing a patch, or there are differences. I have patches from this bug, bug 1147760, bug 1147217, bug 910660 applied on top of mozilla-beta (pre merge, in case you read this Monday/Tuesday).

I'll try this again on aurora/central, I just wanted to make sure I cam calling l10n-repack.py correctly.

Comment 11

[Mike Hommey [:glandium]]

(In reply to Philipp Kewisch [:Fallen] from comment #10)
> extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/=../../dist/xpi-stage/
> lightning-tr

Remove the / before =.

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

Still getting the same error: 

Error: Locale doesn't contain extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/

Comment 13

[Mike Hommey [:glandium]]

Ah, put it before --non-resource.

Comment 14

[Mike Hommey [:glandium]]

Attachment: Allow to give extra l10n directories to l10n-repack.py

With a docstring.

Comment 15

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3357891715c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/685aa4dfe753
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ad1c9b5c44d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e2723115b09

Comment 16

[Mike Hommey [:glandium]]

Backed out one of those patches for bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a9e8fc07a64

Comment 17

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #16)
> Backed out one of those patches for bustage:

(on l10n-check)

Comment 18

[Mike Hommey [:glandium]]

Strictly speaking, that changeset that I backed out was not necessary. Philipp, can you check things as they are now on inbound work?

Comment 19

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #18)
> Strictly speaking, that changeset that I backed out was not necessary.

That is, it was a nice cleanup, but it wasn't needed to make the rest work.

Comment 20

[Mike Hommey [:glandium]]

Gah forget it, repack with an addon does fail without it :(

Comment 21

[Mike Hommey [:glandium]]

Attachment: To squash with "Use SimplePackager code to find manifest entries and base directories during l10n repack"

Turns out the implementation before the "Use SimplePackager" patch didn't find the proper bases for language packs, because it didn't ignore the entries this patch ignores.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3357891715c4
https://hg.mozilla.org/mozilla-central/rev/5ad1c9b5c44d
https://hg.mozilla.org/mozilla-central/rev/9e2723115b09

Comment 23

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89e2807dbd5b

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89e2807dbd5b

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8583614 [details] [diff] [review]
Use SimplePackager code to find manifest entries and base directories during l10n repack

Approval Request Comment (for all 5 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 in two cycles, so I don't think that should be a reason to reject approval.
[String/UUID change made/needed]: none

Comment 26

[Sylvestre Ledru [:Sylvestre]]

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 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/095141488e12
https://hg.mozilla.org/releases/mozilla-aurora/rev/0f02755eabf5
https://hg.mozilla.org/releases/mozilla-aurora/rev/ad8dd51f2958
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2980295ee93

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Beta uplift.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Backed out from Aurora for test_packager_l10n.py failures.
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d0d39c0b228

https://treeherder.mozilla.org/logviewer.html#?job_id=744537&repo=mozilla-aurora

Comment 30

[Mike Hommey [:glandium]]

Subtle thing:
the backout of https://hg.mozilla.org/integration/mozilla-inbound/rev/685aa4dfe753 didn't remove the first hunk, so the second landing doesn't have it.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c9adbc5e73a9
https://hg.mozilla.org/releases/mozilla-aurora/rev/9017d8a54d82
https://hg.mozilla.org/releases/mozilla-aurora/rev/c8eeeed3e66c
https://hg.mozilla.org/releases/mozilla-aurora/rev/caadbc2f51af

You're still on the hook for Beta :)

Comment 32

[Mike Hommey [:glandium]]

My guess is you tried to uplift to beta before bug 910660 was uplifted. Also, bug 1147217 is needed.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Also needs bug 1147283.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fc1e894eec2f
https://hg.mozilla.org/releases/mozilla-beta/rev/46262c24ca5b
https://hg.mozilla.org/releases/mozilla-beta/rev/7f2d41560360
https://hg.mozilla.org/releases/mozilla-beta/rev/0c29ab096b90