Closed Bug 1239707 Opened 4 years ago Closed 3 years ago

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

Categories

(SeaMonkey :: Build Config, defect)

defect
Not set

Tracking

(seamonkey2.49esr fixed)

RESOLVED FIXED
seamonkey2.49
Tracking Status
seamonkey2.49esr --- fixed

People

(Reporter: philip.chee, Assigned: philip.chee)

References

()

Details

Attachments

(1 file, 1 obsolete file)

Callek had some concerns about this though in https://groups.google.com/forum/#!msg/mozilla.dev.builds/0koWKXx7arg/izvBHzy8nmwJ
Attached patch WIP patch (obsolete) — Splinter Review
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
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #8707921 - Flags: feedback?(bugspam.Callek)
Attachment #8707921 - Flags: feedback?(akalla)
Attachment #8707921 - Flags: feedback?(iann_bugzilla)
Comment on attachment 8707921 [details] [diff] [review]
WIP patch

Cancelling review requests due to Bug 1223385 reopened.
Attachment #8707921 - Flags: feedback?(iann_bugzilla)
Attachment #8707921 - Flags: feedback?(bugspam.Callek)
Attachment #8707921 - Flags: feedback?(akalla)
Comment on attachment 8707921 [details] [diff] [review]
WIP patch

Do we need this? Does it work?
Attachment #8707921 - Flags: feedback?(ewong)
Attached patch WIP patch v2Splinter Review
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.
Attachment #8707921 - Attachment is obsolete: true
Attachment #8707921 - Flags: feedback?(ewong)
Attachment #8787337 - Flags: review?(philip.chee)
Attachment #8787337 - Flags: feedback?(ewong)
Comment on attachment 8787337 [details] [diff] [review]
WIP patch v2

Redirect reviews to ewong (buildconfig) and Adrian (L10n)
Attachment #8787337 - Flags: review?(philip.chee)
Attachment #8787337 - Flags: review?(iann_bugzilla)
Attachment #8787337 - Flags: review?(ewong)
Attachment #8787337 - Flags: review?(akalla)
Attachment #8787337 - Flags: feedback?(ewong)
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).
Attachment #8787337 - Flags: review?(akalla) → review-
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.
(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 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.
(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 on attachment 8787337 [details] [diff] [review]
WIP patch v2

As --l10n-base defaults to $(L10NBASEDIR) then this should work.
Attachment #8787337 - Flags: review?(iann_bugzilla) → review+
Pushed to comm-central http://hg.mozilla.org/comm-central/rev/ba39dd982aab
a=Ratty
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.49
Attachment #8787337 - Flags: review?(ewong)
You need to log in before you can comment on or make changes to this bug.