Last Comment Bug 686350 - Check in configure script that the selected Android SDK is valid
: Check in configure script that the selected Android SDK is valid
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Mounir Lamouri (:mounir)
:
: Gregory Szorc [:gps]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-12 13:35 PDT by Mounir Lamouri (:mounir)
Modified: 2011-09-14 06:58 PDT (History)
6 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (2.82 KB, patch)
2011-09-12 14:13 PDT, Mounir Lamouri (:mounir)
mh+mozilla: review-
Details | Diff | Splinter Review
Patch v1.1 (3.63 KB, patch)
2011-09-13 11:10 PDT, Mounir Lamouri (:mounir)
mh+mozilla: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-09-12 13:35:01 PDT
Yesterday, I spent (too much) time trying to understand why my build was failing on an Android layout thing. Luckily, I remember that I saw someone speaking of updating the mozconfig example in the wiki. I went there and see that the SDK version changed. This should not happen and the configure script should let me know I'm trying to use the wrong version.
Comment 1 Mounir Lamouri (:mounir) 2011-09-12 14:13:12 PDT
Created attachment 559897 [details] [diff] [review]
Patch v1

Doug, I'm assigning the review to you because Brad is on vacation. Feel free to drop the review if you are not the best fit here.

Do we want to port that patch for NSS/NSPR?
Comment 2 Doug Turner (:dougt) 2011-09-12 16:29:58 PDT
Comment on attachment 559897 [details] [diff] [review]
Patch v1


the general idea to force a check in configure is fine/great.  Do we know that source.properties is always going to be there?  I think it use to be called tools_source.properties a while ago.

How about just looking at the --with-android-sdk path?  Mine is:

ac_add_options --with-android-sdk="/builds/android_tools/android-sdk-linux_x86/platforms/android-13"

It is going to end with android-<version>

at least that is what all of the other versions do.

Let kyle decide... as he is a peer
Comment 3 Mounir Lamouri (:mounir) 2011-09-12 17:04:53 PDT
(In reply to Doug Turner (:dougt) from comment #2)
> Comment on attachment 559897 [details] [diff] [review]
> Patch v1
> 
> 
> the general idea to force a check in configure is fine/great.  Do we know
> that source.properties is always going to be there?  I think it use to be
> called tools_source.properties a while ago.

It might disappear but if that's the case, the configure script will fail and we will provide a fix. I don't think that's a big issue.

> How about just looking at the --with-android-sdk path?  Mine is:
> 
> ac_add_options
> --with-android-sdk="/builds/android_tools/android-sdk-linux_x86/platforms/
> android-13"
> 
> It is going to end with android-<version>

In my opinion, that's worse because nothing forces the user to have the sdk in a directory named 'android-X' and it doesn't solve the issue of different pattern because Google might change that directory name.

> Let kyle decide... as he is a peer

Ok.
Comment 4 Mike Hommey [:glandium] 2011-09-13 02:36:28 PDT
Comment on attachment 559897 [details] [diff] [review]
Patch v1

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

::: configure.in
@@ +303,5 @@
> +        # Minimum Android SDK API Level we require.
> +        android_min_api_level=13
> +
> +        # Get the api level from "$android_sdk"/source.properties.
> +        android_api_level=`grep -e "AndroidVersion\.ApiLevel" "$android_sdk"/source.properties | $AWK -F '=' '{ print $2 }'`

I'd go for:
android_api_level=`awk -F = '$1 == "AndroidVersion.ApiLevel" {print $2}' "$android_sdk"/source.properties`

@@ +305,5 @@
> +
> +        # Get the api level from "$android_sdk"/source.properties.
> +        android_api_level=`grep -e "AndroidVersion\.ApiLevel" "$android_sdk"/source.properties | $AWK -F '=' '{ print $2 }'`
> +
> +        if test $android_api_level -lt $android_min_api_level ; then

This test will fail if $android_api_level is empty or not a number, and this won't pop the error.

@@ +306,5 @@
> +        # Get the api level from "$android_sdk"/source.properties.
> +        android_api_level=`grep -e "AndroidVersion\.ApiLevel" "$android_sdk"/source.properties | $AWK -F '=' '{ print $2 }'`
> +
> +        if test $android_api_level -lt $android_min_api_level ; then
> +            AC_MSG_ERROR([You must use SDK API Level $android_min_api_level or higher (selected level is $android_api_level).])

I'd go for:
AC_MSG_ERROR([The given Android SDK provides API level $android_api_level ($android_min_api_level or higher required).])
Comment 5 Mounir Lamouri (:mounir) 2011-09-13 11:10:44 PDT
Created attachment 559994 [details] [diff] [review]
Patch v1.1
Comment 6 Mike Hommey [:glandium] 2011-09-13 15:25:25 PDT
Comment on attachment 559994 [details] [diff] [review]
Patch v1.1

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

::: configure.in
@@ +305,5 @@
> +
> +        # Get the api level from "$android_sdk"/source.properties.
> +        android_api_level=`$AWK -F = '$1 == "AndroidVersion.ApiLevel" {print $2}' "$android_sdk"/source.properties`
> +
> +        if test -z $android_api_level ; then

This test and the following are theorically unsafe without double quotes around the variable, though it's unlikely to make any problem (it would require the source.properties file to contain "AndroidVersion.ApiLevel=x y").
Comment 7 Mounir Lamouri (:mounir) 2011-09-13 15:37:01 PDT
Thanks for the review Mike :)
Comment 8 Matt Brubeck (:mbrubeck) 2011-09-14 06:58:31 PDT
https://hg.mozilla.org/mozilla-central/rev/aba82063bca7

Note You need to log in before you can comment on or make changes to this bug.