Closed
Bug 1111607
Opened 10 years ago
Closed 10 years ago
l10n searchplugins should not have precedence over en-US files with the same name
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox35+ wontfix, firefox36+ wontfix)
RESOLVED
FIXED
mozilla37
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
Assignee | ||
Comment 1•10 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)
Comment 2•10 years ago
|
||
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•10 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.
Comment 4•10 years ago
|
||
(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•10 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•10 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•10 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 9•10 years ago
|
||
Attachment #8537835 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 10•10 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•10 years ago
|
||
... I'm making my first attempts at reviewboard here, let's see if I read the right pieces of the doc.
Updated•10 years ago
|
Attachment #8537835 -
Flags: review?(mh+mozilla) → review+
Comment 12•10 years ago
|
||
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•10 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
Comment 14•10 years ago
|
||
Keywords: checkin-needed
Comment 15•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment 16•10 years ago
|
||
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•10 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)
Comment 18•10 years ago
|
||
If it has no practical effect, I'd rather not take any unnecessary code churn on our RC candidate. Wontfix.
Flags: needinfo?(l10n)
Comment 19•10 years ago
|
||
Same as in comment #18
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8537835 -
Attachment is obsolete: true
Attachment #8618923 -
Flags: review+
Assignee | ||
Comment 21•9 years ago
|
||
Updated•6 years ago
|
Component: Build Config → General
Product: Firefox → Firefox Build System
Updated•6 years ago
|
Target Milestone: Firefox 37 → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•