Last Comment Bug 525221 - Loop over DIRS better
: Loop over DIRS better
Status: RESOLVED FIXED
:
Product: NSPR
Classification: Components
Component: NSPR (show other bugs)
: other
: All All
: -- normal (vote)
: 4.8.3
Assigned To: Benjamin Smedberg [:bsmedberg]
:
Mentors:
Depends on:
Blocks: 485412
  Show dependency treegraph
 
Reported: 2009-10-29 06:49 PDT by Benjamin Smedberg [:bsmedberg]
Modified: 2009-11-11 21:18 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Use make syntax, not shell syntax, rev. 1 (1.21 KB, patch)
2009-10-29 06:49 PDT, Benjamin Smedberg [:bsmedberg]
ted: review+
Details | Diff | Review
Use make syntax, not shell syntax, rev. 1.1 (942 bytes, patch)
2009-11-09 10:47 PST, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Review

Description Benjamin Smedberg [:bsmedberg] 2009-10-29 06:49:41 PDT
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.
Comment 1 Ted Mielczarek [:ted.mielczarek] 2009-10-29 11:30:53 PDT
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.
Comment 2 Benjamin Smedberg [:bsmedberg] 2009-11-09 10:47:58 PST
Created attachment 411226 [details] [diff] [review]
Use make syntax, not shell syntax, rev. 1.1
Comment 3 Ted Mielczarek [:ted.mielczarek] 2009-11-11 03:47:05 PST
Checking in config/rules.mk;
/cvsroot/mozilla/nsprpub/config/rules.mk,v  <--  rules.mk
new revision: 3.74; previous revision: 3.73
done
Comment 4 Wan-Teh Chang 2009-11-11 09:13:43 PST
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.
Comment 5 Ted Mielczarek [:ted.mielczarek] 2009-11-11 09:29:43 PST
(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
Comment 6 Nelson Bolyard (seldom reads bugmail) 2009-11-11 10:07:02 PST
> (wildcard will omit nonexistent files/dirs from its expansion.)
Yeah, I thought wildcard was a good idea for that very reason.
Comment 7 Benjamin Smedberg [:bsmedberg] 2009-11-11 10:13:15 PST
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.
Comment 8 Nelson Bolyard (seldom reads bugmail) 2009-11-11 10:18:02 PST
So that I can checkout and build subsets of the tree.
Comment 9 Wan-Teh Chang 2009-11-11 14:18:00 PST
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?
Comment 10 Benjamin Smedberg [:bsmedberg] 2009-11-11 15:18:41 PST
I have no plans to at the present time. I'll deal with NSS in a separate bug, if at all.
Comment 11 Nelson Bolyard (seldom reads bugmail) 2009-11-11 21:18:25 PST
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.

Note You need to log in before you can comment on or make changes to this bug.