[gms updater] Fixes and tests to prepare activation

[gms updater] Fixes and tests to prepare activation

- Fix broken download command on bots (uninitialized variable)
- Fix the issue where logs were not displayed when is_tty returned
False
- Check directory permissions
- ColorStreamHandler: allow forcing colorized output for non tty
- Add tests

BUG=541727
Committed: https://crrev.com/f04e2b29a270cd12200ffc71e35834158e32c22b
Cr-Commit-Position: refs/heads/master@{#358076}

[dgn, 2015-11-02 14:04:13]

dgn@chromium.org changed reviewers:
+ jbudorick@chromium.org

[dgn, 2015-11-02 14:04:14]

PTAL

https://codereview.chromium.org/1418833005/diff/1/build/android/play_services...
File build/android/play_services/update.py (right):

https://codereview.chromium.org/1418833005/diff/1/build/android/play_services...
build/android/play_services/update.py:111:
logging_utils.ColorStreamHandler.MakeDefault(force_color=True)
Works fine with runhooks locally. Need to see what happens when it runs on bots.

[dgn, 2015-11-02 15:41:20]

Interesting:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...

Colors are displayed in the log files when showing as html, but when showing as
text, the escape codes are visible:
[36m/usr/bin/python /b/depot_tools/gsutil.py --force-version 4.15 ls
gs://chromium-android-tools/play-services/8115000/11cc73d4b7fa82560fbf5bbc1095dbac30308e7c
`cat` and `more` display the colors; `less`, text editors will show the codes.

Should I leave it like that? use CHROME_HEADLESS to disable the flag?

I think the appropriate fix would be to have the gsutil script itself tell to
called scripts whether they are running in a tty or not. AFAIK isatty fails
because of the subprocess piping at
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/gcli...
it should be possible to swap stdout with a wrapper that returns true to isatty
depending on whether gsutil itself is called from a tty or not. Then running in
a terminal would output the colors and running on the bots or piping to scripts
would not.

[jbudorick, 2015-11-03 02:59:09]

On 2015/11/02 15:41:20, dgn wrote:
> Interesting:
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...
> 
> Colors are displayed in the log files when showing as html, but when showing
as
> text, the escape codes are visible:
> [36m/usr/bin/python /b/depot_tools/gsutil.py --force-version 4.15 ls
>
gs://chromium-android-tools/play-services/8115000/11cc73d4b7fa82560fbf5bbc1095dbac30308e7c
> `cat` and `more` display the colors; `less`, text editors will show the codes.
> 
> Should I leave it like that? use CHROME_HEADLESS to disable the flag?

Yeah, I think we should use CHROME_HEADLESS to disable the flag for now.

> 
> I think the appropriate fix would be to have the gsutil script itself tell to
> called scripts whether they are running in a tty or not. AFAIK isatty fails
> because of the subprocess piping at
>
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/gcli...
> it should be possible to swap stdout with a wrapper that returns true to
isatty
> depending on whether gsutil itself is called from a tty or not. Then running
in
> a terminal would output the colors and running on the bots or piping to
scripts
> would not.

Hm, interesting.

[dgn, 2015-11-03 13:40:22]

Description was changed from

==========
[pylib] ColorStreamHandler: allow forcing colorized output for non tty

Also fix the issue where logs were not displayed when is_tty returned
False

BUG=
==========

to

==========
[gms updater] Fixes for running on bots
    
- Fix broken download command on bots (uninitialized variable)
- Fix the issue where logs were not displayed when is_tty returned
False
- ColorStreamHandler: allow forcing colorized output for non tty

BUG=541727
==========

[dgn, 2015-11-03 13:43:03]

On 2015/11/03 02:59:09, jbudorick wrote:
> On 2015/11/02 15:41:20, dgn wrote:
> > Interesting:
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...
> > 
> > Colors are displayed in the log files when showing as html, but when showing
> as
> > text, the escape codes are visible:
> > [36m/usr/bin/python /b/depot_tools/gsutil.py --force-version 4.15 ls
> >
>
gs://chromium-android-tools/play-services/8115000/11cc73d4b7fa82560fbf5bbc1095dbac30308e7c
> > `cat` and `more` display the colors; `less`, text editors will show the
codes.
> > 
> > Should I leave it like that? use CHROME_HEADLESS to disable the flag?
> 
> Yeah, I think we should use CHROME_HEADLESS to disable the flag for now.
> 
> > 
> > I think the appropriate fix would be to have the gsutil script itself tell
to
> > called scripts whether they are running in a tty or not. AFAIK isatty fails
> > because of the subprocess piping at
> >
>
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/gcli...
> > it should be possible to swap stdout with a wrapper that returns true to
> isatty
> > depending on whether gsutil itself is called from a tty or not. Then running
> in
> > a terminal would output the colors and running on the bots or piping to
> scripts
> > would not.
> 
> Hm, interesting.

Done.

[dgn, 2015-11-04 15:51:35]

Description was changed from

==========
[gms updater] Fixes for running on bots
    
- Fix broken download command on bots (uninitialized variable)
- Fix the issue where logs were not displayed when is_tty returned
False
- ColorStreamHandler: allow forcing colorized output for non tty

BUG=541727
==========

to

==========
[gms updater] Fixes and tests to prepare activation
    
- Fix broken download command on bots (uninitialized variable)
- Fix the issue where logs were not displayed when is_tty returned
False
- ColorStreamHandler: allow forcing colorized output for non tty
- Add tests

BUG=541727
==========

[dgn, 2015-11-04 16:36:05]

@jbudorick: PTAL. I believe it is now ready.

[jbudorick, 2015-11-04 16:56:32]

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
File build/android/play_services/update.py (right):

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:54: def Main(raw_args):
nit: missed this before, but s/Main/main/

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:68: force=True, sdk_root=True,
verbose=True)
nit: indentation

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:91: force=True, sdk_root=True,
verbose=True)
nit: indentation

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:99: def AddArguments(parser, bucket=False,
config=False, dry_run=False,
Split this into two functions without all of these flags: one that adds bucket,
config, dry_run, and force, and one the adds verbose and sdk_root. Call both for
parser_upload and parser_download, and call only the latter for parser_sdk.

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:100: force=False, sdk_root=False,
verbose=False):
nit: indentation

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:188: _DownloadFromBucket(bucket_path,
license_sha1, new_license,
This is now downloading the license on the bot where it wasn't previously. I
don't envision that being a problem, but was that intentional?

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:482: sys.exit(Main(sys.argv[1:]))
don't pass anything to Main, and don't pass anything to parse_args.

[dgn, 2015-11-04 18:43:50]

Description was changed from

==========
[gms updater] Fixes and tests to prepare activation
    
- Fix broken download command on bots (uninitialized variable)
- Fix the issue where logs were not displayed when is_tty returned
False
- ColorStreamHandler: allow forcing colorized output for non tty
- Add tests

BUG=541727
==========

to

==========
[gms updater] Fixes and tests to prepare activation
    
- Fix broken download command on bots (uninitialized variable)
- Fix the issue where logs were not displayed when is_tty returned
False
- Check directory permissions
- ColorStreamHandler: allow forcing colorized output for non tty
- Add tests

BUG=541727
==========

[dgn, 2015-11-04 18:43:58]

PTAL

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
File build/android/play_services/update.py (right):

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:54: def Main(raw_args):
On 2015/11/04 16:56:32, jbudorick wrote:
> nit: missed this before, but s/Main/main/

Done.

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:68: force=True, sdk_root=True,
verbose=True)
On 2015/11/04 16:56:32, jbudorick wrote:
> nit: indentation

Done.

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:91: force=True, sdk_root=True,
verbose=True)
On 2015/11/04 16:56:32, jbudorick wrote:
> nit: indentation

Done.

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:99: def AddArguments(parser, bucket=False,
config=False, dry_run=False,
On 2015/11/04 16:56:32, jbudorick wrote:
> Split this into two functions without all of these flags: one that adds
bucket,
> config, dry_run, and force, and one the adds verbose and sdk_root. Call both
for
> parser_upload and parser_download, and call only the latter for parser_sdk.

Done.

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:100: force=False, sdk_root=False,
verbose=False):
On 2015/11/04 16:56:32, jbudorick wrote:
> nit: indentation

Done.

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:188: _DownloadFromBucket(bucket_path,
license_sha1, new_license,
On 2015/11/04 16:56:32, jbudorick wrote:
> This is now downloading the license on the bot where it wasn't previously. I
> don't envision that being a problem, but was that intentional?

Yes. It failed at some point where the paths were not initialized before. I
could have put an empty path there instead, but I figured that reducing the
differences between the 2 codepaths as much as possible would not be bad

https://codereview.chromium.org/1418833005/diff/120001/build/android/play_ser...
build/android/play_services/update.py:482: sys.exit(Main(sys.argv[1:]))
On 2015/11/04 16:56:32, jbudorick wrote:
> don't pass anything to Main, and don't pass anything to parse_args.

This is to allow providing arguments in the tests. should I make it the default
value of the parameter instead?

[jbudorick, 2015-11-04 18:46:07]

https://codereview.chromium.org/1418833005/diff/140001/build/android/play_ser...
File build/android/play_services/update.py (right):

https://codereview.chromium.org/1418833005/diff/140001/build/android/play_ser...
build/android/play_services/update.py:93: args = parser.parse_args(raw_args)
parse_args() will pick up sys.argv[1:] and parse those by default, so the only
reason to pass something to parse_args is if you're doing something unusual
(which doesn't appear to be the case here).

[jbudorick, 2015-11-04 18:48:11]

lgtm w/ nits (here + previous review)

https://codereview.chromium.org/1418833005/diff/140001/build/android/play_ser...
File build/android/play_services/update.py (right):

https://codereview.chromium.org/1418833005/diff/140001/build/android/play_ser...
build/android/play_services/update.py:209: os.makedirs(paths.lib)
In light of the root ownership issue, we might want to give users some hints
here if one of these two calls fails.

[dgn, 2015-11-05 12:15:11]

Thanks!

https://codereview.chromium.org/1418833005/diff/140001/build/android/play_ser...
File build/android/play_services/update.py (right):

https://codereview.chromium.org/1418833005/diff/140001/build/android/play_ser...
build/android/play_services/update.py:93: args = parser.parse_args(raw_args)
On 2015/11/04 18:46:07, jbudorick wrote:
> parse_args() will pick up sys.argv[1:] and parse those by default, so the only
> reason to pass something to parse_args is if you're doing something unusual
> (which doesn't appear to be the case here).

It's the case in update_test.py, where I call main() with command line
arguments:
https://codereview.chromium.org/1418833005/diff/140001/build/android/play_ser...

I could call the underlying functions directly, but I preferred this way since
it tests the argument parsing, is easier to write (avoid having to rebuild the
args object), and I believe is also more useful for demo/doc purposes. Should I
change that?

https://codereview.chromium.org/1418833005/diff/140001/build/android/play_ser...
build/android/play_services/update.py:209: os.makedirs(paths.lib)
On 2015/11/04 18:48:10, jbudorick wrote:
> In light of the root ownership issue, we might want to give users some hints
> here if one of these two calls fails.

Done. I also wrapped the calls below, as the license and sha1 files don't go
under that path.

[jbudorick, 2015-11-05 16:03:02]

https://codereview.chromium.org/1418833005/diff/140001/build/android/play_ser...
File build/android/play_services/update.py (right):

https://codereview.chromium.org/1418833005/diff/140001/build/android/play_ser...
build/android/play_services/update.py:93: args = parser.parse_args(raw_args)
On 2015/11/05 12:15:10, dgn wrote:
> On 2015/11/04 18:46:07, jbudorick wrote:
> > parse_args() will pick up sys.argv[1:] and parse those by default, so the
only
> > reason to pass something to parse_args is if you're doing something unusual
> > (which doesn't appear to be the case here).
> 
> It's the case in update_test.py, where I call main() with command line
> arguments:
>
https://codereview.chromium.org/1418833005/diff/140001/build/android/play_ser...
> 
> I could call the underlying functions directly, but I preferred this way since
> it tests the argument parsing, is easier to write (avoid having to rebuild the
> args object), and I believe is also more useful for demo/doc purposes. Should
I
> change that?

oooooh I missed that. This is fine, then.

https://codereview.chromium.org/1418833005/diff/200001/build/android/play_ser...
File build/android/play_services/update.py (right):

https://codereview.chromium.org/1418833005/diff/200001/build/android/play_ser...
build/android/play_services/update.py:7: Script to help uploading and
downloading the Google Play services SDK to and
nit: Why the doc change from "library" to "SDK"? Library seems more appropriate
here...

[dgn, 2015-11-05 16:15:05]

https://codereview.chromium.org/1418833005/diff/200001/build/android/play_ser...
File build/android/play_services/update.py (right):

https://codereview.chromium.org/1418833005/diff/200001/build/android/play_ser...
build/android/play_services/update.py:7: Script to help uploading and
downloading the Google Play services SDK to and
On 2015/11/05 16:03:02, jbudorick wrote:
> nit: Why the doc change from "library" to "SDK"? Library seems more
appropriate
> here...

The developer website says "Google Play services SDK" in most places so I wanted
to be consistent with it. But it's true that since I don't include the
documentation, samples etc directories library is more accurate.

[dgn, 2015-11-05 16:30:21]

https://codereview.chromium.org/1418833005/diff/200001/build/android/play_ser...
File build/android/play_services/update.py (right):

https://codereview.chromium.org/1418833005/diff/200001/build/android/play_ser...
build/android/play_services/update.py:7: Script to help uploading and
downloading the Google Play services SDK to and
On 2015/11/05 16:15:05, dgn wrote:
> On 2015/11/05 16:03:02, jbudorick wrote:
> > nit: Why the doc change from "library" to "SDK"? Library seems more
> appropriate
> > here...
> 
> The developer website says "Google Play services SDK" in most places so I
wanted
> to be consistent with it. But it's true that since I don't include the
> documentation, samples etc directories library is more accurate.

Done.

[dgn, 2015-11-05 16:31:29]

The CQ bit was checked by dgn@chromium.org

[dgn, 2015-11-05 16:31:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1418833005/#ps220001
(title: "use SDK/library appropriately")

[commit-bot: I haz the power, 2015-11-05 16:32:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418833005/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418833005/220001

[commit-bot: I haz the power, 2015-11-05 17:52:01]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-11-05 17:52:46]

Patchset 12 (id:??) landed as
https://crrev.com/f04e2b29a270cd12200ffc71e35834158e32c22b
Cr-Commit-Position: refs/heads/master@{#358076}