Last Comment Bug 680246 - makefiles: add a makefile library rule to support thread safe directory creation
: makefiles: add a makefile library rule to support thread safe directory creation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Joey Armstrong [:joey]
:
Mentors:
Depends on: 634029
Blocks: 734139 738404 739710 743280 750303
  Show dependency treegraph
 
Reported: 2011-08-18 14:10 PDT by Joey Armstrong [:joey]
Modified: 2012-04-30 08:58 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
macros and targets supporting thread safe mkdir w/deps (4.62 KB, patch)
2011-08-18 14:22 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
macros and targets supporting thread safe mkdir w/deps (10.00 KB, patch)
2011-08-25 12:30 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
threadsafe mkdir + cmd path fix for mac (9.98 KB, patch)
2011-09-06 14:08 PDT, Joey Armstrong [:joey]
ted: review-
Details | Diff | Splinter Review
mkdir -p patch with macro dep generators removed. (9.91 KB, patch)
2011-09-22 14:19 PDT, Joey Armstrong [:joey]
ted: review+
Details | Diff | Splinter Review
nit fixes (9.67 KB, patch)
2011-09-23 06:55 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
nit fixes + checkin message (9.69 KB, patch)
2011-09-23 14:10 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
threadsafe mkdir + resync with js/srcconfig (9.57 KB, patch)
2011-09-29 06:58 PDT, Joey Armstrong [:joey]
ted: review+
Details | Diff | Splinter Review
make library rule for threadsafe mkdir + unit tests (13.88 KB, patch)
2012-02-23 12:12 PST, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
make library rule for threadsafe mkdir + unit tests (14.55 KB, text/plain)
2012-02-23 13:21 PST, Joey Armstrong [:joey]
no flags Details
threadsafe mkdir patch with unit test (19.42 KB, patch)
2012-03-06 17:09 PST, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
threadsafe mkdir patch with unit test (19.42 KB, patch)
2012-03-07 04:41 PST, Joey Armstrong [:joey]
khuey: review+
Details | Diff | Splinter Review
minor cleanups for last mkdir patch (19.42 KB, patch)
2012-03-08 09:14 PST, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
cary review forward with req cleanups (25.23 KB, patch)
2012-03-13 08:08 PDT, Joey Armstrong [:joey]
khuey: review+
Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2011-08-18 14:10:10 PDT
Add a target in rules.mk to create directories in a thread safe manner and controlled by dependency.  rules.mk::$(XPIDL_GEN_DIR)/.done could be generalized.

Unit test makefiles are starting to accumulate duplicate directory creation logic so it is probably time to move this into a library.

Also derived targets may allow replacing the asynchronous mkdir calls:
   run_for_side_effects = $(shell mkdir...)
with real dependencies
Comment 1 Joey Armstrong [:joey] 2011-08-18 14:22:43 PDT
Created attachment 554208 [details] [diff] [review]
macros and targets supporting thread safe mkdir w/deps

 DIR_DEPS - new macro logic will pay attention to.
AUTO_DEPS - a list of generated dependencies for all: to use

$(dir)/.dir.done - timestamp dependency file that can be used by individual targets.  $(dir)/.done was not used to avoid potential ambiguity in a library target.

build/unix/test/Makefile.in - unit test updated to use the library target.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-19 05:44:04 PDT
Comment on attachment 554208 [details] [diff] [review]
macros and targets supporting thread safe mkdir w/deps

Some comments:

I think that we should automatically add the directory to GARBAGE_DIRS.

AUTO_DEPS should probably be some sort of macro or generated variable that takes a directory name.

DIR_DEPS doesn't really explain what it's doing.  How about GENERATED_DIRS?  Ted may have a better suggestion.


I imagine it looking something like

GENERATED_DIRS += _xpidlgen

$(XPIDL_GEN_DIR)/%.h: %.idl $(XPIDL_DEPS) $(_xpidlgen_DIR_DEP)
  commands

or

GENERATED_DIRS += _xpidlgen

$(XPIDL_GEN_DIR)/%.h: %.idl $(XPIDL_DEPS) $(call GENERATED_DIR_DEP,_xpidlgen)
  commands
Comment 3 Joey Armstrong [:joey] 2011-08-22 06:49:25 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> Comment on attachment 554208 [details] [diff] [review]
> macros and targets supporting thread safe mkdir w/deps
> 
> Some comments:
> 
> I think that we should automatically add the directory to GARBAGE_DIRS.

This logic was present in an early version but a problem surfaced.  If someone were to place a dependency on a common/shared directory not exclusively maintained by the makefile (ex: dist/include) -- "make clean" would then be able to remove a lot more targets than a single directory/makefile would be able to regenerate.

> AUTO_DEPS should probably be some sort of macro or generated variable that
> takes a directory name.

AUTO_DEPS - I have a longer term usage in mind for this macro.  A function could be added to encapsulate the actual generation logic so values are explicitly derived by path { $(foreach dir,$(call foo $dir),$(GENERATED_DIRS)).

But I opted for a more generic name in this case so it would be trivial to conditionally add more targets later on w/o having to explicitly modify makefiles in the sandbox.
 
> DIR_DEPS doesn't really explain what it's doing.  How about GENERATED_DIRS? 
> Ted may have a better suggestion.

That seems reasonable given naming will become more intuitive.

> I imagine it looking something like
> 
> GENERATED_DIRS += _xpidlgen
> 
> $(XPIDL_GEN_DIR)/%.h: %.idl $(XPIDL_DEPS) $(_xpidlgen_DIR_DEP)
>   commands
> 
> or
> 
> GENERATED_DIRS += _xpidlgen
> 
> $(XPIDL_GEN_DIR)/%.h: %.idl $(XPIDL_DEPS) $(call GENERATED_DIR_DEP,_xpidlgen)
>   commands

Yes exactly
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-22 13:38:51 PDT
(In reply to Joey Armstrong [:joey] from comment #3)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> > Comment on attachment 554208 [details] [diff] [review]
> > macros and targets supporting thread safe mkdir w/deps
> > 
> > Some comments:
> > 
> > I think that we should automatically add the directory to GARBAGE_DIRS.
> 
> This logic was present in an early version but a problem surfaced.  If
> someone were to place a dependency on a common/shared directory not
> exclusively maintained by the makefile (ex: dist/include) -- "make clean"
> would then be able to remove a lot more targets than a single
> directory/makefile would be able to regenerate.

Yeah, that's a good point.
Comment 5 Joey Armstrong [:joey] 2011-08-25 12:30:06 PDT
Created attachment 555810 [details] [diff] [review]
macros and targets supporting thread safe mkdir w/deps

Patch includes Kyle's suggestions.
Renamed DIR_DEPS to GENERATED_DIRS

Added factory macros for dependency generation.
User funcs added to help make the code self-documenting.
.SECONDEXPANSION used to allow generating prereqs from the current target.
Comment 6 Joey Armstrong [:joey] 2011-09-06 14:08:17 PDT
Created attachment 558599 [details] [diff] [review]
threadsafe mkdir + cmd path fix for mac

same mkdir library logic but replaced '/bin/true' refs with 'true' so mac [/usr/bin/true] can find the command.
Comment 7 Chris Cooper [:coop] [away until Aug 29] 2011-09-06 14:31:46 PDT
Comment on attachment 558599 [details] [diff] [review]
threadsafe mkdir + cmd path fix for mac

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

Some indentation questions, but otherwise looks fine to me.

::: config/makefiles/autotargets.mk
@@ +42,5 @@
> +
> +  get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset)
> +gen_auto_macro = $(addsuffix ^$(1),$(2))  # gen(1=target_pattern, 2=value)
> +
> +   threadsafe_mkdir = $(foreach dir,$(1),$(call gen_auto_macro,threadsafe_mkdir,$(dir)))

Is the indentation correct on lines 43 and 46? Is it just to make the = line up?

::: js/src/config/makefiles/autotargets.mk
@@ +42,5 @@
> +
> +  get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset)
> +gen_auto_macro = $(addsuffix ^$(1),$(2))  # gen(1=target_pattern, 2=value)
> +
> +   threadsafe_mkdir = $(foreach dir,$(1),$(call gen_auto_macro,threadsafe_mkdir,$(dir)))

Same comment about indentation here.
Comment 8 Joey Armstrong [:joey] 2011-09-07 05:54:59 PDT
(In reply to Chris Cooper [:coop] from comment #7)
> Comment on attachment 558599 [details] [diff] [review]
> threadsafe mkdir + cmd path fix for mac
> 
> Review of attachment 558599 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some indentation questions, but otherwise looks fine to me.
> 
> ::: config/makefiles/autotargets.mk
> @@ +42,5 @@
> > +
> > +  get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset)
> > +gen_auto_macro = $(addsuffix ^$(1),$(2))  # gen(1=target_pattern, 2=value)
> > +
> > +   threadsafe_mkdir = $(foreach dir,$(1),$(call gen_auto_macro,threadsafe_mkdir,$(dir)))
> 
> Is the indentation correct on lines 43 and 46? Is it just to make the = line
> up?
> 
> ::: js/src/config/makefiles/autotargets.mk
> @@ +42,5 @@
> > +
> > +  get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset)
> > +gen_auto_macro = $(addsuffix ^$(1),$(2))  # gen(1=target_pattern, 2=value)
> > +
> > +   threadsafe_mkdir = $(foreach dir,$(1),$(call gen_auto_macro,threadsafe_mkdir,$(dir)))
> 
> Same comment about indentation here.

Yes some of the lines are odd sizes.  Like macros are alinged on the '=' to limit scanning focus while reading through the logic.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2011-09-07 08:00:24 PDT
Just FWIW, coop's not a build system peer, so his review isn't sufficient to land this.
Comment 10 Joey Armstrong [:joey] 2011-09-21 04:36:00 PDT
ping on pending code review from the 7th
Comment 11 Ted Mielczarek [:ted.mielczarek] 2011-09-21 09:27:05 PDT
Comment on attachment 558599 [details] [diff] [review]
threadsafe mkdir + cmd path fix for mac

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

Thanks for the patch, sorry again for the delay in review. I like the concept a lot, I just have some quibbles with the implementation, and a few style nits.

I'm r-'ing solely because I want you to remove the intermediate dependencies as discussed below, since I think that will result in a fairly large change here. The rest of it is fairly minor stuff.

::: build/unix/test/Makefile.in
@@ +44,5 @@
>  include $(DEPTH)/config/autoconf.mk
> +
> +TS = .ts
> +GENERATED_DIRS += $(TS)
> +  GARBAGE_DIRS += $(TS)

I believe that our style here is usually more like:
GENERATED_DIRS += $(TS)
GARBAGE_DIRS   += $(TS)

that is, line up the operators, but keep variable names left-aligned.

(Not that we have a Makefile style guide anywhere, we really should.)

::: config/makefiles/autotargets.mk
@@ +43,5 @@
> +  get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset)
> +gen_auto_macro = $(addsuffix ^$(1),$(2))  # gen(1=target_pattern, 2=value)
> +
> +   threadsafe_mkdir = $(foreach dir,$(1),$(call gen_auto_macro,threadsafe_mkdir,$(dir)))
> +gen_threadsafe_deps = $(addsuffix /.dir.done,$(strip $(call get_auto_arg,$(1),1)))

As we discussed on IRC, I don't think the intermediate ^threadsafe_mkdir deps here provide any extra value. You've already got the .dir.done targets that serialize the mkdir, and these add a bit of complexity that we could drop. I'd rather that you set AUTO_DEPS = $(foreach dir,$(GENERATED_DIRS),$(dir)/.dir.done), and just use the rules below to generate those. If you'd like the targets to be more self-descriptive, then maybe rename .dir.done to .threadsafe.mkdir.done or something.

@@ +55,5 @@
> +##    all bootstrap: $(AUTO_DEPS)
> +##    target: $(dir)/.dir.done $(dir)/foobar
> +###########################################################################
> +ifdef GENERATED_DIRS
> +  AUTO_DEPS += $(foreach dir,$(GENERATED_DIRS),$(call threadsafe_mkdir,$(GENERATED_DIRS)))

As discussed on IRC, this should be calling threadsafe_mkdir on $(dir), since you're also looping in threadsafe_mkdir above.

@@ +58,5 @@
> +ifdef GENERATED_DIRS
> +  AUTO_DEPS += $(foreach dir,$(GENERATED_DIRS),$(call threadsafe_mkdir,$(GENERATED_DIRS)))
> +endif
> +
> +.SECONDARY:        # preserve intermediates: .dir.done

Doesn't feel right to enable this globally. Can you do .SECONDARY: $(AUTO_DEPS) or something instead? (Assuming you've made the change I asked for above.)

@@ +59,5 @@
> +  AUTO_DEPS += $(foreach dir,$(GENERATED_DIRS),$(call threadsafe_mkdir,$(GENERATED_DIRS)))
> +endif
> +
> +.SECONDARY:        # preserve intermediates: .dir.done
> +.SECONDEXPANSION:  # Enable for prereq deps derived from target/$@

If you drop the ^threadsafe_mkdir bit then you don't need the SECONDEXPANSION, right?

@@ +71,5 @@
> +##   - macro AUTO_DEPS can be used to explicitly add a list of deps
> +##   - single: $(call threadsafe_mkdir,$(var))
> +##   - $(foreach dir,$(list),$(call threadsafe_mkdir,$(dir)))
> +###################################################################
> +%/: %/.dir.done

I think I'd feel better with this as a static pattern rule, so we only match on exactly what we mean, probably like:
$(GENERATED_DIRS): %: %/.dir.done
Comment 12 Joey Armstrong [:joey] 2011-09-22 14:19:58 PDT
Created attachment 561883 [details] [diff] [review]
mkdir -p patch with macro dep generators removed.

Removed the macro dep/string generation layer, nit edits as requested.

Edits verified by:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=37bcb0d8c33a
Comment 13 Ted Mielczarek [:ted.mielczarek] 2011-09-23 05:38:51 PDT
Comment on attachment 561883 [details] [diff] [review]
mkdir -p patch with macro dep generators removed.

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

Looks good, thanks! Just some real minor fiddly comments.

::: config/makefiles/autotargets.mk
@@ +40,5 @@
> +
> +SPACE ?= $(NULL) $(NULL)
> +
> +  get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset)
> +gen_auto_macro = $(addsuffix ^$(1),$(2))  # gen(1=target_pattern, 2=value)

Doesn't look like you're using these anymore.

@@ +52,5 @@
> +##    all bootstrap: $(AUTO_DEPS)
> +##    target: $(dir)/.dir.done $(dir)/foobar
> +###########################################################################
> +ifdef GENERATED_DIRS
> +  GENERATED_DIRS_DEP = $(foreach dir,$(GENERATED_DIRS),$(dir)/.dir.done)

Maybe name this GENERATED_DIRS_DEPS (plural)?

@@ +64,5 @@
> +##   - targets suffixed by a slash will match and be processed
> +##   - macro AUTO_DEPS can be used to explicitly add a list of deps
> +##   - single: $(call threadsafe_mkdir,$(var))
> +##   - $(foreach dir,$(list),$(call threadsafe_mkdir,$(dir)))
> +###################################################################

This comment looks like it's out of sync with what you're actually doing now.

@@ +65,5 @@
> +##   - macro AUTO_DEPS can be used to explicitly add a list of deps
> +##   - single: $(call threadsafe_mkdir,$(var))
> +##   - $(foreach dir,$(list),$(call threadsafe_mkdir,$(dir)))
> +###################################################################
> +$(GENERATED_DIRS_DEP):

Oh huh, this winds up even simpler than I suggested. Nice!
Comment 14 Joey Armstrong [:joey] 2011-09-23 06:53:41 PDT
(In reply to Ted Mielczarek [:ted, :luser] from comment #13)
> Comment on attachment 561883 [details] [diff] [review] [diff] [details] [review]
> mkdir -p patch with macro dep generators removed.
> 
> Review of attachment 561883 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
>  
> ::: config/makefiles/autotargets.mk
> @@ +40,5 @@
> > +
> > +SPACE ?= $(NULL) $(NULL)
> > +
> > +  get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset)
> > +gen_auto_macro = $(addsuffix ^$(1),$(2))  # gen(1=target_pattern, 2=value)
> 
> Doesn't look like you're using these anymore.

I ended up leaving these two in because they have potential downstream use as library functions.  All of the other nits to be fixed shortly.

Thanks -- Joey
Comment 15 Joey Armstrong [:joey] 2011-09-23 06:55:33 PDT
Created attachment 562030 [details] [diff] [review]
nit fixes

nit fixes for the last code review
Comment 16 Ed Morley [:emorley] 2011-09-23 07:19:06 PDT
The try run in comment 12 was orange on Win opt:
{
make[1]: Leaving directory `/e/builds/moz2_slave/try-w32/build/obj-firefox/browser/locales'
sed: can't read components/*.manifest: No such file or directory
make[4]: *** [repackage-zip] Error 2
make[3]: *** [repackage-win32-installer] Error 2
make[2]: *** [repackage-win32-installer-x-test] Error 2
make[1]: *** [l10n-check] Error 2
make: *** [l10n-check] Error 2
}

When you attach the updated patch, could you add a commit message to it, as I probably won't be able to summarise the changes as accurately as you could. Thanks :-)
Comment 17 Joey Armstrong [:joey] 2011-09-23 14:10:36 PDT
Created attachment 562161 [details] [diff] [review]
nit fixes + checkin message

nit fixes + checkin messages
Comment 18 Joey Armstrong [:joey] 2011-09-23 14:18:00 PDT
(In reply to Ed Morley [:edmorley] from comment #16)
> The try run in comment 12 was orange on Win opt:
> {
> make[1]: Leaving directory
> `/e/builds/moz2_slave/try-w32/build/obj-firefox/browser/locales'
> sed: can't read components/*.manifest: No such file or directory
> make[4]: *** [repackage-zip] Error 2
> make[3]: *** [repackage-win32-installer] Error 2
> make[2]: *** [repackage-win32-installer-x-test] Error 2
> make[1]: *** [l10n-check] Error 2
> make: *** [l10n-check] Error 2
> }
> 
> When you attach the updated patch, could you add a commit message to it, as
> I probably won't be able to summarise the changes as accurately as you
> could. Thanks :-)

New patch submitted w/checkin message.
A missing manifest file should be unrelated to the code changes, makefile rule not yet used in the sandbox and a unit test update.

Similar manifest problems are also showing for bug #515374/Packager.pm/try submissions so there may be a bug lurking in the shadows.  I'll open another after the source has been isolated.
Comment 20 :Ehsan Akhgari 2011-09-26 08:54:34 PDT
This broke the build across all platforms, so I backed it out:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=020f14584a2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9c0a2a1a25
Comment 21 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-26 08:56:44 PDT
The copy-paste test strikes again!
Comment 22 Joey Armstrong [:joey] 2011-09-26 09:02:15 PDT
gotta love code duplication...
Comment 23 Joey Armstrong [:joey] 2011-09-29 06:58:15 PDT
Created attachment 563398 [details] [diff] [review]
threadsafe mkdir + resync with js/srcconfig

Two edits:
  o function mkdir_deps declared so caller can generate dependencies directly in place of using $(AUTO_DEPS)
  o renamed the .dir.done dep to .mkdir.done to reduce the chance of conflicts


https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4822254e25e3
  o linuxqt - segv in unit test - bug 673017
  o win32 - make package error - 684095
  o win64 - bug 690337
  o linux-mobile - bug 690198 dpkg-architecture not found
  o linux(64)?, macosx(64)?, win32 - bug 684095 - JS Component Loader ERROR during packaging
Comment 24 Ted Mielczarek [:ted.mielczarek] 2011-09-29 07:25:22 PDT
http://hg.mozilla.org/projects/build-system/rev/cb715f8a1363
Comment 25 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-30 13:44:19 PDT
https://hg.mozilla.org/mozilla-central/rev/cb715f8a1363
Comment 26 Matt Brubeck (:mbrubeck) 2011-09-30 20:48:28 PDT
Backed out: https://hg.mozilla.org/mozilla-central/rev/f25928e4847d

because of Win opt package errors: https://tbpl.mozilla.org/php/getParsedLog.php?id=6632240&tree=Firefox

(The same errors were seen on the original pushes to Try and to b-s.)
Comment 27 Joey Armstrong [:joey] 2011-10-04 07:06:42 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #26)
> Backed out: https://hg.mozilla.org/mozilla-central/rev/f25928e4847d
> 
> because of Win opt package errors:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=6632240&tree=Firefox
> 
> (The same errors were seen on the original pushes to Try and to b-s.)

Packaging errors have absolutely nothing to do with this patch.  Target rules were added to create directories and that logic is currently used by one makefile/unit test: build/unix/test/Makefile.
Comment 28 Joey Armstrong [:joey] 2011-10-04 07:25:13 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #26)
> Backed out: https://hg.mozilla.org/mozilla-central/rev/f25928e4847d
> 
> because of Win opt package errors:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=6632240&tree=Firefox
> 
> (The same errors were seen on the original pushes to Try and to b-s.)

Here is the bug for l10n/packager error that was tickled by the submission:
https://bugzilla.mozilla.org/show_bug.cgi?id=690138
Comment 29 Joey Armstrong [:joey] 2011-10-04 07:44:00 PDT
w32-ix-slave37: abort: There is no Mercurial repository here.
Comment 30 Matt Brubeck (:mbrubeck) 2011-10-04 09:52:16 PDT
Regardless whether the bug is in this patch or elsewhere, it appears to be triggered or exposed by this patch.  This landing turned m-c and b-s permaorange until it was backed out, so bug 690138 will need to be fixed before this can re-land.
Comment 31 neil@parkwaycc.co.uk 2011-10-04 16:36:32 PDT
(In reply to Joey Armstrong from comment #6)
> replaced '/bin/true' refs with 'true' so mac [/usr/bin/true] can find the command.

Or indeed the builtin true will get used in those shells that support it.
Comment 32 Joey Armstrong [:joey] 2012-02-23 12:12:28 PST
Created attachment 600129 [details] [diff] [review]
make library rule for threadsafe mkdir + unit tests

Patch refresh.  Make logic roughly the same but logic moved outside of rules.mk for modularity and isolation.

Unit test rewritten in python.  Test driver will setup a scratch work area then pass arguments to a template makefile to invoke tests.

Copyright block in new files has been replaced with the latest MPL boilerplate.

Patch submitted for review only.  Try submissions have logged several unrelated failures that will need to be triaged before final submission.
Comment 33 Gregory Szorc [:gps] 2012-02-23 12:17:28 PST
Comment on attachment 600129 [details] [diff] [review]
make library rule for threadsafe mkdir + unit tests

New code should be under MPL 2.0. https://www.mozilla.org/MPL/license-policy.html
Comment 34 Joey Armstrong [:joey] 2012-02-23 12:49:19 PST
(In reply to Gregory Szorc [:gps] from comment #33)
> Comment on attachment 600129 [details] [diff] [review]
> make library rule for threadsafe mkdir + unit tests
> 
> New code should be under MPL 2.0.
> https://www.mozilla.org/MPL/license-policy.html

heh updating the mdn coding style page.  The boilerplate link is referencing the 1.1 flavor.
Comment 35 Joey Armstrong [:joey] 2012-02-23 13:21:19 PST
Created attachment 600156 [details]
make library rule for threadsafe mkdir + unit tests

Same patch as last upload but MPL text replaced with the 2.0 flavor.
Also unit test is now dependent on makefiles/autotargets.mk rather than rules.mk.
Comment 36 John O'Duinn [:joduinn] (please use "needinfo?" flag) 2012-03-05 16:25:54 PST
(In reply to Matt Brubeck (:mbrubeck) from comment #30)
> Regardless whether the bug is in this patch or elsewhere, it appears to be
> triggered or exposed by this patch.  This landing turned m-c and b-s
> permaorange until it was backed out, so bug 690138 will need to be fixed
> before this can re-land.

:mbrubeck, after reading both bugs, I'm confused. Did you mean for bug#680246 to depend on bug#690138? (not the opposite?)
Comment 37 Matt Brubeck (:mbrubeck) 2012-03-05 16:27:11 PST
(In reply to John O'Duinn [:joduinn] from comment #36)
> :mbrubeck, after reading both bugs, I'm confused. Did you mean for
> bug#680246 to depend on bug#690138? (not the opposite?)

Yeah, I got the dependency backward.  Fixed now.
Comment 38 Joey Armstrong [:joey] 2012-03-06 06:04:29 PST
replacement patch pending with edits, cancelling the current review.  Another patch should be uploaded today post reviewing the last batch of try logs for critical failure points.
Comment 39 Joey Armstrong [:joey] 2012-03-06 17:09:43 PST
Created attachment 603524 [details] [diff] [review]
threadsafe mkdir patch with unit test

library makefile rules to automate directory in a threadsafe manner.
Creation: GENERATED_DIRS += path1 path2 path3
deps = $(call mkdir_deps,macro_of_dirs)

A unit test and makefile template included to validate creation by either macro or user function.
Comment 40 Gregory Szorc [:gps] 2012-03-06 17:13:41 PST
Comment on attachment 603524 [details] [diff] [review]
threadsafe mkdir patch with unit test

It looks like you took the MPL 1.1 license block and changed "1.1" to "2.0." This is incorrect. See https://www.mozilla.org/MPL/headers/ for the correct license block. Yes, it is only 3 lines!
Comment 41 Joey Armstrong [:joey] 2012-03-07 04:07:05 PST
(In reply to Gregory Szorc [:gps] from comment #40)
> Comment on attachment 603524 [details] [diff] [review]
> threadsafe mkdir patch with unit test
> 
> It looks like you took the MPL 1.1 license block and changed "1.1" to "2.0."
> This is incorrect. See https://www.mozilla.org/MPL/headers/ for the correct
> license block. Yes, it is only 3 lines!

So all of the disclaimer and contributor verbage should be replaced with the 3 line licence block ?
Comment 42 Joey Armstrong [:joey] 2012-03-07 04:31:47 PST
(In reply to Gregory Szorc [:gps] from comment #40)
> Comment on attachment 603524 [details] [diff] [review]
> threadsafe mkdir patch with unit test
> 
> It looks like you took the MPL 1.1 license block and changed "1.1" to "2.0."
> This is incorrect. See https://www.mozilla.org/MPL/headers/ for the correct
> license block. Yes, it is only 3 lines!

Also should the copyright notice within existing files be updated to the 2.0 flavor or should the text remain the same but change the end date to 2012 ?
Comment 43 Joey Armstrong [:joey] 2012-03-07 04:41:48 PST
Created attachment 603675 [details] [diff] [review]
threadsafe mkdir patch with unit test

edit_1.1: licence block for new files pruned to 3 lines
edit_1.1: end date changed to 2012 in files with an existing license block.


threadsafe mkdir patch with unit test

library makefile rules to automate directory in a threadsafe manner.
Creation: GENERATED_DIRS += path1 path2 path3
deps = $(call mkdir_deps,macro_of_dirs)

A unit test and makefile template included to validate creation by either macro or user function.
Comment 44 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-07 09:09:11 PST
Comment on attachment 603675 [details] [diff] [review]
threadsafe mkdir patch with unit test

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

r=me with the comments addressed (either the changes made, or reasons why I'm wrong given).

::: config/makefiles/autotargets.mk
@@ +35,5 @@
> +###########################################################################
> +mkdir_deps =$(foreach dir,$($(1)),$(dir)/.mkdir.done)
> +
> +ifneq (,$(GENERATED_DIRS))
> +  tmpauto :=$(call mkdir_deps,GENERATED_DIRS)

There's a missing space after :=

Is there a compelling reason to do $($(1)) and GENERATED_DIRS, instead of $(1) and $(GENERATED_DIRS)?  I'm not opposed to the way it is, just curious.

@@ +41,5 @@
> +  GARBAGE_DIRS        +=$(tmpauto)
> +endif
> +
> +%/.mkdir.done:
> +	$(subst $(SPACE)-p,$(null),$(MKDIR)) -p $(dir $@)

I assume this is just to avoid 'mkdir -p -p dir'?  Can you add a comment for that?

Also, use $(NULL) with all caps.

::: config/rules.mk
@@ +2039,5 @@
> +#############################################################################
> +# Derived targets and dependencies
> +
> +include $(topsrcdir)/config/makefiles/autotargets.mk
> +all:: $(AUTO_DEPS)

Don't you want 'default all::'?
Comment 45 Joey Armstrong [:joey] 2012-03-08 03:38:15 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #44)
> Comment on attachment 603675 [details] [diff] [review]
> threadsafe mkdir patch with unit test
> 
> Review of attachment 603675 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the comments addressed (either the changes made, or reasons why
> I'm wrong given).
> 
> ::: config/makefiles/autotargets.mk
> @@ +35,5 @@
> > +###########################################################################
> > +mkdir_deps =$(foreach dir,$($(1)),$(dir)/.mkdir.done)
> > +
> > +ifneq (,$(GENERATED_DIRS))
> > +  tmpauto :=$(call mkdir_deps,GENERATED_DIRS)
> 
> There's a missing space after :=

Actually this syntax was intentional, maybe not specifically for this assignment but I am starting to use it in general.  Removing whitespace from the left side of values will help prevent whitespace from accumulating within displayed commands/arguments:

  $cmd -a   -debug ...


> Is there a compelling reason to do $($(1)) and GENERATED_DIRS, instead of
> $(1) and $(GENERATED_DIRS)?  I'm not opposed to the way it is, just curious.

GENERATED_DIRS in the function could be considered a form of hardcoding depending on your point of view.  But there is a performance ding -- if it were listed, extra deps to check are introduced with each call.  Calling mkdir_deps from a library function would re-add all directory deps explicitly defined by the calling makefile.

Also $(1) will not easily allow for directory creation by list, this call would create one dir and silently fail on the rest:
  $(call mkdir_deps,foo bar tans fans) 

$($(1)) is able support creation by list:
  prereq = foo bar tans fans
  $(call mkdir_deps,prereq)

Hmmmm, recently I wrote some utility macros that can identify argument context as either list or scalar and expand args as needed [1].  The functions could be included as an enhancement so the library can support both calling syntax:
    $(call mkdir_deps,a)
    $(call mkdir_deps,a b c d)

[1] - anyone know of syntax similar to $* for make functions that would allow accessing all elements in the parameter list.  Aside from using a hardcoded list: $(1) $(2) $(3) ... $(9) that would drop elements beyond $(10)+ ?


> @@ +41,5 @@
> > +  GARBAGE_DIRS        +=$(tmpauto)
> > +endif
> > +
> > +%/.mkdir.done:
> > +	$(subst $(SPACE)-p,$(null),$(MKDIR)) -p $(dir $@)
> 
> I assume this is just to avoid 'mkdir -p -p dir'?  Can you add a comment for
> that?
> 
> Also, use $(NULL) with all caps.

Yes and I really need to sit down and write a make-lint checker for $(NULL)...

Question: since null= can be used w/o pre-definition  would this help re-define it as a builtin value ?  Syntax will be changed for the bug but I have never really thought of null as a constant taken in this context.


> ::: config/rules.mk
> @@ +2039,5 @@
> > +#############################################################################
> > +# Derived targets and dependencies
> > +
> > +include $(topsrcdir)/config/makefiles/autotargets.mk
> > +all:: $(AUTO_DEPS)
> 
> Don't you want 'default all::'?

This is question not statement...  I normally know when targets will be processed and try to avoid un-necessary deps.  If 'default' were added would this force make to re-check all of the directory deps beyond the initial call that should create them ?  When needed deps can be explicitly mentioned by targets in the handful of instances where needed.
Comment 46 Mike Hommey [:glandium] 2012-03-08 03:52:47 PST
(In reply to Joey Armstrong [:joey] from comment #45)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #44)
> > Comment on attachment 603675 [details] [diff] [review]
> > threadsafe mkdir patch with unit test
> > 
> > Review of attachment 603675 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with the comments addressed (either the changes made, or reasons why
> > I'm wrong given).
> > 
> > ::: config/makefiles/autotargets.mk
> > @@ +35,5 @@
> > > +###########################################################################
> > > +mkdir_deps =$(foreach dir,$($(1)),$(dir)/.mkdir.done)
> > > +
> > > +ifneq (,$(GENERATED_DIRS))
> > > +  tmpauto :=$(call mkdir_deps,GENERATED_DIRS)
> > 
> > There's a missing space after :=
> 
> Actually this syntax was intentional, maybe not specifically for this
> assignment but I am starting to use it in general.  Removing whitespace from
> the left side of values will help prevent whitespace from accumulating
> within displayed commands/arguments:
> 
>   $cmd -a   -debug ...

That kind of thing happens when you do $cmd $(var1) $(var2).
That var1 and var2 are defined with
  var=something
instead of
  var= something
changes nothing.
Comment 47 Joey Armstrong [:joey] 2012-03-08 06:24:38 PST
(In reply to Mike Hommey [:glandium] from comment #46)
> (In reply to Joey Armstrong [:joey] from comment #45)
> > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #44)
> > > Comment on attachment 603675 [details] [diff] [review]
> > > threadsafe mkdir patch with unit test
> > > 
> > > Review of attachment 603675 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > r=me with the comments addressed (either the changes made, or reasons why
> > > I'm wrong given).
> > > 
> > > ::: config/makefiles/autotargets.mk
> > > @@ +35,5 @@
> > > > +###########################################################################
> > > > +mkdir_deps =$(foreach dir,$($(1)),$(dir)/.mkdir.done)
> > > > +
> > > > +ifneq (,$(GENERATED_DIRS))
> > > > +  tmpauto :=$(call mkdir_deps,GENERATED_DIRS)
> > > 
> > > There's a missing space after :=
> > 
> > Actually this syntax was intentional, maybe not specifically for this
> > assignment but I am starting to use it in general.  Removing whitespace from
> > the left side of values will help prevent whitespace from accumulating
> > within displayed commands/arguments:
> > 
> >   $cmd -a   -debug ...
> 
> That kind of thing happens when you do $cmd $(var1) $(var2).
> That var1 and var2 are defined with
>   var=something
> instead of
>   var= something
> changes nothing.

This behavior can be generated using only:
  $(cmd) $(cmd_args) blah

it happens in variable construction not in the command line
Comment 48 Mike Hommey [:glandium] 2012-03-08 06:34:33 PST
(In reply to Joey Armstrong [:joey] from comment #47)
> This behavior can be generated using only:
>   $(cmd) $(cmd_args) blah
> 
> it happens in variable construction not in the command line

a:=a
b:= b
cmd_args:=$(a) $(b)
foo:
    $(cmd) $(cmd_args) blah

The above won't add any extra space because b has a space between = and the value. It will have double space if cmd_args is empty.
Comment 49 Joey Armstrong [:joey] 2012-03-08 09:14:35 PST
Created attachment 604095 [details] [diff] [review]
minor cleanups for last mkdir patch
Comment 50 Ed Morley [:emorley] 2012-03-08 09:30:04 PST
(In reply to Joey Armstrong [:joey] from comment #41)
> So all of the disclaimer and contributor verbage should be replaced with the
> 3 line licence block ?

Yes, the new licence header should replace this entire block:
http://www.mozilla.org/MPL/headers/

eg:
http://mxr.mozilla.org/mozilla-central/source/mozglue/linker/Makefile.in
http://mxr.mozilla.org/mozilla-central/source/b2g/components/b2g.idl

(In reply to Joey Armstrong [:joey] from comment #42)
> Also should the copyright notice within existing files be updated to the 2.0
> flavor or should the text remain the same but change the end date to 2012 ?

Old files can be transitioned to the new format too (and will be mass changed by gerv soon anyway).
Comment 51 Joey Armstrong [:joey] 2012-03-13 08:08:46 PDT
Created attachment 605402 [details] [diff] [review]
cary review forward with req cleanups

r=me, Kyle's review + nit fixes.

edit_1.2 - mkdir -p -p commented & s/null/NULL
edit_1.2 - compelling reason to do $($(1)) rather than $(1) ?
         - bug 734121 will add helpers so func args can process as either scalar, list or ref.  Arg handling is currently by list ref/var expansion $($(1)).
edit_1.2 - Don't you want 'default all::'?  Added target along with export, libs and tools.
edit_1.2 - remaining copyright blocks crushed down to the 3 line default.
Comment 52 Joey Armstrong [:joey] 2012-03-13 12:27:34 PDT
Try submission status for the edits
====================================

Thanks for your try submission (http://hg.mozilla.org/try/pushloghtml?changeset=7d7450575f7d).  It's the best!

Watch https://tbpl.mozilla.org/?tree=Try&rev=7d7450575f7d for your results to come in.

Builds and logs will be available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jarmstrong@mozilla.com-7d7450575f7d.

This directory won't be created until the first builds are uploaded, so please be patient.
==========================================================================

Job passed with the usual set of messages:
can't read dom/tests/mochitest/glboalstorage/Makefile.in: No such file or dir
configure: WARNING: unrecognized options: --enable-debug, --enable-application
tar: image/test/reftest/encoders-lossless/encoder.html?img=size-15x15.png&mime=image: Cannot stat: No such file or directory.
Comment 53 Joey Armstrong [:joey] 2012-03-15 08:36:53 PDT
ping on the code review
Comment 54 Mike Hommey [:glandium] 2012-03-15 09:20:58 PDT
Comment on attachment 605402 [details] [diff] [review]
cary review forward with req cleanups

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

::: allmakefiles.sh
@@ +1,5 @@
>  #! /bin/sh
>  #
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.

Please don't do that.
http://blog.gerv.net/2012/03/mpl-boilerplate-at-mozilla-faq/
Comment 55 Joey Armstrong [:joey] 2012-03-15 14:28:40 PDT
(In reply to Mike Hommey [:glandium] from comment #54)
> Comment on attachment 605402 [details] [diff] [review]
> cary review forward with req cleanups
> 
> Review of attachment 605402 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: allmakefiles.sh
> @@ +1,5 @@
> >  #! /bin/sh
> >  #
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > +# You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> Please don't do that.
> http://blog.gerv.net/2012/03/mpl-boilerplate-at-mozilla-faq/

The license text can be reverted but there have been conflicting requests in this bug to have it changed.
Comment 56 Joey Armstrong [:joey] 2012-03-15 14:43:24 PDT
(In reply to Joey Armstrong [:joey] from comment #55)
> (In reply to Mike Hommey [:glandium] from comment #54)
> > Comment on attachment 605402 [details] [diff] [review]
> > cary review forward with req cleanups
> > 
> > Review of attachment 605402 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: allmakefiles.sh
> > @@ +1,5 @@
> > >  #! /bin/sh
> > >  #
> > > +# This Source Code Form is subject to the terms of the Mozilla Public
> > > +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > > +# You can obtain one at http://mozilla.org/MPL/2.0/.
> > 
> > Please don't do that.
> > http://blog.gerv.net/2012/03/mpl-boilerplate-at-mozilla-faq/
> 
> The license text can be reverted but there have been conflicting requests in
> this bug to have it changed.

Actually this patch has been lingering for far to long to delay being accepted over comment changes.  It has been tested with the MPL replaced and should land in the current form.  Will not bother modifying the notice on future edits.
Comment 57 Mike Hommey [:glandium] 2012-03-15 23:26:27 PDT
(In reply to Joey Armstrong [:joey] from comment #55)
> (In reply to Mike Hommey [:glandium] from comment #54)
> > Comment on attachment 605402 [details] [diff] [review]
> > cary review forward with req cleanups
> > 
> > Review of attachment 605402 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: allmakefiles.sh
> > @@ +1,5 @@
> > >  #! /bin/sh
> > >  #
> > > +# This Source Code Form is subject to the terms of the Mozilla Public
> > > +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > > +# You can obtain one at http://mozilla.org/MPL/2.0/.
> > 
> > Please don't do that.
> > http://blog.gerv.net/2012/03/mpl-boilerplate-at-mozilla-faq/
> 
> The license text can be reverted but there have been conflicting requests in
> this bug to have it changed.

Gerv is the authority on that.

> Actually this patch has been lingering for far to long to delay being
> accepted over comment changes.  It has been tested with the MPL replaced and
> should land in the current form.  Will not bother modifying the notice on
> future edits.

Are you saying you're afraid that changing a comment/license boilerplate is going to break something?
Comment 58 Joey Armstrong [:joey] 2012-03-16 02:12:13 PDT
> > Actually this patch has been lingering for far to long to delay being
> > accepted over comment changes.  It has been tested with the MPL replaced and
> > should land in the current form.  Will not bother modifying the notice on
> > future edits.
> 
> Are you saying you're afraid that changing a comment/license boilerplate is
> going to break something?

No a simple comment change *should* not break anything.  But modifying a working, tested and reviewed patch, to revert ?-verbatim-? text edit that will eventually be inserted.  Seems like busy work.

Since changes have been made, even trivial ones, the patch will need to be re-tested.
So submit to try and see what other random/transient failures are logged, possibly becoming blocked on those along the way.

If the patch edits somehow become blocked on the bulk MPL edits gerv is working on there may also be a delay introduced there.  He did mention encountering build & test problems in the try submission that need to be sorted out.

I do not think blocking finished work on pending work is a good precedent to set.
Comment 59 Joey Armstrong [:joey] 2012-03-22 08:00:00 PDT
these makefile edits should simmer in b-s for a while.
Comment 60 Chris Cooper [:coop] [away until Aug 29] 2012-04-02 10:42:30 PDT
Comment on attachment 605402 [details] [diff] [review]
cary review forward with req cleanups

https://hg.mozilla.org/projects/build-system/rev/9c293bf4df91
Comment 61 Chris Cooper [:coop] [away until Aug 29] 2012-04-03 09:58:46 PDT
All the failures seem to be due to missing files which just came over in the merge from m-c (thanks philor).
Comment 62 Takanori MATSUURA 2012-04-04 12:01:02 PDT
https://hg.mozilla.org/mozilla-central/rev/0fe55a35369f
accidentally adds config/rules.mk.orig and js/src/config/rules.mk.orig
Comment 63 Joey Armstrong [:joey] 2012-04-05 09:59:05 PDT
Patch landed on mozilla-central yesterday by Coop & Philor:
https://hg.mozilla.org/mozilla-central/rev/c598b7b202e7
Comment 64 Benoit Jacob [:bjacob] (mostly away) 2012-04-06 20:21:34 PDT
It seems that you accidentally add two .orig files:

+++ b/config/rules.mk.orig
+++ b/js/src/config/rules.mk.orig

This would probably not have happened if my file adding .orig files to .hgignore had been accepted... see bug 634029.
Comment 65 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-06 20:22:33 PDT
New bug please.
Comment 66 Takanori MATSUURA 2012-04-07 02:31:18 PDT
(In reply to Takanori MATSUURA from comment #62)
(In reply to Benoit Jacob [:bjacob] from comment #64)

Filed as bug 743433.

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