Check in configure script that the selected Android SDK is valid

RESOLVED FIXED in mozilla9

Status

defect
RESOLVED FIXED
8 years ago
Last year

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla9

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted patch Patch v1 (obsolete) — Splinter Review
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?
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #559897 - Flags: review?(doug.turner)
Whiteboard: [needs review]
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
Attachment #559897 - Flags: review?(doug.turner) → review?(khuey)
(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 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).])
Attachment #559897 - Flags: review?(khuey) → review-
Posted patch Patch v1.1Splinter Review
Attachment #559897 - Attachment is obsolete: true
Attachment #559994 - Flags: review?(mh+mozilla)
Attachment #559994 - Flags: review?(khuey)
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").
Attachment #559994 - Flags: review?(mh+mozilla) → review+
Thanks for the review Mike :)
Flags: in-testsuite-
Whiteboard: [needs review] → [inbound]
https://hg.mozilla.org/mozilla-central/rev/aba82063bca7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.