Closed Bug 1111607 Opened 5 years ago Closed 5 years ago

l10n searchplugins should not have precedence over en-US files with the same name

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox35+ wontfix, firefox36+ wontfix)

RESOLVED FIXED
mozilla37
Tracking Status
firefox35 + wontfix
firefox36 + wontfix

People

(Reporter: flod, Assigned: Pike)

References

Details

Attachments

(1 file, 1 obsolete file)

Not sure if it's actually a regression from bug 1073212, also because these are my first tests with builds, so it could be just me doing things wrong.

This is my .mozconfig

mk_add_options MOZ_OBJDIR="$HOME/mozbuild/objdir-firefox"
ac_add_options --with-l10n-base=../l10n-central
ac_add_options --enable-ui-locale=it

I have a local modified clone of l10n-central/it: 
* changed from yahoo-it to yahoo in list.txt
* created a yahoo.xml file in /browser/searchplugins

The build system should ignore yahoo.xml in /it and use the original en-US one, but it doesn't seem to happen.

make echo-variable-SEARCHPLUGINS AB_CD=it LOCALE_MERGEDIR=$PWD/merge

/mozbuild/l10n-central/it/browser/searchplugins/amazon-it.xml
/mozbuild/src/browser/locales/en-US/searchplugins/bing.xml
/mozbuild/l10n-central/it/browser/searchplugins/eBay-it.xml
/mozbuild/src/browser/locales/en-US/searchplugins/google.xml
/mozbuild/l10n-central/it/browser/searchplugins/hoepli.xml
/mozbuild/l10n-central/it/browser/searchplugins/wikipedia-it.xml
/mozbuild/l10n-central/it/browser/searchplugins/yahoo.xml
/mozbuild/src/browser/locales/en-US/searchplugins/ddg.xml
Yes, this is a regression. In that bug, we started to use MERGE_FILE, which is doing the opposite order compared to what it used to be.

needinfo on glandium on how we want to fix this. I see basically two ways, back to hard-coding the order in browser/locales/Makefile.in, or adding a sibling macro for MERGE_FILE that does the opposite order.
Flags: needinfo?(mh+mozilla)
I don't get it. If you have a yahoo.xml file in it, why don't you prefer it over en-US's?
Flags: needinfo?(mh+mozilla)
Mostly because if list.txt says "google", you know what you get, independently of checking through yet more files.

There's also a bunch of historical cruft where localizers just changed stuff without review, and getting the list.txt/filename combo right proved to be more resistant against changes.

Most importantly though, we've regressed what we've used to do, which means that we have changed a bunch of searchplugins without intent.
(In reply to Axel Hecht [:Pike] from comment #1)
> needinfo on glandium on how we want to fix this. I see basically two ways,
> back to hard-coding the order in browser/locales/Makefile.in, or adding a
> sibling macro for MERGE_FILE that does the opposite order.

The latter.
Still trying to figure out how this works. If I duplicate MERGE_FILE as MERGE_SEARCHPLUGINS_FILE, and replace firstword with info, this is what I get for amazon-it.xml

~/mozbuild/l10n-central/it/browser/searchplugins/amazon-it.xml 
~/mozbuild/src/browser/locales/en-US/searchplugins/amazon-it.xml 

The second one doesn't make any sense, and it happens because the third line in MERGE_FILE is not wrapped in wildcard (should it be?)
http://mxr.mozilla.org/mozilla-central/source/config/config.mk#611

Wrapping the third line with wildcard, and using lastword seems to work for me.
This should work. I'd not give it a searchplugins-specific name.

I left out the merge dir, we could put it back in, just to be consistent. We don't merge searchplugins, though. Merge dir would be first again.

EN_US_OR_L10N_FILE = $(firstword \
   $(wildcard $(srcdir)/en-US/$(1)) \
   $(LOCALE_SRCDIR)/$(1) )

Concept: search path, all but te last get a $(wildcard ) around them.
(In reply to Axel Hecht [:Pike] from comment #6)
> This should work. I'd not give it a searchplugins-specific name.
Definitely gives the expected results locally for me.
Taking, I'm working on a patch.
Assignee: nobody → l10n
Attached file MozReview Request: bz://1111607/Pike (obsolete) —
Attachment #8537835 - Flags: review?(mh+mozilla)
/r/1543 - bug 1111607, searchplugins should be picked up from en-US, if possible. r=glandium

Pull down this commit:

hg pull review -r 9f3a59dc077b976083c6c8e0a385e7a23817c9f4
... I'm making my first attempts at reviewboard here, let's see if I read the right pieces of the doc.
Attachment #8537835 - Flags: review?(mh+mozilla) → review+
https://reviewboard.mozilla.org/r/1541/#review951

::: config/config.mk
(Diff revision 1)
> +# similar to MERGE_FILE, but no merging, and en-US first

Add a . at the end of sentences.
Updated the patch and pushed to reviewboard, rev c923d732e7ef is the updated patch (which reviewboard shows, too)

Ready to land.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d7bc5a42a08a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
No longer depends on: 1073212
We need to fix this in Firefox 35, right, given that bug 1073212 landed there?
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #16)
> We need to fix this in Firefox 35, right, given that bug 1073212 landed
> there?

The original patch landed in Firefox 35, but right now we don't have any locale in this condition across all branches (same file of en-US in their repos). Usually I file bugs and get them removed when it happens (e.g. bug 1107223).

So we could land this on beta and aurora for consistency in the build system, or let it ride the train since it doesn't have practical effects. To be honest I'm fine with any of them.
Flags: needinfo?(francesco.lodolo)
If it has no practical effect, I'd rather not take any unnecessary code churn on our RC candidate. Wontfix.
Flags: needinfo?(l10n)
Attachment #8537835 - Attachment is obsolete: true
Attachment #8618923 - Flags: review+
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 37 → mozilla37
You need to log in before you can comment on or make changes to this bug.