Last Comment Bug 591152 - mozilla/js/src/configure: line 10134: test: : integer expression expected
: mozilla/js/src/configure: line 10134: test: : integer expression expected
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86 Linux
: -- minor (vote)
: mozilla6
Assigned To: Tuukka Tolvanen (sp3000)
:
Mentors:
: 616005 (view as bug list)
Depends on:
Blocks: 601910
  Show dependency treegraph
 
Reported: 2010-08-26 20:39 PDT by ISHIKAWA, Chiaki
Modified: 2011-04-21 14:21 PDT (History)
8 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to ./mozilla/js/src/configure.in (966 bytes, patch)
2010-08-26 20:39 PDT, ISHIKAWA, Chiaki
ted: review+
Details | Diff | Review
central (4.82 KB, patch)
2010-11-11 16:25 PST, Tuukka Tolvanen (sp3000)
no flags Details | Diff | Review
comm (3.22 KB, patch)
2010-11-11 16:27 PST, Tuukka Tolvanen (sp3000)
no flags Details | Diff | Review
central (6.15 KB, patch)
2010-11-11 16:35 PST, Tuukka Tolvanen (sp3000)
khuey: review+
Details | Diff | Review
comm (4.56 KB, patch)
2010-11-11 16:36 PST, Tuukka Tolvanen (sp3000)
khuey: review+
Details | Diff | Review

Description ISHIKAWA, Chiaki 2010-08-26 20:39:27 PDT
Created attachment 469753 [details] [diff] [review]
Patch to ./mozilla/js/src/configure.in

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
Comment 1 Tuukka Tolvanen (sp3000) 2010-11-10 06:27:39 PST
checkin-needed?
Comment 2 Tuukka Tolvanen (sp3000) 2010-11-10 06:31:31 PST
oh, the other configure.in needs this too. (and comm) http://mxr.mozilla.org/comm-central/search?string=MACOS_DEPLOYMENT_TARGET
Comment 4 Tuukka Tolvanen (sp3000) 2010-11-10 06:48:26 PST
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
Comment 5 ISHIKAWA, Chiaki 2010-11-10 16:26:01 PST
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.
Comment 6 Tuukka Tolvanen (sp3000) 2010-11-11 16:25:21 PST
Created attachment 489972 [details] [diff] [review]
central

covers gt/ge/lt/le/eq that were touched (ok, so there were only ge)
Comment 7 Tuukka Tolvanen (sp3000) 2010-11-11 16:27:25 PST
Created attachment 489973 [details] [diff] [review]
comm
Comment 8 Tuukka Tolvanen (sp3000) 2010-11-11 16:35:53 PST
Created attachment 489978 [details] [diff] [review]
central

oh bother. can't have blame for lines with bashisms ;>
Comment 9 Tuukka Tolvanen (sp3000) 2010-11-11 16:36:55 PST
Created attachment 489980 [details] [diff] [review]
comm
Comment 10 Justin Wood (:Callek) 2010-11-11 17:25:42 PST
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).
Comment 11 Takanori MATSUURA 2010-11-11 18:28:50 PST
Another choice:

if test -z "$MACOS_DEPLOYMENT_TARGET" -o "${MACOS_DEPLOYMENT_TARGET:-0}" -ge "100300"; then


Here's just for information.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2010-11-11 18:31:07 PST
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?
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2010-11-11 19:39:14 PST
Other than that, these look fine, but I'd like to understand the question posted in comment 12 before I r+ these.
Comment 14 Tuukka Tolvanen (sp3000) 2010-11-12 01:48:54 PST
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.
Comment 15 Tuukka Tolvanen (sp3000) 2010-11-12 01:54:06 PST
> 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.)
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2010-12-01 15:51:25 PST
*** Bug 616005 has been marked as a duplicate of this bug. ***
Comment 17 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2010-12-02 08:30:46 PST
Pushed the m-c parts in http://hg.mozilla.org/mozilla-central/rev/2fb4b186f3af

Leaving open until the c-c parts are landed.
Comment 18 Mounir Lamouri (:mounir) 2011-04-20 13:46:20 PDT
Pushed in c-c:
https://hg.mozilla.org/comm-central/rev/319d4bf10b3d

Note You need to log in before you can comment on or make changes to this bug.