From 1387a8c9586c457bbfda4ba2a81f41efd345abd8 Mon Sep 17 00:00:00 2001 From: Junji Watanabe Date: Tue, 9 Jul 2024 00:52:12 +0000 Subject: [PATCH] autoninja: Refactor the code calling Reclient - Remove reclient_helper.py to avoid calling it directly. - Cleans up autoninja.py I just want to do this cleanup before merging the user notice messages for logs/metrics collection. Bug: 345113094 Change-Id: I0c76426b3cb387eae6dede031727c107e57d5d1a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5668282 Reviewed-by: Philipp Wollermann Reviewed-by: Fumitoshi Ukai Commit-Queue: Josip Sokcevic Reviewed-by: Takuto Ikuta Reviewed-by: Ben Segall Auto-Submit: Junji Watanabe Reviewed-by: Josip Sokcevic --- OWNERS | 1 - autoninja.py | 39 +++++---------- ninja_reclient.py | 28 ----------- reclient_helper.py | 29 +++++++++++- tests/OWNERS | 6 +-- tests/autoninja_test.py | 15 +++--- ...client_test.py => reclient_helper_test.py} | 47 +++++++++---------- 7 files changed, 73 insertions(+), 92 deletions(-) delete mode 100755 ninja_reclient.py rename tests/{ninja_reclient_test.py => reclient_helper_test.py} (92%) diff --git a/OWNERS b/OWNERS index dae70295e..4a8e78afe 100644 --- a/OWNERS +++ b/OWNERS @@ -20,7 +20,6 @@ per-file autoninja*=file://BUILD_OWNERS per-file ninja*=dpranke@google.com per-file ninja*=thakis@chromium.org per-file ninja*=file://BUILD_OWNERS -per-file ninja_reclient.py=file://RECLIENT_OWNERS per-file post_build_ninja_summary.py=brucedawson@chromium.org per-file post_build_ninja_summary.py=file://BUILD_OWNERS diff --git a/autoninja.py b/autoninja.py index 792d95c34..6bca6666d 100755 --- a/autoninja.py +++ b/autoninja.py @@ -32,7 +32,6 @@ from google.auth.transport.requests import AuthorizedSession import gn_helper import ninja -import ninja_reclient import reclient_helper import siso @@ -285,19 +284,14 @@ def main(args): return 1 if use_remoteexec: if use_reclient: - with reclient_helper.build_context(input_args, - 'autosiso') as ret_code: - if ret_code: - return ret_code - return siso.main([ - 'siso', - 'ninja', - # Do not authenticate when using Reproxy. - '-project=', - '-reapi_instance=', - ] + input_args[1:]) - else: - return siso.main(["siso", "ninja"] + input_args[1:]) + return reclient_helper.run_siso([ + 'siso', + 'ninja', + # Do not authenticate when using Reproxy. + '-project=', + '-reapi_instance=', + ] + input_args[1:]) + return siso.main(["siso", "ninja"] + input_args[1:]) return siso.main(["siso", "ninja", "--offline"] + input_args[1:]) if os.path.exists(siso_marker): @@ -349,16 +343,7 @@ def main(args): fileno_limit, hard_limit = resource.getrlimit( resource.RLIMIT_NOFILE) - # 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 use_remoteexec: - ninja_path = os.path.join(SCRIPT_DIR, "ninja_reclient.py") - - args = [sys.executable, ninja_path] - + args = ['ninja'] num_cores = multiprocessing.cpu_count() if not j_specified and not t_specified: if not offline and use_remoteexec: @@ -407,9 +392,9 @@ def main(args): # are being used. _print_cmd(args) - if use_remoteexec: - return ninja_reclient.main(args[1:]) - return ninja.main(args[1:]) + if use_reclient: + return reclient_helper.run_ninja(args) + return ninja.main(args) if __name__ == "__main__": diff --git a/ninja_reclient.py b/ninja_reclient.py deleted file mode 100755 index 0b03fb848..000000000 --- a/ninja_reclient.py +++ /dev/null @@ -1,28 +0,0 @@ -#!/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 sys - -import ninja -import reclient_helper - - -def main(argv): - with reclient_helper.build_context(argv, "ninja_reclient") as ret_code: - if ret_code: - return ret_code - try: - return ninja.main(argv) - except KeyboardInterrupt: - print("Shutting down reproxy...", file=sys.stderr) - return 1 - - -if __name__ == "__main__": - sys.exit(main(sys.argv)) diff --git a/reclient_helper.py b/reclient_helper.py index 16f6c2159..a7399988f 100644 --- a/reclient_helper.py +++ b/reclient_helper.py @@ -19,8 +19,9 @@ import time import uuid import gclient_paths +import ninja import reclient_metrics - +import siso THIS_DIR = os.path.dirname(__file__) RECLIENT_LOG_CLEANUP = os.path.join(THIS_DIR, 'reclient_log_cleanup.py') @@ -376,3 +377,29 @@ Ensure you have completed the reproxy setup instructions: if os.environ.get('NINJA_SUMMARIZE_BUILD') == '1': elapsed = time.time() - start print('%1.3fs to stop reproxy' % elapsed) + + +def run_ninja(ninja_cmd): + """Runs Ninja in build_context().""" + # TODO: crbug.com/345113094 - rename the `tool` label to `ninja`. + with build_context(ninja_cmd, "ninja_reclient") as ret_code: + if ret_code: + return ret_code + try: + return ninja.main(ninja_cmd) + except KeyboardInterrupt: + print("Shutting down reproxy...", file=sys.stderr) + return 1 + + +def run_siso(siso_cmd): + """Runs Siso in build_context().""" + # TODO: crbug.com/345113094 - rename the `autosiso` label to `siso`. + with build_context(siso_cmd, "autosiso") as ret_code: + if ret_code: + return ret_code + try: + return siso.main(siso_cmd) + except KeyboardInterrupt: + print("Shutting down reproxy...", file=sys.stderr) + return 1 diff --git a/tests/OWNERS b/tests/OWNERS index 9198d2a56..72386f9b4 100644 --- a/tests/OWNERS +++ b/tests/OWNERS @@ -3,7 +3,5 @@ per-file autoninja_test.py=file://BUILD_OWNERS per-file bazel_test.py=file://CROS_OWNERS per-file gn_helper_test.py=file://BUILD_OWNERS per-file ninjalog_uploader_test.py=tikuta@chromium.org -per-file ninja_reclient_test.py=file://BUILD_OWNERS -per-file ninja_reclient_test.py=file://RECLIENT_OWNERS -per-file reclient_metrics_test.py=file://BUILD_OWNERS -per-file reclient_metrics_test.py=file://RECLIENT_OWNERS +per-file reclient*=file://BUILD_OWNERS +per-file reclient*=file://RECLIENT_OWNERS diff --git a/tests/autoninja_test.py b/tests/autoninja_test.py index b807ef660..de613669b 100755 --- a/tests/autoninja_test.py +++ b/tests/autoninja_test.py @@ -76,18 +76,18 @@ class AutoninjaTest(trial_dir.TestCase): def test_autoninja_reclient(self): """ Test that when specifying use_remoteexec=true, autoninja delegates to - ninja_reclient. + reclient_helper. """ - with mock.patch('ninja_reclient.main', - return_value=0) as ninja_reclient_main: + with mock.patch('reclient_helper.run_ninja', + return_value=0) as run_ninja: out_dir = os.path.join('out', 'dir') write(os.path.join(out_dir, 'args.gn'), 'use_remoteexec=true') write(os.path.join('buildtools', 'reclient_cfgs', 'reproxy.cfg'), 'RBE_v=2') write(os.path.join('buildtools', 'reclient', 'version.txt'), '0.0') autoninja.main(['autoninja.py', '-C', out_dir]) - ninja_reclient_main.assert_called_once() - args = ninja_reclient_main.call_args.args[0] + run_ninja.assert_called_once() + args = run_ninja.call_args.args[0] self.assertIn('-C', args) self.assertEqual(args[args.index('-C') + 1], out_dir) # Check that autoninja correctly calculated the number of jobs to use @@ -138,8 +138,9 @@ class AutoninjaTest(trial_dir.TestCase): out_dir ]) self.assertEqual(len(reclient_helper_calls), 1) - self.assertEqual(reclient_helper_calls[0][0], - ['autoninja.py', '-C', out_dir]) + self.assertEqual( + reclient_helper_calls[0][0], + ['siso', 'ninja', '-project=', '-reapi_instance=', '-C', out_dir]) self.assertEqual(reclient_helper_calls[0][1], 'autosiso') @parameterized.expand([ diff --git a/tests/ninja_reclient_test.py b/tests/reclient_helper_test.py similarity index 92% rename from tests/ninja_reclient_test.py rename to tests/reclient_helper_test.py index 339371b4a..9538f7d76 100755 --- a/tests/ninja_reclient_test.py +++ b/tests/reclient_helper_test.py @@ -16,7 +16,6 @@ ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, ROOT_DIR) import gclient_paths -import ninja_reclient import reclient_helper from testing_support import trial_dir @@ -31,15 +30,15 @@ def write(filename, content): f.write(content) -class NinjaReclientTest(trial_dir.TestCase): +class ReclientHelperTest(trial_dir.TestCase): def setUp(self): - super(NinjaReclientTest, self).setUp() + super().setUp() self.previous_dir = os.getcwd() os.chdir(self.root_dir) def tearDown(self): os.chdir(self.previous_dir) - super(NinjaReclientTest, self).tearDown() + super().tearDown() @unittest.mock.patch.dict(os.environ, {'AUTONINJA_BUILD_ID': "SOME_RANDOM_ID"}, @@ -58,9 +57,9 @@ class NinjaReclientTest(trial_dir.TestCase): 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"] + argv = ["ninja", "-C", "out/a", "chrome"] - self.assertEqual(0, ninja_reclient.main(argv)) + self.assertEqual(0, reclient_helper.run_ninja(argv)) run_log_dir = os.path.join(self.root_dir, "out", "a", ".reproxy_tmp", "logs", @@ -113,9 +112,9 @@ class NinjaReclientTest(trial_dir.TestCase): 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"] + argv = ["ninja", "-C", "out/a", "chrome"] - self.assertEqual(0, ninja_reclient.main(argv)) + self.assertEqual(0, reclient_helper.run_ninja(argv)) mock_metrics_status.assert_called_once_with("out/a") mock_ninja.assert_called_once_with(argv) @@ -150,9 +149,9 @@ class NinjaReclientTest(trial_dir.TestCase): 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"] + argv = ["ninja", "-C", "out/a", "chrome"] - self.assertEqual(0, ninja_reclient.main(argv)) + self.assertEqual(0, reclient_helper.run_ninja(argv)) self.assertIn("/SOME_RANDOM_ID", os.environ["RBE_invocation_id"]) self.assertEqual(os.environ.get('RBE_metrics_project'), @@ -199,9 +198,9 @@ expiry: { seconds: %d } """ % (int(time.time()) + 10 * 60)) - argv = ["ninja_reclient.py", "-C", "out/a", "chrome"] + argv = ["ninja", "-C", "out/a", "chrome"] - self.assertEqual(0, ninja_reclient.main(argv)) + self.assertEqual(0, reclient_helper.run_ninja(argv)) self.assertIn("/SOME_RANDOM_ID", os.environ["RBE_invocation_id"]) self.assertEqual(os.environ.get('RBE_metrics_project'), @@ -244,9 +243,9 @@ expiry: { seconds: %d } """ % (int(time.time()))) - argv = ["ninja_reclient.py", "-C", "out/a", "chrome"] + argv = ["ninja", "-C", "out/a", "chrome"] - self.assertEqual(0, ninja_reclient.main(argv)) + self.assertEqual(0, reclient_helper.run_ninja(argv)) self.assertIn("/SOME_RANDOM_ID", os.environ["RBE_invocation_id"]) self.assertEqual(os.environ.get('RBE_metrics_project'), @@ -275,9 +274,9 @@ expiry: { 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"] + argv = ["ninja", "-C", "out/a", "chrome"] - self.assertEqual(0, ninja_reclient.main(argv)) + self.assertEqual(0, reclient_helper.run_ninja(argv)) self.assertEqual(os.environ.get('RBE_metrics_project'), None) self.assertEqual(os.environ.get('RBE_metrics_table'), None) @@ -298,7 +297,7 @@ expiry: { 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"] + argv = ["ninja", "-C", "out/a", "chrome"] for i in range(7): run_time = datetime.datetime(2017, 3, 16, 20, 0, 40 + i, 0) @@ -306,7 +305,7 @@ expiry: { with unittest.mock.patch.dict( os.environ, {"AUTONINJA_BUILD_ID": "SOME_RANDOM_ID_%d" % i}): - self.assertEqual(0, ninja_reclient.main(argv)) + self.assertEqual(0, reclient_helper.run_ninja(argv)) run_log_dir = os.path.join( self.root_dir, "out", "a", ".reproxy_tmp", "logs", "20170316T2000%d.000000_SOME_RANDOM_ID_%d" % (40 + i, i)) @@ -349,9 +348,9 @@ expiry: { 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"] + argv = ["ninja", "-C", "out/a", "chrome"] - self.assertEqual(1, ninja_reclient.main(argv)) + self.assertEqual(1, reclient_helper.run_ninja(argv)) mock_ninja.assert_called_once_with(argv) mock_call.assert_has_calls([ @@ -378,9 +377,9 @@ expiry: { write('.gclient_entries', 'entries = {"buildtools": "..."}') write(os.path.join('src', 'buildtools', 'reclient', 'version.txt'), '0.0') - argv = ["ninja_reclient.py", "-C", "out/a", "chrome"] + argv = ["ninja", "-C", "out/a", "chrome"] - self.assertEqual(1, ninja_reclient.main(argv)) + self.assertEqual(1, reclient_helper.run_ninja(argv)) mock_ninja.assert_not_called() mock_call.assert_not_called() @@ -392,9 +391,9 @@ expiry: { 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"] + argv = ["ninja", "-C", "out/a", "chrome"] - self.assertEqual(1, ninja_reclient.main(argv)) + self.assertEqual(1, reclient_helper.run_ninja(argv)) mock_ninja.assert_not_called() mock_call.assert_not_called()