l10n repacks don't work with packaged addons

RESOLVED FIXED in Firefox 38

Status

RESOLVED FIXED
3 years ago
6 months 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

3 years ago
Depends on: 1147217
(Reporter)

Updated

3 years ago
Blocks: 1130854
(Assignee)

Comment 2

3 years ago
Created attachment 8583613 [details] [diff] [review]
Add a ComposedFinder class that acts like a FileFinder proxy over multiple FileFinders
Assignee: nobody → mh+mozilla
Flags: needinfo?(mh+mozilla)
Attachment #8583613 - Flags: review?(gps)
(Assignee)

Comment 3

3 years ago
Created attachment 8583614 [details] [diff] [review]
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.
Attachment #8583614 - Flags: review?(gps)
(Assignee)

Comment 4

3 years ago
Created attachment 8583615 [details] [diff] [review]
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.
Attachment #8583615 - Flags: review?(gps)
(Assignee)

Comment 5

3 years ago
Created attachment 8583616 [details] [diff] [review]
Allow to give extra l10n directories to l10n-repack.py

This allows to use separate l10n staging directories for e.g. addons.
Attachment #8583616 - Flags: review?(gps)

Comment 6

3 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]:
-----------------------------------------------------------------

::: 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+

Updated

3 years ago
Attachment #8583614 - Flags: review?(gps) → review+

Comment 7

3 years ago
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 8

3 years ago
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

3 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

3 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

3 years ago
Ah, put it before --non-resource.

Updated

3 years ago
Attachment #8583613 - Flags: review?(gps) → review+
(Assignee)

Comment 14

3 years ago
Created attachment 8585820 [details] [diff] [review]
Allow to give extra l10n directories to l10n-repack.py

With a docstring.
Attachment #8583616 - Attachment is obsolete: true
Attachment #8585820 - Flags: review?(gps)

Updated

3 years ago
Attachment #8585820 - Flags: review?(gps) → review+
(Assignee)

Comment 16

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

Comment 17

3 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

3 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

3 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

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

Updated

3 years ago
Keywords: leave-open
(Assignee)

Comment 21

3 years ago
Created attachment 8585902 [details] [diff] [review]
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.
Attachment #8585902 - Flags: review?(gps)

Updated

3 years ago
Attachment #8585902 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/89e2807dbd5b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
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?
(Reporter)

Updated

3 years ago
Attachment #8585902 - Flags: approval-mozilla-beta?
Attachment #8585902 - Flags: approval-mozilla-aurora?
(Reporter)

Updated

3 years ago
Attachment #8583613 - Flags: approval-mozilla-beta?
Attachment #8583613 - Flags: approval-mozilla-aurora?
(Reporter)

Updated

3 years ago
Attachment #8583615 - Flags: approval-mozilla-beta?
Attachment #8583615 - Flags: approval-mozilla-aurora?
(Reporter)

Updated

3 years ago
Attachment #8585820 - Flags: approval-mozilla-beta?
Attachment #8585820 - Flags: approval-mozilla-aurora?
status-firefox38: --- → affected
status-firefox39: --- → affected
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

3 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

3 years ago
Depends on: 910660
(Assignee)

Comment 32

3 years ago
My guess is you tried to uplift to beta before bug 910660 was uplifted. Also, bug 1147217 is needed.
(Reporter)

Updated

3 years ago
Blocks: 1153192
(Reporter)

Updated

3 years ago
Blocks: 1153790

Updated

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