Hook sys.stdio directly to the gsutil subprocess for the gsutil call

Hook sys.stdio directly to the gsutil subprocess for the gsutil call

So that gsutil.py config works.

I would've preferred the execv solution, but apparently that didn't work on Windows :(

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

[hinoka, 2015-01-23 20:47:12]

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

[scottmg, 2015-01-23 20:51:54]

Confirm that I the output when running `download_from_google_storage --config`
on Windows with this.

https://codereview.chromium.org/870093003/diff/1/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/870093003/diff/1/gsutil.py#newcode137
gsutil.py:137: call(cmd, native=True)
The only other place it's used is check_gsutil, so probably just get rid of the
arg and make it always equivalent to native=True?

[scottmg, 2015-01-23 20:52:22]

"that I see the output", even.

On Fri, Jan 23, 2015 at 12:51 PM, <scottmg@chromium.org> wrote:

> Confirm that I the output when running `download_from_google_storage
> --config`
> on Windows with this.
>
>
> https://codereview.chromium.org/870093003/diff/1/gsutil.py
> File gsutil.py (right):
>
> https://codereview.chromium.org/870093003/diff/1/gsutil.py#newcode137
> gsutil.py:137: call(cmd, native=True)
> The only other place it's used is check_gsutil, so probably just get rid
> of the arg and make it always equivalent to native=True?
>
> https://codereview.chromium.org/870093003/
>
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hinoka, 2015-01-23 21:05:35]

New patch gets rid of "verbose" also.  Net effect is that "gsutil version: 4.7"
is always printed to stderr (not to stdout, as to not interfere with stdout
parsing)

https://codereview.chromium.org/870093003/diff/1/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/870093003/diff/1/gsutil.py#newcode137
gsutil.py:137: call(cmd, native=True)
On 2015/01/23 20:51:54, scottmg wrote:
> The only other place it's used is check_gsutil, so probably just get rid of
the
> arg and make it always equivalent to native=True?

Done.

[scottmg, 2015-01-23 21:09:41]

https://codereview.chromium.org/870093003/diff/60001/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/870093003/diff/60001/gsutil.py#newcode40
gsutil.py:40: def call(args, **kwargs):
I think this is just subprocess.call() now?

[hinoka, 2015-01-23 21:56:25]

https://codereview.chromium.org/870093003/diff/60001/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/870093003/diff/60001/gsutil.py#newcode40
gsutil.py:40: def call(args, **kwargs):
On 2015/01/23 21:09:41, scottmg wrote:
> I think this is just subprocess.call() now?

You're right.  Done.

I'm keeping the alias, because I like this pattern.  And subprocess.call() by
default sets all the stdio to None.

[pgervais, 2015-01-23 21:59:18]

https://codereview.chromium.org/870093003/diff/80001/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/870093003/diff/80001/gsutil.py#newcode72
gsutil.py:72: print >> sys.stderr, 'Downloading gsutil from %s...' % url
This is changing the output of the vanilla gsutil command. It's nice for
debugging, but might break some tools that rely on parsing the output.

At the very least this show be behind a debugging flag.

[scottmg, 2015-01-23 22:07:23]

lgtm

https://codereview.chromium.org/870093003/diff/80001/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/870093003/diff/80001/gsutil.py#newcode42
gsutil.py:42: args, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr)
stdout=None, etc means it uses the parent's, which will be sys.stdout. So I
think you can just remove the stdin=, stdout=, stderr= args.

[pgervais, 2015-01-23 22:09:36]

On 2015/01/23 22:07:23, scottmg wrote:
> lgtm
> 
> https://codereview.chromium.org/870093003/diff/80001/gsutil.py
> File gsutil.py (right):
> 
> https://codereview.chromium.org/870093003/diff/80001/gsutil.py#newcode42
> gsutil.py:42: args, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr)
> stdout=None, etc means it uses the parent's, which will be sys.stdout. So I
> think you can just remove the stdin=, stdout=, stderr= args.

Make sure to double-check that before committing, please consider my other
comment as well.

[hinoka, 2015-01-23 23:47:40]

Tested locally with ./gsutil.py --force-version 4.7 config

https://codereview.chromium.org/870093003/diff/80001/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/870093003/diff/80001/gsutil.py#newcode42
gsutil.py:42: args, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr)
On 2015/01/23 22:07:23, scottmg wrote:
> stdout=None, etc means it uses the parent's, which will be sys.stdout. So I
> think you can just remove the stdin=, stdout=, stderr= args.

Oh yep, you're right.  Confirmed locally.

https://codereview.chromium.org/870093003/diff/80001/gsutil.py#newcode72
gsutil.py:72: print >> sys.stderr, 'Downloading gsutil from %s...' % url
On 2015/01/23 21:59:18, pgervais wrote:
> This is changing the output of the vanilla gsutil command. It's nice for
> debugging, but might break some tools that rely on parsing the output.
> 
> At the very least this show be behind a debugging flag.

Removed this printout

[hinoka, 2015-01-23 23:50:13]

The CQ bit was checked by hinoka@chromium.org

[commit-bot: I haz the power, 2015-01-23 23:50:30]

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

[commit-bot: I haz the power, 2015-01-23 23:53:22]

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

[commit-bot: I haz the power, 2015-01-23 23:53:23]

Presubmit check for 870093003-120001 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/gsutil.py

tests/gsutil_test.py (0.09s) failed
EEE
======================================================================
ERROR: test_download_gsutil (__main__.GsutilUnitTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gsutil_test.py", line 75, in setUp
    self.old_call = getattr(gsutil, 'call')
AttributeError: 'module' object has no attribute 'call'

======================================================================
ERROR: test_ensure_gsutil_full (__main__.GsutilUnitTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gsutil_test.py", line 75, in setUp
    self.old_call = getattr(gsutil, 'call')
AttributeError: 'module' object has no attribute 'call'

======================================================================
ERROR: test_ensure_gsutil_short (__main__.GsutilUnitTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gsutil_test.py", line 75, in setUp
    self.old_call = getattr(gsutil, 'call')
AttributeError: 'module' object has no attribute 'call'

----------------------------------------------------------------------
Ran 3 tests in 0.002s

FAILED (errors=3)


Presubmit checks took 168.6s 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, 2015-01-24 00:28:18]

The CQ bit was checked by hinoka@chromium.org

[commit-bot: I haz the power, 2015-01-24 00:28:51]

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

[commit-bot: I haz the power, 2015-01-24 00:31:42]

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

[commit-bot: I haz the power, 2015-01-24 00:31:43]

Presubmit check for 870093003-140001 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/gsutil.py
    depot_tools/tests/gsutil_test.py

Presubmit checks took 165.5s to calculate.

[hinoka, 2015-01-24 00:32:26]

Tests pass, pgervais can you OWNERS stamp?

[pgervais, 2015-01-24 00:42:29]

On 2015/01/24 00:32:26, hinoka wrote:
> Tests pass, pgervais can you OWNERS stamp?

sure. lgtm

[hinoka, 2015-01-24 00:52:11]

The CQ bit was checked by hinoka@chromium.org

[commit-bot: I haz the power, 2015-01-24 00:52:54]

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

[commit-bot: I haz the power, 2015-01-24 00:55:48]

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