Add asset management scripts

Add asset management scripts

These provide an easy way to create assets to be used by bots,
eg. Android SDK.

To create an asset:
$ infra/bots/assets/assets.py add android_sdk
(adds scripts in infra/bots/assets/android_sdk)

To upload a new version of an asset:
$ infra/bots/assets/android_sdk/upload.py -t $ANDROID_SDK_ROOT
(uploads Android SDK to GS, writes a version file)
$ git commit
$ git cl upload

To download the current version of the asset:
$ infra/bots/assets/android_sdk/download.py -t ../tmp

BUG=skia:5427
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2069543002

Committed: https://skia.googlesource.com/skia/+/0f1469bcdad11cf8bfe79ace33d28052418ecb48

[borenet2, 2016-06-14 08:22:30]

Description was changed from

==========
Add asset_utils, android SDK create and download scripts

BUG=skia:5427
==========

to

==========
Add asset_utils, android SDK create and download scripts

BUG=skia:5427
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2069543002
==========

[borenet, 2016-06-15 09:44:43]

Description was changed from

==========
Add asset_utils, android SDK create and download scripts

BUG=skia:5427
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2069543002
==========

to

==========
Add asset management scripts

These provide an easy way to create assets to be used by bots,
eg. Android SDK.

To create an asset:
$ infra/bots/assets/assets.py add android_sdk
(adds scripts in infra/bots/assets/android_sdk)

To upload a new version of an asset:
$ infra/bots/assets/android_sdk/upload.py -t $ANDROID_SDK_ROOT
(uploads Android SDK to GS, writes a version file)
$ git commit
$ git cl upload

To download the current version of the asset:
$ infra/bots/assets/android_sdk/download.py -t ../tmp

BUG=skia:5427
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2069543002
==========

[borenet, 2016-06-15 10:24:00]

borenet@google.com changed reviewers:
+ borenet@google.com, kjlubick@google.com, rmistry@google.com,
stephana@google.com

[borenet, 2016-06-15 10:24:37]

Here's an example CL created using assets.py to add the Android SDK asset:
https://codereview.chromium.org/2067133003/

[stephana, 2016-06-15 12:32:19]

https://codereview.chromium.org/2069543002/diff/320001/infra/bots/assets/READ...
File infra/bots/assets/README.md (right):

https://codereview.chromium.org/2069543002/diff/320001/infra/bots/assets/READ...
infra/bots/assets/README.md:29: Add script to automate creation of this asset?
(y/n) n
It would be helpful for anybody coming to this without the knowledge of it's
origin to see what the 'add' command does. 
So after the "Add script to atomate ..." prompt it could output a line items
saying something like:

Creating asset myasset in /path/to/asset
Copying create script to /path/to/asset/copy.py
...

That would be helpful for the user of the script as well as this documentation.

[borenet, 2016-06-15 12:43:10]

https://codereview.chromium.org/2069543002/diff/320001/infra/bots/assets/READ...
File infra/bots/assets/README.md (right):

https://codereview.chromium.org/2069543002/diff/320001/infra/bots/assets/READ...
infra/bots/assets/README.md:29: Add script to automate creation of this asset?
(y/n) n
On 2016/06/15 12:32:19, stephana wrote:
> It would be helpful for anybody coming to this without the knowledge of it's
> origin to see what the 'add' command does. 
> So after the "Add script to atomate ..." prompt it could output a line items
> saying something like:
> 
> Creating asset myasset in /path/to/asset
> Copying create script to /path/to/asset/copy.py
> ...
> 
> That would be helpful for the user of the script as well as this
documentation. 

Done.

[kjlubick, 2016-06-15 12:47:53]

lgtm

[rmistry, 2016-06-15 13:47:18]

This is a good idea. L-G-T-M for design. A few code comments below.

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/READ...
File infra/bots/assets/README.md (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/READ...
infra/bots/assets/README.md:41: $ infra/bots/assets/myasset/create_and_upload.py
Nit: Do you need \_ here as well?
Also in create_asset and MY_ASSET_LOCATION above.

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/asse...
File infra/bots/assets/asset_utils.py (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/asse...
infra/bots/assets/asset_utils.py:117: with utils.chdir(SKIA_DIR):
Does this have to go to SKIA_DIR for the below to work?
If not then you could instead do at the top of the module (like asset.py)-

FILE_DIR = os.path.dirname(os.path.abspath(__file__))
INFRA_BOTS_DIR = os.path.realpath(os.path.join(FILE_DIR, os.pardir))

and use INFRA_BOTS_DIR instead here and on line 64.

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
File infra/bots/assets/scripts/create_and_upload.py (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
infra/bots/assets/scripts/create_and_upload.py:9: """Create the asset and upload
it."""
[Optional]
Will need more maintenance work to keep this script around since you already
have create.py and upload.py
I personally would just use those two scripts but upto you.

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
File infra/bots/assets/scripts/download.py (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
infra/bots/assets/scripts/download.py:25: assets.main(['download', ASSET_NAME] +
sys.argv[1:])
sys.exit(...) here and in all scripts with "if __name__ == '__main__'"
MA always asks for this and I tend to agree.

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
File infra/bots/assets/scripts/upload.py (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
infra/bots/assets/scripts/upload.py:18: sys.path.insert(0, INFRA_BOTS_DIR)
The top 4 lines are in 3 scripts. Might be able to extract into a util and add a
add_infra_to_path() function. I remember seeing that pattern in telemetry
somewhere but now I cannot find it to show you an example :/

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/zip_utils.py
File infra/bots/zip_utils.py (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/zip_utils.p...
infra/bots/zip_utils.py:19: rv = names[:]
Just curious, why is this called rv?

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/zip_utils.p...
infra/bots/zip_utils.py:25: def zip(target_dir, zip_file, blacklist=None):  #
pylint: disable=W0622
I think this will silently pass if target_dir does not exist. Maybe it should
throw an exception to prevent surprises? Could also add a test for that.

For comparison the below unzip function will correctly throw an IOError if the
zip file does not exist. Could also add a test to ensure that does not get
regressed.

[stephana, 2016-06-15 13:57:30]

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/READ...
File infra/bots/assets/README.md (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/READ...
infra/bots/assets/README.md:29: Add script to automate creation of this asset?
(y/n) n
Can you update this example to include the new FYI information ?

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/asse...
File infra/bots/assets/assets.py (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/asse...
infra/bots/assets/assets.py:1: #!/usr/bin/env python
Why not merge asset_utils in here? It looks like it is only imported by
assets.py. So assets.py could be the script and the module in one.

[stephana, 2016-06-15 14:05:41]

https://codereview.chromium.org/2069543002/diff/360001/infra/bots/assets/asse...
File infra/bots/assets/asset_utils.py (right):

https://codereview.chromium.org/2069543002/diff/360001/infra/bots/assets/asse...
infra/bots/assets/asset_utils.py:55: class Asset(object):
It would be useful to add a 'remove' function that removes a version. Even if we
don't want to use that in production it would be handy in testing this module. 

IMO, this needs tests. It's in the critical path, so when we extend this in the
future we want to be able to test against regressions.

[borenet, 2016-06-15 14:31:28]

Addressed most comments in patch set 21.  Working on adding tests for
asset_utils.

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/READ...
File infra/bots/assets/README.md (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/READ...
infra/bots/assets/README.md:29: Add script to automate creation of this asset?
(y/n) n
On 2016/06/15 13:57:30, stephana wrote:
> Can you update this example to include the new FYI information ? 

Done.

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/READ...
infra/bots/assets/README.md:41: $ infra/bots/assets/myasset/create_and_upload.py
On 2016/06/15 13:47:18, rmistry wrote:
> Nit: Do you need \_ here as well?
> Also in create_asset and MY_ASSET_LOCATION above.

I don't think so, since it's in the quoted block.  It looked fine when I pasted
this into a markdown preview app.

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/asse...
File infra/bots/assets/asset_utils.py (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/asse...
infra/bots/assets/asset_utils.py:117: with utils.chdir(SKIA_DIR):
On 2016/06/15 13:47:18, rmistry wrote:
> Does this have to go to SKIA_DIR for the below to work?
> If not then you could instead do at the top of the module (like asset.py)-
> 
> FILE_DIR = os.path.dirname(os.path.abspath(__file__))
> INFRA_BOTS_DIR = os.path.realpath(os.path.join(FILE_DIR, os.pardir))
> 
> and use INFRA_BOTS_DIR instead here and on line 64.

I change to SKIA_DIR for the git commit stuff, just to be safe in case someone
were to run this script from outside of the Skia checkout.  I guess I could
chdir to INFRA_BOTS_DIR, but SKIA_DIR seems cleaner.

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
File infra/bots/assets/scripts/create_and_upload.py (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
infra/bots/assets/scripts/create_and_upload.py:9: """Create the asset and upload
it."""
On 2016/06/15 13:47:18, rmistry wrote:
> [Optional]
> Will need more maintenance work to keep this script around since you already
> have create.py and upload.py
> I personally would just use those two scripts but upto you.

IMO being able to create and upload the asset from a temporary directory which
gets auto-cleaned is a compelling reason to have this script.  Agreed that may
require some maintenance, but that really should just be limited to plumbing
through arguments to create.py.

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
File infra/bots/assets/scripts/download.py (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
infra/bots/assets/scripts/download.py:25: assets.main(['download', ASSET_NAME] +
sys.argv[1:])
On 2016/06/15 13:47:18, rmistry wrote:
> sys.exit(...) here and in all scripts with "if __name__ == '__main__'"
> MA always asks for this and I tend to agree.

I'm not sure what you mean.  We'll exit with code zero if there's no exception,
and since we don't trap exceptions, those will cause us to exit with non-zero
code.

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
File infra/bots/assets/scripts/upload.py (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
infra/bots/assets/scripts/upload.py:18: sys.path.insert(0, INFRA_BOTS_DIR)
On 2016/06/15 13:47:18, rmistry wrote:
> The top 4 lines are in 3 scripts. Might be able to extract into a util and add
a
> add_infra_to_path() function. I remember seeing that pattern in telemetry
> somewhere but now I cannot find it to show you an example :/

Extracted into common.py, no function needed.  It made these scripts a lot
smaller :)

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/zip_utils.py
File infra/bots/zip_utils.py (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/zip_utils.p...
infra/bots/zip_utils.py:19: rv = names[:]
On 2016/06/15 13:47:18, rmistry wrote:
> Just curious, why is this called rv?

short for return value

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/zip_utils.p...
infra/bots/zip_utils.py:25: def zip(target_dir, zip_file, blacklist=None):  #
pylint: disable=W0622
On 2016/06/15 13:47:18, rmistry wrote:
> I think this will silently pass if target_dir does not exist. Maybe it should
> throw an exception to prevent surprises? Could also add a test for that.
> 
> For comparison the below unzip function will correctly throw an IOError if the
> zip file does not exist. Could also add a test to ensure that does not get
> regressed.

Done.

[rmistry, 2016-06-15 14:49:40]

lgtm

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
File infra/bots/assets/scripts/download.py (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/assets/scri...
infra/bots/assets/scripts/download.py:25: assets.main(['download', ASSET_NAME] +
sys.argv[1:])
On 2016/06/15 14:31:27, borenet wrote:
> On 2016/06/15 13:47:18, rmistry wrote:
> > sys.exit(...) here and in all scripts with "if __name__ == '__main__'"
> > MA always asks for this and I tend to agree.
> 
> I'm not sure what you mean.  We'll exit with code zero if there's no
exception,
> and since we don't trap exceptions, those will cause us to exit with non-zero
> code.

Right, the advantage of using sys.exit is when the function you call returns
specific ret codes. You do not need it here because you control everything you
call. I like using it in general because it handles ret codes in addition to
Exceptions incase anything changes in the future. What you have right now is
fine.

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/zip_utils.py
File infra/bots/zip_utils.py (right):

https://codereview.chromium.org/2069543002/diff/340001/infra/bots/zip_utils.p...
infra/bots/zip_utils.py:19: rv = names[:]
On 2016/06/15 14:31:27, borenet wrote:
> On 2016/06/15 13:47:18, rmistry wrote:
> > Just curious, why is this called rv?
> 
> short for return value

You have been infected by the Go.

[borenet, 2016-06-15 16:18:03]

Added tests in patch set 22.

[stephana, 2016-06-15 16:19:25]

On 2016/06/15 16:18:03, borenet wrote:
> Added tests in patch set 22.

lgtm

[borenet, 2016-06-15 16:22:20]

The CQ bit was checked by borenet@google.com

[borenet, 2016-06-15 16:22:20]

The patchset sent to the CQ was uploaded after l-g-t-m from kjlubick@google.com,
rmistry@google.com
Link to the patchset: https://codereview.chromium.org/2069543002/#ps400001
(title: "Add asset_utils_test")

[commit-bot: I haz the power, 2016-06-15 16:22:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069543002/400001

[commit-bot: I haz the power, 2016-06-15 19:07:46]

Description was changed from

==========
Add asset management scripts

These provide an easy way to create assets to be used by bots,
eg. Android SDK.

To create an asset:
$ infra/bots/assets/assets.py add android_sdk
(adds scripts in infra/bots/assets/android_sdk)

To upload a new version of an asset:
$ infra/bots/assets/android_sdk/upload.py -t $ANDROID_SDK_ROOT
(uploads Android SDK to GS, writes a version file)
$ git commit
$ git cl upload

To download the current version of the asset:
$ infra/bots/assets/android_sdk/download.py -t ../tmp

BUG=skia:5427
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2069543002
==========

to

==========
Add asset management scripts

These provide an easy way to create assets to be used by bots,
eg. Android SDK.

To create an asset:
$ infra/bots/assets/assets.py add android_sdk
(adds scripts in infra/bots/assets/android_sdk)

To upload a new version of an asset:
$ infra/bots/assets/android_sdk/upload.py -t $ANDROID_SDK_ROOT
(uploads Android SDK to GS, writes a version file)
$ git commit
$ git cl upload

To download the current version of the asset:
$ infra/bots/assets/android_sdk/download.py -t ../tmp

BUG=skia:5427
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2069543002

Committed:
https://skia.googlesource.com/skia/+/0f1469bcdad11cf8bfe79ace33d28052418ecb48
==========

[commit-bot: I haz the power, 2016-06-15 19:07:48]

Committed patchset #22 (id:400001) as
https://skia.googlesource.com/skia/+/0f1469bcdad11cf8bfe79ace33d28052418ecb48