Get rid of XULPPFLAGS in Chatzilla

RESOLVED FIXED

Status

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: philip.chee, Assigned: frg)

Tracking

(Blocks 1 bug)

Trunk
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected)

Details

(Whiteboard: [cz-0.9.93], )

Attachments

(1 attachment, 3 obsolete attachments)

See Also: → 1213641
Posted patch bug1210791-v1.diff (obsolete) — Splinter Review
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.
Assignee: rginda → kairo
Status: NEW → ASSIGNED
Attachment #8677213 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8677213 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8677213 - Flags: review?(mh+mozilla)
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.
Attachment #8677213 - Flags: review?(mh+mozilla)
(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.
Flags: needinfo?(mh+mozilla)
> Do you know why is l10n.mk not doing that, then?

Probably its history.
Flags: needinfo?(mh+mozilla)
Duplicate of this bug: 1213641
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.
Attachment #8677213 - Flags: feedback+
(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/
(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.
(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).
(In reply to Robert Kaiser (:kairo@mozilla.com) 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...
(In reply to Adrian Kalla [:adriank] from comment #12)
> (In reply to Robert Kaiser (:kairo@mozilla.com) 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?
Duplicate of this bug: 1220342
(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.
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
Blocks: 1244467
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.
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 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?
Attachment #8719157 - Flags: feedback?(akalla)
(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...
Attachment #8719157 - Flags: feedback?(akalla)
>> 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.
Assignee: kairo → frgrahl
Attachment #8719157 - Attachment description: Updated patch for comm-beta and up when pulling default branch → Patch for l10n processing of install.rdf
Attachment #8719157 - Flags: review?(gijskruitbosch+bugs)
Attachment #8677213 - Attachment is obsolete: true
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.
Attachment #8719156 - Attachment is obsolete: true
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.
Attachment #8719157 - Flags: review?(gijskruitbosch+bugs) → review?(mh+mozilla)
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)
Attachment #8719157 - Flags: review?(mh+mozilla)
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.
Attachment #8719157 - Attachment is obsolete: true
Attachment #8735113 - Flags: review?(mh+mozilla)
Attachment #8735113 - Flags: review?(mh+mozilla) → review+
Keywords: checkin-needed
remote:   https://hg.mozilla.org/chatzilla/rev/2d483f9a47ac
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Whiteboard: [cz-0.9.93]
You need to log in before you can comment on or make changes to this bug.