There's logic in our build system that in the case of a missing l10n file, takes source en-US file *as* localized file when building a language pack. In FTL we don't want to do this because we want to fallback on the next locale, rather than localize into the locale with en-US strings.

I asked Pike for the feedback on the patch and he suggested a few changes, but referred to you for the review: > Pike> looking at https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/jar.py#343, I'd go for len() > 1 and strip the last one. but that's just on the logic, not about whether it's the right thing to do or the right place to do it. I'll defer to the build folks for that I added a new test and verified that removing the if/else from jar.mn makes it fail.

Comment on attachment 8920622 [details] Bug 1410451 - Do not merge en-US file for missing .ftl file in JarMaker. https://reviewboard.mozilla.org/r/191616/#review197278 ::: python/mozbuild/mozbuild/jar.py:408 (Diff revision 2) > + # 'en-US' fallbacking. > + # > + # To achieve that, we're testing if we have more than one localedir, > + # and if that's the case, we're removing the last fallback, which is en-US. > + if e.source.endswith('.ftl') and len(self.localedirs) > 1: > + src_base = self.localedirs[:-1] I think this is correct if generateLocaleDirs is used, but might not be if we go through this block to set localedirs directly: https://dxr.mozilla.org/mozilla-central/rev/69e24c678a28dc46a4c1bda3ff04b2f6106ff71a/python/mozbuild/mozbuild/jar.py#321 I believe this happens when --l10n-src is used. In this case I think we might incorrectly prune the last locale for .ftl files. If I'm misunderstanding, please correct me!

> I believe this happens when --l10n-src is used. In this case I think we might incorrectly prune the last locale for .ftl files. If I'm misunderstanding, please correct me! If I understand correctly, all the line 321 does is it normalizes the path. The only place where localedirs is set is here: https://dxr.mozilla.org/mozilla-central/rev/69e24c678a28dc46a4c1bda3ff04b2f6106ff71a/python/mozbuild/mozbuild/jar.py#343 and the possible matrix of outcomes is: merge + None = [merge, en-US] merge + base = [merge, base, en-US] None + base = [base] None + None = [en-US] So, if len>1 remove the last one seems to be matching, but I also liked my previous solution that explicitly removed the en-US only, not any last. Would you prefer me to switch to it?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5) > > I believe this happens when --l10n-src is used. In this case I think we might incorrectly prune the last locale for .ftl files. If I'm misunderstanding, please correct me! > > If I understand correctly, all the line 321 does is it normalizes the path. I think it is normalizing the path that comes from options.l10n_src: https://dxr.mozilla.org/mozilla-central/rev/ce1a86d3b4db161c95d1147676bbed839d7a4732/python/mozbuild/mozbuild/jar.py#580 So if you pass in directories via --l10n-src foo --l10n-src bar, then self.localedirs will be ['foo', 'bar'], which then get normalized. In this case I believe generateLocaleDirs is skipped, so you wouldn't necessarily end up with en-US as the last locale. > The only place where localedirs is set is here: > https://dxr.mozilla.org/mozilla-central/rev/ > 69e24c678a28dc46a4c1bda3ff04b2f6106ff71a/python/mozbuild/mozbuild/jar.py#343 > and the possible matrix of outcomes is: > > merge + None = [merge, en-US] > merge + base = [merge, base, en-US] > None + base = [base] > None + None = [en-US] > > So, if len>1 remove the last one seems to be matching, but I also liked my > previous solution that explicitly removed the en-US only, not any last. I think generateLocaleDirs can also generate a single localedir if l10nmerge and l10nbase are both unset - in that case all you get is a single en-US locale. So we probably need len>1 and the last one matches en-US to safely remove it, otherwise you'll end up with an empty set of localedirs for .ftl files. Note that I'm just basing this on jar.py - I can't find where --l10n-src is actually used, so it may not be relevant in practice.

That makes total sense! Updated the patch. :)

Comment on attachment 8920622 [details] Bug 1410451 - Do not merge en-US file for missing .ftl file in JarMaker. https://reviewboard.mozilla.org/r/191616/#review197388

Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ebe89bb81bc8 Do not merge en-US file for missing .ftl file in JarMaker. r=mshal