Closed Bug 1119921 Opened 5 years ago Closed 5 years ago

ANDROID_VERSION comparisons fail with "test: : integer expression expected"

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla38

People

(Reporter: nalexander, Assigned: babhishek21, Mentored)

References

Details

(Whiteboard: [lang=shell][good first bug])

Attachments

(1 file, 1 obsolete file)

I see

0:02.28 /Users/nalexander/Mozilla/gecko/configure: line 10133: test: : integer expression expected
...
0:02.90 /Users/nalexander/Mozilla/gecko/configure: line 20124: test: : integer expression expected

They correlate to lines in configure.in like:

if test -n "$gonkdir" -a "$ANDROID_VERSION" -ge 21; then

presumably the issue is that ANDROID_VERSION is undefined?  Do these tests do the right thing in this case?
glandium: should we do something like:

if test -n "$gonkdir" -a -n "$ANDROID_VERSION" -a "$ANDROID_VERSION" -ge 21; then
Flags: needinfo?(mh+mozilla)
Apparently test is stupid. Make that test -n "$gonkdir" && test "$ANDROID_VERSION" -ge 21
Flags: needinfo?(mh+mozilla)
Mentor: nalexander
Whiteboard: [lang=shell][good first bug]
I would like to work on this bug. New to FOSS. So need a mentor.

Are we talking about https://dxr.mozilla.org/mozilla-central/source/configure.in#204 ?
Attached patch bug_1119921.patch (obsolete) — Splinter Review
REF: bug 1119921, comment 2
According to what glandium said. Don't know if this was even required. Mentor help please.
Duplicate of this bug: 1120219
Coming from Bug 1120219:
I noticed the similar errors in three places:

(a) /REF-COMM-CENTRAL/comm-central/mozilla/configure: 10193: test: Illegal number: 
(b) /REF-COMM-CENTRAL/comm-central/mozilla/configure: 20165: test: Illegal number: 
(c) js/src> /REF-COMM-CENTRAL/comm-central/mozilla/js/src/configure: 8870: test: Illegal number: 

The patch proposed is about (b).
I have no idea why (a) and (c) were not seen by the original poster [Or were they handled in other bugzilla entries?]

Also maybe this might needed to be discussed separately, but
what about 

--- 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?

TIA
(In reply to ISHIKAWA, Chiaki from comment #6)
> Coming from Bug 1120219:
> I noticed the similar errors in three places:
> 
> (a) /REF-COMM-CENTRAL/comm-central/mozilla/configure: 10193: test: Illegal
> number: 
> (b) /REF-COMM-CENTRAL/comm-central/mozilla/configure: 20165: test: Illegal
> number: 
> (c) js/src> /REF-COMM-CENTRAL/comm-central/mozilla/js/src/configure: 8870:
> test: Illegal number: 
> 
> The patch proposed is about (b).
> I have no idea why (a) and (c) were not seen by the original poster [Or were
> they handled in other bugzilla entries?]
If (a) and (c) need fixing, I would be happy to look into those too.
> Also maybe this might needed to be discussed separately, but
> what about 
> 
> --- 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.
That sounds like a great plan. But IMHO this could lead to some structural changes in the code. Also while only |test| error messages would be checked or reported, but it will still require fixing the nested checks/input. All in all, we are still left with some error messages and aborted builds.:/
> 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?
> 
> TIA
Maintainers/mentors please peruse.
Attachment #8547043 - Flags: review?(nalexander)
Comment on attachment 8547043 [details] [diff] [review]
bug_1119921.patch

Review of attachment 8547043 [details] [diff] [review]:
-----------------------------------------------------------------

Have you tested this by running |./mach configure|?  (I expect not, 'cuz you'd still see bad messages.)  But this is a good first step -- flag for re-review when you've updated.  Thanks!

::: configure.in
@@ +5083,5 @@
>      esac
>  fi
>  
>  dnl Temporary until webrtc works on gonk-L
> +if test -n "$gonkdir" && "$ANDROID_VERSION" -ge 21; then

This needs to be

if test -n "$gonkdir" && test "$ANDROID_VERSION" -ge 21; then

And there's another similar test that needs to be updated at

https://dxr.mozilla.org/mozilla-central/source/build/autoconf/compiler-opts.m4#337
Attachment #8547043 - Flags: review?(nalexander) → feedback+
Fixed /configure.in
Fixed /build/autoconf/compiler-opts.m4

Ran tests by |./mach configure|: Corresponding error removed. (Should I attach the output log as well?)

Encountered several errors for line 5534 and 4679, like:
 ~/mozilla-central/configure: line 5534: test: /usr/local/bin/c: binary operator expected.
 ~/mozilla-central/configure: line 5534: test: ~/mozilla-build/wget/c: binary operator expected
...
 js\src> ~/mozilla-central/js/src/configure: line 4679: test: ~/mozilla-build/wget/c: binary operator expected
...
Is a bug filed for this already? If not then: => New Bug.
Attachment #8547043 - Attachment is obsolete: true
Attachment #8548322 - Flags: review?(nalexander)
Comment on attachment 8548322 [details] [diff] [review]
bug_1119921_1: Rev_1 : Changes to /configure and /build/autoconf/compiler-opts

Review of attachment 8548322 [details] [diff] [review]:
-----------------------------------------------------------------

That should do it!  Thanks!
Attachment #8548322 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #10)
> Comment on attachment 8548322 [details] [diff] [review]
> bug_1119921_1: Rev_1 : Changes to /configure and
> /build/autoconf/compiler-opts
> 
> Review of attachment 8548322 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That should do it!  Thanks!

Hmm. You were quite busy it seems. Thanks for review+ 
Can you assign this bug to me please?
Assignee: nobody → babhishek21
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/e8c53d6e4321
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.