content/media/omx/moz.build may cast empty string to int

RESOLVED FIXED in mozilla34

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
e2d48c70d946 (bug 1037282) broke moz.build processing when ANDROID_VERSION is not defined.

moz.build files should work even in unexpected build configurations. For example, |mach build-docs| walks the filesystem and executes every single moz.build file looking for documentation variables. That is how I detected this bug: |mach build-docs| is now broken using desktop configs.

The fix is trivial: compare CONFIG['ANDROID_VERSION'] lexically. This is how we do it throughout the tree. Not the greatest solution, but it works for now.
(Assignee)

Comment 1

5 years ago
Created attachment 8458346 [details] [diff] [review]
Do not cast ANDROID_VERSION to int

Ideally it should be converted to an int. But empty variables need to
work. The rest of the tree uses string comparisons.
Attachment #8458346 - Flags: review?(nalexander)
(Assignee)

Updated

5 years ago
Assignee: nobody → gps
Status: NEW → ASSIGNED
FYI, by using string comparison, there might be a logic flaw (ex. '9' is bigger than '16'). We should take this case into consideration as well.
How about adding the checking condition for an empty string?
 - if CONFIG['ANDROID_VERSION'] and int(CONFIG['ANDROID_VERSION']) >= 16:
(Assignee)

Comment 4

5 years ago
It is my understanding that single digit android versions won't be encountered by moz.build. This shouldn't be a problem until SDK 100.

https://hg.mozilla.org/mozilla-central/file/99f694d1b50c/configure.in#l280
Thanks. Indeed, practically we won't get troubles in the near future regarding to 'ANDROID_VERSION'. Just want to point out the potential problem to do string comparison in general cases. Maybe we have to deal another CONFIG['XXX_INTEGER_OPTION'] some other day.
(Assignee)

Comment 6

5 years ago
We already introduced AC_SUBST_SET and AC_SUBST_LIST for emitting Python native data types instead of strings from configure. It sounds like we could use AC_SUBST_INT. File a bug if you care passionately about it.
Comment on attachment 8458346 [details] [diff] [review]
Do not cast ANDROID_VERSION to int

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

If it works for you, roll with it.

::: content/media/omx/moz.build
@@ +47,5 @@
>          'RtspOmxDecoder.cpp',
>          'RtspOmxReader.cpp',
>      ]
>  
> +if CONFIG['ANDROID_VERSION'] >= '16':

if CONFIG[...] and CONFIG[...] >= '16'?  It's not obvious to me that None < '16'.  Or is this always '' < '16'?
Attachment #8458346 - Flags: review?(nalexander) → review+
https://hg.mozilla.org/mozilla-central/rev/d83164a32cb5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Oops, I clobbered this in my rebased landing of bug 1038564. Is the "if CONFIG['ANDROID_VERSION'] and ..." form sufficient to address this issue, or would you like to see a patch using string comparison instead?
Flags: needinfo?(gps)
(Assignee)

Comment 11

5 years ago
Everything else in the tree uses string compare. Please use the tree convention.
Flags: needinfo?(gps)
You need to log in before you can comment on or make changes to this bug.