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

RESOLVED FIXED in Firefox 37

Status

()

Firefox
Build Config
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: flod, Assigned: Pike)

Tracking

Trunk
Firefox 37
Points:
---

Firefox Tracking Flags

(firefox35+ wontfix, firefox36+ wontfix)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
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)
(Assignee)

Comment 3

3 years ago
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.
(Reporter)

Comment 5

3 years ago
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.
(Assignee)

Comment 6

3 years ago
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.
(Reporter)

Comment 7

3 years ago
(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.
(Assignee)

Comment 8

3 years ago
Taking, I'm working on a patch.
Assignee: nobody → l10n
(Assignee)

Comment 9

3 years ago
Created attachment 8537835 [details]
MozReview Request: bz://1111607/Pike
Attachment #8537835 - Flags: review?(mh+mozilla)
(Assignee)

Comment 10

3 years ago
/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
(Assignee)

Comment 11

3 years ago
... 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.
(Assignee)

Comment 13

3 years ago
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
Last Resolved: 3 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?
status-firefox35: --- → affected
status-firefox36: --- → affected
tracking-firefox35: --- → +
tracking-firefox36: --- → +
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
(Reporter)

Comment 17

3 years ago
(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.
status-firefox35: affected → wontfix
Flags: needinfo?(l10n)
Same as in comment #18
status-firefox36: affected → wontfix
(Assignee)

Comment 20

3 years ago
Comment on attachment 8537835 [details]
MozReview Request: bz://1111607/Pike
Attachment #8537835 - Attachment is obsolete: true
Attachment #8618923 - Flags: review+
(Assignee)

Comment 21

3 years ago
Created attachment 8618923 [details]
MozReview Request: bug 1111607, searchplugins should be picked up from en-US, if possible. r=glandium
You need to log in before you can comment on or make changes to this bug.