From bf76f3d3ede3671a38ca0b4d805cc82dfef5259b Mon Sep 17 00:00:00 2001 From: Dirk Pranke Date: Thu, 9 Jan 2025 15:47:51 -0800 Subject: [PATCH] Explicitly propagate terminal size to gclient hooks. gclient normally runs hooks in a pseudo terminal which has no sense of the terminal size. If we want hooks to be able to change what they output based on the terminal size, we have to explicitly propagate the terminal size info down through the subprocess environment. Change-Id: I08f7c48ef78ea4eb9f5b791abb2a7e5ef8870050 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6161596 Reviewed-by: Scott Lee Commit-Queue: Dirk Pranke --- gclient_utils.py | 17 +++++++++++++++++ tests/gclient_utils_test.py | 36 +++++++++++++++++++----------------- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/gclient_utils.py b/gclient_utils.py index d9c01ddd8..191a8d86a 100644 --- a/gclient_utils.py +++ b/gclient_utils.py @@ -17,6 +17,7 @@ import platform import queue import re import shlex +import shutil import stat import subprocess import sys @@ -637,6 +638,22 @@ def CheckCallAndFilter(args, else: pipe_reader, pipe_writer = os.pipe() + # subprocess2 will use pseudoterminals (ptys) for the child process, + # and ptys do not support a terminal size directly, so if we want + # a hook to be able to customize what it does based on the terminal + # size, we need to explicitly pass the size information down to + # the subprocess. Setting COLUMNS and LINES explicitly in the + # environment will cause those values to be picked up by + # `shutil.get_terminal_size()` in the subprocess (and of course + # anything that checks for the env vars direcstly as well). + if 'env' not in kwargs: + kwargs['env'] = os.environ.copy() + if 'COLUMNS' not in kwargs['env'] or 'LINES' not in kwargs['env']: + size = shutil.get_terminal_size() + if size.columns and size.lines: + kwargs['env']['COLUMNS'] = str(size.columns) + kwargs['env']['LINES'] = str(size.lines) + kid = subprocess2.Popen(args, bufsize=0, stdout=pipe_writer, diff --git a/tests/gclient_utils_test.py b/tests/gclient_utils_test.py index 978be886f..e4b41a8f1 100755 --- a/tests/gclient_utils_test.py +++ b/tests/gclient_utils_test.py @@ -71,11 +71,15 @@ class CheckCallAndFilterTestCase(unittest.TestCase): ]) self.assertEqual(self.stdout.getvalue(), b'') - mockPopen.assert_called_with(args, - cwd=cwd, - stdout=mock.ANY, - stderr=subprocess2.STDOUT, - bufsize=0) + kall = mockPopen.call_args + self.assertEqual(kall.args, (args, )) + self.assertEqual(kall.kwargs['cwd'], cwd) + self.assertEqual(kall.kwargs['stdout'], mock.ANY) + self.assertEqual(kall.kwargs['stderr'], subprocess2.STDOUT) + self.assertEqual(kall.kwargs['bufsize'], 0) + self.assertIn('env', kall.kwargs) + self.assertIn('COLUMNS', kall.kwargs['env']) + self.assertIn('LINES', kall.kwargs['env']) @mock.patch('time.sleep') @mock.patch('subprocess2.Popen') @@ -117,18 +121,16 @@ class CheckCallAndFilterTestCase(unittest.TestCase): mockTime.assert_called_with(gclient_utils.RETRY_INITIAL_SLEEP) - self.assertEqual(mockPopen.mock_calls, [ - mock.call(args, - cwd=cwd, - stdout=mock.ANY, - stderr=subprocess2.STDOUT, - bufsize=0), - mock.call(args, - cwd=cwd, - stdout=mock.ANY, - stderr=subprocess2.STDOUT, - bufsize=0), - ]) + for i in range(2): + kall = mockPopen.mock_calls[i] + self.assertEqual(kall.args, (args, )) + self.assertEqual(kall.kwargs['cwd'], cwd) + self.assertEqual(kall.kwargs['stdout'], mock.ANY) + self.assertEqual(kall.kwargs['stderr'], subprocess2.STDOUT) + self.assertEqual(kall.kwargs['bufsize'], 0) + self.assertIn('env', kall.kwargs) + self.assertIn('COLUMNS', kall.kwargs['env']) + self.assertIn('LINES', kall.kwargs['env']) self.assertEqual(self.stdout.getvalue(), b'') self.assertEqual(