Add a platform filtering and executable setting abilities to download_from_google_storage.

Add a platform filtering and executable setting abilities to download_from_google_storage.

Adds a command line flag --platform to control which platforms the given download occurs on.

Automatically marks downloads executable on non-Windows if the header x-goog-meta-executable is set. Automatically set this flag on upload when the executable bit is set.

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

[brettw, 2013-10-24 22:52:27]

Here is one way to solve the download problem. I would have multiple entries in
the hooks, one for each platform. It sucks to have to fork some extra times, but
it should early-exit and this should be a very small additional overhead given
how slow the hooks are to run.

[Ryan Tseng, 2013-10-24 23:42:23]

+ilevy FYI: Brettw is working on a download_from_google_storage side fix.

This approach sounds good to me, we can make do with three entries to hooks,
since storing platform data in google storage metadata would probably
over-complicate the process.

https://codereview.chromium.org/42273002/diff/1/download_from_google_storage.py
File download_from_google_storage.py (right):

https://codereview.chromium.org/42273002/diff/1/download_from_google_storage....
download_from_google_storage.py:190: if make_executable and not
sys.platform.startswith('win'):
We discussed this offline, but posting a comment for posterity.

We should store an executable flag in google storage as metadata:
https://developers.google.com/storage/docs/concepts-techniques
and set executable if the flag is present and active (using "gsutl ls -L")

Another change will need to be made in upload_to_google_storage to test for the
executable bit and set the metadata.

https://codereview.chromium.org/42273002/diff/1/download_from_google_storage....
download_from_google_storage.py:290: parser.add_option('', '--platform',
how about '-p'

https://codereview.chromium.org/42273002/diff/1/download_from_google_storage....
download_from_google_storage.py:294: parser.add_option('', '--make_exec',
action='store_true',
how about '-x'

https://codereview.chromium.org/42273002/diff/1/download_from_google_storage....
download_from_google_storage.py:304: return 0  # Skip rnuning on this platform
/s/rnuning/running/

Also lets print a helpful message like "Platform <options.platform> does not
match <sys.platform>, exiting".

[brettw, 2013-10-25 21:25:21]

New snap up that uses metadata rather than the command line flag for
executable-ness.

[Ryan Tseng, 2013-10-25 22:00:01]

lgtm + comments

+maruel, iannucci for OWNERs approval.

https://codereview.chromium.org/42273002/diff/70001/download_from_google_stor...
File download_from_google_storage.py (right):

https://codereview.chromium.org/42273002/diff/70001/download_from_google_stor...
download_from_google_storage.py:190: # "x-goog-meta-executable"
nit: add period at end.

https://codereview.chromium.org/42273002/diff/70001/download_from_google_stor...
download_from_google_storage.py:300: parser.add_option('', '--platform',
Add '-p' as short name.

https://codereview.chromium.org/42273002/diff/70001/download_from_google_stor...
download_from_google_storage.py:312: options.platform)
nit: align to the first quote

[brettw, 2013-10-25 22:05:38]

New snap up with fixes.

[Ryan Tseng, 2013-10-25 22:12:36]

Oh almost forgot, also add BUG=233100 in the CL description.

[M-A Ruel, 2013-10-26 15:32:36]

https://codereview.chromium.org/42273002/diff/140001/download_from_google_sto...
File download_from_google_storage.py (right):

https://codereview.chromium.org/42273002/diff/140001/download_from_google_sto...
download_from_google_storage.py:192: # It is supposedly faster to use "gsutil
stat" but that doesn't appear to
It's effectively a TODO(hinoka):

But why not update gsutil first?

[brettw, 2013-10-26 22:32:20]

It would be nice, given that these things are separate, for me to not be blocked
by pulling various third_party stuff. It's easy to do in either order and I
don't think the performance difference here is practical for checking one file.

[M-A Ruel, 2013-10-28 15:05:47]

On 2013/10/26 22:32:20, brettw wrote:
> It would be nice, given that these things are separate, for me to not be
blocked
> by pulling various third_party stuff. It's easy to do in either order and I
> don't think the performance difference here is practical for checking one
file.

I was not given the greater context here. Here's some changes, after it will be
fine.

https://codereview.chromium.org/42273002/diff/140001/download_from_google_sto...
File download_from_google_storage.py (right):

https://codereview.chromium.org/42273002/diff/140001/download_from_google_sto...
download_from_google_storage.py:309: platform_regexp =
re.compile(options.platform)
It's more verbose than necessary to compile a regexp for only one use, just do:
if not re.match(options.platform, sys.platform):

https://codereview.chromium.org/42273002/diff/140001/upload_to_google_storage.py
File upload_to_google_storage.py (right):

https://codereview.chromium.org/42273002/diff/140001/upload_to_google_storage...
upload_to_google_storage.py:110: if (os.stat(filename).st_mode & stat.S_IEXEC):
if os.stat(filename).st_mode & stat.S_IEXEC:

https://codereview.chromium.org/42273002/diff/140001/upload_to_google_storage...
upload_to_google_storage.py:113: if (code != 0):
if code:

[brettw, 2013-10-28 16:50:52]

Thanks, new snap up.

[M-A Ruel, 2013-10-28 19:40:57]

lgtm with a few changes.

https://codereview.chromium.org/42273002/diff/220001/download_from_google_sto...
File download_from_google_storage.py (right):

https://codereview.chromium.org/42273002/diff/220001/download_from_google_sto...
download_from_google_storage.py:192: # It is supposedly faster to use "gsutil
stat" but that doesn't appear to
TODO(someone):
likely hinoka :)

https://codereview.chromium.org/42273002/diff/220001/download_from_google_sto...
download_from_google_storage.py:310: print ('The current platform doesn\'t match
"%s", skipping.' %
print is now a function call so we do not put a space between () and print.

https://codereview.chromium.org/42273002/diff/220001/upload_to_google_storage.py
File upload_to_google_storage.py (right):

https://codereview.chromium.org/42273002/diff/220001/upload_to_google_storage...
upload_to_google_storage.py:111: code, _, err = gsutil.check_call('setmeta',
'-h',
If gsutil.check_call() is similar to subprocess.check_call(), then it throws
when code != 0. So this code wouldn't work. Just double-check this to be sure.

https://codereview.chromium.org/42273002/diff/220001/upload_to_google_storage...
upload_to_google_storage.py:113: if code != 0:
if code:

[brettw, 2013-10-28 19:53:11]

https://codereview.chromium.org/42273002/diff/220001/upload_to_google_storage.py
File upload_to_google_storage.py (right):

https://codereview.chromium.org/42273002/diff/220001/upload_to_google_storage...
upload_to_google_storage.py:111: code, _, err = gsutil.check_call('setmeta',
'-h',
Looks like it does not throw (I'm just copying the other places, so if I'm wrong
then all calls are broken).

[commit-bot: I haz the power, 2013-10-28 19:54:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brettw@chromium.org/42273002/220002

[commit-bot: I haz the power, 2013-10-28 19:55:50]

Change committed as 231387