Last Comment Bug 662833 - makefiles: split of config/rules.mk into a hierarchy of makefiles
: makefiles: split of config/rules.mk into a hierarchy of makefiles
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Joey Armstrong [:joey]
:
:
Mentors:
Depends on:
Blocks: 668861
  Show dependency treegraph
 
Reported: 2011-06-08 09:57 PDT by Joey Armstrong [:joey]
Modified: 2012-04-04 08:34 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
export, libs & tools targets moved to config/makefiles/common/${target}.mk (23.38 KB, patch)
2011-06-10 10:32 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
export, libs & tools targets moved to config/makefiles/common/${target}.mk (18.24 KB, patch)
2011-06-16 12:37 PDT, Joey Armstrong [:joey]
ted: review+
Details | Diff | Splinter Review
patch nit fixes (17.66 KB, patch)
2011-07-07 08:37 PDT, Joey Armstrong [:joey]
ted: review+
Details | Diff | Splinter Review
split config/rules.mk (17.62 KB, patch)
2011-07-21 13:51 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
split rules.mk (35.18 KB, patch)
2011-08-15 15:44 PDT, Joey Armstrong [:joey]
ted: review+
Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2011-06-08 09:57:30 PDT
config/rules.mk can be split into a hierarchy of makefiles that are conditionally included and driven by context.  A lot of parsing and macro population occurs on each pass, even when targets are not active.

[new] mozilla-central/config/makefiles/makefile.common.{export,lib,tools}

Common targets: tools, export & lib are all good candidates for moving into a new makefile.

Their targets, deps and aggregate dep lists (both serial and parallel) can be moved in bulk into the new makefile.

Conditional inclusion can then be driven by targets present in MAKECMDGOALS.

ifneq ($(null),$(findstring ->[TGT]<-,$(MAKECMDGOALS)))
  include $(topsrcdir)/config/makefiles/makefile.common.->[TGT]<-
endif
Comment 1 Benjamin Smedberg [:bsmedberg] 2011-06-08 10:01:02 PDT
I love splitting the files for readability and maintainability, but how does this improve performance? Is it the parsing which costs, or make having to traverse the rule tree?
Comment 2 Joey Armstrong [:joey] 2011-06-08 11:03:59 PDT
(In reply to comment #1)
> I love splitting the files for readability and maintainability, but how does
> this improve performance? Is it the parsing which costs, or make having to
> traverse the rule tree?

Both would be gains, parsing benefits may be minimal but there will be less for make to keep track of and check in general.  One item that probably should have been mentioned with this is bug #623617.  With non-recursive calls, make will no longer need to process a common target like 'tools' throughout the sandbox, it will only need to be processed within directories where the target is present.
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-08 11:04:54 PDT
Also pymake's parsing is slow, so less stuff to parse may be a decent win on Windows.
Comment 4 Benjamin Smedberg [:bsmedberg] 2011-06-08 12:18:15 PDT
Parsing the common files isn't a big deal, because it's cached.
Comment 5 Joey Armstrong [:joey] 2011-06-10 10:32:17 PDT
Created attachment 538548 [details] [diff] [review]
export, libs & tools targets moved to config/makefiles/common/${target}.mk

Edits to isolate serial/parallel logic for export, libs and tools into named makefiles.  Directory exclusion macros added to support filtering directories during recursive traversal (bug: 623617).
Comment 6 Benjamin Smedberg [:bsmedberg] 2011-06-13 09:44:18 PDT
Comment on attachment 538548 [details] [diff] [review]
export, libs & tools targets moved to config/makefiles/common/${target}.mk

I'm a bit confused: you moved the recursive bits of the export phase into another makefile, but not the "actual" rules that go with the export phase, especially all the XPIDL goop. You moved the recursion and copying bits of the libs phase, but not all of the compile and linking rules which are the meat of the libs phase.

Our standard practice in mozilla makefiles is $(NULL), not $(null).

I'm still a little leery of using MAKECMDGOALS in general: in this case it's ok because we know that export/libs/tools are not the default target (MAKECMDGOALS is empty if you just issue "make"). I'd really like data to show that this actually improves build times before applying the optimization. I'd ideally like us to start out by unconditionally including export/libs/tools, and then add the optimization as a second step.

Is the moved code identical to the code that was removed in rules.mk?

Clearing review pending questions, please renominate if I missed something.
Comment 7 Joey Armstrong [:joey] 2011-06-13 12:50:16 PDT
(In reply to comment #6)
> Comment on attachment 538548 [details] [diff] [review] [review]
> export, libs & tools targets moved to config/makefiles/common/${target}.mk
> 
> I'm a bit confused: you moved the recursive bits of the export phase into
> another makefile, but not the "actual" rules that go with the export phase,
> especially all the XPIDL goop. You moved the recursion and copying bits of
> the libs phase, but not all of the compile and linking rules which are the
> meat of the libs phase.

The edits were being made in small increments to minimize impact and to try and avoid unintended side effects.  Isolating recursive elements for export,libs & tools { and adding exclusions } were changes made in this patch.  The other targets used for compiling/linking, etc can all be moved I've just not dissected them yet enough to know what can be.

> Our standard practice in mozilla makefiles is $(NULL), not $(null).
> 
> I'm still a little leery of using MAKECMDGOALS in general: in this case it's
> ok because we know that export/libs/tools are not the default target
> (MAKECMDGOALS is empty if you just issue "make"). I'd really like data to
> show that this actually improves build times before applying the
> optimization. I'd ideally like us to start out by unconditionally including
> export/libs/tools, and then add the optimization as a second step.

Ok, I'll look into gathering some stats.  Edits from the "container makefile" ticket should be included then, gains will come from the ability to exclude directories by target during a recursive traversal.

> Is the moved code identical to the code that was removed in rules.mk?

Mostly, the addition of directory exclusion macros were the only change.
MD_EXCLUDE* macros have been added but they are currently a nop.

> Clearing review pending questions, please renominate if I missed something.
Comment 8 Joey Armstrong [:joey] 2011-06-16 12:37:25 PDT
Created attachment 539871 [details] [diff] [review]
export, libs & tools targets moved to config/makefiles/common/${target}.mk

First pass: Remove MAKECMDGOALS conditionals.  Only move the basic serial/parallel export, libs & tools targets into config/makefiles/${tgt}.mk
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-17 08:50:50 PDT
Comment on attachment 539871 [details] [diff] [review]
export, libs & tools targets moved to config/makefiles/common/${target}.mk

Punting this to ted, since he's the module owner, he should have the final say here.
Comment 10 Ted Mielczarek [:ted.mielczarek] 2011-07-01 11:04:57 PDT
Comment on attachment 539871 [details] [diff] [review]
export, libs & tools targets moved to config/makefiles/common/${target}.mk

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

I am totally sold on the concept here, rules.mk is way too large to keep in my brain. I'd echo Benjamin's comments though, that moving part of the libs:: etc rules out without the rest doesn't seem right. If you're planning on doing that in a follow-up, please file the follow-up bug before this lands so we at least have something to point at.

::: config/rules.mk
@@ +720,5 @@
>  	$(LOOP_OVER_TOOL_DIRS)
>  endif
>  
> +include $(topsrcdir)/config/makefiles/common/export.mk
> +include $(topsrcdir)/config/makefiles/common/tools.mk

I think you've gone a little too deep on the hierarchy. We already have makefiles in config, it seems like config/{export,libs,tools}.mk would be fine. At most, one new directory under config, not two, please.

@@ +1976,5 @@
>  
>  default all::
>  	if test -d $(DIST)/bin ; then touch $(DIST)/bin/.purgecaches ; fi
> +
> +# EOF

I don't think this adds any value, TBH. It's pretty clear when you're at the end of a file...
Comment 11 Joey Armstrong [:joey] 2011-07-01 11:14:28 PDT
(In reply to comment #10)
> > +# EOF
> 
> I don't think this adds any value, TBH. It's pretty clear when you're at the
> end of a file...

[fyi] for file parsing this helps to avoid problems when the last line of a file is missing a newline.
Comment 12 Joey Armstrong [:joey] 2011-07-07 08:37:27 PDT
Created attachment 544500 [details] [diff] [review]
patch nit fixes

Renamed config/makefiles/common/${foo}.mk to config/makefiles/target_${foo}.mk and updated include statements.

re-merged with trunk and copied recent libs:: target changes into target_libs.mk.
Comment 13 Ted Mielczarek [:ted.mielczarek] 2011-07-08 11:20:21 PDT
Comment on attachment 544500 [details] [diff] [review]
patch nit fixes

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

::: config/makefiles/target_export.mk
@@ +18,5 @@
> +#
> +# The Initial Developer of the Original Code is
> +# Netscape Communications Corporation.
> +# Portions created by the Initial Developer are Copyright (C) 1998
> +# the Initial Developer. All Rights Reserved.

Maybe I missed this last time, but can you update this copyright statement since they're new files? The Initial Developer should be "The Mozilla Foundation", and the copyright date should be 2011.

No need to re-request review, just attach an updated patch.
Comment 14 Joey Armstrong [:joey] 2011-07-21 13:51:41 PDT
Created attachment 547500 [details] [diff] [review]
split config/rules.mk

nit fix: updated copyright notices in config/makefiles/target_*.mk
Comment 15 Ted Mielczarek [:ted.mielczarek] 2011-08-09 07:19:21 PDT
Oh, one thing I missed here, you'll need to make the same changes in js/src/config, otherwise the check-sync-dirs check will fail...
Comment 16 Joey Armstrong [:joey] 2011-08-15 15:44:24 PDT
Created attachment 553289 [details] [diff] [review]
split rules.mk

Merged patch with the latest head.
Propagated rules.mk and makefiles/* edits into js/src/config
Try submission reports unrelated warnings {jetpack, deprecated functions, etc}
Comment 17 Ted Mielczarek [:ted.mielczarek] 2011-08-23 12:41:16 PDT
Comment on attachment 553289 [details] [diff] [review]
split rules.mk

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

Thanks!
Comment 18 Chris Cooper [:coop] 2011-08-30 09:13:31 PDT
I'll get this landed to mozilla-inbound for Joey.
Comment 19 Chris Cooper [:coop] 2011-08-30 09:14:45 PDT
(In reply to Chris Cooper [:coop] from comment #18)
> I'll get this landed to mozilla-inbound for Joey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca5e5bb18c92
Comment 20 Marco Bonardo [::mak] 2011-08-31 02:02:51 PDT
http://hg.mozilla.org/mozilla-central/rev/ca5e5bb18c92

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