l10n repacks don't work with packaged addons

RESOLVED FIXED in Firefox 38

Status

defect
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: Fallen, Assigned: glandium)

Tracking

unspecified
mozilla40
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed, firefox40 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

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).
Flags: needinfo?(mh+mozilla)
(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.
Assignee

Updated

4 years ago
Depends on: 1147217
Blocks: 1130854
Assignee

Comment 2

4 years ago
Assignee: nobody → mh+mozilla
Flags: needinfo?(mh+mozilla)
Attachment #8583613 - Flags: review?(gps)
Assignee

Comment 3

4 years ago
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.
Attachment #8583614 - Flags: review?(gps)
Assignee

Comment 4

4 years ago
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.
Attachment #8583615 - Flags: review?(gps)
Assignee

Comment 5

4 years ago
This allows to use separate l10n staging directories for e.g. addons.
Attachment #8583616 - Flags: review?(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.
Attachment #8583613 - Flags: review?(gps) → feedback+
Attachment #8583614 - Flags: review?(gps) → review+
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.
Attachment #8583615 - Flags: review?(gps) → review+
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.
Attachment #8583616 - Flags: review?(gps) → feedback+
Assignee

Comment 9

4 years ago
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.
Attachment #8583613 - Flags: review?(gps)
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.
Assignee

Comment 11

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

Remove the / before =.
Still getting the same error: 

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

Comment 13

4 years ago
Ah, put it before --non-resource.
Attachment #8583613 - Flags: review?(gps) → review+
Assignee

Comment 14

4 years ago
With a docstring.
Attachment #8583616 - Attachment is obsolete: true
Attachment #8585820 - Flags: review?(gps)
Attachment #8585820 - Flags: review?(gps) → review+
Assignee

Comment 16

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

Comment 17

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

(on l10n-check)
Assignee

Comment 18

4 years ago
Strictly speaking, that changeset that I backed out was not necessary. Philipp, can you check things as they are now on inbound work?
Flags: needinfo?(philipp)
Assignee

Comment 19

4 years ago
(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.
Assignee

Comment 20

4 years ago
Gah forget it, repack with an addon does fail without it :(
Flags: needinfo?(philipp)
Assignee

Updated

4 years ago
Keywords: leave-open
Assignee

Comment 21

4 years ago
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.
Attachment #8585902 - Flags: review?(gps)
Attachment #8585902 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/89e2807dbd5b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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
Attachment #8583614 - Flags: approval-mozilla-beta?
Attachment #8583614 - Flags: approval-mozilla-aurora?
Attachment #8585902 - Flags: approval-mozilla-beta?
Attachment #8585902 - Flags: approval-mozilla-aurora?
Attachment #8583613 - Flags: approval-mozilla-beta?
Attachment #8583613 - Flags: approval-mozilla-aurora?
Attachment #8583615 - Flags: approval-mozilla-beta?
Attachment #8583615 - Flags: approval-mozilla-aurora?
Attachment #8585820 - Flags: approval-mozilla-beta?
Attachment #8585820 - Flags: approval-mozilla-aurora?
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 #8583613 - Flags: approval-mozilla-beta?
Attachment #8583613 - Flags: approval-mozilla-beta+
Attachment #8583613 - Flags: approval-mozilla-aurora?
Attachment #8583613 - Flags: approval-mozilla-aurora+
Attachment #8583614 - Flags: approval-mozilla-beta?
Attachment #8583614 - Flags: approval-mozilla-beta+
Attachment #8583614 - Flags: approval-mozilla-aurora?
Attachment #8583614 - Flags: approval-mozilla-aurora+
Attachment #8583615 - Flags: approval-mozilla-beta?
Attachment #8583615 - Flags: approval-mozilla-beta+
Attachment #8583615 - Flags: approval-mozilla-aurora?
Attachment #8583615 - Flags: approval-mozilla-aurora+
Attachment #8585820 - Flags: approval-mozilla-beta?
Attachment #8585820 - Flags: approval-mozilla-beta+
Attachment #8585820 - Flags: approval-mozilla-aurora?
Attachment #8585820 - Flags: approval-mozilla-aurora+
Attachment #8585902 - Flags: approval-mozilla-beta?
Attachment #8585902 - Flags: approval-mozilla-beta+
Attachment #8585902 - Flags: approval-mozilla-aurora?
Attachment #8585902 - Flags: approval-mozilla-aurora+
Needs rebasing for Beta uplift.
Flags: needinfo?(mh+mozilla)
Assignee

Comment 30

4 years ago
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.
Flags: needinfo?(mh+mozilla)
Assignee

Updated

4 years ago
Depends on: 910660
Assignee

Comment 32

4 years ago
My guess is you tried to uplift to beta before bug 910660 was uplifted. Also, bug 1147217 is needed.
Blocks: 1153192
Blocks: 1153790

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.