Last Comment Bug 756443 - makefiles: add mkdir_dep helper functions
: makefiles: add mkdir_dep helper functions
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Joey Armstrong [:joey]
:
Mentors:
Depends on:
Blocks: 734139 757855
  Show dependency treegraph
 
Reported: 2012-05-18 06:23 PDT by Joey Armstrong [:joey]
Modified: 2012-06-12 12:06 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add mkdir_stem() to complement mkdir_deps. Strip extraneous slashes from given paths". (8.59 KB, patch)
2012-05-18 09:11 PDT, Joey Armstrong [:joey]
mh+mozilla: review+
Details | Diff | Splinter Review
Add mkdir_stem() to complement mkdir_deps. Strip extraneous slashes from given paths". (8.56 KB, patch)
2012-05-30 13:35 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2012-05-18 06:23:44 PDT
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.
Comment 1 Joey Armstrong [:joey] 2012-05-18 09:11:30 PDT
Created attachment 625135 [details] [diff] [review]
Add mkdir_stem() to complement mkdir_deps.  Strip extraneous slashes from given paths".
Comment 2 Joey Armstrong [:joey] 2012-05-18 10:20:29 PDT
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.
Comment 3 Joey Armstrong [:joey] 2012-05-23 08:54:08 PDT
ping on code review
Comment 4 Joey Armstrong [:joey] 2012-05-24 10:53:01 PDT
Try job: https://tbpl.mozilla.org/?tree=Try&rev=d3a260cb739f 

mac failures due to ssh/key problems during checksum/upload
Comment 5 Joey Armstrong [:joey] 2012-05-30 05:55:27 PDT
ping on code review from the 18th
Comment 6 Mike Hommey [:glandium] 2012-05-30 07:49:24 PDT
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,%
Comment 7 Joey Armstrong [:joey] 2012-05-30 11:20:17 PDT
(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.
Comment 8 Joey Armstrong [:joey] 2012-05-30 13:35:04 PDT
Created attachment 628450 [details] [diff] [review]
Add mkdir_stem() to complement mkdir_deps.  Strip extraneous slashes from given paths".
Comment 9 Joey Armstrong [:joey] 2012-05-31 09:02:00 PDT
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.
Comment 10 Ted Mielczarek [:ted.mielczarek] 2012-05-31 09:19:25 PDT
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.
Comment 11 Joey Armstrong [:joey] 2012-06-01 14:31:00 PDT
Inbound push: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7fd4683de77
Comment 12 :Ehsan Akhgari (away Aug 1-5) 2012-06-02 12:12:51 PDT
https://hg.mozilla.org/mozilla-central/rev/b7fd4683de77

Note You need to log in before you can comment on or make changes to this bug.