990161 - Check support library version at configure time

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Josh Dover [:jdover]]

We are updating the Android Support Library to 19.1.0, which requires developers to make sure they have the correct version installed before building. Add a check for this version before building to give a nice error message as to why the build would have failed.

Comment 1

[Josh Dover [:jdover]]

Attachment: patch

This is my first change to any build / configure stuff so I am not sure if this is the place for this check. Also, I am pretty sure that REQUIRED_COMPAT_LIB_VERSION should live somewhere else, but I am not sure where the appropriate place is.

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8400348 [details] [diff] [review]
patch

Review of attachment 8400348 [details] [diff] [review]:
-----------------------------------------------------------------

glandium, we're trying to require version 19.1.0 or higher of the Android support library.  Do you see anything bad here, and do you know of existing m4-fu to do the version check better than splitting on . and manually comparing?

::: build/autoconf/android.m4
@@ +328,5 @@
> +    else
> +        ANDROID_COMPAT_LIB_VERSION=`$AWK -F = changequote(<<, >>)'<<$>>1 == "Pkg.Revision" {print <<$>>2}'changequote([, ]) ${ANDROID_SDK_ROOT}/extras/android/support/source.properties`
> +    fi
> +
> +    REQUIRED_COMPAT_LIB_VERSION=19.1.0

Looks good, except this is very fragile; we really want to take any version >= 19.1.0.  Which, of course, is irritating to check.  Redirecting to :glandium to see if he knows a way to check versions smoothly.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

I think we should just check features instead of version numbers. Something like

> if (unzip -l android-support-v4.jar | grep -q 'SwipeRefreshLayout\.class'); then ...

With a comment saying which version the check corresponds to.

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Jim Chen [:jchen :nchen] from comment #3)
> I think we should just check features instead of version numbers. Something
> like
> 
> > if (unzip -l android-support-v4.jar | grep -q 'SwipeRefreshLayout\.class'); then ...
> 
> With a comment saying which version the check corresponds to.

I suggested this at some time and am happy to see it down this way.  Can we actually test for file existence, and give the full path to avoid code moving around?

Comment 5

[Josh Dover [:jdover]]

Attachment: patch - test for required symbols

(In reply to Nick Alexander :nalexander from comment #4)
> (In reply to Jim Chen [:jchen :nchen] from comment #3)
> > I think we should just check features instead of version numbers. Something
> > like
> > 
> > > if (unzip -l android-support-v4.jar | grep -q 'SwipeRefreshLayout\.class'); then ...
> > 
> > With a comment saying which version the check corresponds to.
> 
> I suggested this at some time and am happy to see it down this way.  Can we
> actually test for file existence, and give the full path to avoid code
> moving around?

This version does a grep for the class file within the jar's file listing without
unzipping the whole jar.

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8402828 [details] [diff] [review]
patch - test for required symbols

Review of attachment 8402828 [details] [diff] [review]:
-----------------------------------------------------------------

If it works for found and missing in your testing, it works for me.  Before we push, let's make sure we're green on try.  No sense busting builds.

::: build/autoconf/android.m4
@@ +322,5 @@
>      if ! test -e $ANDROID_COMPAT_LIB ; then
>          AC_MSG_ERROR([You must download the Android support library when targeting Android.   Run the Android SDK tool and install Android Support Library under Extras.  See https://developer.android.com/tools/extras/support-library.html for more info. (looked for $ANDROID_COMPAT_LIB)])
>      fi
>  
> +    if ! unzip -l $ANDROID_COMPAT_LIB | grep -lq "SwipeRefreshLayout.class" ; then

Let's extract the class name to a variable, and give the full path to the .class, like:

ANDROID_COMPAT_LIB_REQUIRED_CLASS="android/support/v4/print/PrintHelper.class"

or whatever it is.

@@ +323,5 @@
>          AC_MSG_ERROR([You must download the Android support library when targeting Android.   Run the Android SDK tool and install Android Support Library under Extras.  See https://developer.android.com/tools/extras/support-library.html for more info. (looked for $ANDROID_COMPAT_LIB)])
>      fi
>  
> +    if ! unzip -l $ANDROID_COMPAT_LIB | grep -lq "SwipeRefreshLayout.class" ; then
> +        AC_MSG_ERROR([You must have at version 19.1.0 of the Android support library when targeting Android.  Run the Android SDK tool and update the Android Support Library under Extras.  See https://developer.android.com/tools/extras/support-library.html for more info.])

nit: at least.

Comment 7

[Josh Dover [:jdover]]

Attachment: patch

Just pushed this to try, but I suspect it will fail until bug 976064 is finished.

https://tbpl.mozilla.org/?tree=Try&rev=3022a3eccee3

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Josh Dover [:jdover] from comment #7)
> Created attachment 8403015 [details] [diff] [review]
> patch
> 
> Just pushed this to try, but I suspect it will fail until bug 976064 is
> finished.

Yeah.  It does show, however, that the error message should include the current version (fished using the old code, perhaps?) and explain what class was missing.  We expect lots of people to see the error for a short time, so let's make it informative.  (And set the standard for the next time we bump the required version.)

Comment 9

[Josh Dover [:jdover]]

Attachment: patch

Added current version installed and what symbol is missing.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8403481 [details] [diff] [review]
patch

Review of attachment 8403481 [details] [diff] [review]:
-----------------------------------------------------------------

Gentle preference for doing the version outside the if, but nbd.  Now just need releng to update build slaves and a green try!

Comment 11

[Nick Alexander :nalexander [he/him]]

jdover: this should depend on a releng bug to update build slaves, right?

Comment 12

[Josh Dover [:jdover]]

Because we are lifting the source for SwipeRefreshLayout, we do not need to upgrade the support library anymore. Should we still add this check, but for a lower version?

Comment 13

[Nick Alexander :nalexander [he/him]]

(In reply to Josh Dover [:jdover] from comment #12)
> Because we are lifting the source for SwipeRefreshLayout, we do not need to
> upgrade the support library anymore. Should we still add this check, but for
> a lower version?

What would we be checking for?  I'd prefer configure to be as fast as possible, so let's land this only if there's tangible value.