Makefile.in beneath: browser/ ipc/ mobile/ tools/ testing/

Makefile.in edits to use mkdir_deps logic. Added a stub makefile target to handle 'mkdir dot' calls. Report a warning so they can be reviewed for stale logic. A typo calling $(call mkdir_deps,mkdir_deps,real_dir) would make a good April fools prank for gmake v3.82.

ping on the code review

Comment on attachment 611048 [details] [diff] [review] Makefile.in edits to use bug 680246 logic Review of attachment 611048 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/Makefile.in @@ +46,5 @@ > > include $(DEPTH)/config/autoconf.mk > > +DIRS = profile/extensions > +DIST_DEST = $(DIST)/$(MOZ_MACBUNDLE_NAME) Did we decide that we wanted file-local variables to be lowercase? ::: config/makefiles/autotargets.mk @@ +38,5 @@ > +.mkdir.done: > + @echo "WARNING: $(MKDIR) -dot- requested by $(MAKE) -C $(CURDIR) $(MAKECMDGOALS)" > + @$(TOUCH) $@ > + > +INCLUDED_AUTOTARGETS_MK = 1 Is there a reason this ifndef doesn't wrap the entire file? ::: ipc/app/Makefile.in @@ +47,5 @@ > include $(topsrcdir)/ipc/app/defs.mk > PROGRAM = $(MOZ_CHILD_PROCESS_NAME) > > +# path is dot ?!?: mkdir -p . > +GENERATED_DIRS = $(dir $(PROGRAM)) I believe this is here to handle this case: http://mxr.mozilla.org/mozilla-central/source/ipc/app/defs.mk#44 You should probably just special-case that here, like ifneq ($(dir $(PROGRAM)),./) GENERATED_DIRS = $(dir $(PROGRAM)) endif @@ +93,5 @@ > endif > > include $(topsrcdir)/config/rules.mk > > +ifeq ($(OS_ARCH),WINNT) #{ I mentioned this in a previous review. I don't mind cleanup happening in the lines you're touching, but mixing cleanup in random bits of files with useful changes makes it a lot harder to review. It'd be 100x easier to review a patch of actual changes + a separate patch of trivial cleanup. ::: mobile/android/app/Makefile.in @@ +42,5 @@ > > include $(DEPTH)/config/autoconf.mk > > DIRS = profile/extensions > +APP_DIR = $(DIST)/$(APP_NAME).app Same question here about lowercase filenames. Also, you should probably just use the same variable name you used in browser/app/Makefile.in, they serve the same purpose.

(In reply to Ted Mielczarek [:ted] from comment #3) > Comment on attachment 611048 [details] [diff] [review] > Makefile.in edits to use bug 680246 logic > > Review of attachment 611048 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/app/Makefile.in > @@ +46,5 @@ > > > > include $(DEPTH)/config/autoconf.mk > > > > +DIRS = profile/extensions > > +DIST_DEST = $(DIST)/$(MOZ_MACBUNDLE_NAME) > > Did we decide that we wanted file-local variables to be lowercase? Yea I'll have to go back and fix these. This patch was in progress before the lowercase naming convention came into play. > ::: ipc/app/Makefile.in > @@ +47,5 @@ > > include $(topsrcdir)/ipc/app/defs.mk > > PROGRAM = $(MOZ_CHILD_PROCESS_NAME) > > > > +# path is dot ?!?: mkdir -p . > > +GENERATED_DIRS = $(dir $(PROGRAM)) > > I believe this is here to handle this case: > http://mxr.mozilla.org/mozilla-central/source/ipc/app/defs.mk#44 > > You should probably just special-case that here, like > ifneq ($(dir $(PROGRAM)),./) > GENERATED_DIRS = $(dir $(PROGRAM)) > endif How about this answer/patch to avoid the special case: https://bugzilla.mozilla.org/show_bug.cgi?id=742835 If this case is expected in a few places an extra flag could be used to inhibit warning about it. > @@ +93,5 @@ > > endif > > > > include $(topsrcdir)/config/rules.mk > > > > +ifeq ($(OS_ARCH),WINNT) #{ > > I mentioned this in a previous review. I don't mind cleanup happening in the > lines you're touching, but mixing cleanup in random bits of files with > useful changes makes it a lot harder to review. It'd be 100x easier to > review a patch of actual changes + a separate patch of trivial cleanup. Will split this patch and resubmit

(In reply to Ted Mielczarek [:ted] from comment #3) > Comment on attachment 611048 [details] [diff] [review] > Makefile.in edits to use bug 680246 logic > > Review of attachment 611048 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: config/makefiles/autotargets.mk > @@ +38,5 @@ > > +.mkdir.done: > > + @echo "WARNING: $(MKDIR) -dot- requested by $(MAKE) -C $(CURDIR) $(MAKECMDGOALS)" > > + @$(TOUCH) $@ > > + > > +INCLUDED_AUTOTARGETS_MK = 1 > > Is there a reason this ifndef doesn't wrap the entire file? Mostly safety -- if for some reason 'GENERATED_DIRS' were defined multiple times between included files a full conditional wrap would only accumulate GENERATED_DIR_DEPS and GARBAGE_DIRS for the initial assignment prior to the first include of autotargets.

Inline mkdir -p calls replaced with mkdir_dep dependencies. Deps assigned to a ${target}-preqs local var to shorten target length. config/makefiles/autotargets.mk - moved INCLUDE_AUTOTARGETS_MK conditional to wrap more definitions made by the file. Vars that accumulate deps are defined outside the conditional block.

Try Job: https://tbpl.mozilla.org/?tree=Try&rev=8ab1accdbff0 linux debug and osx64 opt both reported problems with bug673812.js jit test: 741544, 743088, ...

ping on the code review

Comment on attachment 613571 [details] [diff] [review] bug related logic from earlier patch with cosmetic edits removed Review of attachment 613571 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/makefiles/autotargets.mk @@ +7,5 @@ > # > > +ifndef INCLUDED_AUTOTARGETS_MK #{ > + > +# Conditinal does not wrap the entire file so multiple "Conditional"

Fixed typo in 'Conditional'. Refresh patch against mozilla-central and resolved merge conflicts the other makefile/mkdir -p patches. r=ted

Comment on attachment 613571 [details] [diff] [review] bug related logic from earlier patch with cosmetic edits removed Review of attachment 613571 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/Makefile.in @@ +199,5 @@ > > +libs-preqs = \ > + $(call mkdir_deps,$(dist_dest)/Contents/MacOS) \ > + $(call mkdir_deps,$(dist_dest)/Contents/Resources/$(AB).lproj) \ > + $(NULL) I'm starting to wonder if it'd be worthwhile to add magic vars like "GENERATED_libs_DIRS / GENERATED_export_DIRS" etc. Food for thought. ::: config/makefiles/autotargets.mk @@ +7,5 @@ > # > > +ifndef INCLUDED_AUTOTARGETS_MK #{ > + > +# Conditinal does not wrap the entire file so multiple typo: "Conditional" ::: js/src/config/makefiles/autotargets.mk @@ +7,5 @@ > # > > +ifndef INCLUDED_AUTOTARGETS_MK #{ > + > +# Conditinal does not wrap the entire file so multiple You managed to copy-paste this typo quite a bit. :)

[sigh] sorry, 2nd patch attached to this bug contained the 'conditional' typo fix but the pending review status was not transferred to the new patch. Patch is mostly the same and will merge cleanly with the other patch that referenced conditional. A new bug was opened for deriving target specific vars in lieu of explicitly listing mkdir_deps as a pre-requisite for individual targets.