GSUtil.py wrapper script

GSUtil.py wrapper script

This is for transitioning us out of the horribly outdated gsutil 3.4 thats
currently residing in depot_tools.  This script:
1. Downloads GSUtil, pinned to a version (4.6 by default).
2. Executes GSUtil with the given parameters
3. (TODO) Runs the fallback GSUtil if #2 fails.


The transition plan is:
1. Set the fallback as default, making this script a no-op
2. Modify recipes (main consumer of gsutil) to pass in --force_version 4.6
3. Switch 4.6 as the default for gsutil.py
4. Make fallback failure a loud red failure
5. Remove depot_tools/third_party/gsutil

BUG=434422
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=293320

[hinoka, 2014-11-20 18:52:21]

hinoka@chromium.org changed reviewers:
+ dnj@chromium.org

[hinoka, 2014-11-20 18:52:22]

PTAL.  This is as we discussed earlier.  Code is done, tests are almost done
(need to get test_ensure_gsutil_full() to work).  Python style is read from
bottom-up.

[Ryan Tseng, 2014-11-20 19:36:51]

hinoka@google.com changed reviewers:
+ hinoka@google.com

[Ryan Tseng, 2014-11-20 19:36:52]

Okay tests are done now.

[Ryan Tseng, 2014-11-20 19:39:24]

Coverage report: 74% on gsutil.py

[dnj, 2014-11-20 19:58:27]

Overall looks really good.

Consider making the "<depot_tools>/gsutil_bin" path more generic (in
anticipation of other utilities similarly handled), such as,
"<depot_tools>/external_bin/gsutil".

https://codereview.chromium.org/742173002/diff/40001/tests/gsutil_test.py
File tests/gsutil_test.py (right):

https://codereview.chromium.org/742173002/diff/40001/tests/gsutil_test.py#new...
tests/gsutil_test.py:122: old_call = getattr(gsutil, 'call')
If this test raises an exception, 'call' will stay mocked out. Any reason why
you don't use a mock library, where you can ensure this is undone in 'tearDown'?

[hinoka, 2014-11-20 21:20:16]

Addressed comment by swapping out with a more generic mock

<depot_tools>/gsutil_bin moved to <depot_tools>/bin/gsutil

https://codereview.chromium.org/742173002/diff/40001/tests/gsutil_test.py
File tests/gsutil_test.py (right):

https://codereview.chromium.org/742173002/diff/40001/tests/gsutil_test.py#new...
tests/gsutil_test.py:122: old_call = getattr(gsutil, 'call')
On 2014/11/20 19:58:27, dnj wrote:
> If this test raises an exception, 'call' will stay mocked out. Any reason why
> you don't use a mock library, where you can ensure this is undone in
'tearDown'?

My (and Sergey's) experience with mock libraries are they're confusing,
unpredictable, and ultimately easily replaceable with setattr/getattrs.

I'll turn this into a more generic mock.

[hinoka, 2014-11-20 21:21:09]

Ignore that, still need a couple fixes.

[Ryan Tseng, 2014-11-20 21:58:21]

Okay now its good, ptal.

[dnj, 2014-11-20 23:03:03]

Still looks pretty good! One comment r.e. directory naming.

https://codereview.chromium.org/742173002/diff/70004/.gitignore
File .gitignore (right):

https://codereview.chromium.org/742173002/diff/70004/.gitignore#newcode27
.gitignore:27: /gsutil_bin
Need to update this w/ whatever new root directory you choose!

https://codereview.chromium.org/742173002/diff/70004/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/742173002/diff/70004/gsutil.py#newcode25
gsutil.py:25: DEFAULT_BIN_DIR = os.path.join(THIS_DIR, 'bin', 'gsutil')
My only concern with 'bin' is that it's part of the LSB, and other people might
start dumping things there. Either dump a 'README' in it or name it something
like 'external_bin' or 'bin.fetch' or something to indicate its transient
nature.

https://codereview.chromium.org/742173002/diff/70004/gsutil.py#newcode110
gsutil.py:110: os.chmod(gsutil_bin, 0755)  # python zipfile doesn't set exec
bit.
Windows will ignore this. Does anything special need to be done, or are all
things executable on Windows? I forget~

https://codereview.chromium.org/742173002/diff/70004/gsutil.py#newcode131
gsutil.py:131: parser.add_argument('args', nargs='*')
Replace '*' with 'argparse.REMAINDER' to make argparse pick up flags and stuff.

https://codereview.chromium.org/742173002/diff/70004/tests/gsutil_test.py
File tests/gsutil_test.py (right):

https://codereview.chromium.org/742173002/diff/70004/tests/gsutil_test.py#new...
tests/gsutil_test.py:37: self.data = data
Might as well do "self.data = data or ''"

[Ryan Tseng, 2014-11-21 02:14:55]

https://codereview.chromium.org/742173002/diff/70004/.gitignore
File .gitignore (right):

https://codereview.chromium.org/742173002/diff/70004/.gitignore#newcode27
.gitignore:27: /gsutil_bin
On 2014/11/20 23:03:02, dnj wrote:
> Need to update this w/ whatever new root directory you choose!

Done.

https://codereview.chromium.org/742173002/diff/70004/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/742173002/diff/70004/gsutil.py#newcode25
gsutil.py:25: DEFAULT_BIN_DIR = os.path.join(THIS_DIR, 'bin', 'gsutil')
On 2014/11/20 23:03:02, dnj wrote:
> My only concern with 'bin' is that it's part of the LSB, and other people
might
> start dumping things there. Either dump a 'README' in it or name it something
> like 'external_bin' or 'bin.fetch' or something to indicate its transient
> nature.

Good point, I'll use external_bin

https://codereview.chromium.org/742173002/diff/70004/gsutil.py#newcode110
gsutil.py:110: os.chmod(gsutil_bin, 0755)  # python zipfile doesn't set exec
bit.
On 2014/11/20 23:03:02, dnj wrote:
> Windows will ignore this. Does anything special need to be done, or are all
> things executable on Windows? I forget~

Actually if i used [sys.executable, <path to gsutil>, ...] then this is
unnecessary :D

https://codereview.chromium.org/742173002/diff/70004/gsutil.py#newcode131
gsutil.py:131: parser.add_argument('args', nargs='*')
On 2014/11/20 23:03:02, dnj wrote:
> Replace '*' with 'argparse.REMAINDER' to make argparse pick up flags and
stuff.

Good catch, done.

https://codereview.chromium.org/742173002/diff/70004/tests/gsutil_test.py
File tests/gsutil_test.py (right):

https://codereview.chromium.org/742173002/diff/70004/tests/gsutil_test.py#new...
tests/gsutil_test.py:37: self.data = data
On 2014/11/20 23:03:02, dnj wrote:
> Might as well do "self.data = data or ''"

Done.

[dnj, 2014-11-21 03:04:01]

lgtm

https://codereview.chromium.org/742173002/diff/90001/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/742173002/diff/90001/gsutil.py#newcode122
gsutil.py:122: # gsutil_bin = ensure_gsutil(force_version, target)
So when do we get to actually use it? :D

[navabi, 2014-12-01 19:47:01]

navabi@google.com changed reviewers:
+ navabi@google.com

[navabi, 2014-12-01 19:47:01]

Ping. This CL is waiting for this to land:
https://codereview.chromium.org/709663002/

[dnj, 2014-12-01 19:50:08]

On 2014/12/01 19:47:01, navabi wrote:
> Ping. This CL is waiting for this to land:
> https://codereview.chromium.org/709663002/

hinoka@ has been on vacation; not sure when he's going to be back. Why does your
CL need this one to land? It looks like it's just using 'gsutil' directly, so
it'll automatically use this script once it replaces
"third_party/gsutil/gsutil".

[navabi, 2014-12-01 20:02:58]

On 2014/12/01 19:50:08, dnj wrote:
> On 2014/12/01 19:47:01, navabi wrote:
> > Ping. This CL is waiting for this to land:
> > https://codereview.chromium.org/709663002/
> 
> hinoka@ has been on vacation; not sure when he's going to be back. Why does
your
> CL need this one to land? It looks like it's just using 'gsutil' directly, so
> it'll automatically use this script once it replaces
> "third_party/gsutil/gsutil".

Because until this lands the only gsutil that we have on the bots is not less
than version 4.0 (AFAIK) and therefore does not have the rsync command. Thus,
when this hook runs it will fail when it tries to do a gsutil rsync.

[dnj, 2014-12-01 20:05:28]

On 2014/12/01 20:02:58, navabi wrote:
> On 2014/12/01 19:50:08, dnj wrote:
> > On 2014/12/01 19:47:01, navabi wrote:
> > > Ping. This CL is waiting for this to land:
> > > https://codereview.chromium.org/709663002/
> > 
> > hinoka@ has been on vacation; not sure when he's going to be back. Why does
> your
> > CL need this one to land? It looks like it's just using 'gsutil' directly,
so
> > it'll automatically use this script once it replaces
> > "third_party/gsutil/gsutil".
> 
> Because until this lands the only gsutil that we have on the bots is not less
> than version 4.0 (AFAIK) and therefore does not have the rsync command. Thus,
> when this hook runs it will fail when it tries to do a gsutil rsync.

Oh I see. This CL alone is just the beginning of the 4.0 update. After this
there is something like:
* Actually have this download/use different 'gsutil'.
* Implement this as the default 'gsutil' invocation.
* Establish 4.0+ as the default 'gsutil' version.

So depending on how confident hinoka@ is about the script, this could be a
while.

[navabi, 2014-12-01 20:12:44]

On 2014/12/01 20:05:28, dnj wrote:
> On 2014/12/01 20:02:58, navabi wrote:
> > On 2014/12/01 19:50:08, dnj wrote:
> > > On 2014/12/01 19:47:01, navabi wrote:
> > > > Ping. This CL is waiting for this to land:
> > > > https://codereview.chromium.org/709663002/
> > > 
> > > hinoka@ has been on vacation; not sure when he's going to be back. Why
does
> > your
> > > CL need this one to land? It looks like it's just using 'gsutil' directly,
> so
> > > it'll automatically use this script once it replaces
> > > "third_party/gsutil/gsutil".
> > 
> > Because until this lands the only gsutil that we have on the bots is not
less
> > than version 4.0 (AFAIK) and therefore does not have the rsync command.
Thus,
> > when this hook runs it will fail when it tries to do a gsutil rsync.
> 
> Oh I see. This CL alone is just the beginning of the 4.0 update. After this
> there is something like:
> * Actually have this download/use different 'gsutil'.
> * Implement this as the default 'gsutil' invocation.
> * Establish 4.0+ as the default 'gsutil' version.
> 
> So depending on how confident hinoka@ is about the script, this could be a
> while.

It looked like this CL adds the --force-version argument. Once we have that, and
download the new gsutil and put it somewhere, I can change the
download_sdk_extras.py script to call this gsutil wrapper (instead of the
regular gsutil) with --force-version 4.0 for this hook. I.e. I don't need to
wait for it to be established as the default gsutil version.

[hinoka, 2014-12-09 22:48:53]

The CQ bit was checked by hinoka@chromium.org

[commit-bot: I haz the power, 2014-12-09 22:49:35]

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

[commit-bot: I haz the power, 2014-12-09 22:51:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-09 22:51:56]

Presubmit check for 742173002-90001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    depot_tools/.gitignore
    depot_tools/gsutil.py
    depot_tools/tests/gsutil_test.py

Presubmit checks took 135.8s to calculate.

[hinoka, 2014-12-10 00:12:06]

hinoka@chromium.org changed reviewers:
+ pgervais@chromium.org

[hinoka, 2014-12-10 00:12:07]

pgervais: Can I get an owners stamp from you?

https://codereview.chromium.org/742173002/diff/90001/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/742173002/diff/90001/gsutil.py#newcode122
gsutil.py:122: # gsutil_bin = ensure_gsutil(force_version, target)
On 2014/11/21 03:04:00, dnj wrote:
> So when do we get to actually use it? :D

Actually I think its safe to enable, changed on the next patchset.

[pgervais, 2014-12-10 00:19:35]

I have two nits.

https://codereview.chromium.org/742173002/diff/110001/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/742173002/diff/110001/gsutil.py#newcode32
gsutil.py:32: class InvalidGsutilError(Exception):
Missing blank line

https://codereview.chromium.org/742173002/diff/110001/gsutil.py#newcode131
gsutil.py:131: parser.add_argument('--force_version')
Please use --force-version instead.

[hinoka, 2014-12-10 01:46:46]

https://codereview.chromium.org/742173002/diff/110001/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/742173002/diff/110001/gsutil.py#newcode32
gsutil.py:32: class InvalidGsutilError(Exception):
On 2014/12/10 00:19:35, pgervais wrote:
> Missing blank line

Done.

https://codereview.chromium.org/742173002/diff/110001/gsutil.py#newcode131
gsutil.py:131: parser.add_argument('--force_version')
On 2014/12/10 00:19:35, pgervais wrote:
> Please use --force-version instead.

Done.

[pgervais, 2014-12-10 01:58:05]

On 2014/12/10 01:46:46, hinoka (OoO until 12-7) wrote:
> https://codereview.chromium.org/742173002/diff/110001/gsutil.py
> File gsutil.py (right):
> 
> https://codereview.chromium.org/742173002/diff/110001/gsutil.py#newcode32
> gsutil.py:32: class InvalidGsutilError(Exception):
> On 2014/12/10 00:19:35, pgervais wrote:
> > Missing blank line
> 
> Done.
> 
> https://codereview.chromium.org/742173002/diff/110001/gsutil.py#newcode131
> gsutil.py:131: parser.add_argument('--force_version')
> On 2014/12/10 00:19:35, pgervais wrote:
> > Please use --force-version instead.
> 
> Done.

lgtm

[hinoka, 2014-12-10 02:01:42]

The CQ bit was checked by hinoka@chromium.org

[commit-bot: I haz the power, 2014-12-10 02:02:24]

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

[commit-bot: I haz the power, 2014-12-10 02:04:42]

Committed patchset #8 (id:130001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=293320