1137364 - remove browser/themes/*/Makefile.in

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - move browser/themes/*/ tab-selected-*.svg generation to GENERATED_FILES

Recently-added GENERATED_FILES support and support for specifying
per-file methods for GENERATED_FILES means that we can move the rules
for generating tab SVGs in browser/themes/ to moz.build.  As a bonus, we
can also eliminate duplicate between the rules by moving them to a
shared mozbuild file in browser/themes/, rather than individual
browser/themes/{linux,osx,windows}/ Makefiles.

Please note, as Ted had to point out to me, that rules.mk includes:

export:: $(GENERATED_FILES)

so getting rid of the export rules in the Makefile.ins is OK.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - move browser themes icon installation to FINAL_TARGET_FILES

Moving this to FINAL_TARGET_FILES also provides an excellent opportunity
to reduce duplication between the theme directories.

Comment 3

[Michael Shal [:mshal]]

Comment on attachment 8570034 [details] [diff] [review]
part 1 - move browser/themes/*/ tab-selected-*.svg generation to GENERATED_FILES

>-	$(call py_action,preprocessor, \

So I haven't looked too closely, but how come we need a new python script to wrap preprocessor.py here instead of setting the .script parameter to "preprocessor.py" and using it directly?

>+script = TOPSRCDIR + '/browser/themes/preprocess-tab-svgs.py'
>+input = [TOPSRCDIR + '/browser/themes/shared/tab-selected.svg']
>+
>+# Context variables can't be used inside functions, so hack around that.
>+generated_files = GENERATED_FILES
>+
>+def generate_svg(svg_name, script_function):
>+    global generated_files
>+    generated_files += [svg_name]
>+    svg = generated_files[svg_name]
>+    svg.script = script + ':' + script_function

IMO this looks ugly, but I know gps already r+'d the colon separator :). Maybe it's less ugly when you are specifying the script and function as strings rather than passing them into a function as variables.

>+    svg.inputs = input

The existence of this function and the generated_files variable hack leads me to believe that maybe we need to have alternative ways of specifying GENERATED_FILES so that they can be as compressed as they are with the single generate_svg line per file. What do you think?

As far as I can tell this patch works, though I hope we can come up with a nicer way to do tab-svgs.mozbuild + process-tab-svgs.py. It doesn't seem too out of the ordinary to warrant something that feels so hacky. Feel free to convince me otherwise :)

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Michael Shal [:mshal] from comment #3)
> Comment on attachment 8570034 [details] [diff] [review]
> part 1 - move browser/themes/*/ tab-selected-*.svg generation to
> GENERATED_FILES
> 
> >-	$(call py_action,preprocessor, \
> 
> So I haven't looked too closely, but how come we need a new python script to
> wrap preprocessor.py here instead of setting the .script parameter to
> "preprocessor.py" and using it directly?

Because we need to pass -D options to preprocessor.py (and --marker) and there's no way of doing that from moz.build's GENERATED_FILES.  cf discussion in bug 883954.

> >+script = TOPSRCDIR + '/browser/themes/preprocess-tab-svgs.py'
> >+input = [TOPSRCDIR + '/browser/themes/shared/tab-selected.svg']
> >+
> >+# Context variables can't be used inside functions, so hack around that.
> >+generated_files = GENERATED_FILES
> >+
> >+def generate_svg(svg_name, script_function):
> >+    global generated_files
> >+    generated_files += [svg_name]
> >+    svg = generated_files[svg_name]
> >+    svg.script = script + ':' + script_function
> 
> IMO this looks ugly, but I know gps already r+'d the colon separator :).
> Maybe it's less ugly when you are specifying the script and function as
> strings rather than passing them into a function as variables.

It does look a little nicer when you're specify one or two files with different scripts, rather than multiple files with the same script but different functions.

> >+    svg.inputs = input
> 
> The existence of this function and the generated_files variable hack leads
> me to believe that maybe we need to have alternative ways of specifying
> GENERATED_FILES so that they can be as compressed as they are with the
> single generate_svg line per file. What do you think?

Yes.  gps suggested using the context manager support from bug 1134072.  I didn't realize that had landed, but I was planning on cleaning up all the GENERATED_FILES bits in the tree with something like that once it had.

Comment 5

[Michael Shal [:mshal]]

Comment on attachment 8570034 [details] [diff] [review]
part 1 - move browser/themes/*/ tab-selected-*.svg generation to GENERATED_FILES

:froydnj did some extra convincing in irc and in https://bugzilla.mozilla.org/show_bug.cgi?id=883954#c44, so r+ for now. I hope we can revisit this one at some point though :)

Comment 6

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ffc7e7233b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b232bc4c75a

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f7ffc7e7233b
https://hg.mozilla.org/mozilla-central/rev/6b232bc4c75a