Closed Bug 701393 Opened 13 years ago Closed 12 years ago

Generic preprocessing rule

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla20

People

(Reporter: glandium, Assigned: gps)

References

Details

Attachments

(3 files, 2 obsolete files)

It is customary to add something like the following to makefiles:

file: file.in
       $(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $< > $@
GARBAGE += file

We should have generic rules for that, like we have for e.g. preprocessed js components.
I'm not positive, but I have a nagging suspicion to get full coverage on this, we'll need a syntax that supports multiple lists for different target directories, much like EXPORTS_NAMESPACES works. https://mxr.mozilla.org/mozilla-central/source/config/rules.mk#1375

  PREPROCESS_foo = a.js b.js
  PREPROCESS_DEFINES_foo = "FOO=1"
  PREPROCESS_TARGET_foo = "modules/foo"

  PREPROCESS_bar = x.js y.js
  PREPROCESS_DEFINES_bar = "BAR=5"
  PREPROCESS_TARGET_bar = "libs/bar"

  PREPROCESS_ITEMS = foo bar

This would result in a.js and b.js being exported to $(out)/module/foo/.
Blocks: 680369
Attached patch Generic Preprocessing Rule, v1 (obsolete) — Splinter Review
If you ask me how to explain the Makefile magic going on in this patch, I'm not sure I could. It took me a while going through every permutation of $(eval) and $(call) to get things working. I used to have multiple $(call)s and error checking inside the define. However, I could not get it to work: make would always scream at me with some kind of error or the target wouldn't get registered. I'm sure there are ways to wrangle things, but they are beyond my make knowledge. If anyone can get it working, I'm all ears.

Anyway, the patch allows you to define multiple lists of files to be preprocessed. Each list will be output to one directory. The targets are defined individually via $(eval) and it assumes the source file is the only dependency.

It does not handle the case where input and output filenames are different. That should probably be addressed, but I'm not sure the best way to do that given that make lacks a map data structure. The best I can come up with is:

1) Define something that can be $(eval $(call PREPROCESS,source,target,defines)). The syntax is ugly. I would prefer declarative, data-driven syntax.

2) Define a variable of a list of filenames. Even indexes are source filenames and the value at the next index is the target filename. We couldn't have per-file defines though because make doesn't have proper lists, only strings separated by spaces. If you pass a define with a space, it gets expanded and chaos ensues. Anyway, this variable would be consumed by rules.mk and rules.mk would spit out appropriate targets, again likely through $(eval) magic.

I converted services/sync/Makefile.in as a proof-of-concept. The conversion was beyond the scope of this bug, so it should probably not be checked in. And, the conversion is not accurate, as it installs files at the wrong directory level (the leading 'module/' is not supposed to appear in dist/bin/modules/services-sync). This poses a question of whether the rule should a) support stripping leading parts of input filenames on target b) support adding prefix to source filenames c) some variation of above or whether we should just force consumers to define files in Makefiles at the right directories. Ugh.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #574542 - Flags: review?(khuey)
(In reply to Gregory Szorc [:gps] from comment #1)
> I'm not positive, but I have a nagging suspicion to get full coverage on
> this, we'll need a syntax that supports multiple lists for different target
> directories, much like EXPORTS_NAMESPACES works.
> https://mxr.mozilla.org/mozilla-central/source/config/rules.mk#1375
> 
>   PREPROCESS_foo = a.js b.js
>   PREPROCESS_DEFINES_foo = "FOO=1"
>   PREPROCESS_TARGET_foo = "modules/foo"
> 
>   PREPROCESS_bar = x.js y.js
>   PREPROCESS_DEFINES_bar = "BAR=5"
>   PREPROCESS_TARGET_bar = "libs/bar"
> 
>   PREPROCESS_ITEMS = foo bar
> 
> This would result in a.js and b.js being exported to $(out)/module/foo/.

Do we need something that finegrained? I mean, do we have places where we need separate defines or targets for files in the same source directory?
Yeah, after going through the tree, I couldn't find a case where we need separate lists going to a single directory, as I have implemented. The cases we need are:

1) Mapping individual files to different source and target name and directories. Need to support multiple rules per makefile.

2) Mapping multiple files to the same output directory. This is really a sub-set of #1, but may want to be supported explicitly so syntax isn't redundant.

I may want to cancel my review and implement what is actually needed...
(In reply to Gregory Szorc [:gps] from comment #1)
> I'm not positive, but I have a nagging suspicion to get full coverage on
> this, we'll need a syntax that supports multiple lists for different target
> directories, much like EXPORTS_NAMESPACES works.
> https://mxr.mozilla.org/mozilla-central/source/config/rules.mk#1375
> 
>   PREPROCESS_foo = a.js b.js
>   PREPROCESS_DEFINES_foo = "FOO=1"
>   PREPROCESS_TARGET_foo = "modules/foo"
> 
>   PREPROCESS_bar = x.js y.js
>   PREPROCESS_DEFINES_bar = "BAR=5"
>   PREPROCESS_TARGET_bar = "libs/bar"

This seems like overkill. We already have rules for EXTRA_PP_MODULES and EXTRA_PP_COMPONENTS, which put things into a specific directory. The point of this bug is to make a generic rule for preprocessing a file from $(srcdir) into the current directory in the objdir. Per-file DEFINES also feels like overkill.
Comment on attachment 574542 [details] [diff] [review]
Generic Preprocessing Rule, v1

I agree that this is overkill. Cancelling review.
Attachment #574542 - Flags: review?(khuey)
No longer blocks: 680369
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Whiteboard: [good first bug]
There are cases that bug 770426 doesn't handle, that is those requiring that the result is in the objdir, not FINAL_TARGET, and thus that the original file and preprocessed file name is different.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(In reply to Mike Hommey [:glandium] from comment #8)
> There are cases that bug 770426 doesn't handle, that is those requiring that
> the result is in the objdir, not FINAL_TARGET, and thus that the original
> file and preprocessed file name is different.

Huh? Does PP_TARGETS + FOO_PATH not control the output directory? DIST_CHROME_FILES uses FINAL_TARGET. But, the generic rule enforces no output directory and there is nothing preventing us from using the generic rule in Makefile.in's.
(In reply to Gregory Szorc [:gps] from comment #9)
> (In reply to Mike Hommey [:glandium] from comment #8)
> > There are cases that bug 770426 doesn't handle, that is those requiring that
> > the result is in the objdir, not FINAL_TARGET, and thus that the original
> > file and preprocessed file name is different.
> 
> Huh? Does PP_TARGETS + FOO_PATH not control the output directory?
> DIST_CHROME_FILES uses FINAL_TARGET. But, the generic rule enforces no
> output directory and there is nothing preventing us from using the generic
> rule in Makefile.in's.

It won't work for an equivalent to:
foo: foo.in
        Preprocessor.py ....
Added FOO_STRIP_SUFFIX which does a $(basename) on all the files if it is defined.

I also converted the robocop Makefile to use this. I stopped short of giving that file a (much needed) complete facelift. If you told me to land minus the robocop Makefile changes, I'd be cool with that.

Try at http://tbpl.mozilla.org/?tree=Try&rev=391899cf4ae1
Attachment #574542 - Attachment is obsolete: true
Attachment #657159 - Flags: review?(mh+mozilla)
Comment on attachment 657159 [details] [diff] [review]
Ability to strip suffix during preprocessing, v1

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

::: build/mobile/robocop/Makefile.in
@@ +35,5 @@
> +java_harness_STRIP_SUFFIX := 1
> +PP_TARGETS += java_harness
> +
> +android_manifest := AndroidManifest.xml.in
> +android_manifest_PATH := 1

1 ?

::: config/rules.mk
@@ +1571,5 @@
> +# Callers can also select whether to strip the suffix from the output
> +# files. e.g. If this is set and the input file in "foo.c.in" the output
> +# file will be "foo.c".
> +#
> +# FOO_STRIP_SUFFIX := 1

I'd rather have any .in suffix removed automatically, and if we have odd cases where we do want to keep the .in suffix, add a variable for that.

Your patch to Makefile.in also makes me think we need a guart against the target file being the same as the source file (which would happen if srcdir==. and the filename doesn't have a .in suffix). Would you mind adding that at the same time?
Attachment #657159 - Flags: review?(mh+mozilla) → review-
I removed the diff to the robocop files because I don't like playing with fire.

New try at http://tbpl.mozilla.org/?tree=Try&rev=ad6d5bfe042e
Attachment #657159 - Attachment is obsolete: true
Attachment #657173 - Flags: review?(mh+mozilla)
Comment on attachment 657173 [details] [diff] [review]
Strip .in suffix automaticaly, v2

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

::: config/rules.mk
@@ +1580,5 @@
>  endef
>  
>  $(foreach category,$(PP_TARGETS),\
>    $(foreach file,$($(category)),\
> +    $(eval $(call preprocess_file_template,$(file),$(patsubst %.in,%,$(file)),$($(category)_PATH),$($(category)_FLAGS)))\

actually, make that $(file:.in=). I was writing patsubst because i was thinking array of files.
Attachment #657173 - Flags: review?(mh+mozilla) → review+
I've merged Gregory's work with the PP_TARGETS support, and rearranged the rule to be a bit more legible (I hope). I also fleshed out the documentation. Seemed like enough changes to merit a re-review.
Attachment #687327 - Flags: review?(mh+mozilla)
Blocks: 506717
Comment on attachment 687327 [details] [diff] [review]
Add suffix stripping to PP_TARGETS generic preprocessor rule

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

::: config/rules.mk
@@ +1596,5 @@
>  endef
>  
>  $(foreach category,$(PP_TARGETS),\
>    $(foreach file,$($(category)),\
> +    $(eval $(call preprocess_file_template,$(file),$($(category)_PATH)/$(notdir $(file:.in=)),$(or $($(category)_TARGET),libs),$($(category)_FLAGS)))\

Can you split after each argument? that is likely to make it more readable.

While here, you might as well make FOO_PATH optional and mean CURDIR if not given.
Attachment #687327 - Flags: review?(mh+mozilla) → review+
Flags: in-testsuite-
OS: Linux → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/3afb865b87e6
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: