Last Comment Bug 739710 - Makefile.in update to use 680246 - file batch #2
: Makefile.in update to use 680246 - file batch #2
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Joey Armstrong [:joey]
:
Mentors:
Depends on: 680246 734121
Blocks: 734139 743280 750303
  Show dependency treegraph
 
Reported: 2012-03-27 11:24 PDT by Joey Armstrong [:joey]
Modified: 2012-05-04 03:58 PDT (History)
2 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Makefile.in edits to use bug 680246 logic (22.39 KB, patch)
2012-03-30 14:48 PDT, Joey Armstrong [:joey]
ted: review-
Details | Diff | Splinter Review
bug related logic from earlier patch with cosmetic edits removed (12.62 KB, patch)
2012-04-10 06:25 PDT, Joey Armstrong [:joey]
ted: review+
Details | Diff | Splinter Review
Makefile.in edits to use mkdir_deps function: file batch #2 (12.25 KB, patch)
2012-04-18 12:33 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2012-03-27 11:24:11 PDT
Makefile.in beneath:
browser/
ipc/
mobile/
tools/
testing/
Comment 1 Joey Armstrong [:joey] 2012-03-30 14:48:21 PDT
Created attachment 611048 [details] [diff] [review]
Makefile.in edits to use bug 680246 logic

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.
Comment 2 Joey Armstrong [:joey] 2012-04-05 10:17:00 PDT
ping on the code review
Comment 3 Ted Mielczarek [:ted.mielczarek] 2012-04-05 11:28:03 PDT
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.
Comment 4 Joey Armstrong [:joey] 2012-04-06 09:21:31 PDT
(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
Comment 5 Joey Armstrong [:joey] 2012-04-06 12:51:43 PDT
(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.
Comment 6 Joey Armstrong [:joey] 2012-04-10 06:25:27 PDT
Created attachment 613571 [details] [diff] [review]
bug related logic from earlier patch with cosmetic edits removed

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.
Comment 7 Joey Armstrong [:joey] 2012-04-10 06:32:06 PDT
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, ...
Comment 8 Joey Armstrong [:joey] 2012-04-13 09:39:38 PDT
ping on the code review
Comment 9 Ted Mielczarek [:ted.mielczarek] 2012-04-17 10:12:32 PDT
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"
Comment 10 Joey Armstrong [:joey] 2012-04-18 12:33:51 PDT
Created attachment 616248 [details] [diff] [review]
Makefile.in edits to use mkdir_deps function: file batch #2

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

r=ted
Comment 11 Joey Armstrong [:joey] 2012-04-19 05:29:57 PDT
https://tbpl.mozilla.org/?tree=Try&rev=d737cf5d585f
Comment 12 Ted Mielczarek [:ted.mielczarek] 2012-04-23 06:00:42 PDT
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. :)
Comment 13 Joey Armstrong [:joey] 2012-04-30 09:49:30 PDT
[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.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-05-03 03:58:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/054b137b9dc2
Comment 15 Ed Morley [:emorley] 2012-05-04 03:58:23 PDT
https://hg.mozilla.org/mozilla-central/rev/054b137b9dc2

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