Closed
Bug 591152
Opened 14 years ago
Closed 14 years ago
mozilla/js/src/configure: line 10134: test: : integer expression expected
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla6
People
(Reporter: ishikawa, Assigned: tuukka.tolvanen)
References
Details
Attachments
(2 files, 3 obsolete files)
6.15 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
4.56 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
During the config/build cycle of TB3, I noticed a warning generated by ./mozilla/js/src/configure which looks very suspicious. I reported the problem and analsys in the newsgroup posting. http://groups.google.co.jp/group/mozilla.dev.builds/browse_thread/thread/c58a1b7825bf95a4# I found that a code in configure looks like this at the line where the warning is generated: 0133 10134 if test -z "$MACOS_DEPLOYMENT_TARGET" -o "$MACOS_DEPLOYMENT_TARGET" -ge "100300"; then 10135 ... blahblahblah ... The "-o" should be rewritten as "|| test " so that the null-string is not passed to `test'. Under my linux configuration, MACOS_DEPLOYMENT_TARGET is null. Now I figured the problem is in configure.in and very easy to fix. So I am attaching the patch to ./mozilla/ms/src/configure.in TIA
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → ishikawa
Attachment #469753 -
Flags: review?(ted.mielczarek)
Updated•14 years ago
|
Attachment #469753 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 1•14 years ago
|
||
checkin-needed?
Assignee | ||
Comment 2•14 years ago
|
||
oh, the other configure.in needs this too. (and comm) http://mxr.mozilla.org/comm-central/search?string=MACOS_DEPLOYMENT_TARGET
Assignee | ||
Comment 3•14 years ago
|
||
fwiw, http://hg.mozilla.org/comm-central/rev/16853b562855 http://hg.mozilla.org/mozilla-central/rev/ee594fb0fa3e
Blocks: 601910
Assignee | ||
Comment 4•14 years ago
|
||
there's also test -n "$XCODEBUILD_VERSION" -a "$XCODEBUILD_VERSION" -ge 620 which should be suspectible to the same sort of issue, but the other numerical comparison stuff seems ok
Reporter | ||
Comment 5•14 years ago
|
||
Thank you for looking at the problem. http://hg.mozilla.org/comm-central/rev/16853b562855 This one seems to need the fix. http://hg.mozilla.org/mozilla-central/rev/ee594fb0fa3e This one, also. It is hard to tell which "test" requires changes because we can't tell easily whether a particular string is defined at all. In the case of "$MACOS_DEPLOYMENT_TARGET", it seems to me - that certain version of MACOS deployment target may not define this string at all, and - if defined, there are version cases (larger than a given number) that need to be tested. Depending on the strings chosen, they may or may not be defined and being undefined means one thing to a particular test, and so only the original coder, and the testers with various configurations would be able to tell. (MACOS builder probably never see the original problem I reported.) The two patches I saw seemed to be an attempt to premature optimization efforts. A series of extra exec()s of "test" don't seem to be a big issue to me for a configuration of big suite like TB3. (Yes, it certainly adds time during configuration, but we have bigger problems of real bugs and too many spurious compiler warnings that need clean up before configure.in cleanup IMHO.) TIA.
Assignee | ||
Comment 6•14 years ago
|
||
covers gt/ge/lt/le/eq that were touched (ok, so there were only ge)
Attachment #469753 -
Attachment is obsolete: true
Attachment #489972 -
Flags: review?(khuey)
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #489973 -
Flags: review?(bugspam.Callek)
Assignee | ||
Comment 8•14 years ago
|
||
oh bother. can't have blame for lines with bashisms ;>
Attachment #489972 -
Attachment is obsolete: true
Attachment #489978 -
Flags: review?(khuey)
Attachment #489972 -
Flags: review?(khuey)
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #489973 -
Attachment is obsolete: true
Attachment #489980 -
Flags: review?(bugspam.Callek)
Attachment #489973 -
Flags: review?(bugspam.Callek)
Comment 10•14 years ago
|
||
Comment on attachment 489980 [details] [diff] [review] comm I'm cheating and letting this get one single round of reviews, and will accept kyle's review here (provided the changed lines are == between m-c and c-c which they appear to be).
Attachment #489980 -
Flags: review?(bugspam.Callek) → review?(khuey)
Comment 11•14 years ago
|
||
Another choice: if test -z "$MACOS_DEPLOYMENT_TARGET" -o "${MACOS_DEPLOYMENT_TARGET:-0}" -ge "100300"; then Here's just for information.
Comment on attachment 489978 [details] [diff] [review] central Maybe I still don't quite understand the problem here, my shell-fu is a bit weak, but why shouldn't > AC_MSG_CHECKING([for Unicode NSIS with major version == $REQ_NSIS_MAJOR_VER and minor version >= $MIN_NSIS_MINOR_VER]) >- if test "$MAKENSISU_VER" == "" -o \ >- ! "$MAKENSISU_MAJOR_VER" == "$REQ_NSIS_MAJOR_VER" -o \ >- ! "$MAKENSISU_MINOR_VER" -ge $MIN_NSIS_MINOR_VER; then >+ if test "$MAKENSISU_VER" = "" || >+ test ! "$MAKENSISU_MAJOR_VER" = "$REQ_NSIS_MAJOR_VER" -o \ >+ ! "$MAKENSISU_MINOR_VER" -ge $MIN_NSIS_MINOR_VER; then be changed to '||' in both places?
Other than that, these look fine, but I'd like to understand the question posted in comment 12 before I r+ these.
Assignee | ||
Comment 14•14 years ago
|
||
Nothing's checking MAKENSISU_MAJOR/MINOR_VER REQ_NSIS_MAJOR/MINOR_VER themselves for integerness or nonblankness there, and in a successful case it wants to look at both anyhow, so it should be no more broken written like that. The patch is just to isolate "not blank" tests from being part of the same syntax that will fail in the "blank" case.
Assignee | ||
Comment 15•14 years ago
|
||
> Nothing's checking MAKENSISU_MAJOR/MINOR_VER REQ_NSIS_MAJOR/MINOR_VER
> themselves for integerness or nonblankness there, and in a successful case it
("there" being the second "test" as patched -- if $MAKENSISU_VER is blank in the first test that does imply that the MAJOR/MINOR derived from it are as well, so the syntax would be broken if those two parts are not isolated.)
Attachment #489980 -
Flags: review?(khuey) → review+
Attachment #489978 -
Flags: review?(khuey) → review+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Pushed the m-c parts in http://hg.mozilla.org/mozilla-central/rev/2fb4b186f3af Leaving open until the c-c parts are landed.
Updated•14 years ago
|
Whiteboard: [needs landing attachment 489980]
Updated•14 years ago
|
Whiteboard: [needs landing attachment 489980] → [needs landing attachment 489980 to comm-central]
Comment 18•14 years ago
|
||
Pushed in c-c: https://hg.mozilla.org/comm-central/rev/319d4bf10b3d
Assignee: ishikawa → tuukka.tolvanen
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing attachment 489980 to comm-central]
Updated•14 years ago
|
Flags: in-testsuite-
Target Milestone: --- → mozilla6
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•