Closed Bug 1204260 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

(firefox43 affected, firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

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.
Bug 1204260 - Pre: Move AAR searches later. r=me

This merely groups the AAR searches in the configure output, which
reads a little easier.
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.
Attachment #8660352 - Flags: review?(mh+mozilla)
Attachment #8660352 - Flags: review?(gbrown)
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.
Attachment #8660353 - Flags: review?(mh+mozilla)
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.
Attachment #8660354 - Flags: review?(mh+mozilla)
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.
Attachment #8660352 - Flags: review?(gbrown) → review+
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
Attachment #8660352 - Flags: review?(mh+mozilla) → review+
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"
Attachment #8660353 - Flags: review?(mh+mozilla)
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).
Attachment #8660354 - Flags: review?(mh+mozilla)
(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.
Flags: needinfo?(mh+mozilla)
(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.
Flags: needinfo?(mh+mozilla)
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.
Attachment #8660351 - Attachment description: MozReview Request: Bug 1204260 - Pre: Move AAR searches later. r=me → MozReview Request: Bug 1204260 - Pre: Move AAR searches later. r=nalexander
Attachment #8660351 - Flags: review?(nalexander)
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.
Attachment #8660352 - Attachment description: MozReview Request: Bug 1204260 - Pre: Don't expose ANDROID_{BUILD,PLATFORM}_TOOLS. r?glandium,gbrown → MozReview Request: Bug 1204260 - Pre: Don't expose ANDROID_{BUILD,PLATFORM}_TOOLS. r=glandium,gbrown
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.
Attachment #8660353 - Flags: review?(mh+mozilla)
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
Attachment #8660354 - Flags: review?(mh+mozilla)
Bug 1204260 - Post: remove platforms/android-* from |mach bootstrap|. r?glandium
Attachment #8664392 - Flags: review?(mh+mozilla)
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
Attachment #8660353 - Flags: review?(mh+mozilla) → review+
Attachment #8660354 - Flags: review?(mh+mozilla) → review+
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 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?
Attachment #8664392 - Flags: review?(mh+mozilla)
(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 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
Attachment #8664392 - Flags: review+
Attachment #8660351 - Flags: review?(nalexander)
Blocks: 1207680
Blocks: 1197147
Blocks: 1187974
Blocks: 1212913
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 44 → mozilla44
You need to log in before you can comment on or make changes to this bug.