From 850ee065dd840ef7112671c11fc54d9e6b52aa2f Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Fri, 20 Aug 2010 18:41:13 +0000 Subject: [PATCH] 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 | 72 ++++++++++--------------------------- gcl.py | 10 ++---- gclient_scm.py | 2 +- gclient_utils.py | 41 +++++++++++---------- scm.py | 14 ++------ tests/gclient_utils_test.py | 2 +- tests/scm_unittest.py | 19 +++++----- 7 files changed, 55 insertions(+), 105 deletions(-) diff --git a/drover.py b/drover.py index 6dbd0d5d9..43c44885d 100755 --- a/drover.py +++ b/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) diff --git a/gcl.py b/gcl.py index 8c89a7a04..7977229f0 100755 --- a/gcl.py +++ b/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: diff --git a/gclient_scm.py b/gclient_scm.py index 734510c49..be176fb45 100644 --- a/gclient_scm.py +++ b/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." % diff --git a/gclient_utils.py b/gclient_utils.py index 1efa036f6..dd5ae1826 100644 --- a/gclient_utils.py +++ b/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 diff --git a/scm.py b/scm.py index 1f2421919..924f12641 100644 --- a/scm.py +++ b/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): diff --git a/tests/gclient_utils_test.py b/tests/gclient_utils_test.py index 01b222107..06388780b 100755 --- a/tests/gclient_utils_test.py +++ b/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', diff --git a/tests/scm_unittest.py b/tests/scm_unittest.py index d2acc04b9..ac1e34026 100755 --- a/tests/scm_unittest.py +++ b/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): """ 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): """ 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)