Closed
Bug 981664
Opened 10 years ago
Closed 10 years ago
Mulet cannot pass l10n-check build step
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla32
People
(Reporter: gerard-majax, Assigned: gerard-majax)
References
Details
Attachments
(1 file, 2 obsolete files)
1.50 KB,
patch
|
gerard-majax
:
review+
|
Details | Diff | Splinter Review |
The l10n-check build step fails.
Comment 1•10 years ago
|
||
Can you give more information here?
Assignee | ||
Comment 2•10 years ago
|
||
Sure, look at https://tbpl.mozilla.org/php/getParsedLog.php?id=35895762&tree=Fig#error0 Traceback (most recent call last): File "/builds/slave/fig-lx-00000000000000000000000/build/toolkit/mozapps/installer/l10n-repack.py", line 48, in <module> main() File "/builds/slave/fig-lx-00000000000000000000000/build/toolkit/mozapps/installer/l10n-repack.py", line 44, in main l10n.repack(args.build, args.l10n, args.non_resource, NON_CHROME) File "/builds/slave/fig-lx-00000000000000000000000/build/python/mozbuild/mozpack/packager/l10n.py", line 172, in repack _repack(app_finder, l10n_finder, copier, formatter, non_chrome) File "/builds/slave/fig-lx-00000000000000000000000/build/python/mozbuild/mozpack/packager/l10n.py", line 83, in _repack for e in app.entries File "/builds/slave/fig-lx-00000000000000000000000/build/python/mozbuild/mozpack/packager/l10n.py", line 84, in <genexpr> if isinstance(e, ManifestEntryWithRelPath)) KeyError: 'b2g-l10n' make[3]: *** [repackage-zip] Error 1 make[3]: Leaving directory `/builds/slave/fig-lx-00000000000000000000000/build/obj-firefox/browser/locales' make[2]: *** [repackage-zip-x-test] Error 2 make[2]: Leaving directory `/builds/slave/fig-lx-00000000000000000000000/build/obj-firefox/browser/locales' make[1]: *** [l10n-check] Error 2 make[1]: Leaving directory `/builds/slave/fig-lx-00000000000000000000000/build/obj-firefox/browser/locales' make: *** [l10n-check] Error 2
Comment 3•10 years ago
|
||
Axel, any idea who can help here?
Comment 4•10 years ago
|
||
Mike, do you have a clue? This is deep in a stacktrace of code that I don't really know.
Comment 6•10 years ago
|
||
I'm still not sure what the code in l10n.py actually does, but conceptually, Mulet probably shouldn't do the sparse toolkit packaging, and we should ifdef the b2g-l10n stuff in b2g/locales/jar.mn, https://hg.mozilla.org/projects/fig/file/0bbef940cc77/b2g/locales/jar.mn#l16 That would match the ifdef in b2g.js, too.
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Axel Hecht [:Pike][pto til 14th] from comment #6) > I'm still not sure what the code in l10n.py actually does, but conceptually, > Mulet probably shouldn't do the sparse toolkit packaging, and we should > ifdef the b2g-l10n stuff in b2g/locales/jar.mn, > https://hg.mozilla.org/projects/fig/file/0bbef940cc77/b2g/locales/jar.mn#l16 > > That would match the ifdef in b2g.js, too. That makes sense now that you explain it. I'm going to try this !
Assignee | ||
Comment 8•10 years ago
|
||
Pike, I did (I hope) what you suggested, but sadly it does not help.
Attachment #8392275 -
Flags: feedback?(l10n)
Comment 9•10 years ago
|
||
Comment on attachment 8392275 [details]
Tentative fix for l10n-check
The endif should be at the very end beneath the overrides for dom.
But that won't remove the b2g-l10n package, but maybe that's not bad.
It'd really depend on what the l10n.py code is actually doing, and I didn't grasp that.
Attachment #8392275 -
Flags: feedback?(l10n) → feedback-
Comment 10•10 years ago
|
||
The following patch should make the error less cryptic: diff --git a/python/mozbuild/mozpack/packager/l10n.py b/python/mozbuild/mozpack/packager/l10n.py --- a/python/mozbuild/mozpack/packager/l10n.py +++ b/python/mozbuild/mozpack/packager/l10n.py @@ -73,20 +73,26 @@ def _repack(app_finder, l10n_finder, cop for e in l10n.entries: if isinstance(e, ManifestChrome): base = mozpack.path.basedir(e.path, app.bases) l10n_paths.setdefault(base, {}) l10n_paths[base][e.name] = e.path # For chrome and non chrome files or directories, store what langpack path # corresponds to a package path. - paths = dict((e.path, - l10n_paths[mozpack.path.basedir(e.path, app.bases)][e.name]) - for e in app.entries - if isinstance(e, ManifestEntryWithRelPath)) + paths = {} + for e in app.entries: + if instance(e, ManifestEntryWithRelPath): + base = mozpack.path.basedir(e.path, app.bases) + if base not in l10n_paths: + errors.fatal("Locale doesn't contain %s/" % base) + if e.name not in l10n_paths[base]: + errors.fatal("Locale doesn't have a manifest entry for '%s'" % + e.name) + path[e.path] = l10n_paths[base][e.name] for pattern in non_chrome: for base in app.bases: path = mozpack.path.join(base, pattern) left = set(p for p, f in app_finder.find(path)) right = set(p for p, f in l10n_finder.find(path)) for p in right: paths[p] = p But essentially this means that staging a locale doesn't produce a locale similar to what building the tree does.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 11•10 years ago
|
||
I did a local test of removing locales from mulet and moving pref overriding to b2g-l10n.js, so far: - mulet boots well - build l10n-check passes
Assignee | ||
Comment 12•10 years ago
|
||
Please find attached a patch that moves the prefs to b2g/locales/en-US/b2g-l10n.js and thus fixes the issue. It fixes correctly locally, and I could not see any regression. There is a Try build pending: https://tbpl.mozilla.org/?tree=Try&rev=5150dd4cc382
Assignee: nobody → lissyx+mozillians
Attachment #8392275 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8403983 -
Flags: review?(l10n)
Comment 13•10 years ago
|
||
Comment on attachment 8403983 [details] [diff] [review] 0001-Bug-981664-Move-B2G-l10n-override-to-b2g-l10n.js.patch Review of attachment 8403983 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I don't feel comfortable to review this. gps?
Attachment #8403983 -
Flags: review?(l10n) → review?(gps)
Comment 15•10 years ago
|
||
Comment on attachment 8403983 [details] [diff] [review] 0001-Bug-981664-Move-B2G-l10n-override-to-b2g-l10n.js.patch Forwarding to mshal
Attachment #8403983 -
Flags: review?(gps) → review?(mshal)
Comment 16•10 years ago
|
||
Comment on attachment 8403983 [details] [diff] [review] 0001-Bug-981664-Move-B2G-l10n-override-to-b2g-l10n.js.patch Hate to keep bouncing this around, but I am only a Build Config peer; I don't think an r+ from me would count.
Attachment #8403983 -
Flags: review?(mshal)
Comment 17•10 years ago
|
||
Comment on attachment 8403983 [details] [diff] [review] 0001-Bug-981664-Move-B2G-l10n-override-to-b2g-l10n.js.patch The first one to stamp r+ on this patch wins the right to be a proud contributor of the Mulet! So what is the story of l10n on b2g? I thought that there was no strings to localize from gecko. All strings should be coming from gaia, right? Given that nor l10n or build system experts actually seem to know what we are really doing here, I'm starting to wonder if b2g/locales folder is any useful? If at least we can know which strings have to be translated, we can then verify that this patch still translate them.
Attachment #8403983 -
Flags: review?(l10n)
Attachment #8403983 -
Flags: review?(fabrice)
Attachment #8403983 -
Flags: review?(21)
Comment 18•10 years ago
|
||
bug 1000631 is an example where gecko strings show. I didn't see form validation kick in, but if we'd enable that on b2g, that'd probably come from gecko. No idea what our story is for a11y of web content, that might also come from gecko? But yes, in general, we lack a profound understanding of which strings are coming from gecko, and actually should, to some extent.
Attachment #8403983 -
Flags: review?(21) → review+
Updated•10 years ago
|
Attachment #8403983 -
Flags: review?(fabrice) → review+
Comment 19•10 years ago
|
||
Comment on attachment 8403983 [details] [diff] [review] 0001-Bug-981664-Move-B2G-l10n-override-to-b2g-l10n.js.patch Thanks for the brave developers who r+ this patch! The mulet told me he is grateful to you. Alex, Could you update the patch with r=fabrice,vingtetun and mark it for checkin?
Attachment #8403983 -
Flags: review?(l10n)
Comment 20•10 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #18) > bug 1000631 is an example where gecko strings show. > > But yes, in general, we lack a profound understanding of which strings are > coming from gecko, and actually should, to some extent. My understanding is that all strings should come from gaia. If one is pulled from gecko, it sounds like a bug. In the past we had network connection page that used to come from gecko, but that was something we had to fixed in order to be handled by gaia. I imagine we would do the same if we discover another UI being defined in Gecko. I recently learned a new expression: cargo-culting. b2g/locales folder looks like an example to describe such expression.
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Alexandre Poirot (:ochameau) from comment #19) > Comment on attachment 8403983 [details] [diff] [review] > 0001-Bug-981664-Move-B2G-l10n-override-to-b2g-l10n.js.patch > > Thanks for the brave developers who r+ this patch! > The mulet told me he is grateful to you. > > Alex, Could you update the patch with r=fabrice,vingtetun and mark it for > checkin? We don't wait for l10n review?
Assignee | ||
Comment 22•10 years ago
|
||
r+ from fabrice and vivien. Patch rebased and ready for checkin.
Attachment #8403983 -
Attachment is obsolete: true
Attachment #8414311 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 23•10 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #21) > (In reply to Alexandre Poirot (:ochameau) from comment #19) > > Comment on attachment 8403983 [details] [diff] [review] > > 0001-Bug-981664-Move-B2G-l10n-override-to-b2g-l10n.js.patch > > > > Thanks for the brave developers who r+ this patch! > > The mulet told me he is grateful to you. > > > > Alex, Could you update the patch with r=fabrice,vingtetun and mark it for > > checkin? > > We don't wait for l10n review? According to comment 13 and comment 18, Pike isn't confident reviewing this patch. Vivien and fabrice were confident enough to approve it and they appear to own b2g/ folder, so we are good to go. Having said that, I'm not trying to avoid l10n review, discussion about b2g/locales story is still welcomed. This patch doesn't change much, but just highlights the fact that we maintain something noone really understand.
Comment 24•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6c2a901df6
Keywords: checkin-needed
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8b6c2a901df6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•