Closed Bug 844068 Opened 8 years ago Closed 8 years ago

Drop localizing metro

Categories

(Firefox Build System :: General, defect)

x86
Windows 8.1
defect
Not set
normal

Tracking

(firefox21 fixed, firefox22+ fixed)

RESOLVED FIXED
mozilla22
Tracking Status
firefox21 --- fixed
firefox22 + fixed

People

(Reporter: Pike, Assigned: Pike)

References

Details

Attachments

(1 file)

We should remove the metro directory from the l10n setup.

It's clearly not anywhere close to be part of Firefox localization
I agree with that
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
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 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+
(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 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)
(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.
Comment on attachment 717163 [details] [diff] [review]
maybe good enough

Asking Mike for a real review, though.
Attachment #717163 - Flags: review?(mh+mozilla)
(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?
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.
(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?
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.
Attachment #717163 - Flags: review?(mh+mozilla) → review+
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: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
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?
(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.
Approving for Aurora 21 and tracking for 22 in case this is needed again.
Attachment #717163 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
As we landed this on central, marking fixed22, too.
Depends on: 852236
OS: Windows 8 Metro → Windows 8.1
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 22 → mozilla22
You need to log in before you can comment on or make changes to this bug.