Closed Bug 756443 Opened 9 years ago Closed 9 years ago

makefiles: add mkdir_dep helper functions

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla15

People

(Reporter: joey, Assigned: joey)

References

Details

Attachments

(1 file, 1 obsolete file)

1) Add a function mkdir_stem that can strip '/.mkdir.done from a given list of paths.  This function can be called when a directory is needed as both a pre-requisite and element of the target command path.

The ability to strip the timestamp file allows use of $<, $*, $^ w/o having to hardcode directory paths within a command.

2) minor edit, not critical - scrub multiple slashes from a given path.  The simple $(subst //,/,$(val)) pattern substitution will not handle a few cases that a series of string functions can produce.
Assignee: nobody → joey
Blocks: 734139
Comment on attachment 625135 [details] [diff] [review]
Add mkdir_stem() to complement mkdir_deps.  Strip extraneous slashes from given paths".

Added mkdir_stem() as a complement to mkdir_deps() allowing a timestmap to be used for pre-requisites which can then be morphed back into a directory for use on the command line.

Expanded the squash-directory-slashes logic to handle more cases.
Logic is able to preserve embedded whitespace within paths.
Also the logic could be used for windows directory slashes if needed by declaring a variable containing the slash type and replacing literal '/' within functions.
Attachment #625135 - Flags: review?(ted.mielczarek)
ping on code review
Try job: https://tbpl.mozilla.org/?tree=Try&rev=d3a260cb739f 

mac failures due to ssh/key problems during checksum/upload
ping on code review from the 18th
Attachment #625135 - Flags: review?(ted.mielczarek) → review?(mh+mozilla)
Comment on attachment 625135 [details] [diff] [review]
Add mkdir_stem() to complement mkdir_deps.  Strip extraneous slashes from given paths".

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

::: config/makefiles/autotargets.mk
@@ +22,5 @@
> +space = $(NULL) $(NULL)
> +
> +
> +## todo: create fileutils.mk - combine directory functions with
> +## toolkit/mozapps/installer/packager.mk: QUOTED_WILDCARD, ESCAPE_SPACE(), etc

Not sure this todo note is very useful.

@@ +45,5 @@
> +	$(call _slashSqueeze,\
> +    $(subst $(space),<--[**]-->,$(1))\
> +  )))
> +
> +# Revert pre-reqs [$<, $*, $^] to dirs for target command line use

Not sure what this comment means.

@@ +46,5 @@
> +    $(subst $(space),<--[**]-->,$(1))\
> +  )))
> +
> +# Revert pre-reqs [$<, $*, $^] to dirs for target command line use
> +mkdir_stem =$(foreach val,$(getargv),$(subst /.mkdir.done,$(NULL),$(val)))

patsubst %/.mkdir.done,%
Attachment #625135 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #6)
> Comment on attachment 625135 [details] [diff] [review]
> Add mkdir_stem() to complement mkdir_deps.  Strip extraneous slashes from
> given paths".
> 
> Review of attachment 625135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/makefiles/autotargets.mk
> @@ +22,5 @@
> > +space = $(NULL) $(NULL)
> > +
> > +
> > +## todo: create fileutils.mk - combine directory functions with
> > +## toolkit/mozapps/installer/packager.mk: QUOTED_WILDCARD, ESCAPE_SPACE(), etc
> 
> Not sure this todo note is very useful.

File based logic from the makefiles mentioned along with related logic in autotargets can be refactored into a standalone 'fileutils.mk' library for reuse and testing.  Todo is just a reminder about this, if a bug is preferable I can open another one.

> 
> @@ +45,5 @@
> > +	$(call _slashSqueeze,\
> > +    $(subst $(space),<--[**]-->,$(1))\
> > +  )))
> > +
> > +# Revert pre-reqs [$<, $*, $^] to dirs for target command line use
> 
> Not sure what this comment means.

mkdir_deps generated pre-requisites will be a timestamp (which will be used to populate make's autovariables).  A string transform is needed to morph the timestamp back into a directory path { ~mkdir_stem($dep) } in order to use it as part of a command line. 


> @@ +46,5 @@
> > +    $(subst $(space),<--[**]-->,$(1))\
> > +  )))
> > +
> > +# Revert pre-reqs [$<, $*, $^] to dirs for target command line use
> > +mkdir_stem =$(foreach val,$(getargv),$(subst /.mkdir.done,$(NULL),$(val)))
> 
> patsubst %/.mkdir.done,%

If patterns are unique {yes I know that is subjective} -- $(patsubst ) will be safer and $(subst ) will be a little more efficient.
Attachment #625135 - Attachment is obsolete: true
Try job: https://tbpl.mozilla.org/?tree=Try&rev=3f76bd22a57f
r=glandium carried forward

Removed todo comment and opened a new makefile bug.
Changed comment to note functionality rather than application for make autovars.
Attachment #628450 - Flags: review?(ted.mielczarek)
Comment on attachment 628450 [details] [diff] [review]
Add mkdir_stem() to complement mkdir_deps.  Strip extraneous slashes from given paths".

If you got r+ from glandium then you don't need to request review from me, you're clear to land.
Attachment #628450 - Flags: review?(ted.mielczarek)
https://hg.mozilla.org/mozilla-central/rev/b7fd4683de77
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Blocks: 757855
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.