diff --git a/tools/cr/brockit.py b/tools/cr/brockit.py index af3ec8b3765..6677fc18e4e 100755 --- a/tools/cr/brockit.py +++ b/tools/cr/brockit.py @@ -86,7 +86,7 @@ The following steps will take place: expected to provide separate commits for deleted patches, explaining the reason. 6. Having resolved all conflicts. Restart *🚀Brockit!* with `--continue` and - other similar arguments you may want to keep (e.g. `--vscode`). + other similar arguments you may want to keep. 7. *🚀Brockit!* will pick up from where it stopped, possibly running `npm run update_patches`, staging all patches, and committing the under *Conflict-resolved patches from Chromium [from] to [to].* @@ -218,6 +218,7 @@ from repository import Repository, CHROMIUM_SRC_PATH from terminal import console, terminal import versioning from versioning import Version +from vscode import VsCodeIpcConnection # This file is updated whenever the version number is updated in package.json PINSLIST_TIMESTAMP_FILE = ( @@ -939,8 +940,7 @@ class Upgrade(Versioned): def status_message(self): return "Upgrading Chromium base version" - def apply_patches_3way(self, - launch_vscode: bool = False) -> ApplyPatchesRecord: + def apply_patches_3way(self) -> ApplyPatchesRecord: """Applies patches that have failed using the --3way option to allow for manual conflict resolution. @@ -949,6 +949,9 @@ class Upgrade(Versioned): are waiting for conflict resolution. A list of the patches applied will be produced as well. + + When running brockit in a vscode terminal, this method will open any + files that need attention in the editor session. """ # A dictionary that holds a list for all patch files affected, by # repository. @@ -996,7 +999,7 @@ class Upgrade(Versioned): for patch in patch_list ])) - vscode_args = ['code'] + vscode_files = [] for repo, patches in patch_files.items(): for patch in patches: apply_result = patch.apply() @@ -1046,7 +1049,7 @@ class Upgrade(Versioned): console.log( Padding(f'✘ {patch.source()} [red bold](deleted)', (0, 4))) - vscode_args.append(patch.path) + vscode_files.append(patch.path) elif status.status == 'R': renamed_to = patch.repository.from_brave( ) / status.renamed_to @@ -1055,7 +1058,7 @@ class Upgrade(Versioned): f'✘ {patch.source_from_brave()}\n ' f'([yellow bold]renamed to[/] {renamed_to})', (0, 4))) - vscode_args += [patch.path, renamed_to] + vscode_files += [patch.path, renamed_to] # Printing the commmit message for the grouped changes. console.log( @@ -1071,22 +1074,16 @@ class Upgrade(Versioned): for patch in broken_patches: source = patch.source_from_brave() console.log(Padding(f'✘ {patch.path} ➜ {source}', (0, 4))) - vscode_args += [patch.path, source] + vscode_files += [patch.path, source] if files_with_conflicts: - vscode_args += files_with_conflicts + vscode_files += files_with_conflicts file_list = '\n'.join(f' ✘ {file}' for file in files_with_conflicts) terminal.log_task(f'[bold]Manually resolve conflicts for ' f'{ACTION_NEEDED_DECORATOR}:[/]\n{file_list}') - if launch_vscode and len(vscode_args) > 1: - try: - terminal.run(vscode_args) - except subprocess.CalledProcessError as e: - logging.error( - 'Failed to launch VSCode with the following args: %s\n%s', - ' '.join(vscode_args), e.stderr) + VsCodeIpcConnection().open_file(vscode_files) # The continuation file is updated at the end of the process, in case # the process has to be continued later. @@ -1502,7 +1499,7 @@ class Upgrade(Versioned): # continuation file around. ContinuationFile.clear() - def _start(self, launch_vscode: bool, ack_advisory: bool): + def _start(self, ack_advisory: bool): """Starts the upgrade process. This function is responsible for starting the upgrade process. It will @@ -1513,11 +1510,6 @@ class Upgrade(Versioned): For cases where no conflict resolution is required, the process will will continue, concluding the whole four steps of the upgrade process. - Args: - launch_vscode: - Indicates if the user wants to launch vscode with the patches that - require manual conflict resolution. - Return: Returns True if the process was successful, and False otherwise. """ @@ -1576,8 +1568,7 @@ class Upgrade(Versioned): if (e.returncode != 0 and 'Exiting as not all patches were successful!' in e.stderr.splitlines()[-1]): - apply_record = self.apply_patches_3way( - launch_vscode=launch_vscode) + apply_record = self.apply_patches_3way() if apply_record.requires_conflict_resolution(): # Manual resolution required. raise ActionNeededException( @@ -1602,8 +1593,8 @@ class Upgrade(Versioned): terminal.run_npm_command('chromium_rebase_l10n') self._save_rebased_l10n() - def execute(self, no_conflict_continuation: bool, launch_vscode: bool, - with_github: bool, ack_advisory: bool): + def execute(self, no_conflict_continuation: bool, with_github: bool, + ack_advisory: bool): """Executes the upgrade process. Keep in this function all code that is common to both start and continue. @@ -1612,9 +1603,6 @@ class Upgrade(Versioned): no_conflict_continuation: Indicates that a continuation does not produce a conflict-resolved change. - launch_vscode: - Indicates the user wants to launch vscode with the patches that - require manual conflict resolution. with_github: Indicates the user wants to create or update the github issue for the upgrade. @@ -1665,7 +1653,7 @@ class Upgrade(Versioned): self._continue(no_conflict_continuation=no_conflict_continuation) else: - self._start(launch_vscode=launch_vscode, ack_advisory=ack_advisory) + self._start(ack_advisory=ack_advisory) if with_github: GitHubIssue(base_version=self.base_version, @@ -2271,11 +2259,6 @@ def main(): action='store_true', help='Creates or updates the github for this branch.', dest='with_github') - lift_parser.add_argument( - '--vscode', - action='store_true', - help= - 'Launches vscode for manual conflict resolution and similar issues.') lift_parser.add_argument( '--no-conflict-change', action='store_true', @@ -2397,7 +2380,6 @@ def main(): args.is_continuation) upgrade.run(no_conflict_continuation=args.no_conflict, - launch_vscode=args.vscode, with_github=args.with_github, ack_advisory=args.ack_advisory) if args.command == 'rebase': diff --git a/tools/cr/brockit_test.py b/tools/cr/brockit_test.py index 03dd2f9de40..3763e5320c9 100755 --- a/tools/cr/brockit_test.py +++ b/tools/cr/brockit_test.py @@ -4,10 +4,12 @@ # License, v. 2.0. If a copy of the MPL was not distributed with this file, # You can obtain one at https://mozilla.org/MPL/2.0/. +import os import unittest from pathlib import Path import shutil from datetime import datetime +from unittest.mock import MagicMock, patch import brockit from brockit import ApplyPatchesRecord diff --git a/tools/cr/vscode.py b/tools/cr/vscode.py new file mode 100644 index 00000000000..f65e4d2db55 --- /dev/null +++ b/tools/cr/vscode.py @@ -0,0 +1,108 @@ +# Copyright (c) 2025 The Brave Authors. All rights reserved. +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at https://mozilla.org/MPL/2.0/. + +import http.client +import io +import json +import logging +import os +from pathlib import Path +import platform +import socket + + +class _VsCodeIpcConnectionBase(http.client.HTTPConnection): + """Base for platform-specific VS Code IPC HTTP connections.""" + + def __init__(self, env_var: str): + super().__init__('vscode') + self._socket_path = os.environ.get(env_var, '') + + def open_file(self, files: list) -> None: + """Opens files in the VS Code window that owns this terminal. + + Communicates directly with the IPC socket rather than spawning + `code`, so the request is routed to the specific window whose + extension host created the socket — not whichever window happens + to be in the foreground. + """ + if not self._socket_path or not files: + return + file_uris = [Path(str(f)).resolve().as_uri() for f in files] + body = json.dumps({ + 'type': 'open', + 'fileURIs': file_uris, + 'forceReuseWindow': True, + }).encode() + logging.debug('VS Code IPC request: socket=%s, body=%s', + self._socket_path, body.decode()) + try: + self.request('POST', '/', body, + {'Content-Type': 'application/json'}) + resp = self.getresponse() + logging.debug('VS Code IPC response: %d %s\n', resp.status, + resp.reason) + except Exception as e: + logging.warning('Could not open files in VS Code window: %s', e) + + +class _PosixVsCodeIpcConnection(_VsCodeIpcConnectionBase): + """VS Code IPC connection via Unix domain socket (POSIX).""" + + def __init__(self): + """Reads the IPC socket path from VSCODE_IPC_HOOK_CLI. + + VS Code sets this variable in every integrated terminal it spawns, + pointing to the Unix socket owned by that window's extension host. + """ + super().__init__('VSCODE_IPC_HOOK_CLI') + + def connect(self) -> None: + """Overrides HTTPConnection.connect to use a Unix domain socket.""" + self.sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + self.sock.connect(self._socket_path) + + +class _NamedPipeSocket: + """Socket-like wrapper around a Windows named pipe for HTTPConnection.""" + + def __init__(self, path: str): + """Opens the named pipe at path for binary read/write.""" + self._pipe = open(path, 'r+b', buffering=0) + + def sendall(self, data: bytes) -> None: + """Writes data to the pipe (called by HTTPConnection).""" + self._pipe.write(data) + + def makefile(self, _mode: str, _bufsize: int = -1) -> io.BufferedReader: + """Returns a BufferedReader over the pipe for HTTPResponse to use.""" + return io.BufferedReader(self._pipe) + + def close(self) -> None: + """Closes the underlying pipe handle.""" + self._pipe.close() + + def settimeout(self, _timeout) -> None: + """No-op: named pipes do not support socket-style timeouts.""" + + +class _WinVsCodeIpcConnection(_VsCodeIpcConnectionBase): + """VS Code IPC connection via Windows named pipe.""" + + def __init__(self): + """Reads the IPC socket path from VSCODE_GIT_IPC_HANDLE. + + VS Code sets this variable in every integrated terminal it spawns, + pointing to the named pipe owned by that window's extension host. + """ + super().__init__('VSCODE_GIT_IPC_HANDLE') + + def connect(self) -> None: + """Overrides HTTPConnection.connect to use a Windows named pipe.""" + self.sock = _NamedPipeSocket(self._socket_path) + + +VsCodeIpcConnection = (_WinVsCodeIpcConnection if platform.system() + == 'Windows' else _PosixVsCodeIpcConnection) diff --git a/tools/cr/vscode_test.py b/tools/cr/vscode_test.py new file mode 100755 index 00000000000..b7ff22963e5 --- /dev/null +++ b/tools/cr/vscode_test.py @@ -0,0 +1,241 @@ +#!/usr/bin/env vpython3 +# Copyright (c) 2025 The Brave Authors. All rights reserved. +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at https://mozilla.org/MPL/2.0/. + +import io +import json +import os +import platform +import socket +import tempfile +import unittest +from unittest.mock import MagicMock, patch + +from vscode import (VsCodeIpcConnection, _NamedPipeSocket, + _PosixVsCodeIpcConnection, _WinVsCodeIpcConnection) + + +class PosixVsCodeIpcConnectionTest(unittest.TestCase): + """Tests for _PosixVsCodeIpcConnection.""" + + def setUp(self): + """Creates a temporary directory and derives a socket path from it.""" + self._tmp_dir = tempfile.TemporaryDirectory() + self.sock_path = os.path.join(self._tmp_dir.name, 'vscode.sock') + self.addCleanup(self._tmp_dir.cleanup) + + def test_init_reads_socket_path_from_env(self): + """_socket_path is set from VSCODE_IPC_HOOK_CLI.""" + with patch.dict(os.environ, {'VSCODE_IPC_HOOK_CLI': self.sock_path}): + conn = _PosixVsCodeIpcConnection() + self.assertEqual(conn._socket_path, self.sock_path) + + def test_init_empty_socket_path_when_env_absent(self): + """_socket_path is empty when VSCODE_IPC_HOOK_CLI is not set.""" + with patch.dict(os.environ, {}): + os.environ.pop('VSCODE_IPC_HOOK_CLI', None) + conn = _PosixVsCodeIpcConnection() + self.assertEqual(conn._socket_path, '') + + def test_connect_creates_unix_socket(self): + """connect() opens an AF_UNIX socket and connects to _socket_path.""" + with patch.dict(os.environ, {'VSCODE_IPC_HOOK_CLI': self.sock_path}): + conn = _PosixVsCodeIpcConnection() + with patch('vscode.socket.socket') as mock_socket_cls: + mock_sock = MagicMock() + mock_socket_cls.return_value = mock_sock + conn.connect() + mock_socket_cls.assert_called_once_with( + socket.AF_UNIX, socket.SOCK_STREAM) + mock_sock.connect.assert_called_once_with(self.sock_path) + + def test_open_file_skips_when_no_socket_path(self): + """open_file does nothing when VSCODE_IPC_HOOK_CLI is not set.""" + with patch.dict(os.environ, {}): + os.environ.pop('VSCODE_IPC_HOOK_CLI', None) + conn = _PosixVsCodeIpcConnection() + with patch.object(conn, 'request') as mock_request: + conn.open_file(['/some/file.cc']) + mock_request.assert_not_called() + + def test_open_file_skips_when_files_empty(self): + """open_file does nothing when the files list is empty.""" + with patch.dict(os.environ, {'VSCODE_IPC_HOOK_CLI': self.sock_path}): + conn = _PosixVsCodeIpcConnection() + with patch.object(conn, 'request') as mock_request: + conn.open_file([]) + mock_request.assert_not_called() + + def test_open_file_sends_correct_request(self): + """open_file sends POST / with the correct JSON body and headers.""" + with patch.dict(os.environ, {'VSCODE_IPC_HOOK_CLI': self.sock_path}): + conn = _PosixVsCodeIpcConnection() + mock_response = MagicMock() + mock_response.status = 200 + mock_response.reason = 'OK' + mock_response.read.return_value = b'' + with patch.object(conn, 'request') as mock_request, \ + patch.object(conn, 'getresponse', return_value=mock_response): + conn.open_file(['/path/to/file.cc']) + mock_request.assert_called_once() + method, path, body, headers = mock_request.call_args[0] + self.assertEqual(method, 'POST') + self.assertEqual(path, '/') + payload = json.loads(body) + self.assertEqual(payload['type'], 'open') + self.assertEqual(payload['fileURIs'], + ['file:///path/to/file.cc']) + self.assertTrue(payload['forceReuseWindow']) + self.assertEqual(headers['Content-Type'], 'application/json') + + def test_open_file_logs_error_on_failure(self): + """open_file logs a warning and does not raise on connection failure.""" + with patch.dict(os.environ, {'VSCODE_IPC_HOOK_CLI': self.sock_path}): + conn = _PosixVsCodeIpcConnection() + with patch.object(conn, + 'request', + side_effect=ConnectionRefusedError('refused')): + with self.assertLogs(level='WARNING') as captured: + conn.open_file(['/path/to/file.cc']) + self.assertTrue( + any('Could not open files in VS Code window' in line + for line in captured.output)) + + +class _ReadableRawIO(io.RawIOBase): + """Minimal readable RawIOBase for use in _NamedPipeSocket tests.""" + + def readable(self): + return True + + def readinto(self, _b): + return 0 + + +class NamedPipeSocketTest(unittest.TestCase): + """Tests for _NamedPipeSocket.""" + + def test_sendall_writes_to_pipe(self): + """sendall() delegates to the underlying pipe's write().""" + mock_pipe = MagicMock() + with patch('builtins.open', return_value=mock_pipe): + sock = _NamedPipeSocket(r'\\.\pipe\test') + sock.sendall(b'hello') + mock_pipe.write.assert_called_once_with(b'hello') + + def test_makefile_returns_buffered_reader_wrapping_pipe(self): + """makefile() wraps the pipe in a BufferedReader.""" + with patch('builtins.open', return_value=_ReadableRawIO()): + sock = _NamedPipeSocket(r'\\.\pipe\test') + result = sock.makefile('rb') + self.assertIsInstance(result, io.BufferedReader) + + def test_close_closes_pipe(self): + """close() delegates to the underlying pipe's close().""" + mock_pipe = MagicMock() + with patch('builtins.open', return_value=mock_pipe): + sock = _NamedPipeSocket(r'\\.\pipe\test') + sock.close() + mock_pipe.close.assert_called_once() + + def test_settimeout_is_noop(self): + """settimeout() does not raise.""" + mock_pipe = MagicMock() + with patch('builtins.open', return_value=mock_pipe): + sock = _NamedPipeSocket(r'\\.\pipe\test') + sock.settimeout(30) + + +class WinVsCodeIpcConnectionTest(unittest.TestCase): + """Tests for _WinVsCodeIpcConnection.""" + + PIPE_PATH = r'\\.\pipe\vscode-ipc-abc123' + + def test_init_reads_socket_path_from_env(self): + """_socket_path is set from VSCODE_GIT_IPC_HANDLE.""" + with patch.dict(os.environ, {'VSCODE_GIT_IPC_HANDLE': self.PIPE_PATH}): + conn = _WinVsCodeIpcConnection() + self.assertEqual(conn._socket_path, self.PIPE_PATH) + + def test_init_empty_socket_path_when_env_absent(self): + """_socket_path is empty when VSCODE_GIT_IPC_HANDLE is not set.""" + with patch.dict(os.environ, {}): + os.environ.pop('VSCODE_GIT_IPC_HANDLE', None) + conn = _WinVsCodeIpcConnection() + self.assertEqual(conn._socket_path, '') + + def test_connect_creates_named_pipe_socket(self): + """connect() sets sock to a _NamedPipeSocket.""" + with patch.dict(os.environ, {'VSCODE_GIT_IPC_HANDLE': self.PIPE_PATH}): + conn = _WinVsCodeIpcConnection() + with patch('builtins.open', return_value=MagicMock()): + conn.connect() + self.assertIsInstance(conn.sock, _NamedPipeSocket) + + def test_open_file_skips_when_no_socket_path(self): + """open_file does nothing when VSCODE_GIT_IPC_HANDLE is not set.""" + with patch.dict(os.environ, {}): + os.environ.pop('VSCODE_GIT_IPC_HANDLE', None) + conn = _WinVsCodeIpcConnection() + with patch.object(conn, 'request') as mock_request: + conn.open_file(['/some/file.cc']) + mock_request.assert_not_called() + + def test_open_file_skips_when_files_empty(self): + """open_file does nothing when the files list is empty.""" + with patch.dict(os.environ, {'VSCODE_GIT_IPC_HANDLE': self.PIPE_PATH}): + conn = _WinVsCodeIpcConnection() + with patch.object(conn, 'request') as mock_request: + conn.open_file([]) + mock_request.assert_not_called() + + def test_open_file_sends_correct_request(self): + """open_file sends POST / with the correct JSON body and headers.""" + with patch.dict(os.environ, {'VSCODE_GIT_IPC_HANDLE': self.PIPE_PATH}): + conn = _WinVsCodeIpcConnection() + mock_response = MagicMock() + mock_response.status = 200 + mock_response.reason = 'OK' + mock_response.read.return_value = b'' + with patch.object(conn, 'request') as mock_request, \ + patch.object(conn, 'getresponse', return_value=mock_response): + conn.open_file(['/path/to/file.cc']) + mock_request.assert_called_once() + method, path, body, headers = mock_request.call_args[0] + self.assertEqual(method, 'POST') + self.assertEqual(path, '/') + payload = json.loads(body) + self.assertEqual(payload['type'], 'open') + self.assertEqual(payload['fileURIs'], + ['file:///path/to/file.cc']) + self.assertTrue(payload['forceReuseWindow']) + self.assertEqual(headers['Content-Type'], 'application/json') + + def test_open_file_logs_error_on_failure(self): + """open_file logs a warning and does not raise on connection failure.""" + with patch.dict(os.environ, {'VSCODE_GIT_IPC_HANDLE': self.PIPE_PATH}): + conn = _WinVsCodeIpcConnection() + with patch.object(conn, + 'request', + side_effect=ConnectionRefusedError('refused')): + with self.assertLogs(level='WARNING') as captured: + conn.open_file(['/path/to/file.cc']) + self.assertTrue( + any('Could not open files in VS Code window' in line + for line in captured.output)) + + +class VsCodeIpcConnectionDispatchTest(unittest.TestCase): + """Plastform-specific VsCodeIpcConnection test.""" + + def test_alias_matches_platform(self): + if platform.system() == 'Windows': + self.assertIs(VsCodeIpcConnection, _WinVsCodeIpcConnection) + else: + self.assertIs(VsCodeIpcConnection, _PosixVsCodeIpcConnection) + + +if __name__ == '__main__': + unittest.main()