Closed
Bug 1147207
Opened 10 years ago
Closed 10 years ago
l10n repacks don't work with packaged addons
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox38 fixed, firefox39 fixed, firefox40 fixed)
RESOLVED
FIXED
mozilla40
People
(Reporter: Fallen, Assigned: glandium)
References
Details
Attachments
(5 files, 1 obsolete file)
5.09 KB,
patch
|
gps
:
review+
gps
:
feedback+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
4.57 KB,
patch
|
gps
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
7.57 KB,
patch
|
gps
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
5.08 KB,
patch
|
gps
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.32 KB,
patch
|
gps
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•10 years ago
|
||
(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 | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 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•10 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•10 years ago
|
||
This allows to use separate l10n staging directories for e.g. addons.
Attachment #8583616 -
Flags: review?(gps)
Comment 6•10 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•10 years ago
|
Attachment #8583614 -
Flags: review?(gps) → review+
Comment 7•10 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•10 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•10 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)
Reporter | ||
Comment 10•10 years ago
|
||
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•10 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 =.
Reporter | ||
Comment 12•10 years ago
|
||
Still getting the same error:
Error: Locale doesn't contain extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/
Assignee | ||
Comment 13•10 years ago
|
||
Ah, put it before --non-resource.
Updated•10 years ago
|
Attachment #8583613 -
Flags: review?(gps) → review+
Assignee | ||
Comment 14•10 years ago
|
||
With a docstring.
Attachment #8583616 -
Attachment is obsolete: true
Attachment #8585820 -
Flags: review?(gps)
Updated•10 years ago
|
Attachment #8585820 -
Flags: review?(gps) → review+
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
Backed out one of those patches for bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a9e8fc07a64
Assignee | ||
Comment 17•10 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•10 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•10 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•10 years ago
|
||
Gah forget it, repack with an addon does fail without it :(
Flags: needinfo?(philipp)
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Assignee | ||
Comment 21•10 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)
Comment 22•10 years ago
|
||
Updated•10 years ago
|
Attachment #8585902 -
Flags: review?(gps) → review+
Assignee | ||
Comment 23•10 years ago
|
||
Keywords: leave-open
Comment 24•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Reporter | ||
Comment 25•10 years ago
|
||
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•10 years ago
|
Attachment #8585902 -
Flags: approval-mozilla-beta?
Attachment #8585902 -
Flags: approval-mozilla-aurora?
Reporter | ||
Updated•10 years ago
|
Attachment #8583613 -
Flags: approval-mozilla-beta?
Attachment #8583613 -
Flags: approval-mozilla-aurora?
Reporter | ||
Updated•10 years ago
|
Attachment #8583615 -
Flags: approval-mozilla-beta?
Attachment #8583615 -
Flags: approval-mozilla-aurora?
Reporter | ||
Updated•10 years ago
|
Attachment #8585820 -
Flags: approval-mozilla-beta?
Attachment #8585820 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Comment 26•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8583613 -
Flags: approval-mozilla-beta?
Attachment #8583613 -
Flags: approval-mozilla-beta+
Attachment #8583613 -
Flags: approval-mozilla-aurora?
Attachment #8583613 -
Flags: approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8583614 -
Flags: approval-mozilla-beta?
Attachment #8583614 -
Flags: approval-mozilla-beta+
Attachment #8583614 -
Flags: approval-mozilla-aurora?
Attachment #8583614 -
Flags: approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8583615 -
Flags: approval-mozilla-beta?
Attachment #8583615 -
Flags: approval-mozilla-beta+
Attachment #8583615 -
Flags: approval-mozilla-aurora?
Attachment #8583615 -
Flags: approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8585820 -
Flags: approval-mozilla-beta?
Attachment #8585820 -
Flags: approval-mozilla-beta+
Attachment #8585820 -
Flags: approval-mozilla-aurora?
Attachment #8585820 -
Flags: approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8585902 -
Flags: approval-mozilla-beta?
Attachment #8585902 -
Flags: approval-mozilla-beta+
Attachment #8585902 -
Flags: approval-mozilla-aurora?
Attachment #8585902 -
Flags: approval-mozilla-aurora+
Comment 27•10 years ago
|
||
Comment 29•10 years ago
|
||
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
Assignee | ||
Comment 30•10 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)
Comment 31•10 years ago
|
||
Assignee | ||
Comment 32•10 years ago
|
||
My guess is you tried to uplift to beta before bug 910660 was uplifted. Also, bug 1147217 is needed.
Comment 33•10 years ago
|
||
Also needs bug 1147283.
Comment 34•10 years ago
|
||
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•