The default bug view has changed. See this FAQ.

Loop over DIRS better

RESOLVED FIXED in 4.8.3

Status

NSPR
NSPR
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: bsmedberg, Assigned: bsmedberg)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Created attachment 409077 [details] [diff] [review]
Use make syntax, not shell syntax, rev. 1

Currently NSPR loops over DIRS using a shell forloop. Using make gives you better performance, especially when using pymake.
Attachment #409077 - Flags: review?(ted.mielczarek)
Blocks: 485412
Comment on attachment 409077 [details] [diff] [review]
Use make syntax, not shell syntax, rev. 1

+++ b/nsprpub/config/rules.mk
@@ -1,8 +1,9 @@
+$(warning bindir: $(bindir))

You missed extra debug goop.

This does change the behavior slightly, in that it will fail on non-directories in DIRS, but that seems like a silly thing to care about. Attach an updated patch and I'll land it for you in CVS.
Attachment #409077 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 2

8 years ago
Created attachment 411226 [details] [diff] [review]
Use make syntax, not shell syntax, rev. 1.1
Attachment #409077 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Assignee: benjamin → ted.mielczarek
Checking in config/rules.mk;
/cvsroot/mozilla/nsprpub/config/rules.mk,v  <--  rules.mk
new revision: 3.74; previous revision: 3.73
done
Assignee: ted.mielczarek → benjamin
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 4

8 years ago
Comment on attachment 411226 [details] [diff] [review]
Use make syntax, not shell syntax, rev. 1.1

Thanks for the patch.

>+define MAKE_DIR
>+	$(MAKE) -C $(dir) $@
>+
>+endef

Nit: MAKE_DIR reminds me of the mkdir command and system call
for creating a directory.  It's a little confusing.

May want to add a comment to explain the blank line.  I removed
it and broke the build :-)

>+LOOP_OVER_DIRS = $(foreach dir,$(wildcard $(DIRS)),$(MAKE_DIR))

Please remove the wildcard function.  Did you add it to match
the original shell code, which expands wildcards implicitly?
We don't use wildcards in our DIRS variables, so we don't need
to expand wildcards.

Updated

8 years ago
OS: Windows NT → All
Hardware: x86 → All
Target Milestone: --- → 4.8.3
(In reply to comment #4)
> Nit: MAKE_DIR reminds me of the mkdir command and system call
> for creating a directory.  It's a little confusing.

I renamed this to MAKE_IN_DIR.
 
> May want to add a comment to explain the blank line.  I removed
> it and broke the build :-)

Added. Sorry, we're using this trick a lot in the build system now (although usually we add a comment for that reason). 

> Please remove the wildcard function.  Did you add it to match
> the original shell code, which expands wildcards implicitly?
> We don't use wildcards in our DIRS variables, so we don't need
> to expand wildcards.

I think his intent was to match part of the "test -d" behavior of the old version, but it seems like overkill. (wildcard will omit nonexistent files/dirs from its expansion.)

Checked in these fixes:
Checking in config/rules.mk;
/cvsroot/mozilla/nsprpub/config/rules.mk,v  <--  rules.mk
new revision: 3.75; previous revision: 3.74
done
> (wildcard will omit nonexistent files/dirs from its expansion.)
Yeah, I thought wildcard was a good idea for that very reason.
(Assignee)

Comment 7

8 years ago
Why would you want to silently skip directories listed in DIRS which don't exist? I prefer the not-wildcard version; I just wrote the patch this way to preserve the existing behavior as much as possible.
So that I can checkout and build subsets of the tree.

Comment 9

8 years ago
For NSPR at least, we don't need wildcard.  Let's keep it simple.

Are you going to fix LOOP_OVER_DIRS in mozilla/security/coreconf?
(Assignee)

Comment 10

8 years ago
I have no plans to at the present time. I'll deal with NSS in a separate bug, if at all.
Benjamin, 
I suspect that Wan-Teh's undirected question in comment 9 was intended for me.

Wan-Teh,
Yes, I have been considering this for NSS.  Reducing the number of processes
required to traverse the tree while building has a BIG effect on windows.
I explored this in bug 435923 comment 18.
You need to log in before you can comment on or make changes to this bug.