Last Comment Bug 738404 - Makefile.in update to use 680246 - file batch #1
: Makefile.in update to use 680246 - file batch #1
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Joey Armstrong [:joey]
:
:
Mentors:
Depends on: 680246 734121 744750
Blocks: 734139 746151
  Show dependency treegraph
 
Reported: 2012-03-22 12:30 PDT by Joey Armstrong [:joey]
Modified: 2012-04-17 07:53 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Makefile edits to begin using threadsafe mkdir logic from bug 680246 (28.90 KB, patch)
2012-03-22 13:36 PDT, Joey Armstrong [:joey]
ted: review-
Details | Diff | Splinter Review
Makefile.in edits to use bug 680246 functionality (29.15 KB, patch)
2012-03-28 10:03 PDT, Joey Armstrong [:joey]
ted: review+
Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2012-03-22 12:30:15 PDT

    
Comment 1 Joey Armstrong [:joey] 2012-03-22 13:36:10 PDT
Created attachment 608452 [details] [diff] [review]
Makefile edits to begin using threadsafe mkdir logic from bug 680246

Target rule mkdir calls replaced with $(call mkdir_deps) dependencies [~bug 680246].
Cosmetic cleanups: replaced rm -f with $(RM) in a few makefiles.
config/config.mk - fixed a quoting problem affecting editor colorization.
Comment 2 Joey Armstrong [:joey] 2012-03-22 13:39:46 PDT
Try job + patches: 680246, 7634121 & 735638
===========================================================
Thanks for your try submission (http://hg.mozilla.org/try/pushloghtml?changeset=22a3e5ba61cd).  It's the best!

Watch https://tbpl.mozilla.org/?tree=Try&rev=22a3e5ba61cd 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-22a3e5ba61cd.

This directory won't be created until the first builds are uploaded, so please be patient.
Comment 3 Ted Mielczarek [:ted.mielczarek] 2012-03-26 13:01:03 PDT
Comment on attachment 608452 [details] [diff] [review]
Makefile edits to begin using threadsafe mkdir logic from bug 680246

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

Overall I'm fine with this patch, I'm just r-ing to get answers to the first two questions here.

::: b2g/app/Makefile.in
@@ +130,5 @@
> +.PHONY: repackage
> +libs repackage:: \
> +  $(call mkdir_deps,$(DIST)/$(APP_NAME).app/Contents/MacOS) \
> +  $(call mkdir_deps,$(DIST)/$(APP_NAME).app/Contents/Resources/$(AB).lproj) \
> +  $(NULL)

The formatting on these dependencies come off a little confusing. Is there a way to do this that doesn't require the line continuations after the ::? Maybe use an intermediate variable and list it as the deps?

::: build/win32/Makefile.in
@@ +102,5 @@
>  endif
>  
>  ifdef REDIST_FILES
> +libs:: \
> +  $(call mkdir_deps,$(FINAL_TARGET)) \

Is there a reason you're using these $(call mkdir_deps) directly instead of just using GENERATED_DIRS and GENERATED_DIR_DEPS?

::: config/Makefile.in
@@ +192,5 @@
>    unit-writemozinfo.py \
>    $(NULL)
>  
> +check:: \
> +  check-python-modules check-jar-mn \

Might as well reformat this to one-dependent-target-per-line while you're here.

::: config/config.mk
@@ +45,5 @@
>  #
>  
>  # Define an include-at-most-once flag
>  ifdef INCLUDED_CONFIG_MK
> +$(error Do not include config.mk twice!)

I'm not opposed to lumping cleanup in a patch, but it does make it easier to review if you localize your cleanup to the places you're already touching, FWIW.

::: config/rules.mk
@@ +480,5 @@
>  # A Makefile that needs $(MDDEPDIR) created but doesn't set any of these
>  # variables we know to check can just set NEED_MDDEPDIR explicitly.
>  ifneq (,$(OBJS)$(XPIDLSRCS)$(SIMPLE_PROGRAMS)$(NEED_MDDEPDIR))
>  MAKE_DIRS		+= $(CURDIR)/$(MDDEPDIR)
> +GARBAGE_DIRS    += $(MDDEPDIR)

This doesn't seem to be a useful change.
Comment 4 Joey Armstrong [:joey] 2012-03-26 14:47:13 PDT
(In reply to Ted Mielczarek [:ted] from comment #3)
> Comment on attachment 608452 [details] [diff] [review]
> Makefile edits to begin using threadsafe mkdir logic from bug 680246
> 
> Review of attachment 608452 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall I'm fine with this patch, I'm just r-ing to get answers to the first
> two questions here.
> 
> ::: b2g/app/Makefile.in
> @@ +130,5 @@
> > +.PHONY: repackage
> > +libs repackage:: \
> > +  $(call mkdir_deps,$(DIST)/$(APP_NAME).app/Contents/MacOS) \
> > +  $(call mkdir_deps,$(DIST)/$(APP_NAME).app/Contents/Resources/$(AB).lproj) \
> > +  $(NULL)
> 
> The formatting on these dependencies come off a little confusing. Is there a
> way to do this that doesn't require the line continuations after the ::?
> Maybe use an intermediate variable and list it as the deps?


Formatting was intended to fit 80cpl and avoid line wrap (the coding style guide also mentioned something about it).
target: offset 0, pre-reqs: indent 2 spaces, cmds: tab indent

Yes vars could be used to shorten the target line.  The reason I opted to not use them here was because of scope.  Var use would morph the value from local use to global which could become a problem source.  If the macro is overwritten or conflicts with one of the included makefiles.


> ::: build/win32/Makefile.in
> @@ +102,5 @@
> >  endif
> >  
> >  ifdef REDIST_FILES
> > +libs:: \
> > +  $(call mkdir_deps,$(FINAL_TARGET)) \
> 
> Is there a reason you're using these $(call mkdir_deps) directly instead of
> just using GENERATED_DIRS and GENERATED_DIR_DEPS?

Yes, this syntax is intended to limit dep checking/generation to only targets that depend on it.  If several targets require the dep ( export, libs, tools, check, ...) assigning to GENERATED_DIRS would make perfect sense.  If 'install' were the only target requiring a dep it would not make sense for 'make check', 'make export', ...  to also generate the dep.

> 
> ::: config/Makefile.in
> @@ +192,5 @@
> >    unit-writemozinfo.py \
> >    $(NULL)
> >  
> > +check:: \
> > +  check-python-modules check-jar-mn \
> 
> Might as well reformat this to one-dependent-target-per-line while you're
> here.

Ok with the next patch upload

 
> ::: config/config.mk
> @@ +45,5 @@
> >  #
> >  
> >  # Define an include-at-most-once flag
> >  ifdef INCLUDED_CONFIG_MK
> > +$(error Do not include config.mk twice!)
> 
> I'm not opposed to lumping cleanup in a patch, but it does make it easier to
> review if you localize your cleanup to the places you're already touching,
> FWIW.

It would be easy enough to pull this edit out of the patch.  config/config.mk will be submitted with other edits and could be submitted with an appropriate bug.

 
> ::: config/rules.mk
> @@ +480,5 @@
> >  # A Makefile that needs $(MDDEPDIR) created but doesn't set any of these
> >  # variables we know to check can just set NEED_MDDEPDIR explicitly.
> >  ifneq (,$(OBJS)$(XPIDLSRCS)$(SIMPLE_PROGRAMS)$(NEED_MDDEPDIR))
> >  MAKE_DIRS		+= $(CURDIR)/$(MDDEPDIR)
> > +GARBAGE_DIRS    += $(MDDEPDIR)
> 
> This doesn't seem to be a useful change.

Ugh yes, wip
Comment 5 Ted Mielczarek [:ted.mielczarek] 2012-03-27 05:38:46 PDT
(In reply to Joey Armstrong [:joey] from comment #4)
> Formatting was intended to fit 80cpl and avoid line wrap (the coding style
> guide also mentioned something about it).
> target: offset 0, pre-reqs: indent 2 spaces, cmds: tab indent

Right, wrapping at 80 columns is fine, I'm just really not a fan of having a line continuation in the prerequisite portion of a rule with a recipe. It makes it really confusing to read.

> Yes vars could be used to shorten the target line.  The reason I opted to
> not use them here was because of scope.  Var use would morph the value from
> local use to global which could become a problem source.  If the macro is
> overwritten or conflicts with one of the included makefiles.

Ah, yes. This is a fair concern, but I think I would prefer this to avoid the ugly formatting. Perhaps we could come up with variable naming conventions that help us avoid conflicts? Someone suggested perhaps using lowercase variable names for non-global variables.

> Yes, this syntax is intended to limit dep checking/generation to only
> targets that depend on it.  If several targets require the dep ( export,
> libs, tools, check, ...) assigning to GENERATED_DIRS would make perfect
> sense.  If 'install' were the only target requiring a dep it would not make
> sense for 'make check', 'make export', ...  to also generate the dep.

Okay. It's a bit of a pain, but specifying our dependencies more clearly is a sane goal.

I didn't have any other issues with this patch, so if you can tweak that one issue it should be an easy r+.
Comment 6 Joey Armstrong [:joey] 2012-03-27 13:52:30 PDT
(In reply to Ted Mielczarek [:ted] from comment #5)
> Right, wrapping at 80 columns is fine, I'm just really not a fan of having a line continuation in the prerequisite portion of a rule with a recipe. It makes it really confusing to read.
> 
> Ah, yes. This is a fair concern, but I think I would prefer this to avoid
> the ugly formatting. Perhaps we could come up with variable naming
> conventions that help us avoid conflicts? Someone suggested perhaps using lowercase variable names for non-global variables.

Lowercase var names for local vars would be a reasonable suggestion.

How about this for a thought/convention (lines>80cpl):
 o Declare a local macro above the rule suffixed by -preqs
 o Accumulate deps, one per line appended with +=
 o Target rule(s) could then be written as:

export-preqs += foo
export-preqs += $(call mkdir_deps,bar tans fans)
export: $(export-preqs)

> I didn't have any other issues with this patch, so if you can tweak that one issue it should be an easy r+.

Ok thanks
Comment 7 Ted Mielczarek [:ted.mielczarek] 2012-03-28 04:59:23 PDT
Your suggestion sounds totally reasonable. An alternate that would match what we do in a lot of Makefiles would be to use continuations in the variable assignment, like:
export-preqs = \
  foo
  $(call mkdir_deps,bar tans fans) \
  $(NULL)

I don't have a problem with continuations in general, just in the prereqs section of a rule with a recipe.

Either way is fine, you can choose.
Comment 8 Joey Armstrong [:joey] 2012-03-28 10:03:51 PDT
Created attachment 610175 [details] [diff] [review]
Makefile.in edits to use bug 680246 functionality

Target line continuation for pre-requisites moved to named macros {export,libs}-preqs.

Common path macros defined in a few places to shorten command paths

GARBAGE_DIRS += $(MDDEPDIR) was visible due to a whitespace change, updated removal path.
Comment 9 Joey Armstrong [:joey] 2012-03-28 10:05:39 PDT
Try job: https://tbpl.mozilla.org/?tree=Try&rev=3b1e4f9bd98d

b2g latex2man failure is known, will be fixed by "dont_build_docs.sh"
Comment 10 Ted Mielczarek [:ted.mielczarek] 2012-03-28 12:39:25 PDT
Comment on attachment 610175 [details] [diff] [review]
Makefile.in edits to use bug 680246 functionality

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

This looks great, thanks!
Comment 11 Chris Cooper [:coop] 2012-04-03 13:09:59 PDT
Comment on attachment 610175 [details] [diff] [review]
Makefile.in edits to use bug 680246 functionality

https://hg.mozilla.org/projects/build-system/rev/1f093ab9df77
Comment 12 Joey Armstrong [:joey] 2012-04-05 10:15:44 PDT
Patch landed on mozilla-central yesterday by Coop & Philor:
https://hg.mozilla.org/mozilla-central/rev/c598b7b202e7

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