686350 - Check in configure script that the selected Android SDK is valid

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mounir Lamouri (:mounir)]

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)]

Attachment: 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)]

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)]

(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]]

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)]

Attachment: Patch v1.1

Comment 6

[Mike Hommey [:glandium]]

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)]

Thanks for the review Mike :)

Comment 8

[Matt Brubeck (:mbrubeck)]

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