Closed Bug 734139 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla22

People

(Reporter: joey, Assigned: joey)

References

Details

Attachments

(1 file, 2 obsolete files)

Replace mkdir -p calls and targets with either:

GENERATED_DIRS +=
  -or-

deps = $(call mkdir_deps,foo[, ...)
Assignee: nobody → joey
Depends on: 680246
Depends on: 738404
Depends on: 734121
Depends on: 739710
Depends on: 743280
Depends on: 750303
No longer depends on: 750303
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.
Attachment #625150 - Flags: review?(ted.mielczarek)
ping on code review
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?]

????
Attachment #625150 - Flags: review?(ted.mielczarek) → review-
(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 ]
(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.
(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.
(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 ?
Attachment #625150 - Attachment is obsolete: true
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.
Attachment #665396 - Flags: review?(mh+mozilla)
Attachment #665396 - Flags: review?(mh+mozilla) → review+
Pushed to build-system:
firebot	Check-in: http://hg.mozilla.org/projects/build-system/rev/09a28340a206
Patch attached to bug 750303 will simplify some of the *-preq mkdir_dep rules in this patch.
Depends on: 750303
(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.
No longer depends on: 750303
Attachment #665396 - Attachment is obsolete: true
Patch refreshed
r=glandium carried forward
https://tbpl.mozilla.org/?tree=Try&rev=b0ac7eb84d43
Ready for inbound.
Pushed to inbound
committed changeset 123847:d0e6dacd8128
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e6dacd8128
https://hg.mozilla.org/mozilla-central/rev/d0e6dacd8128
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.