The default bug view has changed. See this FAQ.

Check in configure script that the selected Android SDK is valid

RESOLVED FIXED in mozilla9

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla9
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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?
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #559897 - Flags: review?(doug.turner)
(Assignee)

Updated

6 years ago
Whiteboard: [needs review]

Comment 2

6 years ago
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)
(Assignee)

Comment 3

6 years ago
(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-
(Assignee)

Comment 5

6 years ago
Created attachment 559994 [details] [diff] [review]
Patch v1.1
Attachment #559897 - Attachment is obsolete: true
Attachment #559994 - Flags: review?(mh+mozilla)
(Assignee)

Updated

6 years ago
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+
Attachment #559994 - Flags: review?(khuey)
(Assignee)

Comment 7

6 years ago
Thanks for the review Mike :)
Flags: in-testsuite-
Whiteboard: [needs review] → [inbound]
https://hg.mozilla.org/mozilla-central/rev/aba82063bca7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.