From d410c664e6e371f8b2ba32be75de2c4adfea3fb9 Mon Sep 17 00:00:00 2001 From: Ben Pastene Date: Wed, 26 Aug 2020 17:07:03 +0000 Subject: [PATCH] gclient: Preserve ANSI color codes when calling hooks. If a hook prints error/warning output that's color-coded, gclient will cause the coloring to be disabled since those hooks aren't called directly from a terminal. By emulating a terminal when launching subprocs from gclient, we can convince them to keep the color escape codes. LED builds with both //third_party/depot_tools rolled to this CL, as well as depot_tools in the recipe bundle rolled to this CL: linux: https://ci.chromium.org/swarming/task/4e40237985888310 mac: https://ci.chromium.org/swarming/task/4e4023ea0c829710 win: https://ci.chromium.org/swarming/task/4e4024612e03dc10 Bug: 1034063 Change-Id: I4150f66ef215ece06f4c32482dcd4ded14eb1bfe Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2368435 Reviewed-by: Dirk Pranke Commit-Queue: Ben Pastene --- gclient_utils.py | 24 +++++++++++++++++++++--- tests/gclient_scm_test.py | 4 ++-- tests/gclient_utils_test.py | 27 +++++++++++++++++++-------- 3 files changed, 42 insertions(+), 13 deletions(-) diff --git a/gclient_utils.py b/gclient_utils.py index c62d58985..dcf52224e 100644 --- a/gclient_utils.py +++ b/gclient_utils.py @@ -10,6 +10,7 @@ import codecs import collections import contextlib import datetime +import errno import functools import io import logging @@ -585,9 +586,19 @@ def CheckCallAndFilter(args, print_stdout=False, filter_fn=None, sleep_interval = RETRY_INITIAL_SLEEP run_cwd = kwargs.get('cwd', os.getcwd()) for attempt in range(RETRY_MAX + 1): + # If our stdout is a terminal, then pass in a psuedo-tty pipe to our + # subprocess when filtering its output. This makes the subproc believe + # it was launched from a terminal, which will preserve ANSI color codes. + if sys.stdout.isatty(): + pipe_reader, pipe_writer = os.openpty() + else: + pipe_reader, pipe_writer = os.pipe() + kid = subprocess2.Popen( - args, bufsize=0, stdout=subprocess2.PIPE, stderr=subprocess2.STDOUT, + args, bufsize=0, stdout=pipe_writer, stderr=subprocess2.STDOUT, **kwargs) + # Close the write end of the pipe once we hand it off to the child proc. + os.close(pipe_writer) GClientChildren.add(kid) @@ -608,7 +619,14 @@ def CheckCallAndFilter(args, print_stdout=False, filter_fn=None, try: line_start = None while True: - in_byte = kid.stdout.read(1) + try: + in_byte = os.read(pipe_reader, 1) + except (IOError, OSError) as e: + if e.errno == errno.EIO: + # An errno.EIO means EOF? + in_byte = None + else: + raise e is_newline = in_byte in (b'\n', b'\r') if not in_byte: break @@ -629,8 +647,8 @@ def CheckCallAndFilter(args, print_stdout=False, filter_fn=None, if line_start is not None: filter_line(command_output, line_start) + os.close(pipe_reader) rv = kid.wait() - kid.stdout.close() # Don't put this in a 'finally,' since the child may still run if we get # an exception. diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index fae7a08c2..d5fafed9d 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -1009,7 +1009,7 @@ class BranchHeadsTest(fake_repos.FakeReposTestBase): self.options = BaseGitWrapperTestCase.OptionsObject() self.url = self.git_base + 'repo_1' self.mirror = None - mock.patch('sys.stdout').start() + mock.patch('sys.stdout', StringIO()).start() self.addCleanup(mock.patch.stopall) def setUpMirror(self): @@ -1122,7 +1122,7 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): self.options = BaseGitWrapperTestCase.OptionsObject() self.url = self.git_base + 'repo_1' self.mirror = None - mock.patch('sys.stdout').start() + mock.patch('sys.stdout', StringIO()).start() self.addCleanup(mock.patch.stopall) def setUpMirror(self): diff --git a/tests/gclient_utils_test.py b/tests/gclient_utils_test.py index 740f095b7..b6e88589c 100755 --- a/tests/gclient_utils_test.py +++ b/tests/gclient_utils_test.py @@ -32,23 +32,31 @@ import subprocess2 class CheckCallAndFilterTestCase(unittest.TestCase): class ProcessIdMock(object): def __init__(self, test_string, return_code=0): - self.stdout = io.BytesIO(test_string.encode('utf-8')) + self.stdout = test_string.encode('utf-8') self.pid = 9284 self.return_code = return_code def wait(self): return self.return_code + def PopenMock(self, *args, **kwargs): + kid = self.kids.pop(0) + stdout = kwargs.get('stdout') + os.write(stdout, kid.stdout) + return kid + def setUp(self): super(CheckCallAndFilterTestCase, self).setUp() self.printfn = io.StringIO() self.stdout = io.BytesIO() + self.kids = [] if sys.version_info.major == 2: mock.patch('sys.stdout', self.stdout).start() mock.patch('__builtin__.print', self.printfn.write).start() else: mock.patch('sys.stdout', mock.Mock()).start() mock.patch('sys.stdout.buffer', self.stdout).start() + mock.patch('sys.stdout.isatty', return_value=False).start() mock.patch('builtins.print', self.printfn.write).start() mock.patch('sys.stdout.flush', lambda: None).start() self.addCleanup(mock.patch.stopall) @@ -59,7 +67,8 @@ class CheckCallAndFilterTestCase(unittest.TestCase): args = ['boo', 'foo', 'bar'] test_string = 'ahah\naccb\nallo\naddb\n✔' - mockPopen.return_value = self.ProcessIdMock(test_string) + self.kids = [self.ProcessIdMock(test_string)] + mockPopen.side_effect = self.PopenMock line_list = [] result = gclient_utils.CheckCallAndFilter( @@ -77,7 +86,7 @@ class CheckCallAndFilterTestCase(unittest.TestCase): self.assertEqual(self.stdout.getvalue(), b'') mockPopen.assert_called_with( - args, cwd=cwd, stdout=subprocess2.PIPE, stderr=subprocess2.STDOUT, + args, cwd=cwd, stdout=mock.ANY, stderr=subprocess2.STDOUT, bufsize=0) @mock.patch('time.sleep') @@ -87,10 +96,11 @@ class CheckCallAndFilterTestCase(unittest.TestCase): args = ['boo', 'foo', 'bar'] test_string = 'ahah\naccb\nallo\naddb\n✔' - mockPopen.side_effect = [ + self.kids = [ self.ProcessIdMock(test_string, 1), - self.ProcessIdMock(test_string, 0), + self.ProcessIdMock(test_string, 0) ] + mockPopen.side_effect = self.PopenMock line_list = [] result = gclient_utils.CheckCallAndFilter( @@ -120,10 +130,10 @@ class CheckCallAndFilterTestCase(unittest.TestCase): mockPopen.mock_calls, [ mock.call( - args, cwd=cwd, stdout=subprocess2.PIPE, + args, cwd=cwd, stdout=mock.ANY, stderr=subprocess2.STDOUT, bufsize=0), mock.call( - args, cwd=cwd, stdout=subprocess2.PIPE, + args, cwd=cwd, stdout=mock.ANY, stderr=subprocess2.STDOUT, bufsize=0), ]) @@ -139,7 +149,8 @@ class CheckCallAndFilterTestCase(unittest.TestCase): args = ['boo', 'foo', 'bar'] test_string = 'ahah\naccb\nallo\naddb\n✔' - mockPopen.return_value = self.ProcessIdMock(test_string) + self.kids = [self.ProcessIdMock(test_string)] + mockPopen.side_effect = self.PopenMock result = gclient_utils.CheckCallAndFilter( args, cwd=cwd, show_header=True, always_show_header=True,