1165733 - ./configure: syntax error. : mozilla/configure: 28717: test: Illegal number

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[ISHIKAWA, Chiaki]

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 ?

Comment 1

[ISHIKAWA, Chiaki]

Attachment: Avoid evaluating empty "$ANDROID_VERSION" where integer is expected

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.

Comment 2

[Mike Hommey [:glandium]]

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.

Comment 3

[ISHIKAWA, Chiaki]

(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

[Mike Hommey [:glandium]]

Please update. test -n "$gonkdir" && test "$ANDROID_VERSION" -ge 17 is the form used for other similar tests in configure.

Comment 5

[ISHIKAWA, Chiaki]

Attachment: Updated version: Attachments Avoid evaluating empty "$ANDROID_VERSION" where integer is expected (carrying over mh+mozilla: review+)

Here is the updated version.
I put in the checkin-needed keyword.

Thank you.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e0ca88db337

Comment 8

[ISHIKAWA, Chiaki]

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

[Mike Hommey [:glandium]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e0ca88db337

Comment 11

[ISHIKAWA, Chiaki]

(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