Closed
Bug 701393
Opened 13 years ago
Closed 12 years ago
Generic preprocessing rule
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla20
People
(Reporter: glandium, Assigned: gps)
References
Details
Attachments
(3 files, 2 obsolete files)
3.18 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
5.25 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
5.91 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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/.
Assignee | ||
Comment 2•13 years ago
|
||
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.
Reporter | ||
Comment 3•13 years ago
|
||
(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?
Assignee | ||
Comment 4•13 years ago
|
||
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...
Comment 5•13 years ago
|
||
(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.
Assignee | ||
Comment 6•13 years ago
|
||
Comment on attachment 574542 [details] [diff] [review] Generic Preprocessing Rule, v1 I agree that this is overkill. Cancelling review.
Attachment #574542 -
Flags: review?(khuey)
Assignee | ||
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Whiteboard: [good first bug]
Reporter | ||
Comment 8•12 years ago
|
||
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 → ---
Assignee | ||
Comment 9•12 years ago
|
||
(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.
Reporter | ||
Comment 10•12 years ago
|
||
(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 ....
Assignee | ||
Comment 11•12 years ago
|
||
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)
Reporter | ||
Comment 12•12 years ago
|
||
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-
Assignee | ||
Comment 13•12 years ago
|
||
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)
Reporter | ||
Comment 14•12 years ago
|
||
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+
Comment 15•12 years ago
|
||
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)
Reporter | ||
Comment 16•12 years ago
|
||
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+
Updated•12 years ago
|
Flags: in-testsuite-
OS: Linux → All
Hardware: x86_64 → All
Comment 17•12 years ago
|
||
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3afb865b87e6
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•