Closed
Bug 466486
Opened 16 years ago
Closed 16 years ago
Don't use a subshell to recurse over DIRS when DIRS is empty
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.1
People
(Reporter: benjamin, Assigned: benjamin)
References
(Blocks 1 open bug)
Details
(Keywords: fixed1.9.1)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
ted
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
While researching build time, I discovered that we're using a subshell to recurse over subdirectories even when there aren't any. This is wasteful, especially on Windows where processes cost a lot.
Attachment #349760 -
Flags: review?(ted.mielczarek)
Comment 1•16 years ago
|
||
Comment on attachment 349760 [details] [diff] [review] Avoid shells for recursing empty DIRS, rev. 1 Do we ever wind up with DIRS set to an empty space? Couldn't we just use |ifdef DIRS| here? Also, amazing what silliness you can find with a little research!
Attachment #349760 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 2•16 years ago
|
||
I found at least one case with something like: DIRS += \ $(EMPTYVALUE) \ $(NULL) oh, for make variables that are lists and not scalars ;-)
Assignee | ||
Comment 3•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/f71446b6fc7e
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Attachment #349760 -
Flags: approval1.9.1?
Assignee | ||
Comment 5•16 years ago
|
||
backed out... weird regressions with some directories not getting built
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•16 years ago
|
Attachment #349760 -
Flags: approval1.9.1?
Comment 6•16 years ago
|
||
Comment on attachment 349760 [details] [diff] [review] Avoid shells for recursing empty DIRS, rev. 1 >- @$(EXIT_ON_ERROR) \ >- $(foreach dir,$(STATIC_DIRS),$(MAKE) -C $(dir); ) true >+ +$(LOOP_OVER_STATIC_DIRS) >-ifdef TOOL_DIRS >- @$(EXIT_ON_ERROR) \ >- $(foreach dir,$(TOOL_DIRS),$(UPDATE_TITLE) $(MAKE) -C $(dir) libs; ) true >-endif >+ +$(LOOP_OVER_TOOL_DIRS) Unfortunately $(LOOP_OVER_XXX_DIRS) uses $@ and so these aren't the same...
Assignee | ||
Comment 7•16 years ago
|
||
In this version, you aren't allowed to modify DIRS after you've included rules.mk... it would have already caused problems, such as missing submakefile dependencies. Unfortunately I can't think of a good way to enforce this rule, but oh well... This also fixes misuse of LOOP_OVER_STATIC_DIRS and LOOP_OVER_TOOL_DIRS
Attachment #349760 -
Attachment is obsolete: true
Attachment #353502 -
Flags: review?(ted.mielczarek)
Comment 8•16 years ago
|
||
Looks like both browser/Makefile.in and mail/Makefile.in muck with DIRS after including rules.mk.
Assignee | ||
Comment 9•16 years ago
|
||
That turns out not to be a problem in practice (because DIRS was already set to something), but I've patched it.
Attachment #353502 -
Attachment is obsolete: true
Attachment #353702 -
Flags: review?(ted.mielczarek)
Attachment #353502 -
Flags: review?(ted.mielczarek)
Updated•16 years ago
|
Attachment #353702 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 10•16 years ago
|
||
Second time's the charm: http://hg.mozilla.org/mozilla-central/rev/d11ccfc1d754
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Attachment #353702 -
Flags: approval1.9.1?
Comment 11•16 years ago
|
||
Comment on attachment 353702 [details] [diff] [review] Avoid shells for recursing empty DIRS, rev. 2.1 a191=beltzner
Attachment #353702 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Comment 12•16 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/758a1804c901
Keywords: fixed1.9.1
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•