Remove code duplication and improve style.

Create a Popen function to reduce code duplication.
Use RemoveDirectory where relevant.
Make drover slightly more posix friendly.

Review URL: http://codereview.chromium.org/3126020

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@56893 0039d316-1c4b-4281-b951-d872f2087c98

drover.py

@@ -8,10 +8,11 @@ import os
 import re
 import subprocess
 import sys
-import webbrowser
 
 import breakpad
 
+import gclient_utils
+
 USAGE = """
 WARNING: Please use this tool in an empty directory
 (or at least one that you don't mind clobbering.)
@@ -46,31 +47,6 @@ files_info_ = None
 delete_map_ = None
 file_pattern_ =  r"[ ]+([MADUC])[ ]+/((?:trunk|branches/.*?)/src(.*)/(.*))"
 
-def deltree(root):
-  """Removes a given directory"""
-  if (not os.path.exists(root)):
-    return
-
-  if sys.platform == 'win32':
-    os.system('rmdir /S /Q ' + root.replace('/','\\'))
-  else:
-    for name in os.listdir(root):
-      path = os.path.join(root, name)
-      if os.path.isdir(path):
-        deltree(path)
-      else:
-        os.unlink(path)
-    os.rmdir(root)
-
-def clobberDir(dirname):
-  """Removes a given directory"""
-
-  if (os.path.exists(dirname)):
-    print dir + " directory found, deleting"
-    # The following line was removed due to access controls in Windows
-    # which make os.unlink(path) calls impossible.
-    #TODO(laforge) : Is this correct?
-    deltree(dirname)
 
 def runGcl(subcommand):
   gcl_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), "gcl")
@@ -87,11 +63,9 @@ def gclUpload(revision, author):
   return runGcl(command)
 
 def getSVNInfo(url, revision):
-  command = 'svn info ' + url + "@"+str(revision)
-  svn_info = subprocess.Popen(command,
-                              shell=True,
-                              stdout=subprocess.PIPE,
-                              stderr=subprocess.PIPE).stdout.readlines()
+  svn_info = gclient_utils.Popen(['svn', 'info', '%s@%s' % (url, revision)],
+                                 stdout=subprocess.PIPE,
+                                 stderr=subprocess.PIPE).stdout.readlines()
   info = {}
   for line in svn_info:
     match = re.search(r"(.*?):(.*)", line)
@@ -101,11 +75,9 @@ def getSVNInfo(url, revision):
   return info
 
 def isSVNDirty():
-  command = 'svn status'
-  svn_status = subprocess.Popen(command,
-                                shell=True,
-                                stdout=subprocess.PIPE,
-                                stderr=subprocess.PIPE).stdout.readlines()
+  svn_status = gclient_utils.Popen(['svn', 'status'],
+                                   stdout=subprocess.PIPE,
+                                   stderr=subprocess.PIPE).stdout.readlines()
   for line in svn_status:
     match = re.search(r"^[^X?]", line)
     if match:
@@ -145,22 +117,18 @@ def inCheckoutRoot(path):
 
 def getRevisionLog(url, revision):
   """Takes an svn url and gets the associated revision."""
-  command = 'svn log ' + url + " -r"+str(revision)
-  svn_log = subprocess.Popen(command,
-                             shell=True,
-                             stdout=subprocess.PIPE,
-                             stderr=subprocess.PIPE).stdout.readlines()
+  svn_log = gclient_utils.Popen(['svn', 'log', url, '-r', str(revision)],
+                                stdout=subprocess.PIPE,
+                                stderr=subprocess.PIPE).stdout.readlines()
   # Don't include the header lines and the trailing "---..." line and eliminate
   # any '\r's.
   return ''.join([l.replace('\r','') for l in svn_log[3:-1]])
 
 def getSVNVersionInfo():
   """Extract version information from SVN"""
-  command = 'svn --version'
-  svn_info = subprocess.Popen(command,
-                              shell=True,
-                              stdout=subprocess.PIPE,
-                              stderr=subprocess.PIPE).stdout.readlines()
+  svn_info = gclient_utils.Popen(['svn', '--version'],
+                                 stdout=subprocess.PIPE,
+                                 stderr=subprocess.PIPE).stdout.readlines()
   info = {}
   for line in svn_info:
     match = re.search(r"svn, version ((\d+)\.(\d+)\.(\d+)) \(r(\d+)\)", line)
@@ -304,16 +272,14 @@ def revertRevision(url, revision):
     os.system(command)
 
 def getFileInfo(url, revision):
-  global files_info_, file_pattern_
+  global files_info_
 
   if (files_info_ != None):
     return files_info_
 
-  command = 'svn log ' + url + " -r " + str(revision) + " -v"
-  svn_log = subprocess.Popen(command,
-                             shell=True,
-                             stdout=subprocess.PIPE,
-                             stderr=subprocess.PIPE).stdout.readlines()
+  svn_log = gclient_utils.Popen(['svn', 'log', url, '-r', str(revision), '-v'],
+                                stdout=subprocess.PIPE,
+                                stderr=subprocess.PIPE).stdout.readlines()
 
   info = []
   for line in svn_log:
@@ -470,7 +436,7 @@ def drover(options, args):
     if not (options.revertbot or SKIP_CHECK_WORKING or
         prompt("Working directory: '%s' already exists, clobber?" % working)):
       return 0
-    deltree(working)
+    gclient_utils.RemoveDirectory(working)
 
   if not options.local:
     os.makedirs(working)

gcl.py

@@ -208,14 +208,8 @@ def ErrorExit(msg):
 
 def RunShellWithReturnCode(command, print_output=False):
   """Executes a command and returns the output and the return code."""
-  # Use a shell for subcommands on Windows to get a PATH search, and because svn
-  # may be a batch file.
-  use_shell = sys.platform.startswith("win")
-  env = os.environ.copy()
-  env['LANGUAGE'] = 'en'
-  p = subprocess.Popen(command, stdout=subprocess.PIPE,
-                       stderr=subprocess.STDOUT, shell=use_shell, env=env,
-                       universal_newlines=True)
+  p = gclient_utils.Popen(command, stdout=subprocess.PIPE,
+                          stderr=subprocess.STDOUT, universal_newlines=True)
   if print_output:
     output_array = []
     while True:

gclient_scm.py

@@ -653,7 +653,7 @@ class GitWrapper(SCMWrapper):
     cmd.extend(args)
     logging.debug(cmd)
     try:
-      sp = subprocess.Popen(cmd, cwd=cwd, stdout=stdout)
+      sp = gclient_utils.Popen(cmd, cwd=cwd, stdout=stdout)
       output = sp.communicate()[0]
     except OSError:
       raise gclient_utils.Error("git command '%s' failed to run." %

gclient_utils.py

@@ -23,7 +23,6 @@ import subprocess
 import sys
 import threading
 import time
-import threading
 import xml.dom.minidom
 import xml.parsers.expat
 
@@ -39,8 +38,20 @@ class CheckCallError(OSError):
     self.stderr = stderr
 
 
+def Popen(*args, **kwargs):
+  # *Sigh*:  Windows needs shell=True, or else it won't search %PATH% for the
+  # executable, but shell=True makes subprocess on Linux fail when it's called
+  # with a list because it only tries to execute the first item in the list.
+  if not 'env' in kwargs:
+    # It's easier to parse the stdout if it is always in English.
+    kwargs['env'] = os.environ.copy()
+    kwargs['env']['LANGUAGE'] = 'en'
+  return subprocess.Popen(*args, shell=(sys.platform=='win32'), **kwargs)
+
+
 def CheckCall(command, cwd=None, print_error=True):
-  """Like subprocess.check_call() but returns stdout.
+  """Similar subprocess.check_call() but redirects stdout and
+  returns (stdout, stderr).
 
   Works on python 2.4
   """
@@ -49,13 +60,7 @@ def CheckCall(command, cwd=None, print_error=True):
     stderr = None
     if not print_error:
       stderr = subprocess.PIPE
-    env = os.environ.copy()
-    env['LANGUAGE'] = 'en'
-    process = subprocess.Popen(command, cwd=cwd,
-                               shell=sys.platform.startswith('win'),
-                               stdout=subprocess.PIPE,
-                               stderr=stderr,
-                               env=env)
+    process = Popen(command, cwd=cwd, stdout=subprocess.PIPE, stderr=stderr)
     std_out, std_err = process.communicate()
   except OSError, e:
     raise CheckCallError(command, cwd, e.errno, None)
@@ -275,15 +280,9 @@ def SubprocessCallAndFilter(command,
   if print_messages:
     print('\n________ running \'%s\' in \'%s\''
           % (' '.join(command), in_directory))
-  env = os.environ.copy()
-  env['LANGUAGE'] = 'en'
 
-  # *Sigh*:  Windows needs shell=True, or else it won't search %PATH% for the
-  # executable, but shell=True makes subprocess on Linux fail when it's called
-  # with a list because it only tries to execute the first item in the list.
-  kid = subprocess.Popen(command, bufsize=0, cwd=in_directory,
-      shell=(sys.platform == 'win32'), stdout=subprocess.PIPE,
-      stderr=subprocess.STDOUT, env=env)
+  kid = Popen(command, bufsize=0, cwd=in_directory,
+              stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
 
   # Do a flush of sys.stdout before we begin reading from the subprocess's
   # stdout.
@@ -323,7 +322,7 @@ def SubprocessCallAndFilter(command,
     msg = 'failed to run command: %s' % ' '.join(command)
 
     if fail_status != None:
-      print >>sys.stderr, msg
+      print >> sys.stderr, msg
       sys.exit(fail_status)
 
     raise Error(msg)
@@ -333,10 +332,10 @@ def FindGclientRoot(from_dir, filename='.gclient'):
   """Tries to find the gclient root."""
   path = os.path.realpath(from_dir)
   while not os.path.exists(os.path.join(path, filename)):
-    next = os.path.split(path)
-    if not next[1]:
+    split_path = os.path.split(path)
+    if not split_path[1]:
       return None
-    path = next[0]
+    path = split_path[0]
   logging.info('Found gclient root at ' + path)
   return path
 

scm.py

@@ -215,7 +215,7 @@ class GIT(object):
       # pipe at a time.
       # The -100 is an arbitrary limit so we don't search forever.
       cmd = ['git', 'log', '-100', '--pretty=medium']
-      proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, cwd=cwd)
+      proc = gclient_utils.Popen(cmd, stdout=subprocess.PIPE, cwd=cwd)
       for line in proc.stdout:
         match = git_svn_re.match(line)
         if match:
@@ -371,19 +371,11 @@ class SVN(object):
     """
     c = [SVN.COMMAND]
     c.extend(args)
-
-    # *Sigh*:  Windows needs shell=True, or else it won't search %PATH% for
-    # the svn.exe executable, but shell=True makes subprocess on Linux fail
-    # when it's called with a list because it only tries to execute the
-    # first string ("svn").
     stderr = None
     if not print_error:
       stderr = subprocess.PIPE
-    return subprocess.Popen(c,
-                            cwd=in_directory,
-                            shell=(sys.platform == 'win32'),
-                            stdout=subprocess.PIPE,
-                            stderr=stderr).communicate()[0]
+    return gclient_utils.Popen(c, cwd=in_directory, stdout=subprocess.PIPE,
+        stderr=stderr).communicate()[0]
 
   @staticmethod
   def RunAndGetFileList(verbose, args, in_directory, file_list):

tests/gclient_utils_test.py

@@ -24,7 +24,7 @@ class GclientUtilsUnittest(GclientUtilBase):
         'CheckCall', 'CheckCallError', 'Error', 'ExecutionQueue', 'FileRead',
         'FileWrite', 'FindFileUpwards', 'FindGclientRoot',
         'GetGClientRootAndEntries', 'GetNamedNodeText',
-        'GetNodeNamedAttributeText', 'PathDifference', 'ParseXML',
+        'GetNodeNamedAttributeText', 'PathDifference', 'ParseXML', 'Popen',
         'PrintableObject', 'RemoveDirectory', 'SplitUrlRevision',
         'SubprocessCall', 'SubprocessCallAndFilter', 'SyntaxErrorToError',
         'WorkItem',

tests/scm_unittest.py

@@ -28,6 +28,7 @@ class BaseTestCase(SuperMoxTestBase):
 class BaseSCMTestCase(BaseTestCase):
   def setUp(self):
     BaseTestCase.setUp(self)
+    self.mox.StubOutWithMock(scm.gclient_utils, 'Popen')
     self.mox.StubOutWithMock(scm.gclient_utils, 'SubprocessCall')
     self.mox.StubOutWithMock(scm.gclient_utils, 'SubprocessCallAndFilter')
 
@@ -295,11 +296,10 @@ class SVNTestCase(BaseSCMTestCase):
 </status>
 """
     proc = self.mox.CreateMockAnything()
-    scm.subprocess.Popen(['svn', 'status', '--xml', '.'],
-                         cwd=None,
-                         shell=scm.sys.platform.startswith('win'),
-                         stderr=None,
-                         stdout=scm.subprocess.PIPE).AndReturn(proc)
+    scm.gclient_utils.Popen(['svn', 'status', '--xml', '.'],
+                            cwd=None,
+                            stderr=None,
+                            stdout=scm.subprocess.PIPE).AndReturn(proc)
     proc.communicate().AndReturn((text, 0))
 
     self.mox.ReplayAll()
@@ -328,11 +328,10 @@ class SVNTestCase(BaseSCMTestCase):
        </target>
        </status>"""
     proc = self.mox.CreateMockAnything()
-    scm.subprocess.Popen(['svn', 'status', '--xml'],
-                         cwd=None,
-                         shell=scm.sys.platform.startswith('win'),
-                         stderr=None,
-                         stdout=scm.subprocess.PIPE).AndReturn(proc)
+    scm.gclient_utils.Popen(['svn', 'status', '--xml'],
+                            cwd=None,
+                            stderr=None,
+                            stdout=scm.subprocess.PIPE).AndReturn(proc)
     proc.communicate().AndReturn((text, 0))
     self.mox.ReplayAll()
     info = scm.SVN.CaptureStatus(None)