1239707 - Switch to use in-tree version of compare-locales (SeaMonkey Part)

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Philip Chee]

Callek had some concerns about this though in https://groups.google.com/forum/#!msg/mozilla.dev.builds/0koWKXx7arg/izvBHzy8nmwJ

Comment 1

[Philip Chee]

Attachment: WIP patch

I have no idea if this works.

> -	MACOSX_DEPLOYMENT_TARGET= compare-locales -m $(LOCALE_MERGEDIR) $(srcdir)/l10n.ini $(L10NBASEDIR) $*
> +	$(topsrcdir)/mozilla/mach compare-locales --merge-dir $(LOCALE_MERGEDIR) $*
I'm assuming topsrcdir is comm-central but after m-c/c-c merge this should change to $(topsrcdir)/mach

Comment 2

[Philip Chee]

Comment on attachment 8707921 [details] [diff] [review]
WIP patch

Cancelling review requests due to Bug 1223385 reopened.

Comment 3

[Philip Chee]

Comment on attachment 8707921 [details] [diff] [review]
WIP patch

Do we need this? Does it work?

Comment 4

[Ian Stakenvicius]

Attachment: WIP patch v2

Updated patch -- the default value for --l10n-ini doesn't look like it matches what seamonkey uses, so this argument should be retained.  The --l10n-base option however is identical so argument 3 can still be dropped.

I used this to restore 'make merge-$(AB_CD)' functionality when rolling langpacks for suite out of the 45.x ESR releases.  I don't know how much things have changed on c-c / mozilla-52 but I expect this patch will be needed regardless.

Comment 5

[Philip Chee]

Comment on attachment 8787337 [details] [diff] [review]
WIP patch v2

Redirect reviews to ewong (buildconfig) and Adrian (L10n)

Comment 6

[Adrian Kalla [:adriank]]

Comment on attachment 8787337 [details] [diff] [review]
WIP patch v2

Review of attachment 8787337 [details] [diff] [review]:
-----------------------------------------------------------------

While this patch matches what upstream did, I don't like dropping the "--l10n-base" option as afaics the code here https://hg.mozilla.org/mozilla-central/file/tip/python/compare-locales/mach_commands.py#l57 will assume, that the L10NBASEDIR is a relative path to topsrcdir. If I am wrong and an L10NBASEDIR will accept and work with absolute paths that have nothing in common with topsrcdir you can ignore the comment and assume r+. Otherwise it's r-.

Also: I don't think we absolutely need this patch, as without it the locally installed compare-locales will be used, so it should still work (as long as compare-locales is installed on the build machine).

Comment 7

[Ian Stakenvicius]

So FYI, I checked the compare-locales call in both mail/locales/Makefile.in and browser/locales/Makefile.in, and neither of those have changed either.  I am going to take a guess that 'make merge-$(AB_CD) LOCALE_MERGEDIR=./merge-$(AB_CD)' is simply not used anymore as part of the overall langpack generation procedure, despite the guide saying otherwise.

Comment 8

[Adrian Kalla [:adriank]]

(In reply to Ian Stakenvicius from comment #7)
> So FYI, I checked the compare-locales call in both mail/locales/Makefile.in
> and browser/locales/Makefile.in, and neither of those have changed either. 

What do you mean by "neither of those have changed either"???

> I am going to take a guess that 'make merge-$(AB_CD)
> LOCALE_MERGEDIR=./merge-$(AB_CD)' is simply not used anymore as part of the
> overall langpack generation procedure, despite the guide saying otherwise.


I'd doubt that. A merge must happen - at least somewhere - otherwise you get a broken build...

Comment 9

[Edmund Wong (:ewong)]

Comment on attachment 8787337 [details] [diff] [review]
WIP patch v2

Review of attachment 8787337 [details] [diff] [review]:
-----------------------------------------------------------------

::: suite/locales/Makefile.in
@@ +217,5 @@
>  
>  merge-%:
>  ifdef LOCALE_MERGEDIR
>  	$(RM) -rf $(LOCALE_MERGEDIR)
> +	$(topsrcdir)/mozilla/mach compare-locales --merge-dir $(LOCALE_MERGEDIR) --l10n-ini $(srcdir)/l10n.ini $*

upstream uses mach in its builds. We don't.  So this
could just break automation.  I'll need to try it out
in automation.

Comment 10

[Edmund Wong (:ewong)]

(In reply to Edmund Wong (:ewong) from comment #9)
> Comment on attachment 8787337 [details] [diff] [review]
> WIP patch v2
> 
> Review of attachment 8787337 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: suite/locales/Makefile.in
> @@ +217,5 @@
> >  
> >  merge-%:
> >  ifdef LOCALE_MERGEDIR
> >  	$(RM) -rf $(LOCALE_MERGEDIR)
> > +	$(topsrcdir)/mozilla/mach compare-locales --merge-dir $(LOCALE_MERGEDIR) --l10n-ini $(srcdir)/l10n.ini $*
> 
> upstream uses mach in its builds. We don't.  So this
> could just break automation.  I'll need to try it out
> in automation.

by 'we don't, I mean we don't use mach in automation.

Comment 11

[Ian Neal]

Comment on attachment 8787337 [details] [diff] [review]
WIP patch v2

As --l10n-base defaults to $(L10NBASEDIR) then this should work.

Comment 12

[Philip Chee]

Pushed to comm-central http://hg.mozilla.org/comm-central/rev/ba39dd982aab
a=Ratty