662833 - makefiles: split of config/rules.mk into a hierarchy of makefiles

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Joey Armstrong [:joey]]

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]

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]]

(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 (Exited; not receiving bugmail, old account, do not use)]

Also pymake's parsing is slow, so less stuff to parse may be a decent win on Windows.

Comment 4

[Benjamin Smedberg]

Parsing the common files isn't a big deal, because it's cached.

Comment 5

[Joey Armstrong [:joey]]

Attachment: 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]

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]]

(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]]

Attachment: 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 (Exited; not receiving bugmail, old account, do not use)]

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

[(not currently active) Ted Mielczarek]

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]]

(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]]

Attachment: 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

[(not currently active) Ted Mielczarek]

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]]

Attachment: split config/rules.mk

nit fix: updated copyright notices in config/makefiles/target_*.mk

Comment 15

[(not currently active) Ted Mielczarek]

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]]

Attachment: 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

[(not currently active) Ted Mielczarek]

Comment on attachment 553289 [details] [diff] [review]
split rules.mk

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

Thanks!

Comment 18

[Chris Cooper [:coop] (he/him)]

I'll get this landed to mozilla-inbound for Joey.

Comment 19

[Chris Cooper [:coop] (he/him)]

(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]]

http://hg.mozilla.org/mozilla-central/rev/ca5e5bb18c92