1210791 - Get rid of XULPPFLAGS in Chatzilla

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Philip Chee]

One occurrence in chatzilla:

> http://mxr.mozilla.org/comm-central/source/mozilla/extensions/irc/locales/Makefile.in?rev=dbeacae3a1f8#39

Comment 1

[Robert Kaiser]

Attachment: bug1210791-v1.diff

This patch should do it. What it does is to replace the "magic" we had in the other variables with the same logic as we have in http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/l10n.mk#176

In an incremental build for a German SeaMonkey, this worked fine for me, I'll try a clean full build tomorrow.

Comment 2

[:Gijs (he/him)]

Comment on attachment 8677213 [details] [diff] [review]
bug1210791-v1.diff

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

I have no idea about XULPPFLAGS, what it does or how it works. Please request review from someone who knows (that aspect of) the build system.

Comment 3

[Robert Kaiser]

Comment on attachment 8677213 [details] [diff] [review]
bug1210791-v1.diff

Hmm, OK. It's easy as that var just did specify options to hand to the preprocessor, not that magical after all.

Mike, handing to you as you know it for sure, does this patch look right?

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8677213 [details] [diff] [review]
bug1210791-v1.diff

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

::: locales/Makefile.in
@@ -35,5 @@
>  XPI_NAME               = chatzilla-$(AB_CD)
>  INSTALL_EXTENSION_ID   = langpack-$(AB_CD)@chatzilla.mozilla.org
>  XPI_PKGNAME            = chatzilla-$(CHATZILLA_VERSION).$(AB_CD)
> -DIST_FILES             = generic/install.rdf
> -XULPPFLAGS += \

Instead, you could set DIST_FILES_FLAGS (but do so after rules.mk is included)

This would make things slightly clearer for future changes.

Comment 5

[Robert Kaiser]

(In reply to Mike Hommey [:glandium] from comment #4)
> Instead, you could set DIST_FILES_FLAGS (but do so after rules.mk is
> included)
> 
> This would make things slightly clearer for future changes.

Do you know why is l10n.mk not doing that, then? I really dislike using different code paths there and here in something that pretty much does the same thing.

Comment 6

[Mike Hommey [:glandium]]

> Do you know why is l10n.mk not doing that, then?

Probably its history.

Comment 8

[Adrian Kalla [:adriank]]

Comment on attachment 8677213 [details] [diff] [review]
bug1210791-v1.diff

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

The patch works, as can be seen by my builds here:


For it to apply, the line:
-DIST_FILES             = generic/install.rdf
needs to be changed to:
-DIST_FILES             = generic/install.rdf generic/install.js

::: locales/Makefile.in
@@ -34,5 @@
>  ifneq (en-US,$(AB_CD))
>  XPI_NAME               = chatzilla-$(AB_CD)
>  INSTALL_EXTENSION_ID   = langpack-$(AB_CD)@chatzilla.mozilla.org
>  XPI_PKGNAME            = chatzilla-$(CHATZILLA_VERSION).$(AB_CD)
> -DIST_FILES             = generic/install.rdf

This line needs to be changed to:
-DIST_FILES             = generic/install.rdf generic/install.js
for the patch to apply.

Comment 9

[Adrian Kalla [:adriank]]

(In reply to Adrian Kalla [:adriank] from comment #8)
> The patch works, as can be seen by my builds here:

https://l10n.mozilla-community.org/~akalla/unofficial/seamonkey/nightly/latest-comm-aurora-linux64/

Comment 10

[Robert Kaiser]

(In reply to Adrian Kalla [:adriank] from comment #8)
> For it to apply, the line:
> -DIST_FILES             = generic/install.rdf
> needs to be changed to:
> -DIST_FILES             = generic/install.rdf generic/install.js

On the release branch probably, yes, on trunk the .js variant has been removed.

Comment 11

[Robert Kaiser]

(In reply to Adrian Kalla [:adriank] from comment #8)
> The patch works, as can be seen by my builds here:

Thanks, that's great, now we only need to have an updated patch for what glandium stated. I hope to get to that in the next two weeks (I had very little time recently).

Comment 12

[Adrian Kalla [:adriank]]

(In reply to Robert Kaiser (:[e-mail]) from comment #10)
> (In reply to Adrian Kalla [:adriank] from comment #8)
> > For it to apply, the line:
> > -DIST_FILES             = generic/install.rdf
> > needs to be changed to:
> > -DIST_FILES             = generic/install.rdf generic/install.js
> 
> On the release branch probably, yes, on trunk the .js variant has been
> removed.

You are right. The aurora branch still pulls cZ 0.9.91.1 instead of 0.9.92, hence it needs the modified patch...

Comment 13

[Tony Mechelynck [:tonymec]]

(In reply to Adrian Kalla [:adriank] from comment #12)
> (In reply to Robert Kaiser (:[e-mail]) from comment #10)
> > (In reply to Adrian Kalla [:adriank] from comment #8)
> > > For it to apply, the line:
> > > -DIST_FILES             = generic/install.rdf
> > > needs to be changed to:
> > > -DIST_FILES             = generic/install.rdf generic/install.js
> > 
> > On the release branch probably, yes, on trunk the .js variant has been
> > removed.
> 
> You are right. The aurora branch still pulls cZ 0.9.91.1 instead of 0.9.92,
> hence it needs the modified patch...

Hm, installing a new SeaMonkey (aurora, beta or release) will create a new profile with cZ 0.9.91.1 in it, and then at the first opportunity it will pull 0.9.92 (the current "stable" cZ release) from AMO? Why not distribute 0.9.92 immediately?

Comment 15

[Tony Mechelynck [:tonymec]]

(In reply to Adrian Kalla [:adriank] from comment #12)
> You are right. The aurora branch still pulls cZ 0.9.91.1 instead of 0.9.92,
> hence it needs the modified patch...

After checking http://hg.mozilla.org/chatzilla/ in "graph" mode and looking at the tags, I notice that all branches since 2.38b1_build1 have been pulling the official "release" of cZ 0.9.92. Somehow I don't see SEAMONKEY_* tags for 2.41 and 2.42 but OTOH, AFAICT neither 2.41b nor 2.42a2 have been built yet. I can only assume that when their building resumes they won't use an earlier cZ than on the current stable release of SeaMonkey.

Trunk SeaMonkey of course always pulls the head of the "default" branch of the cZ repo, which at the moment is 3 "code" changesets later than the stable release.

Comment 16

[Frank-Rainer Grahl (:frg)]

The original patch no longer seems to work with c-c suite builds. Dropping the -I from the preprocessor solved it for me under Windows.

> libs realchrome::
>	@echo "Comparing $(AB_CD) to en-US"
>	@$(PERL) $(topsrcdir)/toolkit/locales/compare-locales.pl $(srcdir)/en-US $(LOCALE_SRCDIR)
>	$(call py_action,preprocessor,$(DEFINES) $(ACDEFINES) \
>	$(call EXPAND_LOCALE_SRCDIR,toolkit/locales)/defines.inc \
>	$(LOCALE_SRCDIR)/defines.inc \
>       $(srcdir)/generic/install.rdf -o $(DIST)/xpi-stage/$(XPI_NAME)/install.rdf)
>endif

Comment 17

[Frank-Rainer Grahl (:frg)]

Attachment: Updated patch for comm-release when pulling default branch

If you build Seamonkey 2.41 from comm-release you should pull from the default Chatzilla branch in client.py. This patch works in this case and allows me to locally build Seamonkey with an up to date Chatzilla including a correct l10n language pack. It's basically the patch from kairo adjusted for recent Chatzilla changes in Bug 1240304.

Comment 18

[Frank-Rainer Grahl (:frg)]

Attachment: Patch for l10n processing of install.rdf

If you build Seamonkey 2.42+ from c-b, c-a or c-c you should pull from the default Chatzilla branch in client.py. This patch works in this case and allowed me to locally build Seamonkey with an up to date Chatzilla including a correct l10n language pack. It's basically the patch from kairo adjusted for recent Chatzilla changes in Bug 1240304 and preprocessor changes.

Comment 19

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8719157 [details] [diff] [review]
Patch for l10n processing of install.rdf

Adrian,

does this patch from me for Chatzilla locales work ok for your locale builds?

Comment 20

[Adrian Kalla [:adriank]]

(In reply to Frank-Rainer Grahl from comment #19)
> Comment on attachment 8719157 [details] [diff] [review]
> Updated patch for comm-beta and up when pulling default branch
> 
> Adrian,
> 
> does this patch from me for Chatzilla locales work ok for your locale builds?

It does in the sense of, that with it it is possible to successfully build a localized SeaMonkey build. Nonetheless, there is still no Chatzilla language pack included in the package... but this is bug 1244467.

Other thing: while removing "-I" in the patch makes it run - is it really the proper fix (as in: are the parameters behind the former "-I"'s really treated properly)? Just asking, as I didn't check...

Comment 21

[Frank-Rainer Grahl (:frg)]

>> Nonetheless, there is still no Chatzilla language pack included in the package... but this is bug 1244467.

Hmm I wonder if the l10n merge screws something up there. I haven't set up merge yet and the language xpi is included in the windows de installer.

>>
Other thing: while removing "-I" in the patch makes it run - is it really the proper fix (as in: are the parameters behind the former "-I"'s really treated properly)? Just asking, as I didn't check...
<<

The rdf is correctly preprocessed this way. I build a Windows x86 and x64 de Seamonkey regularly for 2 friends and checked this out. I tried to find out if this is the correct solution and found myself deep in script after barely documented script. Just called it a day then because it works for now. Maybe someone else more familiar with the build system should look at it.

Comment 22

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8719156 [details] [diff] [review]
Updated patch for comm-release when pulling default branch

Taking over the bug from Robert as suggested in the last Seamonkey Status meeting. -I is no longer valid in c-r because of build preprocessor changes so this patch is obsolete too.

Comment 23

[:Gijs (he/him)]

Comment on attachment 8719157 [details] [diff] [review]
Patch for l10n processing of install.rdf

Mike or Callek (not accepting review requests) would be a better reviewer for this change.

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8719157 [details] [diff] [review]
Patch for l10n processing of install.rdf

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

::: locales/Makefile.in
@@ +55,5 @@
>  	@$(PERL) $(topsrcdir)/toolkit/locales/compare-locales.pl $(srcdir)/en-US $(LOCALE_SRCDIR)
> +	$(call py_action,preprocessor,$(DEFINES) $(ACDEFINES) \
> +	$(call EXPAND_LOCALE_SRCDIR,toolkit/locales)/defines.inc \
> +	$(LOCALE_SRCDIR)/defines.inc \
> +	$(srcdir)/generic/install.rdf -o $(DIST)/xpi-stage/$(XPI_NAME)/install.rdf)

You should be able to use $(MOZILLA_DIR)/toolkit/locales/generic/install.rdf modulo some modifications. (see bug 1235151)

Comment 25

[Frank-Rainer Grahl (:frg)]

Attachment: 1210791-chatzilla-l10n-V2.patch

Mike,

thanks for pointing me to the other rdf. It's more or less a 1:1 copy here with an additional ifndef in the rdf so that the standalone build still works.

Tested standalone and on c-r and c-c with en-US and de locale.

Comment 26

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/chatzilla/rev/2d483f9a47ac