Restart codereview issue 6792042

lib/cros_build_lib.py

-# 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.
 
 """Common python commands used by various build scripts."""
 
 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
 from terminal import Color
 
 
 _STDOUT_IS_TTY = hasattr(sys.stdout, 'isatty') and sys.stdout.isatty()
 
 
 class CommandResult(object):
@@ skipping 16 matching lines @@
     return (type(self) == type(other) and
             str(self) == str(other) and
             self.cmd == other.cmd)
 
   def __ne__(self, other):
     return not self.__eq__(other)
 
 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.
     exit_code: returns the return code of the shell command.
     redirect_stdout: returns the stdout.
     redirect_stderr: holds stderr output until input is communicated.
     cwd: the working directory to run this cmd.
     input: input to pipe into this command through stdin.
     enter_chroot: this command should be run from within the chroot.  If set,
       cwd must point to the scripts directory.
     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
       Ctrl-C at the same time, which means we'll forcefully kill the child.
     combine_stdout_stderr: Combines stdout and stdin streams into stdout.
 
   Returns:
     A CommandResult object.
 
   Raises:
     Exception:  Raises generic exception on error with optional error_message.
   """
   # Set default for variables.
   stdout = None
   stderr = None
   stdin = None
   cmd_result = CommandResult()
 
   # Modify defaults based on parameters.
   if redirect_stdout: stdout = subprocess.PIPE
   if redirect_stderr: stderr = subprocess.PIPE
   if combine_stdout_stderr: stderr = subprocess.STDOUT
   # 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:
       (cmd_result.output, cmd_result.error) = proc.communicate(input)
     finally:
       if ignore_sigint:
         signal.signal(signal.SIGINT, old_sigint)
 
     if exit_code:
       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?
   except Exception, e:
     if not error_ok:
       raise
     else:
       Warning(str(e))
 
   return cmd_result
@@ skipping 247 matching lines @@
     except RunCommandException as e:
       if not error_ok and retry_count == num_retries:
         raise e
       else:
         Warning(str(e))
         if print_cmd:
           Info('PROGRAM(%s) -> RunCommand: retrying %r in dir %s' %
                (GetCallerName(), cmd, cwd))
 
   return output