Closed
Bug 608423
Opened 14 years ago
Closed 13 years ago
configure assumes GNU sed
Categories
(Firefox Build System :: General, defect)
Tracking
(blocking2.0 -)
RESOLVED
FIXED
mozilla2.0b8
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: niederstrasser, Assigned: justin.lebar+bug)
References
Details
Attachments
(1 file)
1.00 KB,
patch
|
ted
:
review+
ted
:
approval2.0+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
I thought we fixed most of the GNUisms, but I guess not. I hate autoconf and shell scripting.
Assignee | ||
Comment 2•14 years ago
|
||
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?
Reporter | ||
Comment 3•14 years ago
|
||
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.
Assignee | ||
Comment 4•14 years ago
|
||
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.
Assignee | ||
Comment 5•14 years ago
|
||
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•14 years ago
|
||
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.)
Assignee | ||
Comment 7•14 years ago
|
||
I see. This seems to fix it on my machine.
Assignee: nobody → justin.lebar+bug
Status: NEW → ASSIGNED
Attachment #487620 -
Flags: review?(ted.mielczarek)
Updated•14 years ago
|
Attachment #487620 -
Flags: review?(ted.mielczarek) → review+
Updated•14 years ago
|
Attachment #487620 -
Flags: approval2.0+
Comment 8•14 years ago
|
||
(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•14 years ago
|
||
I would certainly take a patch to fix that too. You could probably port the NSPR patch pretty directly.
Assignee | ||
Comment 10•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e3230a374d65
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 11•14 years ago
|
||
(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 +...
Assignee | ||
Comment 12•14 years ago
|
||
So this patch probably didn't help you much then...
Reporter | ||
Comment 13•14 years ago
|
||
Fixed on OS X from comment #0. Not VERIFYING in case it gets further changes for comment #11.
Comment 14•13 years ago
|
||
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
Comment 15•13 years ago
|
||
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: ? → -
Comment 17•13 years ago
|
||
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•13 years ago
|
||
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 ago → 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
blocking2.0: ? → -
Comment 19•13 years ago
|
||
(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•13 years ago
|
||
(In reply to comment #18) > Serge: you can file a followup to fix that. Justin filed bug 634003 :-)
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•