525869 - Use suite/branding as default --with-branding

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Mike Hommey [:glandium]]

Attachment: Patch against seamonkey 2.0

Currently, when using --with-branding=somedir, the content from suite/branding still gets used and may clash with whatever is done in the externally given branding directory.

Not going into suite/branding directly, but using suite/branding as a branding directory makes that work better (though it still leaves some complains during mozilla/configure)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Patch against seamonkey 2.0, v2

Same patch with some additional changes needed to have the Makefile in the given branding directory generated properly.

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 409922 [details] [diff] [review]
Patch against seamonkey 2.0, v2

>diff --git a/allmakefiles.sh b/allmakefiles.sh

>+if test -n "$MOZ_BRANDING_DIRECTORY"; then
>+  add_makefiles "
>+    $MOZ_BRANDING_DIRECTORY/Makefile
>+  "
>+fi

This is incorrect. It assumes that Thunderbird and Sunbird need this precise set up, and they don't - they need something different in both cases. Please put this in suite/makefiles.sh.

>diff --git a/suite/confvars.sh b/suite/confvars.sh

>+MOZ_BRANDING_DIRECTORY=suite/branding

Would it be better if this was suite/branding/nightly (which would mean the default defined in configure.in could be used), or maybe suite/branding/official? This would then allow other setups in exactly the same way as Firefox and Thunderbird do (though I'm not sure SeaMonkey needs them)

I realise that means moving the directories, but hg makes that easy.

Comment 3

[Robert Kaiser]

Comment on attachment 409922 [details] [diff] [review]
Patch against seamonkey 2.0, v2

r- for the comments Mark made (who actually knows all the branding stuff better than me as he figured it all out for Thunderbird).
We want to match Firefox and Thunderbird where possible and useful, as that makes life easier for everyone. What we want to avoid right now is creating any different branding for nightlies or unofficial builds in our tree, though.

Comment 4

[Robert Kaiser]

Attachment: new patch to model things after what Firefox/Thunderbird have

This new patch models the SeaMonkey branding code after what Firefox and Thunderbird have. The one area I've left out (and which probably deserves a followup, but not sure of the exact solution) is locales.
And it puts the only branding we have in nightly/ as suggested in this bug earlier.

Comment 5

[Mike Hommey [:glandium]]

Actually, the venkman icons should simply not been part of the seamonkey branding, but part of mozilla/extensions/venkman, a bit like mozilla/extensions/irc which contains the chatzilla icons (although the branding also has them).

Comment 6

[Mike Hommey [:glandium]]

Sorry this was intended for bug 546456

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 425464 [details] [diff] [review]
new patch to model things after what Firefox/Thunderbird have

I've not tested this, but the general idea seems fine.

> ifdef MOZ_BRANDING_DIRECTORY
> tier_app_dirs += $(MOZ_BRANDING_DIRECTORY)
>+else
>+tier_app_dirs += suite/branding/nightly
> endif

I think there's another bug around somewhere that is changing this. If I see it I'll try and review it today.

>+SUBMAKEFILES += \
>+	$(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/Makefile \
>+#	$(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales/Makefile \
>+	$(NULL)

That's not going to work quite how you expect, make will see this line as:

SUBMAKEFILES += $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/Makefile # $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales/Makefile $(NULL)

Which obviously isn't what you want.

>+	@$(MAKE) -C $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales AB_CD=$* XPI_NAME=locale-$* BOTH_MANIFESTS=1

Won't this fail because you've not got a locales directory?

>+  add_makefiles "
>+    $MOZ_BRANDING_DIRECTORY/Makefile
>+#    $MOZ_BRANDING_DIRECTORY/locales/Makefile
>+  "

Again, this doesn't look quite right unless add_makefiles supports it.

Comment 8

[Robert Kaiser]

Attachment: v1.1: leave out locales for now

I hoped that the comments would be parsed before the file being interpreted, but it may just have worked magically (after commenting out that other call as well) because those entries were always the last ones in their respective area.

I now moved the whole locales topic off to bug 551485 and this new patch just leaves them out, which should fix all your comments.

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> This is incorrect. It assumes that Thunderbird and Sunbird need this precise
> set up, and they don't - they need something different in both cases.

(Maybe now they do?)

(In reply to comment #7)
> I think there's another bug around somewhere that is changing this.

Bug 514519.

Comment 10

[Robert Kaiser]

(In reply to comment #9)
> (In reply to comment #7)
> > I think there's another bug around somewhere that is changing this.
> 
> Bug 514519.

Grr, so I need to rework this patch once again just because it took so long to review it? :(

Comment 11

[Robert Kaiser]

Attachment: Also update for bug 514519

OK, so here's yet another patch that is updated for bug 514519 changes as well.

I added the var for official branding even if it points to the same dir, but that makes sure that people can use --enable-official-branding just like for Thunderbird and Firefox, even if the builds don't differ from nightlies (for now).

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 435602 [details] [diff] [review]
Also update for bug 514519

I've done a quick build on Mac with this, and all seems fine.

>+add_makefiles "
>+$MOZ_BRANDING_DIRECTORY/Makefile
>+"

nit: I don't really see any point in making this a separate section to the main add_makefiles.

Comment 13

[Robert Kaiser]

Thanks, pushed as http://hg.mozilla.org/comm-central/rev/1331ca255dd4

Comment 14

[Robert Kaiser]

Needed to fix up Windows bustage due to forgotten file moves, see http://hg.mozilla.org/comm-central/rev/29fcb3e1e2c6