Use gsutil.py for download_from_google_storage instead of the builtin one

Use gsutil.py for download_from_google_storage instead of the builtin one

This pins gsutil to a vanilla 4.7 instead of the weird custom 3.4 we have in depot_tools

BUG=
R=pgervais@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=293413

[hinoka, 2014-12-11 21:36:53]

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

[hinoka, 2014-12-11 22:25:34]

Smoketests (depot_tools/tests/download_from_google_storage_tests.py) pass, a
local attempt at downloading chrome-goma passes, and a chromium gclient sync (to
test no_auth) in chromium/src.

A little nervous about landing this, but the above verification helps a little.

[hinoka, 2014-12-11 22:25:47]

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

[hinoka, 2014-12-17 00:48:16]

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

[hinoka, 2014-12-17 00:48:16]

So I'm pretty sure this will work, but I'll still have to do the whole email +
land + monitor gig with this CL.

[pgervais, 2014-12-17 01:07:08]

One nit.

Checking: have you tested that it works locally?

https://codereview.chromium.org/797663003/diff/20001/download_from_google_sto...
File download_from_google_storage.py (right):

https://codereview.chromium.org/797663003/diff/20001/download_from_google_sto...
download_from_google_storage.py:63: self.version = '4.7'
Keyword parameters are good for default values:

def __init__(self, ..., version='4.7'):
   ...
   self.version = version

You get additional flexibility for almost no cost.

[hinoka, 2014-12-17 01:09:41]

Yep I've tested that this works locally with chrome-goma (build/) and chromium
src.

https://codereview.chromium.org/797663003/diff/20001/download_from_google_sto...
File download_from_google_storage.py (right):

https://codereview.chromium.org/797663003/diff/20001/download_from_google_sto...
download_from_google_storage.py:63: self.version = '4.7'
On 2014/12/17 01:07:07, pgervais wrote:
> Keyword parameters are good for default values:
> 
> def __init__(self, ..., version='4.7'):
>    ...
>    self.version = version
> 
> You get additional flexibility for almost no cost.

Done.

[pgervais, 2014-12-17 01:11:00]

On 2014/12/17 01:09:41, hinoka wrote:
> Yep I've tested that this works locally with chrome-goma (build/) and chromium
> src.
> 
>
https://codereview.chromium.org/797663003/diff/20001/download_from_google_sto...
> File download_from_google_storage.py (right):
> 
>
https://codereview.chromium.org/797663003/diff/20001/download_from_google_sto...
> download_from_google_storage.py:63: self.version = '4.7'
> On 2014/12/17 01:07:07, pgervais wrote:
> > Keyword parameters are good for default values:
> > 
> > def __init__(self, ..., version='4.7'):
> >    ...
> >    self.version = version
> > 
> > You get additional flexibility for almost no cost.
> 
> Done.

lgtm then, I trust you not to commit it at a right time :-)

[pgervais, 2014-12-17 01:13:17]

On 2014/12/17 01:11:00, pgervais wrote:
> On 2014/12/17 01:09:41, hinoka wrote:
> > Yep I've tested that this works locally with chrome-goma (build/) and
chromium
> > src.
> > 
> >
>
https://codereview.chromium.org/797663003/diff/20001/download_from_google_sto...
> > File download_from_google_storage.py (right):
> > 
> >
>
https://codereview.chromium.org/797663003/diff/20001/download_from_google_sto...
> > download_from_google_storage.py:63: self.version = '4.7'
> > On 2014/12/17 01:07:07, pgervais wrote:
> > > Keyword parameters are good for default values:
> > > 
> > > def __init__(self, ..., version='4.7'):
> > >    ...
> > >    self.version = version
> > > 
> > > You get additional flexibility for almost no cost.
> > 
> > Done.
> 
> lgtm then, I trust you not to commit it at a right time :-)

Oops, I mean trust you TO commit it at the right time. Sorry about that.

[hinoka, 2014-12-17 01:21:54]

The CQ bit was checked by hinoka@chromium.org

[commit-bot: I haz the power, 2014-12-17 01:22:40]

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

[commit-bot: I haz the power, 2014-12-17 01:25:59]

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

[commit-bot: I haz the power, 2014-12-17 01:26:00]

Presubmit check for 797663003-40001 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 **
Pylint (113 files) (124.78s) failed
/b/depot_tools/third_party/logilab/astroid/modutils.py:41: UserWarning: Module
six was already imported from /b/depot_tools/third_party/six/__init__.py, but
/usr/local/lib/python2.7/dist-packages is being added to sys.path
  import pkg_resources
Problem importing module .svn: No module named .svn
Problem importing module .svn: No module named .svn
************* Module git_cache
E:279,13: Unexpected keyword argument 'bypass_prodaccess' in constructor call
(unexpected-keyword-arg)
************* Module android
W: 12, 0: Method 'expected_root' is abstract in class 'Recipe' but is not
overridden (abstract-method)
************* Module blink
W: 13, 0: Method 'expected_root' is abstract in class 'Recipe' but is not
overridden (abstract-method)
************* Module ios
W: 12, 0: Method 'expected_root' is abstract in class 'Recipe' but is not
overridden (abstract-method)
************* Module presubmit_unittest
E:2003, 6: Unexpected keyword argument 'include_deleted' in function call
(unexpected-keyword-arg)
E:2003, 6: No value for argument 'file_filter' in function call
(no-value-for-parameter)
E:2003, 6: No value for argument 'include_deletes' in function call
(no-value-for-parameter)
E:2020, 6: Unexpected keyword argument 'include_deleted' in function call
(unexpected-keyword-arg)
E:2020, 6: No value for argument 'file_filter' in function call
(no-value-for-parameter)
E:2020, 6: No value for argument 'include_deletes' in function call
(no-value-for-parameter)
E:2377, 4: No value for argument 'include_deletes' in function call
(no-value-for-parameter)
E:2396, 4: No value for argument 'file_filter' in function call
(no-value-for-parameter)
E:2636, 6: No value for argument 'include_deletes' in function call
(no-value-for-parameter)
************* Module upload_to_google_storage
E:237,13: Unexpected keyword argument 'bypass_prodaccess' in constructor call
(unexpected-keyword-arg)


Presubmit checks took 195.5s to calculate.

Was the presubmit check useful? If not, run "git cl presubmit -v"
to figure out which PRESUBMIT.py was run, then run git blame
on the file to figure out who to ask for help.

[hinoka, 2014-12-17 01:47:13]

Added 2 more files because I changed the function signature and forgot other
things referenced it (caught by CQ's pylint).

ptal again?

[pgervais, 2014-12-17 01:53:10]

On 2014/12/17 01:47:13, hinoka wrote:
> Added 2 more files because I changed the function signature and forgot other
> things referenced it (caught by CQ's pylint).
> 
> ptal again?

A quick grep reveals a match for download_from_google_storage.Gsutil in 
- win_toolchain/toolchain2013.py
- win_toolchain/get_toolchain_if_necessary.py 
- gclient_scm.py. 

Have you checked those as well?

[hinoka, 2014-12-17 01:58:59]

Yep, and those don't contain the problematic "bypass_prodaccess" string.

They do however point to a different gsutil, but thats expected.  I only intend
to change the gsutil path for this specific script, the others will follow in a
separate CL.

[pgervais, 2014-12-17 02:00:28]

On 2014/12/17 01:58:59, hinoka wrote:
> Yep, and those don't contain the problematic "bypass_prodaccess" string.
> 
> They do however point to a different gsutil, but thats expected.  I only
intend
> to change the gsutil path for this specific script, the others will follow in
a
> separate CL.

lgtm again then

[hinoka, 2014-12-17 02:01:21]

The CQ bit was checked by hinoka@chromium.org

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

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

[commit-bot: I haz the power, 2014-12-17 02:05:00]

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

[commit-bot: I haz the power, 2014-12-17 02:05:01]

Presubmit check for 797663003-60001 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 **
Pylint (113 files) (125.94s) failed
/b/depot_tools/third_party/logilab/astroid/modutils.py:41: UserWarning: Module
six was already imported from /b/depot_tools/third_party/six/__init__.py, but
/usr/local/lib/python2.7/dist-packages is being added to sys.path
  import pkg_resources
Problem importing module .svn: No module named .svn
Problem importing module .svn: No module named .svn
************* Module android
W: 12, 0: Method 'expected_root' is abstract in class 'Recipe' but is not
overridden (abstract-method)
************* Module blink
W: 13, 0: Method 'expected_root' is abstract in class 'Recipe' but is not
overridden (abstract-method)
************* Module ios
W: 12, 0: Method 'expected_root' is abstract in class 'Recipe' but is not
overridden (abstract-method)
************* Module presubmit_unittest
E:2003, 6: Unexpected keyword argument 'include_deleted' in function call
(unexpected-keyword-arg)
E:2003, 6: No value for argument 'file_filter' in function call
(no-value-for-parameter)
E:2003, 6: No value for argument 'include_deletes' in function call
(no-value-for-parameter)
E:2020, 6: Unexpected keyword argument 'include_deleted' in function call
(unexpected-keyword-arg)
E:2020, 6: No value for argument 'file_filter' in function call
(no-value-for-parameter)
E:2020, 6: No value for argument 'include_deletes' in function call
(no-value-for-parameter)
E:2377, 4: No value for argument 'include_deletes' in function call
(no-value-for-parameter)
E:2396, 4: No value for argument 'file_filter' in function call
(no-value-for-parameter)
E:2636, 6: No value for argument 'include_deletes' in function call
(no-value-for-parameter)


Presubmit checks took 164.0s to calculate.

[hinoka, 2014-12-17 02:05:40]

The CQ bit was checked by hinoka@chromium.org

[commit-bot: I haz the power, 2014-12-17 02:06:32]

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

[commit-bot: I haz the power, 2014-12-17 02:09:19]

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

[commit-bot: I haz the power, 2014-12-17 02:09:20]

Presubmit check for 797663003-60001 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 **
Pylint (113 files) (124.37s) failed
/b/depot_tools/third_party/logilab/astroid/modutils.py:41: UserWarning: Module
six was already imported from /b/depot_tools/third_party/six/__init__.py, but
/usr/local/lib/python2.7/dist-packages is being added to sys.path
  import pkg_resources
Problem importing module .svn: No module named .svn
Problem importing module .svn: No module named .svn
************* Module android
W: 12, 0: Method 'expected_root' is abstract in class 'Recipe' but is not
overridden (abstract-method)
************* Module blink
W: 13, 0: Method 'expected_root' is abstract in class 'Recipe' but is not
overridden (abstract-method)
************* Module ios
W: 12, 0: Method 'expected_root' is abstract in class 'Recipe' but is not
overridden (abstract-method)
************* Module presubmit_unittest
E:2003, 6: Unexpected keyword argument 'include_deleted' in function call
(unexpected-keyword-arg)
E:2003, 6: No value for argument 'file_filter' in function call
(no-value-for-parameter)
E:2003, 6: No value for argument 'include_deletes' in function call
(no-value-for-parameter)
E:2020, 6: Unexpected keyword argument 'include_deleted' in function call
(unexpected-keyword-arg)
E:2020, 6: No value for argument 'file_filter' in function call
(no-value-for-parameter)
E:2020, 6: No value for argument 'include_deletes' in function call
(no-value-for-parameter)
E:2377, 4: No value for argument 'include_deletes' in function call
(no-value-for-parameter)
E:2396, 4: No value for argument 'file_filter' in function call
(no-value-for-parameter)
E:2636, 6: No value for argument 'include_deletes' in function call
(no-value-for-parameter)


Presubmit checks took 163.4s to calculate.

[hinoka, 2014-12-17 02:17:18]

Committed patchset #4 (id:60001) manually as 293413.

[jamesr, 2014-12-17 21:19:52]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2014-12-17 21:19:52]

This breaks setting the +x bit on downloaded binaries.  The
download_from_google_storage.py script greps the output of gsutil ls -L for
"x-goog-metadata-executable".  The output of more recent gsutils just says
"executable".

[hinoka, 2014-12-17 21:25:53]

Actually if I force the version to 3.4 it still outputs "executable = 1" instead
of x-goog-meta-executable.

This is really weird, its as if the GS service is intentionally stripping out
the "x-goog-meta" prefix

[jamesr, 2014-12-17 21:27:17]

Here's the output of depot_tool's third_party/gsutil prior to this patch:

$ ./third_party/gsutil/gsutil ls -L
gs://chromium-gn/e31efdf9e6ea82aa75556968b71adf194928be34
gs://chromium-gn/e31efdf9e6ea82aa75556968b71adf194928be34:
	Creation time:	Fri, 12 Dec 2014 02:33:48 GMT
	Cache-Control:	public, max-age=3600
	Content-Length:	1226584
	Content-Type:	application/octet-stream
	x-goog-meta-executable:	1
	x-goog-meta-content-language:	en
	ETag:		87dcac0921414b40b3752b7024351dfa
	ACL:		ACCESS DENIED. Note: you need FULL_CONTROL permission
			on the object to read its ACL.
TOTAL: 1 objects, 1226584 bytes (1.17 MB)

[jamesr, 2014-12-17 21:28:37]

And here's output from the google cloud SDK's gsutil (version 4.7):

$ gsutil ls -L gs://chromium-gn/e31efdf9e6ea82aa75556968b71adf194928be34
gs://chromium-gn/e31efdf9e6ea82aa75556968b71adf194928be34:
	Creation time:		Fri, 12 Dec 2014 02:33:48 GMT
	Cache-Control:		public, max-age=3600
	Content-Length:		1226584
	Content-Type:		application/octet-stream
	Metadata:
		executable:		1
		content-language:		en
	Hash (crc32c):		KJyJaQ==
	Hash (md5):		h9ysCSFBS0CzdStwJDUd+g==
	ETag:			CKjh2ce7v8ICEAM=
	Generation:		1418351628153000
	Metageneration:		3
	ACL:			ACCESS DENIED. Note: you need OWNER permission
				on the object to read its ACL.
TOTAL: 1 objects, 1226584 bytes (1.17 MB)


the indentation is extra weird

[jamesr, 2014-12-17 21:31:49]

Ah, they just moved it under a 'Metadata' header.  Guessing they didn't expect
people to scrape the output of this.

The sha1s changed last friday. The behavior I'm seeing from folks who sit near
me is that anybody who pulled the new binary down since last friday and before
this patch landed is fine, but anybody who's downloading the new binaries with
this patch is busted.  The next GN roll will also break everybody.  Seems pretty
bad :(

[hinoka, 2014-12-17 21:32:20]

Ok I talked to dnj, he explained this is because GS now intentionally strips off
the x-goog-meta header.  I'll fire off a CL to fix this.