1163442 - Use FINAL_TARGET_FILES and PP_TARGETS for SeaMonkey themes

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Ian Neal]

Attachment: Port changes to SeaMonkey

This ports:
Theme specific parts for Bug 870891 - Move DIST_FILES to moz.build
Bug 1137364 - part 2 - move browser themes icon installation to FINAL_TARGET_FILES

Comment 1

[Ian Neal]

Comment on attachment 8603887 [details] [diff] [review]
Port changes to SeaMonkey

So, this almost works.
Without "NO_DIST_INSTALL = True" the chrome is packaged into the classic xpi
With it, classic's chrome isn't put into dist/bin/chrome

Should we just have classic chrome within the xpi or is there another way?

Comment 2

[[e-mail]]

(In reply to Ian Neal from comment #1)
> So, this almost works.
> Without "NO_DIST_INSTALL = True" the chrome is packaged into the classic xpi
> With it, classic's chrome isn't put into dist/bin/chrome
Sorry, I'm not seeing what you're trying to say here. Can you list what happens:
a) currently
b) with this patch
c) with this patch but without NO_DIST_INSTALL = True

> Should we just have classic chrome within the xpi or is there another way?
I'm pretty sure it belongs in dist/bin/chrome

Comment 3

[Ian Neal]

a) currently
There is a classic xpi stub in dist/bin/extensions
There is a manifest and directory for classic in dist/bin/chrome
b) with the patch
There is a classic xpi stub in dist/bin/extensions
There is no classic directory and manifest in dist/bin/chrome
c) with the patch but without NO_DIST_INSTALL = True
There is a classic xpi, containing the manifest and chrome, in dist/bin/extensions

Doing pre-processing of install.rdf with DIST_FILES/DIST_SUBDIR seems to force the chrome to be put into the same place. Have to revert to plan B, using PP_TARGETS in Makefile.in

Comment 4

[Ian Neal]

Ok, plan B doesn't work either.
If in the Makefile.in you have something like:
CLASSIC_EXTENSION_DIR = {972ce4c6-7e08-4474-a285-3208198ce6fd}
INSTALL_FILE := install.rdf
INSTALL_FILE_PATH := $(FINAL_TARGET)/extensions/$(CLASSIC_EXTENSION_DIR)
PP_TARGETS += INSTALL_FILE
It doesn't even try to copy the file, whereas if you set the INSTALL_FILE_PATH to something that doesn't exist then it attempts to copy the file (and fails for obvious reasons)!
This makes no sense to me, but perhaps glandium knows more

Comment 5

[Mike Hommey [:glandium]]

Without seeing the actual patch, I can only do guesswork. Is your snippet before including rules.mk? Have you tried adding quotes around the class extension dir?

Comment 6

[Ian Neal]

Attachment: Plan B patch

So this is the patch for plan B

Comment 7

[Ian Neal]

Doing a bit more testing, if I don't include rules.mk and specify the path explicitly to the directory without using the $(FINAL_TARGET), it copies the file.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8604691 [details] [diff] [review]
Plan B patch

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

::: suite/themes/classic/Makefile.in
@@ +5,1 @@
>  include $(topsrcdir)/config/rules.mk

This include must be *after* PP_TARGETS. which means it would be last in the file, which means it can be removed altogether.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8604691 [details] [diff] [review]
Plan B patch

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

::: suite/themes/classic/Makefile.in
@@ +8,3 @@
>  
> +INSTALL_RDF := install.rdf
> +INSTALL_RDF_PATH := $(FINAL_TARGET)/extensions/$(CLASSIC_EXTENSION_DIR)

Use = here instead of :=, FINAL_TARGET is not defined by the time this line is reached.

Comment 10

[Ian Neal]

Attachment: Use FINAL_TARGET_FILES and PP_TARGETS [Checked in: Comment 12]

Comment 11

[[e-mail]]

Comment on attachment 8606208 [details] [diff] [review]
Use FINAL_TARGET_FILES and PP_TARGETS [Checked in: Comment 12]

Ah yes, Thunderbird manages it by having separate themes for each platform so they can install install.rdf in a different Makefile, while Firefox is just weird.

Is it worth giving classic_extension_dir a symbolic name like Thunderbird does in mail/themes/moz.build?

Comment 12

[Ian Neal]

Comment on attachment 8606208 [details] [diff] [review]
Use FINAL_TARGET_FILES and PP_TARGETS [Checked in: Comment 12]

Checked in with suggested change
https://hg.mozilla.org/comm-central/rev/1fafcee93411