Closed Bug 680246 Opened 13 years ago Closed 12 years ago

makefiles: add a makefile library rule to support thread safe directory creation

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: joey, Assigned: joey)

References

Details

Attachments

(1 file, 12 obsolete files)

25.23 KB, patch
khuey
: review+
Details | Diff | Splinter Review
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
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.
Assignee: nobody → joey
Status: NEW → ASSIGNED
Attachment #554208 - Flags: review?(khuey)
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
Attachment #554208 - Flags: review?(khuey)
(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
(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.
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.
Attachment #554208 - Attachment is obsolete: true
Attachment #555810 - Flags: review?(ted.mielczarek)
same mkdir library logic but replaced '/bin/true' refs with 'true' so mac [/usr/bin/true] can find the command.
Attachment #558599 - Flags: review?(coop)
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.
Attachment #558599 - Flags: review?(coop) → review+
(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.
Just FWIW, coop's not a build system peer, so his review isn't sufficient to land this.
Attachment #555810 - Attachment is obsolete: true
Attachment #555810 - Flags: review?(ted.mielczarek)
Attachment #558599 - Flags: review+ → review?(ted.mielczarek)
ping on pending code review from the 7th
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
Attachment #558599 - Flags: review?(ted.mielczarek) → review-
Removed the macro dep/string generation layer, nit edits as requested.

Edits verified by:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=37bcb0d8c33a
Attachment #558599 - Attachment is obsolete: true
Attachment #561883 - Flags: review?(ted.mielczarek)
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!
Attachment #561883 - Flags: review?(ted.mielczarek) → review+
(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
Attached patch nit fixes (obsolete) — Splinter Review
nit fixes for the last code review
Keywords: checkin-needed
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 :-)
Keywords: checkin-needed
Attached patch nit fixes + checkin message (obsolete) — Splinter Review
nit fixes + checkin messages
Attachment #562030 - Attachment is obsolete: true
(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.
Keywords: checkin-needed
Keywords: checkin-needed
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
Target Milestone: mozilla9 → ---
The copy-paste test strikes again!
gotta love code duplication...
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
Attachment #561883 - Attachment is obsolete: true
Attachment #562161 - Attachment is obsolete: true
Attachment #563398 - Flags: review?(ted.mielczarek)
Attachment #563398 - Flags: review?(ted.mielczarek) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cb715f8a1363
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
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.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: fixed-in-bs
Target Milestone: mozilla10 → ---
(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.
(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
w32-ix-slave37: abort: There is no Mercurial repository here.
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.
Blocks: 690138
(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.
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.
Attachment #563398 - Attachment is obsolete: true
Attachment #600129 - Flags: review?(ted.mielczarek)
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
(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.
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.
Attachment #600156 - Flags: review?(ted.mielczarek)
Summary: makefiles: add a generic rule.mk to support thread safe directory creation → makefiles: add a makefile library rule to support thread safe directory creation
(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?)
(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.
No longer blocks: 690138
Depends on: 690138
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.
Attachment #600156 - Flags: review?(ted.mielczarek)
Attachment #600129 - Flags: review?(ted.mielczarek)
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.
Attachment #600129 - Attachment is obsolete: true
Attachment #600156 - Attachment is obsolete: true
Attachment #603524 - Flags: review?(khuey)
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!
Attachment #603524 - Attachment is patch: true
(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 ?
(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 ?
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.
Attachment #603524 - Attachment is obsolete: true
Attachment #603524 - Flags: review?(khuey)
Attachment #603675 - Flags: review?(khuey)
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::'?
Attachment #603675 - Flags: review?(khuey) → review+
(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.
(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.
(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
(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.
Attachment #603675 - Attachment is obsolete: true
(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).
No longer depends on: 690138
Blocks: 690138
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.
Attachment #604095 - Attachment is obsolete: true
Attachment #605402 - Flags: review?(khuey)
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.
Blocks: 734139
No longer blocks: 690138
ping on the code review
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/
(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.
(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.
(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?
> > 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.
these makefile edits should simmer in b-s for a while.
Blocks: 738404
Blocks: 739710
All the failures seem to be due to missing files which just came over in the merge from m-c (thanks philor).
https://hg.mozilla.org/mozilla-central/rev/0fe55a35369f
accidentally adds config/rules.mk.orig and js/src/config/rules.mk.orig
Patch landed on mozilla-central yesterday by Coop & Philor:
https://hg.mozilla.org/mozilla-central/rev/c598b7b202e7
Status: REOPENED → RESOLVED
Closed: 13 years ago12 years ago
Resolution: --- → FIXED
Blocks: 743280
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.
Status: RESOLVED → REOPENED
Depends on: 634029
Resolution: FIXED → ---
New bug please.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
(In reply to Takanori MATSUURA from comment #62)
(In reply to Benoit Jacob [:bjacob] from comment #64)

Filed as bug 743433.
Blocks: 750303
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.