Closed Bug 738404 Opened 12 years ago Closed 12 years ago

Makefile.in update to use 680246 - file batch #1

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: joey, Assigned: joey)

References

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Assignee: nobody → joey
Blocks: 734139
Depends on: 680246, 734121
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.
Attachment #608452 - Flags: review?(ted.mielczarek)
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 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.
Attachment #608452 - Flags: review?(ted.mielczarek) → review-
(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
(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+.
(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
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.
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.
Attachment #608452 - Attachment is obsolete: true
Attachment #610175 - Flags: review?(ted.mielczarek)
Try job: https://tbpl.mozilla.org/?tree=Try&rev=3b1e4f9bd98d

b2g latex2man failure is known, will be fixed by "dont_build_docs.sh"
Attachment #610175 - Attachment is patch: true
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!
Attachment #610175 - Flags: review?(ted.mielczarek) → review+
Patch landed on mozilla-central yesterday by Coop & Philor:
https://hg.mozilla.org/mozilla-central/rev/c598b7b202e7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 746151
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.