Closed
Bug 844068
Opened 12 years ago
Closed 12 years ago
Drop localizing metro
Categories
(Firefox Build System :: General, defect)
Tracking
(firefox21 fixed, firefox22+ fixed)
RESOLVED
FIXED
mozilla22
People
(Reporter: Pike, Assigned: Pike)
References
Details
Attachments
(1 file)
(deleted),
patch
|
mbrubeck
:
review+
glandium
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
We should remove the metro directory from the l10n setup.
It's clearly not anywhere close to be part of Firefox localization
Comment 1•12 years ago
|
||
I agree with that
Comment 2•12 years ago
|
||
Yes, I agree. We can re-enable it when we are ready to freeze strings and go to Aurora.
OS: Mac OS X → Windows 8 Metro
Assignee | ||
Comment 3•12 years ago
|
||
Matt, this might be the best option now. Are you a good person to review?
This change only disables metro in compare-locales, and the dashboard and stuff.
This one is needed for sure, I'm not sure what happens if we'd remove/comment out
@$(MAKE) -C ../metro/locales AB_CD=$* XPI_NAME=locale-$*
in browser/locales/Makefile.in. I expect things to break horribly there.
This patch is going to switch off l10n-merge for existing files in our localizations, though, which means that localized builds will break over time on metro. But I think that's OK. I can't come up with a low-risk solution here.
Given that metro is off on aurora anyway (?), we want this to land on aurora, too.
Assignee: nobody → l10n
Attachment #717163 -
Flags: review?(mbrubeck)
Comment 4•12 years ago
|
||
Comment on attachment 717163 [details] [diff] [review]
maybe good enough
Looks good to me. Asking gavin just to get a Firefox peer's eyes on this, or in case there's anyone else he things should check it.
And yes, if this works we should definitely land it on Aurora.
Attachment #717163 -
Flags: review?(mbrubeck)
Attachment #717163 -
Flags: review?(gavin.sharp)
Attachment #717163 -
Flags: review+
Comment 5•12 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #3)
> Created attachment 717163 [details] [diff] [review]
> maybe good enough
>
> Matt, this might be the best option now. Are you a good person to review?
>
> This change only disables metro in compare-locales, and the dashboard and
> stuff.
>
> This one is needed for sure, I'm not sure what happens if we'd
> remove/comment out
>
> @$(MAKE) -C ../metro/locales AB_CD=$* XPI_NAME=locale-$*
>
> in browser/locales/Makefile.in. I expect things to break horribly there.
You'd basically break metro on nightly (en-US) builds.
> This patch is going to switch off l10n-merge for existing files in our
> localizations, though, which means that localized builds will break over
> time on metro. But I think that's OK. I can't come up with a low-risk
> solution here.
Does changing l10n.ini actually impact the l10n merge happening when doing make -C metro/locales?
Comment 6•12 years ago
|
||
Comment on attachment 717163 [details] [diff] [review]
maybe good enough
I really have no opinion here - I don't think you need anyone but Axel to sign off on this :)
Attachment #717163 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #5)
> (In reply to Axel Hecht [:Pike] from comment #3)
> > Created attachment 717163 [details] [diff] [review]
> > maybe good enough
> >
> > Matt, this might be the best option now. Are you a good person to review?
> >
> > This change only disables metro in compare-locales, and the dashboard and
> > stuff.
> >
> > This one is needed for sure, I'm not sure what happens if we'd
> > remove/comment out
> >
> > @$(MAKE) -C ../metro/locales AB_CD=$* XPI_NAME=locale-$*
> >
> > in browser/locales/Makefile.in. I expect things to break horribly there.
>
> You'd basically break metro on nightly (en-US) builds.
Well, I've copied that out of the libs-% target, that shouldn't affect en-US.
For existing users, though, it'd drop doing l10n fallback on a per-file basis to en-US, and wouldn't register any chrome for metro.
Or....would it, with the new packager? Makes me wonder, can we abuse the packager to keep the en-US files and chrome reg lines in?
> > This patch is going to switch off l10n-merge for existing files in our
> > localizations, though, which means that localized builds will break over
> > time on metro. But I think that's OK. I can't come up with a low-risk
> > solution here.
>
> Does changing l10n.ini actually impact the l10n merge happening when doing
> make -C metro/locales?
Yes, as it's not creating the merged files for files that exist in locales, but have missing entries. ysod-worthy here, but it wouldn't break the build for sure.
Assignee | ||
Comment 8•12 years ago
|
||
Comment on attachment 717163 [details] [diff] [review]
maybe good enough
Asking Mike for a real review, though.
Attachment #717163 -
Flags: review?(mh+mozilla)
Comment 9•12 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #7)
> Or....would it, with the new packager? Makes me wonder, can we abuse the
> packager to keep the en-US files and chrome reg lines in?
The new packager removes all localized chrome before importing the new locale, but it could surely be modified to only drop the localized categories where we do have data from the new locale.
> > Does changing l10n.ini actually impact the l10n merge happening when doing
> > make -C metro/locales?
>
> Yes, as it's not creating the merged files for files that exist in locales,
> but have missing entries. ysod-worthy here, but it wouldn't break the build
> for sure.
So, in fact, wouldn't removing it from l10n.ini without removing it from libs-%:: break the build because of the missing files for JarMaker?
Assignee | ||
Comment 10•12 years ago
|
||
Missing files get picked up from en-US. That's an optimization I did in l10n-merge, 'mergedir', 'localedir', 'en-US'; so that I don't have to copy files around for nothing. mergedir is going to be empty for metro, though.
Re packager, I suggest we wait with that until we have real problems? "category" could also be chrome package? And then we're having desktop "browser", but not metro "browser", not sure if that's easy to digest.
If we really don't need *any* of the stuff that's coming through browser/locales/jar.mn, we can probably just switch it off in libs-%, and leave the browser package on metro as en-US only.
Comment 11•12 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #10)
> Missing files get picked up from en-US. That's an optimization I did in
> l10n-merge, 'mergedir', 'localedir', 'en-US'; so that I don't have to copy
> files around for nothing. mergedir is going to be empty for metro, though.
Ok, so while l10n.ini affects the content of the mergedir, it doesn't affect the rest of the process, so it will still pick missing files from en-US?
Assignee | ||
Comment 12•12 years ago
|
||
Yep. Caveat: it'll also pick files from l10n if the exist, so as our current localizations bitrot away, the localized builds will break. I'll reach out to the community to get their stuff moved away or thrown away, given that we can't really tell how appropriate their current work is going to be at the end.
Updated•12 years ago
|
Attachment #717163 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 13•12 years ago
|
||
Landed on central, https://hg.mozilla.org/mozilla-central/rev/10f28efcb0b7, as DONTBUILD as this only affects l10n builds, which don't get build for en-US check-ins anyway.
Status: NEW → RESOLVED
Closed: 12 years ago
tracking-firefox21:
--- → ?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 717163 [details] [diff] [review]
maybe good enough
[Approval Request Comment]
Bug caused by (feature/regressing bug #): landing metro with l10n strings, but the strings are ready for l10n yet
User impact if declined:
Testing completed (on m-c, etc.): landed on mozilla-central, is going to be in tomorrow's nightlies
Risk to taking this patch (and alternatives if risky): existing localizations bitrot on purpose
String or UUID changes made by this patch: only string removals in effect
Attachment #717163 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 15•12 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #14)
> Comment on attachment 717163 [details] [diff] [review]
> maybe good enough
>
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): landing metro with l10n strings,
> but the strings are ready for l10n yet
> User impact if declined:
> Testing completed (on m-c, etc.): landed on mozilla-central, is going to be
> in tomorrow's nightlies
> Risk to taking this patch (and alternatives if risky): existing
> localizations bitrot on purpose
> String or UUID changes made by this patch: only string removals in effect
Note that bug 841919 disabled metro for aurora, and this patch is actually a missing piece in doing so.
Comment 16•12 years ago
|
||
Approving for Aurora 21 and tracking for 22 in case this is needed again.
tracking-firefox21:
? → ---
tracking-firefox22:
--- → +
Updated•12 years ago
|
Attachment #717163 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 17•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/4fe41db7c3dd, landed on aurora.
status-firefox21:
--- → fixed
Assignee | ||
Comment 18•12 years ago
|
||
As we landed this on central, marking fixed22, too.
status-firefox22:
--- → fixed
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
Updated•6 years ago
|
Component: Build Config → General
Product: Firefox → Firefox Build System
Updated•6 years ago
|
Target Milestone: Firefox 22 → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•