Closed Bug 608423 Opened 14 years ago Closed 13 years ago

configure assumes GNU sed

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(blocking2.0 -)

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- -

People

(Reporter: niederstrasser, Assigned: justin.lebar+bug)

References

Details

Attachments

(1 file)

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.
Blocks: 583849
I thought we fixed most of the GNUisms, but I guess not. I hate autoconf and shell scripting.
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?
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.
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.
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?
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.)
Attached patch Patch v1Splinter Review
I see.  This seems to fix it on my machine.
Assignee: nobody → justin.lebar+bug
Status: NEW → ASSIGNED
Attachment #487620 - Flags: review?(ted.mielczarek)
Attachment #487620 - Flags: review?(ted.mielczarek) → review+
(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 ;-)
I would certainly take a patch to fix that too. You could probably port the NSPR patch pretty directly.
http://hg.mozilla.org/mozilla-central/rev/e3230a374d65
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
(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 +...
So this patch probably didn't help you much then...
Fixed on OS X from comment #0.  Not VERIFYING in case it gets further changes for comment #11.
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...
Status: RESOLVED → REOPENED
blocking2.0: --- → ?
Flags: in-testsuite-
Resolution: FIXED → ---
Target Milestone: --- → mozilla2.0b8
This should be backed out, immediately.
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.
blocking2.0: ? → -
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
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.
Status: REOPENED → RESOLVED
blocking2.0: - → ?
Closed: 14 years ago13 years ago
Resolution: --- → FIXED
blocking2.0: ? → -
(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\}
Depends on: 634003
(In reply to comment #18)
> Serge: you can file a followup to fix that.

Justin filed bug 634003 :-)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.