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

@@ -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

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()

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)
 

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'):

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.

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')

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)

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)