Refactor and rename install-build-deps-android-sdk

Fix and refactor android install-deps-sdk
    
    - Rewrote install sdk script in python
    - Removing the trivial bb_install fun
    - Renaming the script to better explain what it does
    - Removed the second call to bb_install_build_deps in the buildbot
      shell scripts.  This is also called in bb_setup!
    - Removing a bunch of unnecessary logic in the script
    - Added an versioning system function to clear the sdk/ndk directories
      when necessary.  Using this to clear SDK/NDK on all bots and redeploy.
    - Fixed issue where SDK_ROOT and NDK_ROOT were required to have
      specific names for the base directories.

[Isaac (away), 2012-07-01 23:05:04]

Have some lint warnings but uploaded anyway for initial review

[bulach, 2012-07-02 10:19:40]

as mentioned in the other patch, not too familiar but I like this
simplification, thanks!!

just some drive-by suggestions:

https://chromiumcodereview.appspot.com/10694048/diff/5002/build/install-andro...
File build/install-android-sdk-ndk.sh (right):

https://chromiumcodereview.appspot.com/10694048/diff/5002/build/install-andro...
build/install-android-sdk-ndk.sh:32: mkdir "$base_dir"
nit: -p

https://chromiumcodereview.appspot.com/10694048/diff/5002/build/install-andro...
build/install-android-sdk-ndk.sh:47: ### FUNCTION DEFINITIONS - SCRIPT CONTINUES
AT BOTTOM ###
:)
tbh, it'd be better to create functions for the script above as well...
like:
put 14-33 in something like:
function setup_install {
}

^^^ this function should probably have the SDK_TARGET_ID / URL / MD5SUM, so it's
clearer that when changing any of those values, one should also tweak the
SCRIPT_VERSION variable..

then 35-44 in:
function check_version_file {
}

[Isaac (away), 2012-07-03 06:49:59]

thanks for feedback, will have another draft in a day or two!

https://chromiumcodereview.appspot.com/10694048/diff/5002/build/install-andro...
File build/install-android-sdk-ndk.sh (right):

https://chromiumcodereview.appspot.com/10694048/diff/5002/build/install-andro...
build/install-android-sdk-ndk.sh:32: mkdir "$base_dir"
On 2012/07/02 10:19:41, bulach wrote:
> nit: -p

Wanted the script to try to create one level and then terminate if failure. 
Will add better commenting and disable this on the buildbot --auto flag

https://chromiumcodereview.appspot.com/10694048/diff/5002/build/install-andro...
build/install-android-sdk-ndk.sh:47: ### FUNCTION DEFINITIONS - SCRIPT CONTINUES
AT BOTTOM ###
On 2012/07/02 10:19:41, bulach wrote:
> :)
> tbh, it'd be better to create functions for the script above as well...
> like:
> put 14-33 in something like:
> function setup_install {
> }
> 
> ^^^ this function should probably have the SDK_TARGET_ID / URL / MD5SUM, so
it's
> clearer that when changing any of those values, one should also tweak the
> SCRIPT_VERSION variable..
> 
> then 35-44 in:
> function check_version_file {
> }

Sounds good, will do.

[Yaron, 2012-07-10 05:18:49]

please ping back when this is ready for review (sounds like you have more
changes in the pipe)

[Isaac (away), 2012-07-17 10:47:26]

OK now ready for review.

[bulach, 2012-07-17 11:27:20]

good stuff, thanks isaac! I have a few suggestion below, mostly to clarify the
"version" term in here that seems a bit ambiguous... let me know your thoughts:

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
File build/install_android_sdk_ndk.py (right):

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:20: 'version': 1}
nit: "version" here seems related to the package version itself... is it
necessary per package?
how about a single global SCRIPT_REVISION, with comments saying something like 
"# Bump this number when changing the configs to trigger a re-installation."

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:39: def GetRoot(config):
nit: maybe clearer as "GetInstallPathForConfig". more below.

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:44: raw_input('%s not set.  Hit enter to use
%s' % (env_var_name,
nit: since this is going to be used by bots, it'd be better to avoid any
interaction.. it'd be simpler as:

path = os.environ.get(env_var_name, default_path)

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:54: def VersionedInstall(root, version,
installer, options):
nit: rather than root, perhaps install_dir would be clearer (root looks like
chrome/src or CLANK_DIR)..

also, as above, the "version" seems conflicting with the actual package
version.. it seems that this script has two modes:
1. "forced" (default): will overinstall regardless.
2. "refresh" (aka --version): will re-install only if this script has changed..

would it be clearer to call this "RefreshInstall" ? my point is that it's always
"VersionedInstall" because we're installing a very specific version of the
packages. :)

also, if we don't need separate "versions" for sdk / ndk, it'd be simpler to
have something like this on main():

if not options.refresh or NeedsRefresh():
  InstallSdk(sdk_root, options)
  InstallNdk(ndk_root, options)

so that this function only checks for a SCRIPT_REVISION file and returns True
whether or not we should refresh the installation..

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:61: except IOError:
probably need ValueError as well for the int cast.

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:112: def Android(args, stdin=None):
nit: remove the default value, there's only one caller that needs to specify..

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:176: parser.add_option('--versioned',
action='store_true')
nit: see above, I think this would be clearer as --refresh..

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:179: parser.add_option('--src-root',
default=os.path.realpath(__file__ + '/../..'))
nit: probably best to keep as dry_run and src_root so a simple grep can find all
usages..

[Isaac (away), 2012-07-17 16:35:30]

+cc jrg

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
File build/install_android_sdk_ndk.py (right):

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:44: raw_input('%s not set.  Hit enter to use
%s' % (env_var_name,
If a user calls the script I want to confirm before using certain paths, but the
bot should be setting up their environment properly.  

On 2012/07/17 11:27:20, bulach wrote:
> nit: since this is going to be used by bots, it'd be better to avoid any
> interaction.. it'd be simpler as:
> 
> path = os.environ.get(env_var_name, default_path)

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:61: except IOError:
On 2012/07/17 11:27:20, bulach wrote:
> probably need ValueError as well for the int cast.

IOError is expected because file might not exist.  ValueError should never
happen because file is created inside this script.  Not sure want to catch that.

[John Grabowski, 2012-07-17 18:38:15]

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/android/buil...
File build/android/buildbot_functions.sh (right):

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/android/buil...
build/android/buildbot_functions.sh:95:
"$SRC_ROOT/build/install_android_sdk_ndk.py" --versioned
Don't like removal of directory check.
Prefer to avoid running script if possible.
Saves time.

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/android/envs...
File build/android/envsetup.sh (right):

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/android/envs...
build/android/envsetup.sh:16: if [[ -z $ANDROID_NDK_ROOT || -z $ANDROID_SDK_ROOT
\
If you don't quote these, then you will get the OPPOSITE result of what you
expect if the variables are not set.

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
File build/install_android_sdk_ndk.py (right):

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:44: raw_input('%s not set.  Hit enter to use
%s' % (env_var_name,
On 2012/07/17 11:27:20, bulach wrote:
> nit: since this is going to be used by bots, it'd be better to avoid any
> interaction.. it'd be simpler as:
> 
> path = os.environ.get(env_var_name, default_path)

Strongly agree.  Raw input is bad form.

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:54: def VersionedInstall(root, version,
installer, options):
Looks like you're adding undocumented features that can cause mysterious
behavior on bots.  I don't like this either.

[Isaac (away), 2012-07-18 09:45:05]

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/android/buil...
File build/android/buildbot_functions.sh (right):

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/android/buil...
build/android/buildbot_functions.sh:95:
"$SRC_ROOT/build/install_android_sdk_ndk.py" --versioned
On 2012/07/17 18:38:15, John Grabowski wrote:
> Don't like removal of directory check.
> Prefer to avoid running script if possible.
> Saves time.

Script is a no-op for install NDK if directory exists.  So will not really cost
any time.  I would rather consolidate all the logic for installing sdk / ndk
into a single place (i.e. the script)

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/android/envs...
File build/android/envsetup.sh (right):

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/android/envs...
build/android/envsetup.sh:16: if [[ -z $ANDROID_NDK_ROOT || -z $ANDROID_SDK_ROOT
\
On 2012/07/17 18:38:15, John Grabowski wrote:
> If you don't quote these, then you will get the OPPOSITE result of what you
> expect if the variables are not set.

I thought quotes were unnecessary in double bracket tests?  Why would I get
opposite result?

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
File build/install_android_sdk_ndk.py (right):

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:20: 'version': 1}
On 2012/07/17 11:27:20, bulach wrote:
> nit: "version" here seems related to the package version itself... is it
> necessary per package?
> how about a single global SCRIPT_REVISION, with comments saying something like

> "# Bump this number when changing the configs to trigger a re-installation."

Will fix.

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:39: def GetRoot(config):
On 2012/07/17 11:27:20, bulach wrote:
> nit: maybe clearer as "GetInstallPathForConfig". more below.

OK will adjust

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:44: raw_input('%s not set.  Hit enter to use
%s' % (env_var_name,
On 2012/07/17 18:38:15, John Grabowski wrote:
> On 2012/07/17 11:27:20, bulach wrote:
> > nit: since this is going to be used by bots, it'd be better to avoid any
> > interaction.. it'd be simpler as:
> > 
> > path = os.environ.get(env_var_name, default_path)
> 
> Strongly agree.  Raw input is bad form.

I want to distinguish between an automated 'bot mode' and user mode.  For users
running this script to install the sdk and ndk, a confirmation before setting up
and installing to a default folder seems reasonable.

For bots, this will result in a timeout but should never happen as long as we
don't mess up scripts to not set path.  But I could add another option --auto
and fail rather than raw_input here.

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:54: def VersionedInstall(root, version,
installer, options):
On 2012/07/17 18:38:15, John Grabowski wrote:
> Looks like you're adding undocumented features that can cause mysterious
> behavior on bots.  I don't like this either.

Well, I'm happy to document it before landing.  forced and refresh are quite
different paradigms than what's going on here:

- in 'versioned', we try to ensure that all bots have exactly the same SDK and
NDK folders, rather than the current state which accumulates SDK targets in the
older bots.  We also will not auto upgrade the sdk tools without a version
number bump.  If the SDK tools releases an r21 which breaks our build, I don't
want all the bots to go red.  This version number prevents that (although
deploying new bots would be broken until underlying issues are fixed)

- In unversioned, we don't delete the sdk directory, since the user might want
the targets for other applications.  We always upgrade to the latest version of
things.

[John Grabowski, 2012-07-18 20:48:43]

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/android/envs...
File build/android/envsetup.sh (right):

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/android/envs...
build/android/envsetup.sh:16: if [[ -z $ANDROID_NDK_ROOT || -z $ANDROID_SDK_ROOT
\
On 2012/07/18 09:45:05, Isaac wrote:
> On 2012/07/17 18:38:15, John Grabowski wrote:
> > If you don't quote these, then you will get the OPPOSITE result of what you
> > expect if the variables are not set.
> 
> I thought quotes were unnecessary in double bracket tests?  Why would I get
> opposite result?

You tell me.  Run the script below for a hint.


#!/bin/bash

if [[ -z $UNDEFINED ]]; then
  echo UNDEFINED
fi

if [[ -z $"{UNDEFINED}" ]]; then
  echo UNDEFINED_QUOTED
fi

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
File build/install_android_sdk_ndk.py (right):

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:44: raw_input('%s not set.  Hit enter to use
%s' % (env_var_name,
On 2012/07/18 09:45:05, Isaac wrote:
> On 2012/07/17 18:38:15, John Grabowski wrote:
> > On 2012/07/17 11:27:20, bulach wrote:
> > > nit: since this is going to be used by bots, it'd be better to avoid any
> > > interaction.. it'd be simpler as:
> > > 
> > > path = os.environ.get(env_var_name, default_path)
> > 
> > Strongly agree.  Raw input is bad form.
> 
> I want to distinguish between an automated 'bot mode' and user mode.  For
users
> running this script to install the sdk and ndk, a confirmation before setting
up
> and installing to a default folder seems reasonable.
> 
> For bots, this will result in a timeout but should never happen as long as we
> don't mess up scripts to not set path.  But I could add another option --auto
> and fail rather than raw_input here.

No.  No option.  Just no raw input.  If user doesn't want the behavior of the
script, don't run it.

https://chromiumcodereview.appspot.com/10694048/diff/13001/build/install_andr...
build/install_android_sdk_ndk.py:54: def VersionedInstall(root, version,
installer, options):
On 2012/07/18 09:45:05, Isaac wrote:
> On 2012/07/17 18:38:15, John Grabowski wrote:
> > Looks like you're adding undocumented features that can cause mysterious
> > behavior on bots.  I don't like this either.
> 
> Well, I'm happy to document it before landing.  forced and refresh are quite
> different paradigms than what's going on here:
> 
> - in 'versioned', we try to ensure that all bots have exactly the same SDK and
> NDK folders, rather than the current state which accumulates SDK targets in
the
> older bots.  We also will not auto upgrade the sdk tools without a version
> number bump.  If the SDK tools releases an r21 which breaks our build, I don't
> want all the bots to go red.  This version number prevents that (although
> deploying new bots would be broken until underlying issues are fixed)
> 
> - In unversioned, we don't delete the sdk directory, since the user might want
> the targets for other applications.  We always upgrade to the latest version
of
> things.

You missed the point.
This is unnecessary complexity.
Don't do it.

[Isaac (away), 2012-07-19 13:22:23]

OK let's take a step back on the version number and I'll explain the background
behind this change.

Android SDK tools are unversioned, you can only update to the latest version and
they do not have a good track record of maintaining backwards compatibility.  So
it's dangerous to always have every bot upgrade to the latest version of tools,
and it's time consuming (about 60s) to check for updates each build.

The current sdk installer script will only update tools when we change the sdk
version in the script.  That means they don't have to check for updates all the
time, but as a consequence different bots have different versions of the SDK
tools and different revisions of a given sdk target.

One proposition was to put the SDK tools into a repo and roll that when
necessary.  This script essentially does this, but built into the script.

[John Grabowski, 2012-07-19 17:26:07]

Let's put them in a repo.

jrg


On Thu, Jul 19, 2012 at 6:22 AM, <ilevy@chromium.org> wrote:

> OK let's take a step back on the version number and I'll explain the
> background
> behind this change.
>
> Android SDK tools are unversioned, you can only update to the latest
> version and
> they do not have a good track record of maintaining backwards
> compatibility.  So
> it's dangerous to always have every bot upgrade to the latest version of
> tools,
> and it's time consuming (about 60s) to check for updates each build.
>
> The current sdk installer script will only update tools when we change the
> sdk
> version in the script.  That means they don't have to check for updates
> all the
> time, but as a consequence different bots have different versions of the
> SDK
> tools and different revisions of a given sdk target.
>
> One proposition was to put the SDK tools into a repo and roll that when
> necessary.  This script essentially does this, but built into the script.
>
>
https://chromiumcodereview.**appspot.com/10694048/<https://chromiumcodereview...
>

[Isaac (away), 2012-09-02 04:53:45]

abandoning