981664 - Mulet cannot pass l10n-check build step

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[:gerard-majax]

The l10n-check build step fails.

Comment 1

[Gregor Wagner [:gwagner]]

Can you give more information here?

Comment 2

[:gerard-majax]

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

[Gregor Wagner [:gwagner]]

Axel, any idea who can help here?

Comment 4

[Axel Hecht [:Pike]]

Mike, do you have a clue? This is deep in a stacktrace of code that I don't really know.

Comment 5

[Gregor Wagner [:gwagner]]

Can we get some help here please?

Comment 6

[Axel Hecht [:Pike]]

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.

Comment 7

[:gerard-majax]

(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 !

Comment 8

[:gerard-majax]

Attachment: Tentative fix for l10n-check

Pike, I did (I hope) what you suggested, but sadly it does not help.

Comment 9

[Axel Hecht [:Pike]]

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.

Comment 10

[Mike Hommey [:glandium]]

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.

Comment 11

[:gerard-majax]

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

Comment 12

[:gerard-majax]

Attachment: 0001-Bug-981664-Move-B2G-l10n-override-to-b2g-l10n.js.patch

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

Comment 13

[Axel Hecht [:Pike]]

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?

Comment 15

[Gregor Wagner [:gwagner]]

Comment on attachment 8403983 [details] [diff] [review]
0001-Bug-981664-Move-B2G-l10n-override-to-b2g-l10n.js.patch

Forwarding to mshal

Comment 16

[Michael Shal [: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.

Comment 17

[Alexandre Poirot [:ochameau]]

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.

Comment 18

[Axel Hecht [:Pike]]

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.

Comment 19

[Alexandre Poirot [:ochameau]]

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?

Comment 20

[Alexandre Poirot [:ochameau]]

(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.

Comment 21

[:gerard-majax]

(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?

Comment 22

[:gerard-majax]

Attachment: 0001-Bug-981664-Move-B2G-l10n-override-to-b2g-l10n.js-r-f_hg.patch

r+ from fabrice and vivien. Patch rebased and ready for checkin.

Comment 23

[Alexandre Poirot [:ochameau]]

(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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6c2a901df6

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b6c2a901df6