734139 - makefiles: modify tree makefiles to use mkdir library logic from bug 680246

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Joey Armstrong [:joey]]

Replace mkdir -p calls and targets with either:

GENERATED_DIRS +=
  -or-

deps = $(call mkdir_deps,foo[, ...)

Comment 1

[Joey Armstrong [:joey]]

Attachment: update makefiles to use mkdir_deps.

Comment 2

[Joey Armstrong [:joey]]

Comment on attachment 625150 [details] [diff] [review]
update makefiles to use mkdir_deps.

Last batch of makefile edits to use mkdir_deps.  Remaining target references to mkdir will require a non-trivial amount of effort to convert so relegate that work to separate bugs.

mkdir and $(NSINSTALL) -D calls replaced with mkdir_deps dependencies.  Directory targets mentioned in config/rules.mk have yet to be altered.  Another bug is open to handle target removal and updating makefiles dependent on these directory targets with a file dep.

-preqs list added for targets requiring directory creation.

dom/bindings/Makefile.in: replaced local $(CACHE_DIR)/.done target with a call to $(mkdir_deps) - same functionality provided by the library call.

A try job is in the pipeline.

Comment 3

[Joey Armstrong [:joey]]

ping on code review

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 625150 [details] [diff] [review]
update makefiles to use mkdir_deps.

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

Note that some of these changes create a file in final destinations (FINAL_TARGET, DIST/bin, etc.), please make very sure it doesn't end up in what we ship.

::: config/rules.mk
@@ +648,5 @@
>  
>  # Create dependencies on static (and shared EXTRA_DSO_LIBS) libraries
>  ifneq (,$(strip $(filter %.$(LIB_SUFFIX),$(LIBS) $(EXTRA_DSO_LDOPTS)) $(SHARED_LIBRARY_LIBS) $(EXTRA_DSO_LIBS)))
> +
> +$(MDDEPDIR)/libs: Makefile.in $(call mkdir_deps,$(MDDEPDIR))

Please leave this out, this is being dealt with in bug 757339

@@ +1173,5 @@
> +  $(call mkdir_deps,$(_JAVA_DIR)) \
> +  $(addprefix $(_JAVA_DIR)/,$(JAVA_SRCS:.java=.class)) \
> +  $(GLOBAL_DEPS) \
> +  $(NULL)
> +

That is not necessary, since all the class files from there already depend on $(_JAVA_DIR).

@@ +1215,2 @@
>  
> +export:: $(call mkdir_deps,$(FINAL_TARGET))

Considering the latter, it doesn't seem like you need the former.

@@ +1258,5 @@
> +  $(call mkdir_deps,$(FINAL_TARGET)/$(PREF_DIR)) \
> +  $(PREF_JS_EXPORTS) \
> +  $(NULL)
> +
> +libs:: $(pref-js-exports-preqs)

Like above.

@@ +1416,1 @@
>  	for i in $^; do \

$^ is going to contain $(FINAL_TARGET)/components/.mkdir.done and run commands for it.

@@ +1444,5 @@
> +  $(EXTRA_PP_JS_MODULES) \
> +  $(call mkdir_deps,$(FINAL_TARGET)/modules) \
> +  $(NULL)
> +
> +libs:: $(extra-pp-js-modules-preqs)

Same as above.

@@ +1801,5 @@
>  libs export::
>  	$(CHECK_FROZEN_VARIABLES)
>  
>  default all::
> +	if test -d $(DIST)/bin ; then $(TOUCH) $(DIST)/bin/.purgecaches ; fi

Unrelated change.

::: dom/bindings/Makefile.in
@@ +82,5 @@
>                                       $(webidl_base)/%.webidl \
>                                       $(NULL)
>  	PYTHONDONTWRITEBYTECODE=1 $(PYTHON) $(topsrcdir)/config/pythonpath.py \
>  	  $(PLY_INCLUDE) -I$(srcdir)/parser \
> +	  $(srcdir)/BindingGen.py $(ACCESSOR_OPT) header \

Looks like a spurious hunk.

::: xpcom/typelib/xpidl/Makefile.in
@@ +62,3 @@
>    $(NULL)
>  
> +# mkdir_dep not appended to auto-variable $^ [hmmm?]

????

Comment 5

[Joey Armstrong [:joey]]

(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 625150 [details] [diff] [review]
> update makefiles to use mkdir_deps.
> 
> Review of attachment 625150 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Note that some of these changes create a file in final destinations
> (FINAL_TARGET, DIST/bin, etc.), please make very sure it doesn't end up in
> what we ship.
> 

Ok

> ::: config/rules.mk
> @@ +648,5 @@
> >  
> >  # Create dependencies on static (and shared EXTRA_DSO_LIBS) libraries
> >  ifneq (,$(strip $(filter %.$(LIB_SUFFIX),$(LIBS) $(EXTRA_DSO_LDOPTS)) $(SHARED_LIBRARY_LIBS) $(EXTRA_DSO_LIBS)))
> > +
> > +$(MDDEPDIR)/libs: Makefile.in $(call mkdir_deps,$(MDDEPDIR))
> 
> Please leave this out, this is being dealt with in bug 757339

Why should we be using an answer other than a central, consistent function for directory creation ?  Answers that do not involve makefile dependencies normally contribute to spurious "mkdir -p" calls.  Which in part is what this patch will fix.


> @@ +1173,5 @@
> > +  $(call mkdir_deps,$(_JAVA_DIR)) \
> > +  $(addprefix $(_JAVA_DIR)/,$(JAVA_SRCS:.java=.class)) \
> > +  $(GLOBAL_DEPS) \
> > +  $(NULL)
> > +
> 
> That is not necessary, since all the class files from there already depend
> on $(_JAVA_DIR).

ps> these deps pre-date the patch.

Same condition for $(JAVA_DIR), the makefile dep is to limit directory creation to only when the directory does not exist.

 
> @@ +1215,2 @@
> >  
> > +export:: $(call mkdir_deps,$(FINAL_TARGET))
> 
> Considering the latter, it doesn't seem like you need the former.

The 'export' and 'libs' targets are distinct and would require independent directory dependencies.  If the dep were removed from export, attempting to run 'gmake export' in the build directory would fail when export targets attempt to write into the $(FINAL_TARGET) directory.


> @@ +1258,5 @@
> > +  $(call mkdir_deps,$(FINAL_TARGET)/$(PREF_DIR)) \
> > +  $(PREF_JS_EXPORTS) \
> > +  $(NULL)
> > +
> > +libs:: $(pref-js-exports-preqs)
> 
> Like above.

export:: and libs:: need distinct dependencies to function properly.


[ snip -- will continue after looking into remaining items ]

Comment 6

[Mike Hommey [:glandium]]

(In reply to Joey Armstrong [:joey] from comment #5)
> > Please leave this out, this is being dealt with in bug 757339
> 
> Why should we be using an answer other than a central, consistent function
> for directory creation ?  Answers that do not involve makefile dependencies
> normally contribute to spurious "mkdir -p" calls.  Which in part is what
> this patch will fix.

Bug 757339 removes this code. I want to avoid the extra conflict on your or my end, depending on which lands first.

> > @@ +1173,5 @@
> > > +  $(call mkdir_deps,$(_JAVA_DIR)) \
> > > +  $(addprefix $(_JAVA_DIR)/,$(JAVA_SRCS:.java=.class)) \
> > > +  $(GLOBAL_DEPS) \
> > > +  $(NULL)
> > > +
> > 
> > That is not necessary, since all the class files from there already depend
> > on $(_JAVA_DIR).
> 
> ps> these deps pre-date the patch.
> 
> Same condition for $(JAVA_DIR), the makefile dep is to limit directory
> creation to only when the directory does not exist.
> 
>  
> > @@ +1215,2 @@
> > >  
> > > +export:: $(call mkdir_deps,$(FINAL_TARGET))
> > 
> > Considering the latter, it doesn't seem like you need the former.
> 
> The 'export' and 'libs' targets are distinct and would require independent
> directory dependencies.  If the dep were removed from export, attempting to
> run 'gmake export' in the build directory would fail when export targets
> attempt to write into the $(FINAL_TARGET) directory.

I was suggesting to remove the $(FINAL_TARGET) target, not the export dependencies. Likewise for libs.

Comment 7

[Joey Armstrong [:joey]]

(In reply to Mike Hommey [:glandium] from comment #6)
> (In reply to Joey Armstrong [:joey] from comment #5)
> > > Please leave this out, this is being dealt with in bug 757339
> > 
> > Why should we be using an answer other than a central, consistent function
> > for directory creation ?  Answers that do not involve makefile dependencies
> > normally contribute to spurious "mkdir -p" calls.  Which in part is what
> > this patch will fix.
> 
> Bug 757339 removes this code. I want to avoid the extra conflict on your or
> my end, depending on which lands first.

I do not think that is a good idea as it contributes to patch ordering/blocking and in this case forces patch edits and re-testing.  Allow patches to land and one of us will need to deal with the merge conflict.


> > > @@ +1215,2 @@
> > > >  
> > > > +export:: $(call mkdir_deps,$(FINAL_TARGET))
> > > 
> > > Considering the latter, it doesn't seem like you need the former.
> > 
> I was suggesting to remove the $(FINAL_TARGET) target, not the export
> dependencies. Likewise for libs.

This edit would introduce a change in behavior for the patch.  Logic to create the directory by export exists in the unmodified version of rules.mk.  The library call simply short-circuits future mkdir calls to create the directory.

Comment 8

[Joey Armstrong [:joey]]

(In reply to Mike Hommey [:glandium] from comment #4)
> @@ +1416,1 @@
> >  	for i in $^; do \
> 
> $^ is going to contain $(FINAL_TARGET)/components/.mkdir.done and run
> commands for it.

Yes it definitely would appear that way from the syntax and expected functionality. But the same behavior will be in play here as relates to that cryptic comment down below:

> > +# mkdir_dep not appended to auto-variable $^ [hmmm?]
> 
> ????

mkdir_dep generated a timestamp file to force directory creation but oddly enough make did not include this dep when populating the $^ automatic variable.  

Not sure exactly why, maybe due to it being a hidden file, failed ^[a-zA-Z] check on the filename or a timing problem -- but $^ does not contain the dependency, even when directory creation was forced by the dependency.


Hmmm I thought $(ERROR_EXIT) might also be able to catch stray files as an error condition but Preprocessor.py has been able to exit successfully given odd input:

    touch foo; python config/Preprocessor.py foo; echo $?
:)  python config/Preprocessor.py /dev/null; echo $?

ps>  If a source file were truncated during checkout or copying, a could return successfully with this behavior.

 
> ::: dom/bindings/Makefile.in
> @@ +82,5 @@
> >                                       $(webidl_base)/%.webidl \
> >                                       $(NULL)
> >  	PYTHONDONTWRITEBYTECODE=1 $(PYTHON) $(topsrcdir)/config/pythonpath.py \
> >  	  $(PLY_INCLUDE) -I$(srcdir)/parser \
> > +	  $(srcdir)/BindingGen.py $(ACCESSOR_OPT) header \
> 
> Looks like a spurious hunk.

Hmmm, not sure why this was reported as a delta.  sdiff patch m-c | grep BindingGen was not able to find the modified entry ?

Comment 9

[Joey Armstrong [:joey]]

Attachment: replace mkdir -p calls with mkdir_deps

Comment 10

[Joey Armstrong [:joey]]

Comment on attachment 665396 [details] [diff] [review]
replace mkdir -p calls with mkdir_deps

https://tbpl.mozilla.org/?tree=Try&rev=816bfe20336b

Replaced a few lingering mkdir -p calls with mkdir_deps.
Typo fixed in mobile/locales/Makefile.in, use mkdir_deps rather than mkdir-deps.

Cosmetic cleanups for problems reported by the code review script:
  o trim trailing whitespace
  o prefix whitespace around line continuations.

Comment 11

[Joey Armstrong [:joey]]

Pushed to build-system:
firebot	Check-in: http://hg.mozilla.org/projects/build-system/rev/09a28340a206

Comment 12

[Joey Armstrong [:joey]]

Patch attached to bug 750303 will simplify some of the *-preq mkdir_dep rules in this patch.

Comment 13

[Joey Armstrong [:joey]]

(In reply to Joey Armstrong [:joey] from comment #12)
> Patch attached to bug 750303 will simplify some of the *-preq mkdir_dep
> rules in this patch.

750303 would be an enhancement.  TODO: re-test and land this patch.  Followup on enhancements in a separate bug.

Comment 14

[Joey Armstrong [:joey]]

Attachment: replace mkdir -p calls with mkdir_deps

Comment 15

[Joey Armstrong [:joey]]

Patch refreshed
r=glandium carried forward
https://tbpl.mozilla.org/?tree=Try&rev=b0ac7eb84d43
Ready for inbound.

Comment 16

[Joey Armstrong [:joey]]

Pushed to inbound
committed changeset 123847:d0e6dacd8128
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e6dacd8128

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0e6dacd8128