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

Status: RESOLVED FIXED

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Gregory Szorc [:gps]]

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.

Comment 2

[Bruce Sun [:brsun]]

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.

Comment 3

[Bruce Sun [:brsun]]

How about adding the checking condition for an empty string?
 - if CONFIG['ANDROID_VERSION'] and int(CONFIG['ANDROID_VERSION']) >= 16:

Comment 4

[Gregory Szorc [:gps]]

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

Comment 5

[Bruce Sun [:brsun]]

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.

Comment 6

[Gregory Szorc [:gps]]

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 7

[Nick Alexander :nalexander [he/him]]

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

Comment 8

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d83164a32cb5

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d83164a32cb5

Comment 10

[Ralph Giles (:rillian) | needinfo me]

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?

Comment 11

[Gregory Szorc [:gps]]

Everything else in the tree uses string compare. Please use the tree convention.