From 24146be8336f4431e79afd24dee5545a059e4267 Mon Sep 17 00:00:00 2001 From: Edward Lemur Date: Thu, 1 Aug 2019 21:44:52 +0000 Subject: [PATCH] depot_tools: Simplify CheckCallAndFilter[AndHeader] - Merge CheckCallAndFilter[AndHeader] - print_stdout will cause command output to be redirected to sys.stdout. - Made compatible with Python 3. Bug: 984182 Change-Id: Ida30e295b872c8c1a1474a376a90aecea924f307 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1727404 Commit-Queue: Edward Lesmes Reviewed-by: Robbie Iannucci --- gclient.py | 5 +- gclient_scm.py | 26 +++--- gclient_utils.py | 162 ++++++++++++++++++----------------- tests/gclient_scm_test.py | 11 +-- tests/gclient_utils_test.py | 166 ++++++++++++++++++++++-------------- tests/scm_unittest.py | 2 - 6 files changed, 204 insertions(+), 168 deletions(-) diff --git a/gclient.py b/gclient.py index a77decab0a..f940f68308 100755 --- a/gclient.py +++ b/gclient.py @@ -252,8 +252,9 @@ class Hook(object): try: start_time = time.time() - gclient_utils.CheckCallAndFilterAndHeader( - cmd, cwd=self.effective_cwd, always=self._verbose) + gclient_utils.CheckCallAndFilter( + cmd, cwd=self.effective_cwd, print_stdout=True, show_header=True, + always_show_header=self._verbose) except (gclient_utils.Error, subprocess2.CalledProcessError) as 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 diff --git a/gclient_scm.py b/gclient_scm.py index 72a5dd50ac..6bd056678f 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -912,7 +912,7 @@ class GitWrapper(SCMWrapper): merge_base = [] self._Run( ['-c', 'core.quotePath=false', 'diff', '--name-status'] + merge_base, - options, stdout=self.out_fh, always=options.verbose) + options, always_show_header=options.verbose) if file_list is not None: files = self._GetDiffFilenames(merge_base[0] if merge_base else None) file_list.extend([os.path.join(self.checkout_path, f) for f in files]) @@ -1037,12 +1037,12 @@ class GitWrapper(SCMWrapper): clone_cmd.append(tmp_dir) if self.print_outbuf: print_stdout = True - stdout = gclient_utils.WriteToStdout(self.out_fh) + filter_fn = None else: print_stdout = False - stdout = self.out_fh + filter_fn = self.filter self._Run(clone_cmd, options, cwd=self._root_dir, retry=True, - print_stdout=print_stdout, stdout=stdout) + print_stdout=print_stdout, filter_fn=filter_fn) gclient_utils.safe_makedirs(self.checkout_path) gclient_utils.safe_rename(os.path.join(tmp_dir, '.git'), os.path.join(self.checkout_path, '.git')) @@ -1358,18 +1358,14 @@ class GitWrapper(SCMWrapper): revision = self._Capture(['rev-parse', 'FETCH_HEAD']) return revision - def _Run(self, args, options, show_header=True, **kwargs): + def _Run(self, args, options, **kwargs): # Disable 'unused options' warning | pylint: disable=unused-argument kwargs.setdefault('cwd', self.checkout_path) - kwargs.setdefault('stdout', self.out_fh) - kwargs['filter_fn'] = self.filter - kwargs.setdefault('print_stdout', False) + kwargs.setdefault('filter_fn', self.filter) + kwargs.setdefault('show_header', True) env = scm.GIT.ApplyEnvVars(kwargs) cmd = ['git'] + args - if show_header: - gclient_utils.CheckCallAndFilterAndHeader(cmd, env=env, **kwargs) - else: - gclient_utils.CheckCallAndFilter(cmd, env=env, **kwargs) + gclient_utils.CheckCallAndFilter(cmd, env=env, **kwargs) class CipdPackage(object): @@ -1480,7 +1476,8 @@ class CipdRoot(object): '-root', self.root_dir, '-ensure-file', ensure_file, ] - gclient_utils.CheckCallAndFilterAndHeader(cmd) + gclient_utils.CheckCallAndFilter( + cmd, print_stdout=True, show_header=True) def run(self, command): if command == 'update': @@ -1565,8 +1562,7 @@ class CipdWrapper(SCMWrapper): '-version', self._package.version, '-json-output', describe_json_path ] - gclient_utils.CheckCallAndFilter( - cmd, filter_fn=lambda _line: None, print_stdout=False) + gclient_utils.CheckCallAndFilter(cmd) with open(describe_json_path) as f: describe_json = json.load(f) return describe_json.get('result', {}).get('pin', {}).get('instance_id') diff --git a/gclient_utils.py b/gclient_utils.py index 3999fab8d6..0957ec1e73 100644 --- a/gclient_utils.py +++ b/gclient_utils.py @@ -312,38 +312,6 @@ def CommandToStr(args): return ' '.join(pipes.quote(arg) for arg in args) -def CheckCallAndFilterAndHeader(args, always=False, header=None, **kwargs): - """Adds 'header' support to CheckCallAndFilter. - - 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.setdefault('stdout', sys.stdout) - if header is None: - # The automatically generated header only prepends newline if always is - # false: always is usually set to false if there's an external progress - # display, and it's better not to clobber it in that case. - header = "%s________ running '%s' in '%s'\n" % ( - '' if always else '\n', - ' '.join(args), kwargs.get('cwd', '.')) - - if always: - stdout.write(header) - else: - filter_fn = kwargs.get('filter_fn') - def filter_msg(line): - if line is None: - stdout.write(header) - elif filter_fn: - filter_fn(line) - kwargs['filter_fn'] = filter_msg - kwargs['call_filter_on_first_line'] = True - # Obviously. - kwargs.setdefault('print_stdout', True) - return CheckCallAndFilter(args, **kwargs) - - class Wrapper(object): """Wraps an object, acting as a transparent proxy for all properties by default. @@ -355,22 +323,6 @@ class Wrapper(object): return getattr(self._wrapped, name) -class WriteToStdout(Wrapper): - """Creates a file object clone to also print to sys.stdout.""" - def __init__(self, wrapped): - super(WriteToStdout, self).__init__(wrapped) - if not hasattr(self, 'lock'): - self.lock = threading.Lock() - - def write(self, out, *args, **kwargs): - self._wrapped.write(out, *args, **kwargs) - self.lock.acquire() - try: - sys.stdout.write(out, *args, **kwargs) - finally: - self.lock.release() - - class AutoFlush(Wrapper): """Creates a file object clone to automatically flush after N seconds.""" def __init__(self, wrapped, delay): @@ -536,9 +488,9 @@ class GClientChildren(object): print(' ', zombie.pid, file=sys.stderr) -def CheckCallAndFilter(args, stdout=None, filter_fn=None, - print_stdout=None, call_filter_on_first_line=False, - retry=False, **kwargs): +def CheckCallAndFilter(args, print_stdout=False, filter_fn=None, + show_header=False, always_show_header=False, retry=False, + **kwargs): """Runs a command and calls back a filter function if needed. Accepts all subprocess2.Popen() parameters plus: @@ -546,28 +498,68 @@ def CheckCallAndFilter(args, stdout=None, filter_fn=None, filter_fn: A function taking a single string argument called with each line of the subprocess2's output. Each line has the trailing newline character trimmed. - stdout: Can be any bufferable output. + show_header: Whether to display a header before the command output. + always_show_header: Show header even when the command produced no output. retry: If the process exits non-zero, sleep for a brief interval and try again, up to RETRY_MAX times. stderr is always redirected to stdout. + + Returns the output of the command as a binary string. """ - assert print_stdout or filter_fn - stdout = stdout or sys.stdout - output = io.BytesIO() - filter_fn = filter_fn or (lambda x: None) + def show_header_if_necessary(needs_header, attempt): + """Show the header at most once.""" + if not needs_header[0]: + return + + needs_header[0] = False + # Automatically generated header. We only prepend a newline if + # always_show_header is false, since it usually indicates there's an + # external progress display, and it's better not to clobber it in that case. + header = '' if always_show_header else '\n' + header += '________ running \'%s\' in \'%s\'' % ( + ' '.join(args), kwargs.get('cwd', '.')) + if attempt: + header += ' attempt %s / %s' % (attempt + 1, RETRY_MAX + 1) + header += '\n' + + if print_stdout: + sys.stdout.write(header) + if filter_fn: + filter_fn(header) + + def filter_line(command_output, line_start): + """Extract the last line from command output and filter it.""" + if not filter_fn or line_start is None: + return + command_output.seek(line_start) + filter_fn(command_output.read().decode('utf-8')) + + # Initialize stdout writer if needed. On Python 3, sys.stdout does not accept + # byte inputs and sys.stdout.buffer must be used instead. + if print_stdout: + sys.stdout.flush() + stdout_write = getattr(sys.stdout, 'buffer', sys.stdout).write + else: + stdout_write = lambda _: None sleep_interval = RETRY_INITIAL_SLEEP run_cwd = kwargs.get('cwd', os.getcwd()) - for _ in range(RETRY_MAX + 1): + for attempt in range(RETRY_MAX + 1): kid = subprocess2.Popen( args, bufsize=0, stdout=subprocess2.PIPE, stderr=subprocess2.STDOUT, **kwargs) GClientChildren.add(kid) - # Do a flush of stdout before we begin reading from the subprocess2's stdout - stdout.flush() + # Store the output of the command regardless of the value of print_stdout or + # filter_fn. + command_output = io.BytesIO() + + # Passed as a list for "by ref" semantics. + needs_header = [show_header] + if always_show_header: + show_header_if_necessary(needs_header, attempt) # Also, we need to forward stdout to prevent weird re-ordering of output. # This has to be done on a per byte basis to make sure it is not buffered: @@ -575,25 +567,29 @@ def CheckCallAndFilter(args, stdout=None, filter_fn=None, # input, no end-of-line character is output after the prompt and it would # not show up. try: - in_byte = kid.stdout.read(1) - if in_byte: - if call_filter_on_first_line: - filter_fn(None) - in_line = b'' - while in_byte: - output.write(in_byte) - if print_stdout: - stdout.write(in_byte) - if in_byte not in ['\r', '\n']: - in_line += in_byte - else: - filter_fn(in_line) - in_line = b'' - 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) + line_start = None + while True: + in_byte = kid.stdout.read(1) + is_newline = in_byte in (b'\n', b'\r') + if not in_byte: + break + + show_header_if_necessary(needs_header, attempt) + + if is_newline: + filter_line(command_output, line_start) + line_start = None + elif line_start is None: + line_start = command_output.tell() + + stdout_write(in_byte) + command_output.write(in_byte) + + # Flush the rest of buffered output. + sys.stdout.flush() + if line_start is not None: + filter_line(command_output, line_start) + rv = kid.wait() kid.stdout.close() @@ -606,13 +602,16 @@ def CheckCallAndFilter(args, stdout=None, filter_fn=None, raise if rv == 0: - return output.getvalue() + return command_output.getvalue() + if not retry: break + print("WARNING: subprocess '%s' in %s failed; will retry after a short " 'nap...' % (' '.join('"%s"' % x for x in args), run_cwd)) time.sleep(sleep_interval) sleep_interval *= 2 + raise subprocess2.CalledProcessError( rv, args, kwargs.get('cwd', None), None, None) @@ -635,6 +634,7 @@ class GitFilter(object): The line will be skipped if predicate(line) returns False. out_fh: File handle to write output to. """ + self.first_line = True self.last_time = 0 self.time_throttle = time_throttle self.predicate = predicate @@ -656,7 +656,9 @@ class GitFilter(object): elif now - self.last_time < self.time_throttle: return self.last_time = now - self.out_fh.write('[%s] ' % Elapsed()) + if not self.first_line: + self.out_fh.write('[%s] ' % Elapsed()) + self.first_line = False print(line, file=self.out_fh) diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index 1e99f52f50..bfb7be748f 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -60,8 +60,6 @@ class BaseTestCase(GCBaseTestCase, SuperMoxTestBase): def setUp(self): SuperMoxTestBase.setUp(self) self.mox.StubOutWithMock(gclient_scm.gclient_utils, 'CheckCallAndFilter') - self.mox.StubOutWithMock(gclient_scm.gclient_utils, - 'CheckCallAndFilterAndHeader') self.mox.StubOutWithMock(gclient_scm.gclient_utils, 'FileRead') self.mox.StubOutWithMock(gclient_scm.gclient_utils, 'FileWrite') self.mox.StubOutWithMock(gclient_scm.gclient_utils, 'rmtree') @@ -347,7 +345,7 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase): self.assertEquals(file_list, [file_path]) self.checkstdout( ('\n________ running \'git -c core.quotePath=false diff --name-status ' - '069c602044c5388d2d15c3f875b057c852003458\' in \'%s\'\nM\ta\n') % + '069c602044c5388d2d15c3f875b057c852003458\' in \'%s\'\n\nM\ta\n') % join(self.root_dir, '.')) def testStatus2New(self): @@ -367,8 +365,8 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase): self.assertEquals(sorted(file_list), expected_file_list) self.checkstdout( ('\n________ running \'git -c core.quotePath=false diff --name-status ' - '069c602044c5388d2d15c3f875b057c852003458\' in \'%s\'\nM\ta\nM\tb\n') % - join(self.root_dir, '.')) + '069c602044c5388d2d15c3f875b057c852003458\' in \'%s\'\n\nM\ta\nM\tb\n') + % join(self.root_dir, '.')) def testUpdateUpdate(self): if not self.enabled: @@ -1024,8 +1022,7 @@ class CipdWrapperTestCase(BaseTestCase): self.mox.StubOutWithMock(tempfile, 'mkdtemp') tempfile.mkdtemp().AndReturn(self._workdir) - gclient_scm.gclient_utils.CheckCallAndFilter( - cmd, filter_fn=mox.IgnoreArg(), print_stdout=False) + gclient_scm.gclient_utils.CheckCallAndFilter(cmd) gclient_scm.gclient_utils.rmtree(self._workdir) self.mox.ReplayAll() diff --git a/tests/gclient_utils_test.py b/tests/gclient_utils_test.py index 8ffd1d3f66..f5b57d8c43 100755 --- a/tests/gclient_utils_test.py +++ b/tests/gclient_utils_test.py @@ -4,8 +4,10 @@ # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. +from __future__ import unicode_literals + +import io import os -import StringIO import sys sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) @@ -27,19 +29,19 @@ class GclientUtilBase(SuperMoxTestBase): class CheckCallAndFilterTestCase(GclientUtilBase): class ProcessIdMock(object): - def __init__(self, test_string): - self.stdout = StringIO.StringIO(test_string) + def __init__(self, test_string, return_code=0): + self.stdout = io.BytesIO(test_string.encode('utf-8')) self.pid = 9284 - # pylint: disable=no-self-use + self.return_code = return_code + def wait(self): - return 0 + return self.return_code - def _inner(self, args, test_string): + def testCheckCallAndFilter(self): cwd = 'bleh' - gclient_utils.sys.stdout.write( - '________ running \'boo foo bar\' in \'bleh\'\n') - for i in test_string: - gclient_utils.sys.stdout.write(i) + args = ['boo', 'foo', 'bar'] + test_string = 'ahah\naccb\nallo\naddb\n✔' + # pylint: disable=no-member subprocess2.Popen( args, @@ -50,30 +52,70 @@ class CheckCallAndFilterTestCase(GclientUtilBase): os.getcwd() self.mox.ReplayAll() - compiled_pattern = gclient_utils.re.compile(r'a(.*)b') line_list = [] - 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.CheckCallAndFilterAndHeader( - args, cwd=cwd, always=True, filter_fn=FilterLines) - self.assertEquals(line_list, ['ahah', 'accb', 'allo', 'addb', '✔']) - self.assertEquals(capture_list, ['cc', 'dd']) - def testCheckCallAndFilter(self): + result = gclient_utils.CheckCallAndFilter( + args, cwd=cwd, show_header=True, always_show_header=True, + filter_fn=line_list.append) + + self.assertEqual(result, test_string.encode('utf-8')) + self.assertEqual(line_list, [ + '________ running \'boo foo bar\' in \'bleh\'\n', + 'ahah', + 'accb', + 'allo', + 'addb', + '✔']) + + def testCheckCallAndFilter_RetryOnce(self): + cwd = 'bleh' args = ['boo', 'foo', 'bar'] - test_string = 'ahah\naccb\nallo\naddb\n✔\n' - self._inner(args, test_string) + test_string = 'ahah\naccb\nallo\naddb\n✔' + + # pylint: disable=no-member + subprocess2.Popen( + args, + cwd=cwd, + stdout=subprocess2.PIPE, + stderr=subprocess2.STDOUT, + bufsize=0).AndReturn(self.ProcessIdMock(test_string, 1)) + + os.getcwd() + + # pylint: disable=no-member + subprocess2.Popen( + args, + cwd=cwd, + stdout=subprocess2.PIPE, + stderr=subprocess2.STDOUT, + bufsize=0).AndReturn(self.ProcessIdMock(test_string, 0)) + self.mox.ReplayAll() + + line_list = [] + result = gclient_utils.CheckCallAndFilter( + args, cwd=cwd, show_header=True, always_show_header=True, + filter_fn=line_list.append, retry=True) + + self.assertEqual(result, test_string.encode('utf-8')) + + self.assertEqual(line_list, [ + '________ running \'boo foo bar\' in \'bleh\'\n', + 'ahah', + 'accb', + 'allo', + 'addb', + '✔', + '________ running \'boo foo bar\' in \'bleh\' attempt 2 / 4\n', + 'ahah', + 'accb', + 'allo', + 'addb', + '✔', + ]) + self.checkstdout( - '________ running \'boo foo bar\' in \'bleh\'\n' - 'ahah\naccb\nallo\naddb\n✔\n' - '________ running \'boo foo bar\' in \'bleh\'\n' - 'ahah\naccb\nallo\naddb\n✔' - '\n') + 'WARNING: subprocess \'"boo" "foo" "bar"\' in bleh failed; will retry ' + 'after a short nap...\n') class SplitUrlRevisionTestCase(GclientUtilBase): @@ -81,60 +123,60 @@ class SplitUrlRevisionTestCase(GclientUtilBase): url = "ssh://test@example.com/test.git" rev = "ac345e52dc" out_url, out_rev = gclient_utils.SplitUrlRevision(url) - self.assertEquals(out_rev, None) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, None) + self.assertEqual(out_url, url) out_url, out_rev = gclient_utils.SplitUrlRevision("%s@%s" % (url, rev)) - self.assertEquals(out_rev, rev) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, rev) + self.assertEqual(out_url, url) url = "ssh://example.com/test.git" out_url, out_rev = gclient_utils.SplitUrlRevision(url) - self.assertEquals(out_rev, None) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, None) + self.assertEqual(out_url, url) out_url, out_rev = gclient_utils.SplitUrlRevision("%s@%s" % (url, rev)) - self.assertEquals(out_rev, rev) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, rev) + self.assertEqual(out_url, url) url = "ssh://example.com/git/test.git" out_url, out_rev = gclient_utils.SplitUrlRevision(url) - self.assertEquals(out_rev, None) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, None) + self.assertEqual(out_url, url) out_url, out_rev = gclient_utils.SplitUrlRevision("%s@%s" % (url, rev)) - self.assertEquals(out_rev, rev) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, rev) + self.assertEqual(out_url, url) rev = "test-stable" out_url, out_rev = gclient_utils.SplitUrlRevision("%s@%s" % (url, rev)) - self.assertEquals(out_rev, rev) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, rev) + self.assertEqual(out_url, url) url = "ssh://user-name@example.com/~/test.git" out_url, out_rev = gclient_utils.SplitUrlRevision(url) - self.assertEquals(out_rev, None) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, None) + self.assertEqual(out_url, url) out_url, out_rev = gclient_utils.SplitUrlRevision("%s@%s" % (url, rev)) - self.assertEquals(out_rev, rev) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, rev) + self.assertEqual(out_url, url) url = "ssh://user-name@example.com/~username/test.git" out_url, out_rev = gclient_utils.SplitUrlRevision(url) - self.assertEquals(out_rev, None) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, None) + self.assertEqual(out_url, url) out_url, out_rev = gclient_utils.SplitUrlRevision("%s@%s" % (url, rev)) - self.assertEquals(out_rev, rev) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, rev) + self.assertEqual(out_url, url) url = "git@github.com:dart-lang/spark.git" out_url, out_rev = gclient_utils.SplitUrlRevision(url) - self.assertEquals(out_rev, None) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, None) + self.assertEqual(out_url, url) out_url, out_rev = gclient_utils.SplitUrlRevision("%s@%s" % (url, rev)) - self.assertEquals(out_rev, rev) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, rev) + self.assertEqual(out_url, url) def testSVNUrl(self): url = "svn://example.com/test" rev = "ac345e52dc" out_url, out_rev = gclient_utils.SplitUrlRevision(url) - self.assertEquals(out_rev, None) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, None) + self.assertEqual(out_url, url) out_url, out_rev = gclient_utils.SplitUrlRevision("%s@%s" % (url, rev)) - self.assertEquals(out_rev, rev) - self.assertEquals(out_url, url) + self.assertEqual(out_rev, rev) + self.assertEqual(out_url, url) class GClientUtilsTest(trial_dir.TestCase): @@ -171,7 +213,7 @@ class GClientUtilsTest(trial_dir.TestCase): ['foo:', 'https://foo:'], ] for content, expected in values: - self.assertEquals( + self.assertEqual( expected, gclient_utils.UpgradeToHttps(content)) def testParseCodereviewSettingsContent(self): @@ -191,7 +233,7 @@ class GClientUtilsTest(trial_dir.TestCase): ['VIEW_VC:http://r/s', {'VIEW_VC': 'https://r/s'}], ] for content, expected in values: - self.assertEquals( + self.assertEqual( expected, gclient_utils.ParseCodereviewSettingsContent(content)) diff --git a/tests/scm_unittest.py b/tests/scm_unittest.py index 0cdede39a7..42a7f9ddab 100755 --- a/tests/scm_unittest.py +++ b/tests/scm_unittest.py @@ -37,8 +37,6 @@ class BaseTestCase(SuperMoxTestBase): class BaseSCMTestCase(BaseTestCase): def setUp(self): BaseTestCase.setUp(self) - self.mox.StubOutWithMock(scm.gclient_utils, 'CheckCallAndFilter') - self.mox.StubOutWithMock(scm.gclient_utils, 'CheckCallAndFilterAndHeader') self.mox.StubOutWithMock(subprocess2, 'Popen') self.mox.StubOutWithMock(subprocess2, 'communicate')