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.

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 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.

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.

(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?

(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 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.

Just pushed this to try, but I suspect it will fail until bug 976064 is finished. https://tbpl.mozilla.org/?tree=Try&rev=3022a3eccee3

(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.)

Added current version installed and what symbol is missing.

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!

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

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?

(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.