From eb2866e6541607f63cdc50038379886c77f17506 Mon Sep 17 00:00:00 2001 From: Ben Segall Date: Fri, 20 Jan 2023 20:14:44 +0000 Subject: [PATCH] Create reclient specific ninja wrapper to properly handle reproxy lifecyle This is required as it is impossible to catch a keyboard interrupt in a windows batch file and we dont want zombie reproxy instances running on developer's machines for performance and metric collection reasons. Bug: b/264405266 Change-Id: I00f036c8f14451cdb1b99a5cad1c2af03dd57d57 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4171506 Reviewed-by: Dirk Pranke Auto-Submit: Ben Segall Commit-Queue: Ben Segall --- OWNERS | 3 +- autoninja.py | 52 ++------------- ninja_reclient.py | 91 +++++++++++++++++++++++++ tests/OWNERS | 1 + tests/autoninja_test.py | 58 +++++++++++++--- tests/ninja_reclient_test.py | 124 +++++++++++++++++++++++++++++++++++ 6 files changed, 271 insertions(+), 58 deletions(-) create mode 100755 ninja_reclient.py create mode 100755 tests/ninja_reclient_test.py diff --git a/OWNERS b/OWNERS index 80abb77c92..582c85e6bf 100644 --- a/OWNERS +++ b/OWNERS @@ -11,10 +11,11 @@ per-file ninja*=thakis@chromium.org per-file autoninja*=brucedawson@chromium.org per-file post_build_ninja_summary.py=brucedawson@chromium.org +per-file ninja_reclient.py=file://GOMA_OWNERS per-file presubmit*.py=brucedawson@chromium.org -per-file ninjalog*=tikuta@chromium.org +per-file cipd_manifest*=file://GOMA_OWNERS per-file pylint*=vapier@chromium.org diff --git a/autoninja.py b/autoninja.py index 6418fccafb..6f45af28c8 100755 --- a/autoninja.py +++ b/autoninja.py @@ -68,15 +68,6 @@ def main(args): use_remoteexec = False use_rbe = False - # Currently get reclient binary and config dirs relative to output_dir. If - # they exist and using remoteexec, then automatically call bootstrap to start - # reproxy. This works under the current assumption that the output - # directory is two levels up from chromium/src. - reclient_bin_dir = os.path.join(output_dir, '..', '..', 'buildtools', - 'reclient') - reclient_cfg = os.path.join(output_dir, '..', '..', 'buildtools', - 'reclient_cfgs', 'reproxy.cfg') - # Attempt to auto-detect remote build acceleration. We support gn-based # builds, where we look for args.gn in the build tree, and cmake-based builds # where we look for rules.ninja. @@ -116,29 +107,6 @@ def main(args): use_goma = True break - # If use_remoteexec is set, but the reclient binaries or configs don't - # exist, display an error message and stop. Otherwise, the build will - # attempt to run with rewrapper wrapping actions, but will fail with - # possible non-obvious problems. - # As of August 2022, dev builds with reclient are not supported, so - # indicate that use_goma should be swapped for use_remoteexec. This - # message will be changed when dev builds are fully supported. - if (not offline and use_remoteexec and ( - not os.path.exists(reclient_bin_dir) or not os.path.exists(reclient_cfg)) - ): - print(("Build is configured to use reclient but necessary binaries " - "or config files can't be found. Developer builds with " - "reclient are not yet supported. Try regenerating your " - "build with use_goma in place of use_remoteexec for now."), - file=sys.stderr) - if sys.platform.startswith('win'): - # Set an exit code of 1 in the batch file. - print('cmd "/c exit 1"') - else: - # Set an exit code of 1 by executing 'false' in the bash script. - print('false') - sys.exit(1) - # If GOMA_DISABLED is set to "true", "t", "yes", "y", or "1" # (case-insensitive) then gomacc will use the local compiler instead of doing # a goma compile. This is convenient if you want to briefly disable goma. It @@ -185,6 +153,10 @@ def main(args): # Call ninja.py so that it can find ninja binary installed by DEPS or one in # PATH. ninja_path = os.path.join(SCRIPT_DIR, 'ninja.py') + # If using remoteexec, use ninja_reclient.py which wraps ninja.py with + # starting and stopping reproxy. + if not offline and use_remoteexec: + ninja_path = os.path.join(SCRIPT_DIR, 'ninja_reclient.py') args = prefix_args + [sys.executable, ninja_path] + input_args[1:] num_cores = multiprocessing.cpu_count() @@ -234,22 +206,6 @@ def main(args): if os.environ.get('NINJA_SUMMARIZE_BUILD', '0') == '1': args += ['-d', 'stats'] - # If using remoteexec and the necessary environment variables are set, - # also start reproxy (via bootstrap) before running ninja. - if (not offline and use_remoteexec and os.path.exists(reclient_bin_dir) - and os.path.exists(reclient_cfg)): - bootstrap = os.path.join(reclient_bin_dir, 'bootstrap') - setup_args = [ - bootstrap, '--cfg=' + reclient_cfg, - '--re_proxy=' + os.path.join(reclient_bin_dir, 'reproxy') - ] - - teardown_args = [bootstrap, '--cfg=' + reclient_cfg, '--shutdown'] - - cmd_sep = '\n' if sys.platform.startswith('win') else '&&' - cmd_always_sep = '\n' if sys.platform.startswith('win') else '; ' - args = setup_args + [cmd_sep] + args + [cmd_always_sep] + teardown_args - if offline and not sys.platform.startswith('win'): # Tell goma or reclient to do local compiles. On Windows these environment # variables are set by the wrapper batch file. diff --git a/ninja_reclient.py b/ninja_reclient.py new file mode 100755 index 0000000000..2a62888b18 --- /dev/null +++ b/ninja_reclient.py @@ -0,0 +1,91 @@ +#!/usr/bin/env python3 +# Copyright 2023 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. +"""This script is a wrapper around the ninja.py script that also +handles the client lifecycle safely. It will automatically start +reproxy before running ninja and stop reproxy when ninja stops +for any reason eg. build completes, keyboard interupt etc.""" + +import os +import subprocess +import sys + +import ninja +import gclient_paths + + +def find_reclient_bin_dir(): + tools_path = gclient_paths.GetBuildtoolsPath() + if not tools_path: + return None + + reclient_bin_dir = os.path.join(tools_path, 'reclient') + if os.path.isdir(reclient_bin_dir): + return reclient_bin_dir + return None + + +def find_reclient_cfg(): + tools_path = gclient_paths.GetBuildtoolsPath() + if not tools_path: + return None + + reclient_cfg = os.path.join(tools_path, 'reclient_cfgs', 'reproxy.cfg') + if os.path.isfile(reclient_cfg): + return reclient_cfg + return None + + +def run(cmd_args): + if os.environ.get('NINJA_SUMMARIZE_BUILD') == '1': + print(' '.join(cmd_args)) + return subprocess.call(cmd_args) + + +def start_reproxy(reclient_cfg, reclient_bin_dir): + return run([ + os.path.join(reclient_bin_dir, 'bootstrap'), + '--re_proxy=' + os.path.join(reclient_bin_dir, 'reproxy'), + '--cfg=' + reclient_cfg + ]) + + +def stop_reproxy(reclient_cfg, reclient_bin_dir): + return run([ + os.path.join(reclient_bin_dir, 'bootstrap'), '--shutdown', + '--cfg=' + reclient_cfg + ]) + + +def main(argv): + # If use_remoteexec is set, but the reclient binaries or configs don't + # exist, display an error message and stop. Otherwise, the build will + # attempt to run with rewrapper wrapping actions, but will fail with + # possible non-obvious problems. + # As of January 2023, dev builds with reclient are not supported, so + # indicate that use_goma should be swapped for use_remoteexec. This + # message will be changed when dev builds are fully supported. + reclient_bin_dir = find_reclient_bin_dir() + reclient_cfg = find_reclient_cfg() + if reclient_bin_dir is None or reclient_cfg is None: + print(("Build is configured to use reclient but necessary binaries " + "or config files can't be found. Developer builds with " + "reclient are not yet supported. Try regenerating your " + "build with use_goma in place of use_remoteexec for now."), + file=sys.stderr) + return 1 + reproxy_ret_code = start_reproxy(reclient_cfg, reclient_bin_dir) + if reproxy_ret_code != 0: + return reproxy_ret_code + try: + return ninja.main(argv) + except KeyboardInterrupt: + # Suppress python stack trace if ninja is interrupted + return 1 + finally: + stop_reproxy(reclient_cfg, reclient_bin_dir) + + +if __name__ == '__main__': + sys.exit(main(sys.argv)) diff --git a/tests/OWNERS b/tests/OWNERS index 2dc80dcdab..03a0100ca1 100644 --- a/tests/OWNERS +++ b/tests/OWNERS @@ -1,3 +1,4 @@ per-file autoninja_test.py=brucedawson@chromium.org per-file autoninja_test.py=tikuta@chromium.org per-file ninjalog_uploader_test.py=tikuta@chromium.org +per-file ninja_reclient_test.py=file://GOMA_OWNERS diff --git a/tests/autoninja_test.py b/tests/autoninja_test.py index 55e6b5b178..8b1909c0a7 100755 --- a/tests/autoninja_test.py +++ b/tests/autoninja_test.py @@ -14,26 +14,66 @@ ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, ROOT_DIR) import autoninja +from testing_support import trial_dir -class AutoninjaTest(unittest.TestCase): +def write(filename, content): + """Writes the content of a file and create the directories as needed.""" + filename = os.path.abspath(filename) + dirname = os.path.dirname(filename) + if not os.path.isdir(dirname): + os.makedirs(dirname) + with open(filename, 'w') as f: + f.write(content) + + +class AutoninjaTest(trial_dir.TestCase): + def setUp(self): + super(AutoninjaTest, self).setUp() + self.previous_dir = os.getcwd() + os.chdir(self.root_dir) + + def tearDown(self): + os.chdir(self.previous_dir) + super(AutoninjaTest, self).tearDown() + def test_autoninja(self): autoninja.main([]) def test_autoninja_goma(self): with unittest.mock.patch( - 'os.path.exists', - return_value=True) as mock_exists, unittest.mock.patch( - 'autoninja.open', unittest.mock.mock_open( - read_data='use_goma=true')) as mock_open, unittest.mock.patch( - 'subprocess.call', return_value=0): - args = autoninja.main([]).split() - mock_exists.assert_called() - mock_open.assert_called_once() + 'subprocess.call', + return_value=0) as mock_call, unittest.mock.patch.dict( + os.environ, {"GOMA_DIR": os.path.join(self.root_dir, 'goma_dir')}): + out_dir = os.path.join('out', 'dir') + write(os.path.join(out_dir, 'args.gn'), 'use_goma=true') + write( + os.path.join( + 'goma_dir', + 'gomacc.exe' if sys.platform.startswith('win') else 'gomacc'), + 'content') + args = autoninja.main(['autoninja.py', '-C', out_dir]).split() + mock_call.assert_called_once() + + self.assertIn('-j', args) + parallel_j = int(args[args.index('-j') + 1]) + self.assertGreater(parallel_j, multiprocessing.cpu_count()) + self.assertIn(os.path.join(autoninja.SCRIPT_DIR, 'ninja.py'), args) + + def test_autoninja_reclient(self): + out_dir = os.path.join('out', 'dir') + write(os.path.join(out_dir, 'args.gn'), 'use_remoteexec=true') + write('.gclient', '') + write('.gclient_entries', 'entries = {"buildtools": "..."}') + write(os.path.join('buildtools', 'reclient_cfgs', 'reproxy.cfg'), 'RBE_v=2') + write(os.path.join('buildtools', 'reclient', 'version.txt'), '0.0') + + args = autoninja.main(['autoninja.py', '-C', out_dir]).split() self.assertIn('-j', args) parallel_j = int(args[args.index('-j') + 1]) self.assertGreater(parallel_j, multiprocessing.cpu_count()) + self.assertIn(os.path.join(autoninja.SCRIPT_DIR, 'ninja_reclient.py'), args) if __name__ == '__main__': diff --git a/tests/ninja_reclient_test.py b/tests/ninja_reclient_test.py new file mode 100755 index 0000000000..6c3b2dc8c8 --- /dev/null +++ b/tests/ninja_reclient_test.py @@ -0,0 +1,124 @@ +#!/usr/bin/env python3 +# Copyright (c) 2023 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +import os +import os.path +import sys +import unittest +import unittest.mock + +ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +sys.path.insert(0, ROOT_DIR) + +import ninja_reclient +from testing_support import trial_dir + + +def write(filename, content): + """Writes the content of a file and create the directories as needed.""" + filename = os.path.abspath(filename) + dirname = os.path.dirname(filename) + if not os.path.isdir(dirname): + os.makedirs(dirname) + with open(filename, 'w') as f: + f.write(content) + + +class NinjaReclientTest(trial_dir.TestCase): + def setUp(self): + super(NinjaReclientTest, self).setUp() + self.previous_dir = os.getcwd() + os.chdir(self.root_dir) + + def tearDown(self): + os.chdir(self.previous_dir) + super(NinjaReclientTest, self).tearDown() + + @unittest.mock.patch('subprocess.call', return_value=0) + @unittest.mock.patch('ninja.main', return_value=0) + def test_ninja_reclient(self, mock_ninja, mock_call): + reclient_bin_dir = os.path.join('src', 'buildtools', 'reclient') + reclient_cfg = os.path.join('src', 'buildtools', 'reclient_cfgs', + 'reproxy.cfg') + write('.gclient', '') + write('.gclient_entries', 'entries = {"buildtools": "..."}') + write(os.path.join(reclient_bin_dir, 'version.txt'), '0.0') + write(reclient_cfg, '0.0') + argv = ["ninja_reclient.py", "-C", "out/a", "chrome"] + + self.assertEqual(0, ninja_reclient.main(argv)) + + mock_ninja.assert_called_once_with(argv) + mock_call.assert_has_calls([ + unittest.mock.call([ + os.path.join(self.root_dir, reclient_bin_dir, + 'bootstrap'), "--re_proxy=" + + os.path.join(self.root_dir, reclient_bin_dir, 'reproxy'), + "--cfg=" + os.path.join(self.root_dir, reclient_cfg) + ]), + unittest.mock.call([ + os.path.join(self.root_dir, reclient_bin_dir, 'bootstrap'), + "--shutdown", "--cfg=" + os.path.join(self.root_dir, reclient_cfg) + ]), + ]) + + @unittest.mock.patch('subprocess.call', return_value=0) + @unittest.mock.patch('ninja.main', side_effect=KeyboardInterrupt()) + def test_ninja_reclient_ninja_interrupted(self, mock_ninja, mock_call): + reclient_bin_dir = os.path.join('src', 'buildtools', 'reclient') + reclient_cfg = os.path.join('src', 'buildtools', 'reclient_cfgs', + 'reproxy.cfg') + write('.gclient', '') + write('.gclient_entries', 'entries = {"buildtools": "..."}') + write(os.path.join(reclient_bin_dir, 'version.txt'), '0.0') + write(reclient_cfg, '0.0') + argv = ["ninja_reclient.py", "-C", "out/a", "chrome"] + + self.assertEqual(1, ninja_reclient.main(argv)) + + mock_ninja.assert_called_once_with(argv) + mock_call.assert_has_calls([ + unittest.mock.call([ + os.path.join(self.root_dir, reclient_bin_dir, + 'bootstrap'), "--re_proxy=" + + os.path.join(self.root_dir, reclient_bin_dir, 'reproxy'), + "--cfg=" + os.path.join(self.root_dir, reclient_cfg) + ]), + unittest.mock.call([ + os.path.join(self.root_dir, reclient_bin_dir, 'bootstrap'), + "--shutdown", "--cfg=" + os.path.join(self.root_dir, reclient_cfg) + ]), + ]) + + @unittest.mock.patch('subprocess.call', return_value=0) + @unittest.mock.patch('ninja.main', return_value=0) + def test_ninja_reclient_cfg_not_found(self, mock_ninja, mock_call): + write('.gclient', '') + write('.gclient_entries', 'entries = {"buildtools": "..."}') + write(os.path.join('src', 'buildtools', 'reclient', 'version.txt'), '0.0') + argv = ["ninja_reclient.py", "-C", "out/a", "chrome"] + + self.assertEqual(1, ninja_reclient.main(argv)) + + mock_ninja.assert_not_called() + mock_call.assert_not_called() + + @unittest.mock.patch('subprocess.call', return_value=0) + @unittest.mock.patch('ninja.main', return_value=0) + def test_ninja_reclient_bins_not_found(self, mock_ninja, mock_call): + write('.gclient', '') + write('.gclient_entries', 'entries = {"buildtools": "..."}') + write(os.path.join('src', 'buildtools', 'reclient_cfgs', 'reproxy.cfg'), + '0.0') + argv = ["ninja_reclient.py", "-C", "out/a", "chrome"] + + self.assertEqual(1, ninja_reclient.main(argv)) + + mock_ninja.assert_not_called() + mock_call.assert_not_called() + + +if __name__ == '__main__': + unittest.main()