Replacing NSPR variables in config/autoconf.mk is slow

RESOLVED FIXED

Status

()

Core
Build Config
--
minor
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Peter Weilbacher, Unassigned)

Tracking

({fixed1.8})

Trunk
x86
OS/2
fixed1.8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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

12 years ago
Created attachment 192807 [details] [diff] [review]
Faster version for configure.in

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

12 years ago
Attachment #192807 - Flags: review?(chase)

Updated

12 years ago
Attachment #192807 - Flags: review?(chase) → review+

Comment 2

12 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

12 years ago
Peter, do you want to try Steven's suggestion before I check this in?
(Reporter)

Comment 4

12 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

12 years ago
Created attachment 193250 [details] [diff] [review]
Updated autoconf.mk edit patch

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

12 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

12 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

12 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Updated

12 years ago
Attachment #192807 - Attachment is obsolete: true
(Reporter)

Comment 9

12 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

12 years ago
Attachment #193250 - Flags: approval1.8b5? → approval1.8b5+

Comment 10

12 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

12 years ago
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.