608423 - configure assumes GNU sed

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Hanspeter Niederstrasser]

Caused by bug 583849: 

On OS X 10.5 with only the system sed, configure outputs 

checking for localeconv... (cached) yes
checking for YASM assembler... checking for yasm... (cached) yasm
sed: illegal option -- r
usage: sed script [-Ealn] [-i extension] [file ...]
       sed [-Ealn] [-i extension] [-e script] ... [-f script_file] ... [file ...]
checking if app-specific confvars.sh exists... /src/mozilla-central/browser/confvars.sh
checking __attribute__ ((aligned ())) support... (cached) 64



mozilla-central/configure has

>  YASM_VERSION=`yasm --version | sed -nre "$_YASM_VER_FILTER"`
>  _YASM_MAJOR_VERSION=`echo ${YASM_VERSION} | $AWK -F\. '{ print $1 }'`

but -r is not a valid option for OS X's BSD sed.  There are a few other instances of "sed -nre" in configure.in, but they seem to be only for MSVC and so presumably wouldn't affect other platforms that don't have GNU sed.

Comment 1

[(not currently active) Ted Mielczarek]

I thought we fixed most of the GNUisms, but I guess not. I hate autoconf and shell scripting.

Comment 2

[Justin Lebar (not reading bugmail)]

Ugh.

Thankfully, I think the -r is not needed here.  At least, it works on my GNU/Linux box without -r.  

Hanspeter, can you confirm that configure works on your machine without -r?

Comment 3

[Hanspeter Niederstrasser]

Not sure that it does:

checking for localeconv... (cached) yes
checking for YASM assembler... checking for yasm... (cached) yasm
sed: 1: "s|.* ([0-9]+\.[0-9]+\.[ ...": \1 not defined in the RE
checking if app-specific confvars.sh exists... /src/mozilla-central/browser/confvars.sh
checking __attribute__ ((aligned ())) support... (cached) 64
checking for java... (cached) /usr/bin/java

The relevant part of configure looks like this:

*****
14290 if test -n "$YASM"; then
14291 
14292   _YASM_VER_FILTER='s|.* ([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+).*|\1|p'
14293 
14294 
14295   YASM_VERSION=`yasm --version | sed -ne "$_YASM_VER_FILTER"`
14296   _YASM_MAJOR_VERSION=`echo ${YASM_VERSION} | $AWK -F\. '{ print $1 }'`
14297   _YASM_MINOR_VERSION=`echo ${YASM_VERSION} | $AWK -F\. '{ print $2 }'`
14298   _YASM_RELEASE=`      echo ${YASM_VERSION} | $AWK -F\. '{ print $3 }'`
14299   _YASM_BUILD=`        echo ${YASM_VERSION} | $AWK -F\. '{ print $4 }'`
14300 fi
14301 
*****

And just in case:

nieder $ yasm --version
yasm 1.0.1.2326
Compiled on Jun 22 2010.
Copyright (c) 2001-2010 Peter Johnson and other Yasm developers.
Run yasm --license for licensing overview and summary.

Comment 4

[Justin Lebar (not reading bugmail)]

Oh, I see.  I'd figured that a sed error would halt configure.  But of course that doesn't happen, which also explains why nobody has noticed the problem.

Without -r I get

  test: 1: Illegal number: 
  sed: -e expression #1, char 44: invalid reference \1 on `s' command's RHS

on my machine.

Comment 5

[Justin Lebar (not reading bugmail)]

AFAICT we need extended regexps in order to get backreferences.  I guess we could pass -E when we detect a BSD-type sed?

Ted, presumably we've encountered this problem before.  How do you suggest we handle this?

Comment 6

[(not currently active) Ted Mielczarek]

We hit this same problem in NSPR's configure in bug 559133. Attachment 450680 [details] [diff] has the fix that we used. (I'm not really a sed expert, honestly.)

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

I see.  This seems to fix it on my machine.

Comment 8

[[e-mail]]

(In reply to comment #0)
> There are a few other
> instances of "sed -nre" in configure.in, but they seem to be only for MSVC and
> so presumably wouldn't affect other platforms that don't have GNU sed.
Except for those of us unlucky enough to be cross-compiling between the two ;-)

Comment 9

[(not currently active) Ted Mielczarek]

I would certainly take a patch to fix that too. You could probably port the NSPR patch pretty directly.

Comment 10

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/e3230a374d65

Comment 11

[[e-mail]]

(In reply to comment #8)
> (In reply to comment #0)
> > There are a few other
> > instances of "sed -nre" in configure.in, but they seem to be only for MSVC and
> > so presumably wouldn't affect other platforms that don't have GNU sed.
> Except for those of us unlucky enough to be cross-compiling between the two ;-)
In fact, my sed is so lame it doesn't even understand ? or +...

Comment 12

[Justin Lebar (not reading bugmail)]

So this patch probably didn't help you much then...

Comment 13

[Hanspeter Niederstrasser]

Fixed on OS X from comment #0.  Not VERIFYING in case it gets further changes for comment #11.

Comment 14

[Serge Gautherie (:sgautherie)]

Afaict, this regressed compiling with Windows mozbuild (1.5.1) sed :-<

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297681434.1297693795.20399.gz&fulltext=1
WINNT 5.2 mozilla-central nightly on 2011/02/14 03:03:54
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1297674047.1297685678.14352.gz&fulltext=1
WINNT 5.2 comm-central-trunk nightly on 2011/02/14 01:00:47
{
/e/builds/slave/comm-cen-trunk-w32-ntly/build/mozilla/configure: line 17927: test: : integer expression expected
}

Fixing MacOSX while silently breaking Windows is not good :-(

***

I can reproduce manually on my Windows 2000 (after working around bug 599570):
{
$ yasm --version | sed -nre "s|.* ([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+).*|\1|p"
1.0.0.2319

$ yasm --version | sed -ne "s|.* \([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+\).*|\1|p"
(no output)
}

We need configure code that can handle both cases...

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

This should be backed out, immediately.

Comment 16

[Mike Shaver (:shaver emeritus)]

jlebar: can you just back the patch out? having it require gnu sed is much better than not working in mozbuild. builds seems to work OK, though, so not blocking.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Ted promises that this doesn't need to block, and he'll follow up with more information:

12:00 < ted> it didn't break compilation on windows
12:01 < ted> and backing that out will break compilation on OS X

Comment 18

[(not currently active) Ted Mielczarek]

Right. Let's leave this bug closed. Serge: you can file a followup to fix that.

This isn't a big deal because the only thing broken here is the yasm version check. We only support building with MozillaBuild on Windows, and MozillaBuild ships with a known-good version of YASM, so the version check isn't really important. A user with an older MozillaBuild that didn't have YASM at all would fail the earlier check for yasm.exe.

Comment 19

[[e-mail]]

(In reply to comment #14)
> $ yasm --version | sed -nre "s|.* ([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+).*|\1|p"
> 1.0.0.2319
> 
> $ yasm --version | sed -ne "s|.* \([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+\).*|\1|p"
> (no output)
FYI non-GNU sed also doesn't support + or ?, you have to use \{1,\} and \{0,1\}

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #18)
> Serge: you can file a followup to fix that.

Justin filed bug 634003 :-)