1204260 - Pin to explicit versions of Android SDK platform and build-tools

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[Nick Alexander :nalexander [he/him]]

This ticket tracks pinning our build requirements to a particular platform (like android-22) and build-tools (like build-tools-22.0.1).

This is a companion to Bug 1108782.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1204260 - Pre: Move AAR searches later. r=nalexander

Bug 1204260 - Pre: Move AAR searches later. r=me

This merely groups the AAR searches in the configure output, which
reads a little easier.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1204260 - Pre: Don't expose ANDROID_{BUILD,PLATFORM}_TOOLS. r=glandium,gbrown

Bug 1204260 - Pre: Don't expose ANDROID_{BUILD,PLATFORM}_TOOLS. r?glandium,gbrown

This stops exposing ANDROID_BUILD_TOOLS and ANDROID_PLATFORM_TOOLS via
AC_SUBST.  We expose most tools already, and this adds EMULATOR, and
consumes it (and ADB) where appropriate.

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1204260 - Pin Android package versions to android-22 and build-tools-22.0.1. r?glandium

Bug 1204260 - Pin Android package versions to android-22 and build-tools-22.0.1. r?glandium

Right now, --with-android-sdk expects a path to a specific Android SDK
version, like /path/to/platforms/android-22.  That path is exposed as
ANDROID_SDK; the Android SDK root is exposed as ANDROID_SDK_ROOT.

Right now, the provided platform's version number is extracted into
ANDROID_TARGET_SDK.  The extracted ANDROID_TARGET_SDK is checked
against a minimum version number (supplied as a parameter to
MOZ_ANDROID_SDK).

After this patch, --with-android-sdk expects what is now
ANDROID_SDK_ROOT, and then derives ANDROID_SDK from that path and a
pinned SDK platform version number.  The exact version number which we
search for is now a parameter given to MOZ_ANDROID_SDK.  We accept and
warn if we recognize an old-style ANDROID_SDK path.

The existing MOZ_ANDROID_{MIN,MAX}_SDK_VERSION variables remain as
they are.

Right now, the Android build tools are searched in a deterministic but
non-obvious manner.  After this patch, the exact build tools version
number is now a parameter given to MOZ_ANDROID_SDK.

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1204260 - Post: remove platforms/android-* from --with-android-sdk. r?glandium

Bug 1204260 - Post: remove platforms/android-* from --with-android-sdk. r?glandium

The |mach bootstrap| code should continue to work (when it does work)
even though it produces a deprecated --with-android-sdk incantation.

Comment 5

[Geoff Brown [:gbrown]]

Comment on attachment 8660352 [details]
MozReview Request: Bug 1204260 - Pre: Don't expose ANDROID_{BUILD,PLATFORM}_TOOLS. r=glandium,gbrown

https://reviewboard.mozilla.org/r/19109/#review17019

::: testing/mozbase/mozrunner/mozrunner/devices/android_device.py:474
(Diff revision 1)
> +            # than something we find on the PATH or crawl for.

I have made a similar change to the search order in bug 1203627 (on inbound now). My new order is: 

$ANDROID_SDK_ROOT
ANDROID_TOOLS
~/.mozbuild
$PATH

You can change that to

EMULATOR/ADB
$ANDROID_SDK_ROOT
~/.mozbuild
$PATH

or similar, as long as $PATH is one of the last places checked.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8660352 [details]
MozReview Request: Bug 1204260 - Pre: Don't expose ANDROID_{BUILD,PLATFORM}_TOOLS. r=glandium,gbrown

https://reviewboard.mozilla.org/r/19109/#review17139

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8660353 [details]
MozReview Request: Bug 1204260 - Pin Android package versions to android-22 and build-tools-22.0.1. r?glandium

https://reviewboard.mozilla.org/r/19111/#review17141

::: build/autoconf/android.m4:311
(Diff revision 1)
> +AC_DEFUN([ANDROID_INSTALL_MSG], [mach android update sdk --all --no-ui --filter "$1"])

Command suggests are nice, but in all likeliness, what will happen to people like me who don't build often is:
- configure fails saying to install $foo
- install fo
- configure fails saying to install $bar
- install bar
- configure fails saying to install $qux
- ...

why not just suggest to run mach bootstrap, or a command that updates the sdk for _all_ our requirements?

::: build/autoconf/android.m4:334
(Diff revision 1)
> +        AC_MSG_WARN([Including platforms/android-* in --with-android-sdk arguments is deprecated.  Any platform specified is being ignored.])

Warnings are essentially invisible. It's better to error out if you want people's awareness.

::: build/autoconf/android.m4:337
(Diff revision 1)
> -        if test -z "$ANDROID_TARGET_SDK" ; then
> +    android_target_sdk=$1

This is where you enter the weirdness of the m4/shell mess: m4macro($foo) will replace $1 with $foo. In this case, the resulting configure will contain, literally: android_target_sdk=$android_target_sdk.

::: build/autoconf/android.m4:338
(Diff revision 1)
> -            AC_MSG_ERROR([Unexpected error: no AndroidVersion.ApiLevel field has been found in source.properties.])
> +    if ! test "$android_target_sdk" -eq "$android_target_sdk" ; then

shells will emit a noisy error here, it it's not a number.

::: configure.in:4091
(Diff revision 1)
> -    MOZ_ANDROID_SDK($android_min_api_level)
> +    MOZ_ANDROID_SDK($android_target_sdk, $android_build_tools_version)

Come to think of it, this would be better above the last ";; esac"

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8660354 [details]
MozReview Request: Bug 1204260 - Post: remove platforms/android-* from --with-android-sdk. r?glandium

https://reviewboard.mozilla.org/r/19113/#review17143

::: mobile/android/config/mozconfigs/common:42
(Diff revision 1)
>  else
>    ac_add_options --with-android-ndk="/tools/android-ndk-$ANDROID_NDK_VERSION_32BIT"
> -  ac_add_options --with-android-sdk="/tools/android-sdk-r$ANDROID_SDK_VERSION/platforms/android-$ANDROID_SDK_VERSION"
> +  ac_add_options --with-android-sdk="/tools/android-sdk-r$ANDROID_TARGET_SDK"
>  fi

I'm sure 98% this codepath already doesn't work. Might as well remove it (and remove the condition altogether, we haven't had 32-bits builders for a long while).

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #7)
> Comment on attachment 8660353 [details]
> MozReview Request: Bug 1204260 - Pin Android package versions to android-22
> and build-tools-22.0.1. r?glandium
> 
> https://reviewboard.mozilla.org/r/19111/#review17141
> 
> ::: build/autoconf/android.m4:311
> (Diff revision 1)
> > +AC_DEFUN([ANDROID_INSTALL_MSG], [mach android update sdk --all --no-ui --filter "$1"])
> 
> Command suggests are nice, but in all likeliness, what will happen to people
> like me who don't build often is:
> - configure fails saying to install $foo
> - install fo
> - configure fails saying to install $bar
> - install bar
> - configure fails saying to install $qux
> - ...
> 
> why not just suggest to run mach bootstrap, or a command that updates the
> sdk for _all_ our requirements?

Well, |mach bootstrap| is constantly broken, and I don't have time to fix it.  Further to that, it's broken 'cuz finding one command is non-trivial -- Google has broken us multiple times.  But whatever, I'll say run |mach bootstrap| and someday maybe it will work.

> ::: build/autoconf/android.m4:334
> (Diff revision 1)
> > +        AC_MSG_WARN([Including platforms/android-* in --with-android-sdk arguments is deprecated.  Any platform specified is being ignored.])
> 
> Warnings are essentially invisible. It's better to error out if you want
> people's awareness.

Will do.

> ::: build/autoconf/android.m4:337
> (Diff revision 1)
> > -        if test -z "$ANDROID_TARGET_SDK" ; then
> > +    android_target_sdk=$1
> 
> This is where you enter the weirdness of the m4/shell mess: m4macro($foo)
> will replace $1 with $foo. In this case, the resulting configure will
> contain, literally: android_target_sdk=$android_target_sdk.

OK, I was just getting lucky with my variable names.  Can you teach me what the solution to this issue is?  Is there a way to force evaluation of the macro argument?  Does one never use symbolic arguments?

> ::: build/autoconf/android.m4:338
> (Diff revision 1)
> > -            AC_MSG_ERROR([Unexpected error: no AndroidVersion.ApiLevel field has been found in source.properties.])
> > +    if ! test "$android_target_sdk" -eq "$android_target_sdk" ; then
> 
> shells will emit a noisy error here, it it's not a number.

This is copy-paste of existing stuff.  I think it's pointless.  How about I just drop it?

> ::: configure.in:4091
> (Diff revision 1)
> > -    MOZ_ANDROID_SDK($android_min_api_level)
> > +    MOZ_ANDROID_SDK($android_target_sdk, $android_build_tools_version)
> 
> Come to think of it, this would be better above the last ";; esac"

Sure.

Comment 10

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #9)
> (In reply to Mike Hommey [:glandium] from comment #7)
> > Comment on attachment 8660353 [details]
> > MozReview Request: Bug 1204260 - Pin Android package versions to android-22
> > and build-tools-22.0.1. r?glandium
> > 
> > https://reviewboard.mozilla.org/r/19111/#review17141
> > 
> > ::: build/autoconf/android.m4:311
> > (Diff revision 1)
> > > +AC_DEFUN([ANDROID_INSTALL_MSG], [mach android update sdk --all --no-ui --filter "$1"])
> > 
> > Command suggests are nice, but in all likeliness, what will happen to people
> > like me who don't build often is:
> > - configure fails saying to install $foo
> > - install fo
> > - configure fails saying to install $bar
> > - install bar
> > - configure fails saying to install $qux
> > - ...
> > 
> > why not just suggest to run mach bootstrap, or a command that updates the
> > sdk for _all_ our requirements?
> 
> Well, |mach bootstrap| is constantly broken, and I don't have time to fix
> it.  Further to that, it's broken 'cuz finding one command is non-trivial --
> Google has broken us multiple times.  But whatever, I'll say run |mach
> bootstrap| and someday maybe it will work.

Well, in this patch, you're giving a mach android update sdk command. Does that work better than mach bootstrap? If not, why point at it? (and why does it exist at all?)

> > ::: build/autoconf/android.m4:337
> > (Diff revision 1)
> > > -        if test -z "$ANDROID_TARGET_SDK" ; then
> > > +    android_target_sdk=$1
> > 
> > This is where you enter the weirdness of the m4/shell mess: m4macro($foo)
> > will replace $1 with $foo. In this case, the resulting configure will
> > contain, literally: android_target_sdk=$android_target_sdk.
> 
> OK, I was just getting lucky with my variable names.  Can you teach me what
> the solution to this issue is?  Is there a way to force evaluation of the
> macro argument?  Does one never use symbolic arguments?

The simplest is to just pass the literal values to the MOZ_ANDROID_SDK macro.

> > ::: build/autoconf/android.m4:338
> > (Diff revision 1)
> > > -            AC_MSG_ERROR([Unexpected error: no AndroidVersion.ApiLevel field has been found in source.properties.])
> > > +    if ! test "$android_target_sdk" -eq "$android_target_sdk" ; then
> > 
> > shells will emit a noisy error here, it it's not a number.
> 
> This is copy-paste of existing stuff.  I think it's pointless.  How about I
> just drop it?

Fair enough.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8660351 [details]
MozReview Request: Bug 1204260 - Pre: Move AAR searches later. r=nalexander

Bug 1204260 - Pre: Move AAR searches later. r=nalexander

This merely groups the AAR searches in the configure output, which
reads a little easier.

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8660352 [details]
MozReview Request: Bug 1204260 - Pre: Don't expose ANDROID_{BUILD,PLATFORM}_TOOLS. r=glandium,gbrown

Bug 1204260 - Pre: Don't expose ANDROID_{BUILD,PLATFORM}_TOOLS. r=glandium,gbrown

This stops exposing ANDROID_BUILD_TOOLS and ANDROID_PLATFORM_TOOLS via
AC_SUBST.  We expose most tools already, and this adds EMULATOR, and
consumes it (and ADB) where appropriate.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8660353 [details]
MozReview Request: Bug 1204260 - Pin Android package versions to android-22 and build-tools-22.0.1. r?glandium

Bug 1204260 - Pin Android package versions to android-22 and build-tools-22.0.1. r?glandium

Right now, --with-android-sdk expects a path to a specific Android SDK
version, like /path/to/platforms/android-22.  That path is exposed as
ANDROID_SDK; the Android SDK root is exposed as ANDROID_SDK_ROOT.

Right now, the provided platform's version number is extracted into
ANDROID_TARGET_SDK.  The extracted ANDROID_TARGET_SDK is checked
against a minimum version number (supplied as a parameter to
MOZ_ANDROID_SDK).

After this patch, --with-android-sdk expects what is now
ANDROID_SDK_ROOT, and then derives ANDROID_SDK from that path and a
pinned SDK platform version number.  The exact version number which we
search for is now a parameter given to MOZ_ANDROID_SDK.  We accept and
fail, with a helpful message, if we recognize an old-style ANDROID_SDK
path.

The existing MOZ_ANDROID_{MIN,MAX}_SDK_VERSION variables remain as
they are.

Right now, the Android build tools are searched in a deterministic but
non-obvious manner.  After this patch, the exact build tools version
number is now a parameter given to MOZ_ANDROID_SDK.

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8660354 [details]
MozReview Request: Bug 1204260 - Post: remove platforms/android-* from --with-android-sdk. r?glandium

Bug 1204260 - Post: remove platforms/android-* from --with-android-sdk. r?glandium

Comment 15

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1204260 - Post: remove platforms/android-* from |mach bootstrap|. r?glandium

Bug 1204260 - Post: remove platforms/android-* from |mach bootstrap|. r?glandium

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8660353 [details]
MozReview Request: Bug 1204260 - Pin Android package versions to android-22 and build-tools-22.0.1. r?glandium

https://reviewboard.mozilla.org/r/19111/#review18009

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8660354 [details]
MozReview Request: Bug 1204260 - Post: remove platforms/android-* from --with-android-sdk. r?glandium

https://reviewboard.mozilla.org/r/19113/#review18011

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8664392 [details]
MozReview Request: Bug 1204260 - Post: remove platforms/android-* from |mach bootstrap|. r?glandium

https://reviewboard.mozilla.org/r/19949/#review18013

::: python/mozboot/mozboot/android.py
(Diff revision 1)
> -    'extra-android-support',
> -    'extra-google-google_play_services',

Why are these packages removed as well?

Comment 19

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #18)
> Comment on attachment 8664392 [details]
> MozReview Request: Bug 1204260 - Post: remove platforms/android-* from |mach
> bootstrap|. r?glandium
> 
> https://reviewboard.mozilla.org/r/19949/#review18013
> 
> ::: python/mozboot/mozboot/android.py
> (Diff revision 1)
> > -    'extra-android-support',
> > -    'extra-google-google_play_services',
> 
> Why are these packages removed as well?

These supplied the old style libraries; after this patch, they're unused.  (Yay!  They were unversioned.)  The dependencies are supplied by the m2repository packages, which have always supplied the AAR dependencies for Gradle builds.  (Yay!  The AAR files are versioned.)

I don't think this patch needs your re-review.  I have additional patches, with mcomella's testing and r+, to address |mach bootstrap| thoroughly.

Thanks for teaching me some m4 and autoconf!

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8664392 [details]
MozReview Request: Bug 1204260 - Post: remove platforms/android-* from |mach bootstrap|. r?glandium

https://reviewboard.mozilla.org/r/19949/#review18017

Thanks for the clarification

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4376e4945e8b
https://hg.mozilla.org/integration/fx-team/rev/a0088558009a
https://hg.mozilla.org/integration/fx-team/rev/ddb298c32415
https://hg.mozilla.org/integration/fx-team/rev/e5ab2152fb00
https://hg.mozilla.org/integration/fx-team/rev/4d2b605ee1fa

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4376e4945e8b
https://hg.mozilla.org/mozilla-central/rev/a0088558009a
https://hg.mozilla.org/mozilla-central/rev/ddb298c32415
https://hg.mozilla.org/mozilla-central/rev/e5ab2152fb00
https://hg.mozilla.org/mozilla-central/rev/4d2b605ee1fa