Restart codereview issue 6792042

lib/cros_build_lib.py

Index: lib/cros_build_lib.py
diff --git a/lib/cros_build_lib.py b/lib/cros_build_lib.py
index f7ea5e9c332397e8b639f1e1407f13ed8efdc22c..177d013c8ea84df83a81bb1c48d8f172e7102411 100644
--- a/lib/cros_build_lib.py
+++ b/lib/cros_build_lib.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
+ # Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
@@ -7,6 +7,7 @@
 import inspect
 import os
 import re
+import shlex

[davidjames, 2011/04/14 21:59:05]

This is unused now, right?

[Peter Mayo, 2011/04/19 03:59:39]

Done.

 import signal
 import subprocess
 import sys
@@ -43,11 +44,13 @@ class RunCommandError(Exception):
 def RunCommand(cmd, print_cmd=True, error_ok=False, error_message=None,
                exit_code=False, redirect_stdout=False, redirect_stderr=False,
                cwd=None, input=None, enter_chroot=False, shell=False,
-               env=None, ignore_sigint=False, combine_stdout_stderr=False):
+               env=None, extra_env=None, ignore_sigint=False,
+               combine_stdout_stderr=False):
   """Runs a command.
 
   Args:
-    cmd: cmd to run.  Should be input to subprocess.Popen.
+    cmd: cmd to run.  Should be input to subprocess.Popen. If a string, it will
+        be converted to an array by shlex.split()

[sosa, 2011/04/14 21:22:18]

Indentation is inconsistent here.

[davidjames, 2011/04/14 21:59:05]

Document that if shell is False, cmd must be an array. Otherwise it must be a
string.

[Peter Mayo, 2011/04/19 03:59:39]

Done.

     print_cmd: prints the command before running it.
     error_ok: does not raise an exception on error.
     error_message: prints out this message when an error occurrs.
@@ -61,6 +64,7 @@ def RunCommand(cmd, print_cmd=True, error_ok=False, error_message=None,
     shell: If shell is True, the specified command will be executed through
       the shell.

[davidjames, 2011/04/14 21:59:05]

Document that if shell is True then we expect a string as input.

[Peter Mayo, 2011/04/19 03:59:39]

Done.

     env: If non-None, this is the environment for the new process.

[davidjames, 2011/04/14 21:59:05]

Document that this is not compatible with enter_chroot mode

[Peter Mayo, 2011/04/19 03:59:39]

It's not incompatible, just mostly useless - documented that.

+    extra_env: If set, this is added to the environment for the new process.
     ignore_sigint: If True, we'll ignore signal.SIGINT before calling the
       child.  This is the desired behavior if we know our child will handle
       Ctrl-C.  If we don't do this, I think we and the child will both get
@@ -86,24 +90,36 @@ def RunCommand(cmd, print_cmd=True, error_ok=False, error_message=None,
   # TODO(sosa): gpylint complains about redefining built-in 'input'.
   #   Can we rename this variable?
   if input: stdin = subprocess.PIPE
-  if isinstance(cmd, basestring):
-    if enter_chroot: cmd = './enter_chroot.sh -- ' + cmd
-    cmd_str = cmd
-  else:
-    if enter_chroot: cmd = ['./enter_chroot.sh', '--'] + cmd
-    cmd_str = ' '.join(cmd)
+
+  if isinstance(cmd, basestring) or shell:

[davidjames, 2011/04/14 21:59:05]

Can we report errors when the shell variable is not consistent with cmd? E.g.,
if shell = True and cmd is not a string, it's an error. Further, if shell =
False and cmd is a string, it's an error.

Python does support both of the above but it's not a very useful thing so I'd
rather error.

[Peter Mayo, 2011/04/19 03:59:39]

Done.

+    cmd = ['sh', '-c', cmd]

[sosa, 2011/04/14 21:22:18]

Does this work for piped commands?

[davidjames, 2011/04/14 21:59:05]

It should work. We should probably use /bin/sh for consistency with the Python
implementation. Here's the official implementation (from Python itself)
http://svn.python.org/view/*checkout*/python/trunk/Lib/subprocess.py

  if isinstance(args, types.StringTypes):
    args = [args]
  else:
    args = list(args)

  if shell:
    args = ["/bin/sh", "-c"] + args

[Peter Mayo, 2011/04/19 03:59:39]

Done.  But I didn't change the isinstance that way ... I don't know it and I
don't think it adds value.

+    shell = False
+
+  # If we are using enter_chroot we need to use enterchroot pass env through
+  # to the final command.
+  if enter_chroot:
+    cmd = ['./enter_chroot.sh', '--'] + cmd
+    if extra_env:

[sosa, 2011/04/14 21:22:18]

So env is incompatible with extra_env ... should there be an assert and an
additional docstring in the method describing this?

[davidjames, 2011/04/14 21:59:05]

env is just plain ignored in enter_chroot mode. Yeah I think it would make sense
to error if env is set and we're in enter_chroot mode.

[Peter Mayo, 2011/04/19 03:59:39]

I don't think so ... I have documented the probable uselessness of the effort in
the docstring, and if enter_chroot ever does need some environment to get it to
do the right thing,  this would be the simple and natural way to do it. 
Clearing out the environment even for those things that enter_chroot does pass
through could also be useful.

+      for (key, value) in extra_env.items():
+        cmd.insert(1, '%s=%s' % (key, value))
+  elif extra_env:
+    if env is not None:

[sosa, 2011/04/14 21:22:18]

if env:

[davidjames, 2011/04/14 21:59:05]

What if env is the empty array? It's useful to be able to clear the environment,
so I think the current check (env is not None) is better.

[Peter Mayo, 2011/04/19 03:59:39]

I agree with davidjames here.

+      env = env.copy()
+    else:
+      env = os.environ.copy()

[sosa, 2011/04/14 21:22:18]

Add extra line

[Peter Mayo, 2011/04/19 03:59:39]

OK, Done...  it reads worse to my eye the new way because the indentation is
harder to estimate.  Python is still new to me though.  I will happily put it
back if reviewers agree.

+    env.update(extra_env)
 
   # Print out the command before running.
   if print_cmd:
     if cwd:
-      Info('RunCommand: %s in %s' % (cmd_str, cwd))
+      Info('RunCommand: %r in %s' % (cmd, cwd))
     else:
-      Info('RunCommand: %s' % cmd_str)
+      Info('RunCommand: %r' % cmd)
   cmd_result.cmd = cmd
 
   try:
     proc = subprocess.Popen(cmd, cwd=cwd, stdin=stdin, stdout=stdout,
-                            stderr=stderr, shell=shell, env=env)
+                            stderr=stderr, shell=False, env=env)
     if ignore_sigint:
       old_sigint = signal.signal(signal.SIGINT, signal.SIG_IGN)
     try:
@@ -116,7 +132,7 @@ def RunCommand(cmd, print_cmd=True, error_ok=False, error_message=None,
       cmd_result.returncode = proc.returncode
 
     if not error_ok and proc.returncode:
-      msg = ('Command "%s" failed.\n' % cmd_str +
+      msg = ('Command "%r" failed.\n' % cmd +
              (error_message or cmd_result.error or cmd_result.output or ''))
       raise RunCommandError(msg, cmd)
   # TODO(sosa): is it possible not to use the catch-all Exception here?