Closed
Bug 1040413
Opened 8 years ago
Closed 8 years ago
content/media/omx/moz.build may cast empty string to int
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: gps, Assigned: gps)
References
Details
Attachments
(1 file)
1.02 KB,
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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•8 years ago
|
Assignee: nobody → gps
Status: NEW → ASSIGNED
Comment 2•8 years ago
|
||
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•8 years ago
|
||
How about adding the checking condition for an empty string? - if CONFIG['ANDROID_VERSION'] and int(CONFIG['ANDROID_VERSION']) >= 16:
Assignee | ||
Comment 4•8 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
Comment 5•8 years ago
|
||
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•8 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 7•8 years ago
|
||
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+
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d83164a32cb5
Comment 9•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d83164a32cb5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Comment 10•8 years ago
|
||
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•8 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.
Description
•