Mulet cannot pass l10n-check build step

RESOLVED FIXED in mozilla32

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: gerard-majax, Assigned: gerard-majax)

Tracking

30 Branch
mozilla32
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

The l10n-check build step fails.
Can you give more information here?
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
Axel, any idea who can help here?
Mike, do you have a clue? This is deep in a stacktrace of code that I don't really know.
Can we get some help here please?
Flags: needinfo?(mh+mozilla)
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.
(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 !
Posted file Tentative fix for l10n-check (obsolete) —
Pike, I did (I hope) what you suggested, but sadly it does not help.
Attachment #8392275 - Flags: feedback?(l10n)
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-
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)
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
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 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)
Blocks: 961745
Duplicate of this bug: 962075
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 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 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)
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?(fabrice) → review+
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)
(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.
(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?
r+ from fabrice and vivien. Patch rebased and ready for checkin.
Attachment #8403983 - Attachment is obsolete: true
Attachment #8414311 - Flags: review+
Keywords: checkin-needed
(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.
https://hg.mozilla.org/mozilla-central/rev/8b6c2a901df6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.