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

RESOLVED FIXED in mozilla9

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: joey, Assigned: joey)

Tracking

Trunk
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Updated

6 years ago
Assignee: nobody → joey
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?
(Assignee)

Comment 2

6 years ago
(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.
Also pymake's parsing is slow, so less stuff to parse may be a decent win on Windows.
Parsing the common files isn't a big deal, because it's cached.
(Assignee)

Comment 5

6 years ago
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).
Attachment #538548 - Flags: review?(khuey)
Attachment #538548 - Flags: review?(benjamin)

Updated

6 years ago
Attachment #538548 - Flags: review?(khuey)
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.
Attachment #538548 - Flags: review?(benjamin)
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
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
Attachment #538548 - Attachment is obsolete: true
Attachment #538548 - Flags: review?(khuey)
Attachment #539871 - Flags: superreview?(benjamin)
Attachment #539871 - Flags: review?(khuey)
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.
Attachment #539871 - Flags: review?(khuey) → review?(ted.mielczarek)
Attachment #539871 - Flags: superreview?(benjamin)
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...
Attachment #539871 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Updated

6 years ago
Blocks: 668861
(Assignee)

Comment 12

6 years ago
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.
Attachment #539871 - Attachment is obsolete: true
Attachment #544500 - Flags: review?(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.
Attachment #544500 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 14

6 years ago
Created attachment 547500 [details] [diff] [review]
split config/rules.mk

nit fix: updated copyright notices in config/makefiles/target_*.mk
Attachment #544500 - Attachment is obsolete: true
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...
(Assignee)

Comment 16

6 years ago
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}
Attachment #547500 - Attachment is obsolete: true
Attachment #553289 - Flags: review?(ted.mielczarek)
Comment on attachment 553289 [details] [diff] [review]
split rules.mk

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

Thanks!
Attachment #553289 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
I'll get this landed to mozilla-inbound for Joey.
(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

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla9
Version: unspecified → Trunk
http://hg.mozilla.org/mozilla-central/rev/ca5e5bb18c92
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 742391
(Assignee)

Updated

5 years ago
No longer blocks: 742391
You need to log in before you can comment on or make changes to this bug.