Closed
Bug 1165733
Opened 9 years ago
Closed 9 years ago
./configure: syntax error. : mozilla/configure: 28717: test: Illegal number
Categories
(Firefox Build System :: General, defect)
Tracking
(firefox41 fixed)
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: ishikawa, Assigned: ishikawa)
References
Details
Attachments
(1 file, 1 obsolete file)
I refreshed the local C-C source tree less than 30 minutes ago. During the configuration of C-C TB, I noticed the following error. from the log: checking for posix_fadvise... (cached) yes checking for posix_fallocate... (cached) yes /REF-COMM-CENTRAL/comm-central/mozilla/configure: 28717: test: Illegal number: creating ./config.status The offending code is: 28715 28716 MOZ_SHARED_ICU=1 28717 elif test -n "$gonkdir" -a "$ANDROID_VERSION" -ge 17; then 28718 if test -d "$gonkdir/external/icu/icu4c/source"; then 28719 MOZ_ICU_GONK_PATH="$gonkdir/external/icu/icu4c/source" Since I am configuring the code in linux amd64 on an Intel CPU, I think ANDROID version is not defined, and that is why we see the error. When I began filing this bugzilla, I noticed there was a very similary bugzilla entry: Bug 1119921 ANDROID_VERSION comparisons fail with "test: : integer expression expected" And I even filed a comment in that bugzilla entry: From: https://bugzilla.mozilla.org/show_bug.cgi?id=1119921#c6 --- BEGIN QUOTE --- --- begin quote --- Maybe the warnings from |test| and other warnings produced such as these should be collected during the execution of configure. Then higher build command can decide to stop/continue based on known problematic error/warning messages by using whitelist and blacklist. At least, "test Illegal number" warnings have been noticed before several times and fixed in due time from what I found in bugzilla. So the blacklist should contain this warning at least. "binary operator expected" is another manifestation of undefined shell variable issues. "integer expression expected" is another one. Hmm, maybe we need to collect a few more messages printed by "test" variants. Considerations by build/config maintainers will be appreciated --- end quote --- We don't want to repeat this manual cycle (which lags behind the introduction of buggy expressions) forever. Right? --- END QUOTE History repeats itself. No those who don't learn history make the same mistakes again and again, right ?
Assignee | ||
Comment 1•9 years ago
|
||
I refreshed the C-C source tree again and carefully checked the configure process. I noticed a couple of "Illegal number" errors and they point to the cause of the same origin. /REF-COMM-CENTRAL/comm-central/mozilla/configure: 28673: test: Illegal number: 28671 28672 MOZ_SHARED_ICU=1 28673 elif test -n "$gonkdir" -a "$ANDROID_VERSION" -ge 17; then 28674 if test -d "$gonkdir/external/icu/icu4c/source"; then 28675 MOZ_ICU_GONK_PATH="$gonkdir/external/icu/icu4c/source" js/src> /REF-COMM-CENTRAL/comm-central/mozilla/js/src/configure: 15264: test: Illegal number: 15262 15263 MOZ_SHARED_ICU=1 15264 elif test -n "$gonkdir" -a "$ANDROID_VERSION" -ge 17; then 15265 if test -d "$gonkdir/external/icu/icu4c/source"; then 15266 MOZ_ICU_GONK_PATH="$gonkdir/external/icu/icu4c/source" 15267 elif test -d "$gonkdir/external/icu4c"; then So it has something to do with ICU. I searched for the string ICU in mozilla/configure and found the following snippet. --- begin quote --- dnl ======================================================== dnl ICU Support dnl ======================================================== # Internationalization isn't built or exposed by default in non-desktop # builds. Bugs to enable: # # Android: bug 864843 if test "$MOZ_WIDGET_TOOLKIT" = "android"; then _INTL_API=no else _INTL_API=yes fi if test "$MOZ_WIDGET_TOOLKIT" = "cocoa"; then USE_ICU=1 fi MOZ_CONFIG_ICU() --- end quote So where is this MOZ_CONFIG_ICU()? I finally found the following file, and noticed the offending code. mozilla/build/autoconf/icu.m4 Attached is the patch. TIA PS: At least, with the patch, local build of C-C TB proceeds without the warnings. I am tempted to put "set -e" or something like that into generated ./configure from autoconf to catch this type of errors during configure, but that may be too drastic. PPS: I requested review from a reviewer in the suggested reviewer list, who happens to have the least # of pending reviews at the moment.
Attachment #8608292 -
Flags: review?(mshal)
Comment 2•9 years ago
|
||
Comment on attachment 8608292 [details] [diff] [review] Avoid evaluating empty "$ANDROID_VERSION" where integer is expected Review of attachment 8608292 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/autoconf/icu.m4 @@ +18,5 @@ > > if test -n "$MOZ_NATIVE_ICU"; then > PKG_CHECK_MODULES(MOZ_ICU, icu-i18n >= 50.1) > MOZ_SHARED_ICU=1 > +elif test -n "$gonkdir" && test -n "$ANDROID_VERSION" && test "$ANDROID_VERSION" -ge 17; then You don't need test -n "$ANDROID_VERSION". ANDROID_VERSION is always set when gonkdir is.
Attachment #8608292 -
Flags: review?(mshal) → review+
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #2) > Comment on attachment 8608292 [details] [diff] [review] > Avoid evaluating empty "$ANDROID_VERSION" where integer is expected > > Review of attachment 8608292 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: build/autoconf/icu.m4 > @@ +18,5 @@ > > > > if test -n "$MOZ_NATIVE_ICU"; then > > PKG_CHECK_MODULES(MOZ_ICU, icu-i18n >= 50.1) > > MOZ_SHARED_ICU=1 > > +elif test -n "$gonkdir" && test -n "$ANDROID_VERSION" && test "$ANDROID_VERSION" -ge 17; then > > You don't need test -n "$ANDROID_VERSION". ANDROID_VERSION is always set > when gonkdir is. I am a little puzzled. Can I put in checkin-needed keyword??? Oh I see, test -n "$gonkdir" && test "$ANDROID_VERSION" -ge 17 won't evaluate test -ge 17 when "$ANDROID_VERSION" is "" according to the implicit assumption. Would you like me to remove |test -n "$ANDROID_VERSION"|? Well, I would prefer the my somewhat redundant version since it is not entirely clear to people not heavily involved in ANDROID-related development that "ANDROID_VERSION is always set when gonkdir is." Also, this is not meant for a speed daemon, too :-) TIA
Comment 4•9 years ago
|
||
Please update. test -n "$gonkdir" && test "$ANDROID_VERSION" -ge 17 is the form used for other similar tests in configure.
Assignee | ||
Comment 5•9 years ago
|
||
Here is the updated version. I put in the checkin-needed keyword. Thank you.
Attachment #8608413 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8608292 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ishikawa
Keywords: checkin-needed
Assignee | ||
Comment 8•9 years ago
|
||
Hmm... Automatic pulsebot integration has not set "RESOLVED" flag. Or should we wait for a successful build after the patch landed or something? Anyway, thanks for checking in (to pulsebot? There is no human being behind it, is there?)
Comment 9•9 years ago
|
||
The push was done by a human: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=4e0ca88db337 This says philor did it. Pulsebot only reports about the push, and bugs are not RESOLVED until the changeset reaches mozilla-central.
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4e0ca88db337
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #9) > The push was done by a human: > https://hg.mozilla.org/integration/mozilla-inbound/ > pushloghtml?changeset=4e0ca88db337 > This says philor did it. > > Pulsebot only reports about the push, and bugs are not RESOLVED until the > changeset reaches mozilla-central. Thank you for the clarification and thank you, philor and carsten book. CI
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
•