From 17d01795a631f5985d87a6ee82ba8302334245e1 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Wed, 1 Sep 2010 18:07:10 +0000 Subject: [PATCH] Cleanup the code in gclient_utils to standardize on CheckCall nomenclature. Simplify code by removing fail_status Rename print_messages to always Simplify the doc. Review URL: http://codereview.chromium.org/3104036 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@58201 0039d316-1c4b-4281-b951-d872f2087c98 --- gcl.py | 4 +- gclient.py | 14 ++-- gclient_scm.py | 13 ++-- gclient_utils.py | 131 ++++++++++++++++++------------------ scm.py | 38 ++++------- tests/gclient_scm_test.py | 2 +- tests/gclient_utils_test.py | 41 +++++------ tests/scm_unittest.py | 19 +++--- 8 files changed, 118 insertions(+), 144 deletions(-) diff --git a/gcl.py b/gcl.py index dc5b25fc2..a9327579d 100755 --- a/gcl.py +++ b/gcl.py @@ -150,8 +150,8 @@ def GetCachedFile(filename, max_age=60*60*24*3, use_root=False): # The fix doesn't work if the user upgraded to svn 1.6.x. Bleh. # I don't have time to fix their broken stuff. args.append('--non-interactive') - SVN.RunAndFilterOutput(args, cwd='.', - filter_fn=content_array.append) + gclient_utils.CheckCallAndFilter( + args, cwd='.', filter_fn=content_array.append) # Exit the loop if the file was found. Override content. content = '\n'.join(content_array) break diff --git a/gclient.py b/gclient.py index 778e52f37..5ca703635 100644 --- a/gclient.py +++ b/gclient.py @@ -448,11 +448,15 @@ class Dependency(GClientKeywords, gclient_utils.WorkItem): splice_index = command.index('$matching_files') command[splice_index:splice_index + 1] = matching_file_list - # Use a discrete exit status code of 2 to indicate that a hook action - # failed. Users of this script may wish to treat hook action failures - # differently from VC failures. - return gclient_utils.SubprocessCall(command, cwd=self.root_dir(), - fail_status=2) + try: + gclient_utils.CheckCallAndFilterAndHeader( + command, cwd=self.root_dir(), always=True) + except gclient_utils.Error, e: + # Use a discrete exit status code of 2 to indicate that a hook action + # failed. Users of this script may wish to treat hook action failures + # differently from VC failures. + print >> sys.stderr, 'Error: %s' % str(e) + sys.exit(2) def root_dir(self): return self.parent.root_dir() diff --git a/gclient_scm.py b/gclient_scm.py index 8e6518d57..4f2efc45a 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -152,10 +152,10 @@ class GitWrapper(SCMWrapper): """ path = os.path.join(self._root_dir, self.relpath) merge_base = self._Run(['merge-base', 'HEAD', 'origin']) - command = ['diff', merge_base] + command = ['git', 'diff', merge_base] filterer = DiffFilterer(self.relpath) - scm.GIT.RunAndFilterOutput(command, cwd=path, filter_fn=filterer.Filter, - stdout=options.stdout) + gclient_utils.CheckCallAndFilter( + command, cwd=path, filter_fn=filterer.Filter, stdout=options.stdout) def update(self, options, args, file_list): """Runs git to update or transparently checkout the working copy. @@ -650,8 +650,7 @@ class GitWrapper(SCMWrapper): stdout = subprocess.PIPE if cwd == None: cwd = self.checkout_path - cmd = [scm.GIT.COMMAND] - cmd.extend(args) + cmd = ['git'] + args logging.debug(cmd) try: sp = gclient_utils.Popen(cmd, cwd=cwd, stdout=stdout) @@ -699,11 +698,11 @@ class SVNWrapper(SCMWrapper): path = os.path.join(self._root_dir, self.relpath) if not os.path.isdir(path): raise gclient_utils.Error('Directory %s is not present.' % path) - command = ['diff', '-x', '--ignore-eol-style'] + command = ['svn', 'diff', '-x', '--ignore-eol-style'] command.extend(args) filterer = DiffFilterer(self.relpath) - scm.SVN.RunAndFilterOutput(command, cwd=path, print_messages=False, + gclient_utils.CheckCallAndFilter(command, cwd=path, always=False, print_stdout=False, filter_fn=filterer.Filter, stdout=options.stdout) diff --git a/gclient_utils.py b/gclient_utils.py index e0c9e8e27..66514f12a 100644 --- a/gclient_utils.py +++ b/gclient_utils.py @@ -118,6 +118,7 @@ def GetNodeNamedAttributeText(node, node_name, attribute_name): class Error(Exception): """gclient exception class.""" + # TODO(maruel): Merge with CheckCallError. pass @@ -251,54 +252,56 @@ def RemoveDirectory(*path): os.rmdir(file_path) -def SubprocessCall(args, **kwargs): - """Wraps SubprocessCallAndFilter() with different default arguments. +def CheckCallAndFilterAndHeader(args, always=False, **kwargs): + """Adds 'header' support to CheckCallAndFilter. - Calls subprocess and capture nothing.""" - kwargs['print_messages'] = True + If |always| is True, a message indicating what is being done + is printed to stdout all the time even if not output is generated. Otherwise + the message header is printed only if the call generated any ouput. + """ + stdout = kwargs.get('stdout', None) or sys.stdout + if always: + stdout.write('\n________ running \'%s\' in \'%s\'\n' + % (' '.join(args), kwargs.get('cwd', '.'))) + else: + filter_fn = kwargs.get('filter_fn', None) + def filter_msg(line): + if line is None: + stdout.write('\n________ running \'%s\' in \'%s\'\n' + % (' '.join(args), kwargs.get('cwd', '.'))) + elif filter_fn: + filter_fn(line) + kwargs['filter_fn'] = filter_msg + kwargs['call_filter_on_first_line'] = True + # Obviously. kwargs['print_stdout'] = True - return SubprocessCallAndFilter(args, **kwargs) - - -def SubprocessCallAndFilter(args, **kwargs): - """Runs a command and prints a header line if appropriate. + return CheckCallAndFilter(args, **kwargs) - If |print_messages| is True, a message indicating what is being done - is printed to stdout. Otherwise the message is printed only if the call - generated any ouput. If both |print_messages| and |print_stdout| are False, - no output at all is generated. - If |print_stdout| is True, the command's stdout is also forwarded to stdout. +def CheckCallAndFilter(args, stdout=None, filter_fn=None, + print_stdout=None, call_filter_on_first_line=False, + **kwargs): + """Runs a command and calls back a filter function if needed. - If |filter_fn| function is specified, it is expected to take a single - string argument, and it will be called with each line of the - subprocess's output. Each line has had the trailing newline character - trimmed. + Accepts all subprocess.Popen() parameters plus: + print_stdout: If True, the command's stdout is forwarded to stdout. + filter_fn: A function taking a single string argument called with each line + of the subprocess's output. Each line has the trailing newline + character trimmed. + stdout: Can be any bufferable output. - If the command fails, as indicated by a nonzero exit status, gclient will - exit with an exit status of fail_status. If fail_status is None (the - default), gclient will raise an Error exception. - - Other subprocess.Popen parameters can be specified. + stderr is always redirected to stdout. """ - stdout = kwargs.pop('stdout', sys.stdout) or sys.stdout + assert print_stdout or filter_fn + stdout = stdout or sys.stdout + filter_fn = filter_fn or (lambda x: None) assert not 'stderr' in kwargs - filter_fn = kwargs.pop('filter_fn', None) - print_messages = kwargs.pop('print_messages', False) - print_stdout = kwargs.pop('print_stdout', False) - fail_status = kwargs.pop('fail_status', None) - logging.debug(args) - if print_messages: - stdout.write('\n________ running \'%s\' in \'%s\'\n' - % (' '.join(args), kwargs['cwd'])) - kid = Popen(args, bufsize=0, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, **kwargs) - # Do a flush of sys.stdout before we begin reading from the subprocess's - # stdout. + # Do a flush of stdout before we begin reading from the subprocess's stdout last_flushed_at = time.time() stdout.flush() @@ -307,40 +310,34 @@ def SubprocessCallAndFilter(args, **kwargs): # normally buffering is done for each line, but if svn requests input, no # end-of-line character is output after the prompt and it would not show up. in_byte = kid.stdout.read(1) - in_line = '' - while in_byte: - if in_byte != '\r': - if print_stdout: - if not print_messages: - stdout.write('\n________ running \'%s\' in \'%s\'\n' - % (' '.join(args), kwargs['cwd'])) - print_messages = True - stdout.write(in_byte) - if in_byte != '\n': - in_line += in_byte - if in_byte == '\n': - if filter_fn: - filter_fn(in_line) - in_line = '' - # Flush at least 10 seconds between line writes. We wait at least 10 - # seconds to avoid overloading the reader that called us with output, - # which can slow busy readers down. - if (time.time() - last_flushed_at) > 10: - last_flushed_at = time.time() - stdout.flush() - in_byte = kid.stdout.read(1) - # Flush the rest of buffered output. This is only an issue with files not - # ending with a \n. - if len(in_line) and filter_fn: - filter_fn(in_line) + if in_byte: + if call_filter_on_first_line: + filter_fn(None) + in_line = '' + while in_byte: + if in_byte != '\r': + if print_stdout: + stdout.write(in_byte) + if in_byte != '\n': + in_line += in_byte + else: + filter_fn(in_line) + in_line = '' + # Flush at least 10 seconds between line writes. We wait at least 10 + # seconds to avoid overloading the reader that called us with output, + # which can slow busy readers down. + if (time.time() - last_flushed_at) > 10: + last_flushed_at = time.time() + stdout.flush() + in_byte = kid.stdout.read(1) + # Flush the rest of buffered output. This is only an issue with + # stdout/stderr not ending with a \n. + if len(in_line): + filter_fn(in_line) rv = kid.wait() - if rv: - msg = 'failed to run command: %s' % ' '.join(args) - if fail_status != None: - sys.stderr.write(msg + '\n') - sys.exit(fail_status) - raise Error(msg) + raise Error('failed to run command: %s' % ' '.join(args)) + return 0 def FindGclientRoot(from_dir, filename='.gclient'): diff --git a/scm.py b/scm.py index ae765f0cd..1c1df13a4 100644 --- a/scm.py +++ b/scm.py @@ -65,8 +65,6 @@ def GenFakeDiff(filename): class GIT(object): - COMMAND = "git" - @staticmethod def Capture(args, in_directory=None, print_error=True, error_ok=False): """Runs git, capturing output sent to stdout as a string. @@ -78,10 +76,8 @@ class GIT(object): Returns: The output sent to stdout as a string. """ - c = [GIT.COMMAND] - c.extend(args) try: - return gclient_utils.CheckCall(c, in_directory, print_error) + return gclient_utils.CheckCall(['git'] + args, in_directory, print_error) except gclient_utils.CheckCallError: if error_ok: return ('', '') @@ -116,11 +112,6 @@ class GIT(object): results.append(('%s ' % m.group(1), m.group(2))) return results - @staticmethod - def RunAndFilterOutput(args, **kwargs): - """Wrapper to gclient_utils.SubprocessCallAndFilter().""" - return gclient_utils.SubprocessCallAndFilter([GIT.COMMAND] + args, **kwargs) - @staticmethod def GetEmail(repo_root): """Retrieves the user email address if known.""" @@ -312,13 +303,13 @@ class GIT(object): class SVN(object): - COMMAND = "svn" current_version = None @staticmethod def Run(args, **kwargs): - """Wrappers to gclient_utils.SubprocessCall().""" - return gclient_utils.SubprocessCall([SVN.COMMAND] + args, **kwargs) + """Wrappers to gclient_utils.CheckCallAndFilterAndHeader().""" + return gclient_utils.CheckCallAndFilterAndHeader(['svn'] + args, + always=True, **kwargs) @staticmethod def Capture(args, in_directory=None, print_error=True): @@ -331,13 +322,11 @@ class SVN(object): Returns: The output sent to stdout as a string. """ - c = [SVN.COMMAND] - c.extend(args) stderr = None if not print_error: stderr = subprocess.PIPE - return gclient_utils.Popen(c, cwd=in_directory, stdout=subprocess.PIPE, - stderr=stderr).communicate()[0] + return gclient_utils.Popen(['svn'] + args, cwd=in_directory, + stdout=subprocess.PIPE, stderr=stderr).communicate()[0] @staticmethod def RunAndGetFileList(verbose, args, cwd, file_list, stdout=None): @@ -393,10 +382,12 @@ class SVN(object): failure.append(line) try: - SVN.RunAndFilterOutput(args, cwd=cwd, print_messages=verbose, - print_stdout=True, - filter_fn=CaptureMatchingLines, - stdout=stdout) + gclient_utils.CheckCallAndFilterAndHeader( + ['svn'] + args, + cwd=cwd, + always=verbose, + filter_fn=CaptureMatchingLines, + stdout=stdout) except gclient_utils.Error: def IsKnownFailure(): for x in failure: @@ -437,11 +428,6 @@ class SVN(object): continue break - @staticmethod - def RunAndFilterOutput(args, **kwargs): - """Wrapper for gclient_utils.SubprocessCallAndFilter().""" - return gclient_utils.SubprocessCallAndFilter([SVN.COMMAND] + args, **kwargs) - @staticmethod def CaptureInfo(relpath, in_directory=None, print_error=True): """Returns a dictionary from the svn info output for the given file. diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index 4503ef61f..b8399df2e 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -32,9 +32,9 @@ class GCBaseTestCase(SuperMoxTestBase): class BaseTestCase(GCBaseTestCase): def setUp(self): GCBaseTestCase.setUp(self) + self.mox.StubOutWithMock(gclient_scm.gclient_utils, 'CheckCallAndFilter') self.mox.StubOutWithMock(gclient_scm.gclient_utils, 'FileRead') self.mox.StubOutWithMock(gclient_scm.gclient_utils, 'FileWrite') - self.mox.StubOutWithMock(gclient_scm.gclient_utils, 'SubprocessCall') self.mox.StubOutWithMock(gclient_scm.gclient_utils, 'RemoveDirectory') self._CaptureSVNInfo = gclient_scm.scm.SVN.CaptureInfo self.mox.StubOutWithMock(gclient_scm.scm.SVN, 'Capture') diff --git a/tests/gclient_utils_test.py b/tests/gclient_utils_test.py index b9e7fa70b..43c5dd911 100755 --- a/tests/gclient_utils_test.py +++ b/tests/gclient_utils_test.py @@ -15,19 +15,20 @@ class GclientUtilBase(SuperMoxTestBase): def setUp(self): super(GclientUtilBase, self).setUp() gclient_utils.sys.stdout.flush = lambda: None + self.mox.StubOutWithMock(gclient_utils, 'Popen') class GclientUtilsUnittest(GclientUtilBase): """General gclient_utils.py tests.""" def testMembersChanged(self): members = [ - 'CheckCall', 'CheckCallError', 'Error', 'ExecutionQueue', 'FileRead', + 'CheckCall', 'CheckCallError', 'CheckCallAndFilter', + 'CheckCallAndFilterAndHeader', 'Error', 'ExecutionQueue', 'FileRead', 'FileWrite', 'FindFileUpwards', 'FindGclientRoot', 'GetGClientRootAndEntries', 'GetNamedNodeText', 'GetNodeNamedAttributeText', 'PathDifference', 'ParseXML', 'Popen', 'PrintableObject', 'RemoveDirectory', 'SplitUrlRevision', - 'SubprocessCall', 'SubprocessCallAndFilter', 'SyntaxErrorToError', - 'WorkItem', + 'SyntaxErrorToError', 'WorkItem', 'errno', 'logging', 'os', 're', 'stat', 'subprocess', 'sys', 'threading', 'time', 'xml', ] @@ -40,14 +41,10 @@ class CheckCallTestCase(GclientUtilBase): args = ['boo', 'foo', 'bar'] process = self.mox.CreateMockAnything() process.returncode = 0 - env = gclient_utils.os.environ.copy() - env['LANGUAGE'] = 'en' - gclient_utils.subprocess.Popen( + gclient_utils.Popen( args, cwd=None, stderr=None, - env=env, - stdout=gclient_utils.subprocess.PIPE, - shell=gclient_utils.sys.platform.startswith('win')).AndReturn(process) + stdout=gclient_utils.subprocess.PIPE).AndReturn(process) process.communicate().AndReturn(['bleh', 'foo']) self.mox.ReplayAll() gclient_utils.CheckCall(args) @@ -56,14 +53,10 @@ class CheckCallTestCase(GclientUtilBase): args = ['boo', 'foo', 'bar'] process = self.mox.CreateMockAnything() process.returncode = 42 - env = gclient_utils.os.environ.copy() - env['LANGUAGE'] = 'en' - gclient_utils.subprocess.Popen( + gclient_utils.Popen( args, cwd=None, stderr=None, - env=env, - stdout=gclient_utils.subprocess.PIPE, - shell=gclient_utils.sys.platform.startswith('win')).AndReturn(process) + stdout=gclient_utils.subprocess.PIPE).AndReturn(process) process.communicate().AndReturn(['bleh', 'foo']) self.mox.ReplayAll() try: @@ -77,7 +70,7 @@ class CheckCallTestCase(GclientUtilBase): self.assertEqual(e.stderr, 'foo') -class SubprocessCallAndFilterTestCase(GclientUtilBase): +class CheckCallAndFilterTestCase(GclientUtilBase): class ProcessIdMock(object): def __init__(self, test_string): self.stdout = StringIO.StringIO(test_string) @@ -86,17 +79,13 @@ class SubprocessCallAndFilterTestCase(GclientUtilBase): def _inner(self, args, test_string): cwd = 'bleh' - env = gclient_utils.os.environ.copy() - env['LANGUAGE'] = 'en' gclient_utils.sys.stdout.write( '\n________ running \'boo foo bar\' in \'bleh\'\n') for i in test_string: gclient_utils.sys.stdout.write(i) - gclient_utils.subprocess.Popen( + gclient_utils.Popen( args, cwd=cwd, - shell=(gclient_utils.sys.platform == 'win32'), - env=env, stdout=gclient_utils.subprocess.PIPE, stderr=gclient_utils.subprocess.STDOUT, bufsize=0).AndReturn(self.ProcessIdMock(test_string)) @@ -107,22 +96,22 @@ class SubprocessCallAndFilterTestCase(GclientUtilBase): capture_list = [] def FilterLines(line): line_list.append(line) + assert isinstance(line, str), type(line) match = compiled_pattern.search(line) if match: capture_list.append(match.group(1)) - gclient_utils.SubprocessCallAndFilter( - args, cwd=cwd, print_messages=True, print_stdout=True, - filter_fn=FilterLines) + gclient_utils.CheckCallAndFilterAndHeader( + args, cwd=cwd, always=True, filter_fn=FilterLines) self.assertEquals(line_list, ['ahah', 'accb', 'allo', 'addb']) self.assertEquals(capture_list, ['cc', 'dd']) - def testSubprocessCallAndFilter(self): + def testCheckCallAndFilter(self): args = ['boo', 'foo', 'bar'] test_string = 'ahah\naccb\nallo\naddb\n' self._inner(args, test_string) def testNoLF(self): - # Exactly as testSubprocessCallAndFilter without trailing \n + # Exactly as testCheckCallAndFilterAndHeader without trailing \n args = ['boo', 'foo', 'bar'] test_string = 'ahah\naccb\nallo\naddb' self._inner(args, test_string) diff --git a/tests/scm_unittest.py b/tests/scm_unittest.py index b5ea9ba23..43d1c796d 100755 --- a/tests/scm_unittest.py +++ b/tests/scm_unittest.py @@ -29,8 +29,8 @@ 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') + self.mox.StubOutWithMock(scm.gclient_utils, 'CheckCallAndFilter') + self.mox.StubOutWithMock(scm.gclient_utils, 'CheckCallAndFilterAndHeader') class RootTestCase(BaseSCMTestCase): @@ -129,12 +129,11 @@ from :3 def testMembersChanged(self): self.mox.ReplayAll() members = [ - 'COMMAND', 'AssertVersion', 'Capture', 'CaptureStatus', + 'AssertVersion', 'Capture', 'CaptureStatus', 'FetchUpstreamTuple', 'GenerateDiff', 'GetBranch', 'GetBranchRef', 'GetCheckoutRoot', 'GetDifferentFiles', 'GetEmail', 'GetPatchName', 'GetSVNBranch', - 'GetUpstreamBranch', 'IsGitSvn', 'RunAndFilterOutput', - 'ShortBranchName', + 'GetUpstreamBranch', 'IsGitSvn', 'ShortBranchName', ] # If this test fails, you should add the relevant test. self.compareMembers(scm.GIT, members) @@ -158,12 +157,11 @@ class SVNTestCase(BaseSCMTestCase): def testMembersChanged(self): self.mox.ReplayAll() members = [ - 'COMMAND', 'AssertVersion', 'Capture', 'CaptureBaseRevision', + 'AssertVersion', 'Capture', 'CaptureBaseRevision', 'CaptureHeadRevision', 'CaptureInfo', 'CaptureStatus', 'current_version', 'DiffItem', 'GenerateDiff', 'GetCheckoutRoot', 'GetEmail', 'GetFileProperty', 'IsMoved', - 'IsMovedInfo', 'ReadSimpleAuth', 'Run', 'RunAndFilterOutput', - 'RunAndGetFileList', + 'IsMovedInfo', 'ReadSimpleAuth', 'Run', 'RunAndGetFileList', ] # If this test fails, you should add the relevant test. self.compareMembers(scm.SVN, members) @@ -315,8 +313,9 @@ class SVNTestCase(BaseSCMTestCase): def testRun(self): param2 = 'bleh' - scm.gclient_utils.SubprocessCall(['svn', 'foo', 'bar'], - cwd=param2).AndReturn(None) + scm.gclient_utils.CheckCallAndFilterAndHeader( + ['svn', 'foo', 'bar'], cwd=param2, + always=True).AndReturn(None) self.mox.ReplayAll() scm.SVN.Run(['foo', 'bar'], cwd=param2)