Closed Bug 304785 Opened 17 years ago Closed 17 years ago

Replacing NSPR variables in config/ is slow


(Firefox Build System :: General, defect)

Not set


(Not tracked)



(Reporter: mozilla, Unassigned)


(Keywords: fixed1.8)


(1 file, 1 obsolete file)

Near the end of the configure run (line 7135 of mozilla/ a message
    configure: warning: Recreating 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/ 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
Attached patch Faster version for (obsolete) — Splinter Review
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
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.
Attachment #192807 - Flags: review?(chase)
Attachment #192807 - Flags: review?(chase) → review+
Regarding your hack comment, there is a better way if you get the urge.  Unix
shells do sane quoting.  This means that

  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"' }'

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.
Peter, do you want to try Steven's suggestion before I check this in?
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.
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.
Comment on attachment 193250 [details] [diff] [review]
Updated 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 on attachment 193250 [details] [diff] [review]
Updated edit patch

Yeah, even better.
Attachment #193250 - Flags: review?(chase) → review+
Fix checked in.
Closed: 17 years ago
Resolution: --- → FIXED
Attachment #192807 - Attachment is obsolete: true
Comment on attachment 193250 [details] [diff] [review]
Updated 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?
Attachment #193250 - Flags: approval1.8b5? → approval1.8b5+
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.
Keywords: fixed1.8
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.