Closed Bug 1040413 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(1 file)

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.
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: 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:
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.
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
Closed: 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)
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.