Closed
Bug 304785
Opened 17 years ago
Closed 17 years ago
Replacing NSPR variables in config/autoconf.mk is slow
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mozilla, Unassigned)
Details
(Keywords: fixed1.8)
Attachments
(1 file, 1 obsolete file)
1.50 KB,
patch
|
chase
:
review+
asa
:
approval1.8b5+
|
Details | Diff | Splinter Review |
Near the end of the configure run (line 7135 of mozilla/configure.in) a message appears configure: warning: Recreating autoconf.mk with updated nspr-config output after which the configure process seems to hang for some time while using 100% CPU. On my relatively fast machine this takes about 1 minute but others said (in netscape.public.mozilla.os2) that it takes tens of minutes on their machines, so this is worth improving on. The "hang" is due to a perl loop used to replace the variables NSPR_LIBS and NSPR_CFLAGS in config/autoconf.mk. This loop is very slow on platforms that are not as effective at spawning shells (like OS/2) as nspr-config is called for every line in autoconf.mk.
Reporter | ||
Comment 1•17 years ago
|
||
This alleviates the problem by using the nspr-config call just once, before the perl loop, to input it into a variable that in turn is used in the perl one-liner. I didn't find any other instance of this export + perl $ENV hack so there probabl y is a better way, but the patch doesn't break anything on Linux while on OS/2 the time it takes to do this variable replacement is down from one minute to a fraction of a second.
Updated•17 years ago
|
Attachment #192807 -
Flags: review?(chase)
Updated•17 years ago
|
Attachment #192807 -
Flags: review?(chase) → review+
Comment 2•17 years ago
|
||
Regarding your hack comment, there is a better way if you get the urge. Unix shells do sane quoting. This means that v='d' echo 'abc'$v'def' will do what you expect, so you can get rid of the export and replace the perl invocation with $PERL -pi.bak -e 's {^NSPR_LIBS\s*=.*} {NSPR_LIBS = '"$NSPR_LIBS_REPL"' }' config/autoconf.mk I suspect autoconf style would prefer the variable to be lowercased too. I'd need to test but the double quotes are unlikely to be required.
Comment 3•17 years ago
|
||
Peter, do you want to try Steven's suggestion before I check this in?
Reporter | ||
Comment 4•17 years ago
|
||
Now I tried it but it doesn't work. With normall commands Steven's trick works but perl somehow interprets the $\(DIST\) and whatever I try with escaping and quoting it always gives me CFLAGS like "-I430 430DIST)/include/nspr". So, I would vote to check in the patch as is.
Comment 5•17 years ago
|
||
I neglected to avoid excess variable interpolation. The revised patch should handle all cases as long as nspr_config does not output a value containing one or more single quotes. FWIW, $( is a variable that represents the read gid of the current process. Since OS/2 doesn't do gids, perl always supplies a 0 value.
Reporter | ||
Comment 6•17 years ago
|
||
Comment on attachment 193250 [details] [diff] [review] Updated autoconf.mk edit patch Yes, that works for me both on OS/2 and Linux with bash or ash as shell. chase, do you like this one better, too?
Attachment #193250 -
Flags: review?(chase)
Comment 7•17 years ago
|
||
Comment on attachment 193250 [details] [diff] [review] Updated autoconf.mk edit patch Yeah, even better.
Attachment #193250 -
Flags: review?(chase) → review+
Comment 8•17 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•17 years ago
|
Attachment #192807 -
Attachment is obsolete: true
Reporter | ||
Comment 9•17 years ago
|
||
Comment on attachment 193250 [details] [diff] [review] Updated autoconf.mk edit patch Mike, thanks for the checkin. We probably have to do quite a lot of builds on the 1.8 branch, so it would be nice to get this in there, too.
Attachment #193250 -
Flags: approval1.8b5?
Updated•17 years ago
|
Attachment #193250 -
Flags: approval1.8b5? → approval1.8b5+
Comment 10•17 years ago
|
||
Mike or Peter, time is running out on 1.8b5 and I suspect we're soon going to start withdrawing the approvals for non-blocking bugs to minimize risk. If this still needs to land on the branch, please do so ASAP and add the "fixed1.8" keyword. Thanks.
Updated•5 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•