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 signal
 import subprocess
@@ skipping 24 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, shell
+      must be true, and it will be passed to one, otherwise the command is

[davidjames, 2011/04/19 06:38:59]

Run-on sentence. The phrase "and it will be passed to one" is confusing.
Suggestion: s/, and it will be passed to one, otherwise the command is/.
Otherwise, the command must be/

[Peter Mayo, 2011/04/19 17:43:17]

Done.

+      an array of arguments, and shell must be false.
     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.
-    env: If non-None, this is the environment for the new process.
+      cwd must point to the scripts directory.  Shell must also be false, and

[davidjames, 2011/04/19 06:38:59]

Looks like this implementation works now if enter_chroot=True and cmd is a
string. Should we remove this documentation about how shell must be false?

[Peter Mayo, 2011/04/19 17:43:17]

Done.

+      the cmd must be an array.
+    shell: Controls whether we add a shell as a command interpreter.  See cmd
+      since it has to agree as to the type.
+    env: If non-None, this is the environment for the new process.  If
+      enter_chroot is true then this is the environment of the enter_chroot,
+      most of which gets removed from the cmd run.

[davidjames, 2011/04/19 06:38:59]

These are removed by sudo itself thanks to the env_reset option, which is
currently enabled.

[Peter Mayo, 2011/04/19 17:43:17]

I think that baking in any more of the implementation of enter_chroot into
documentation here makes the combination more brittle, but not more useful.

+    extra_env: If set, this is added to the environment for the new process.

[davidjames, 2011/04/19 06:38:59]

Unlike env, these variables are explicitly whitelisted in our enter_chroot
command so that they will be visible to the underlying command.

[Peter Mayo, 2011/04/19 17:43:17]

I meant to comment that, as well as the assymetry for clearing variables.  
Added.  Please check the way it reads to you.

     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 not shell:
+      raise Exception(msg="Can not run a string command without a shell")

[davidjames, 2011/04/19 06:38:59]

s/Can not/Cannot/
s/"/'/g

[Peter Mayo, 2011/04/19 17:43:17]

Done.

[Peter Mayo, 2011/04/19 17:43:17]

Done.

+    cmd = ['/bin/sh', '-c', cmd]
+    shell = False
+  elif shell:
+    raise Exception(msg="Can not run an array command with a shell")

[davidjames, 2011/04/19 06:38:59]

Same here.

[Peter Mayo, 2011/04/19 17:43:17]

Done.

+
+  # 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:
+      for (key, value) in extra_env.items():
+        cmd.insert(1, '%s=%s' % (key, value))
+  elif extra_env:
+    if env is not None:
+      env = env.copy()
+    else:
+      env = os.environ.copy()
+
+    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