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)

defect
Not set
minor

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)

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 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+
I found at least one case with something like:

DIRS += \
  $(EMPTYVALUE) \
  $(NULL)

oh, for make variables that are lists and not scalars ;-)
http://hg.mozilla.org/mozilla-central/rev/f71446b6fc7e
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attachment #349760 - Flags: approval1.9.1?
this broke x86_64/linux builds
backed out... weird regressions with some directories not getting built
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #349760 - Flags: approval1.9.1?
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...
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)
Looks like both browser/Makefile.in and mail/Makefile.in muck with DIRS after including rules.mk.
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)
Depends on: 470320
Attachment #353702 - Flags: review?(ted.mielczarek) → review+
Second time's the charm: http://hg.mozilla.org/mozilla-central/rev/d11ccfc1d754
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Attachment #353702 - Flags: approval1.9.1?
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+
Blocks: 475111
Blocks: 476149
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.