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 <philwo@google.com>
Reviewed-by: Fumitoshi Ukai <ukai@google.com>
Commit-Queue: Josip Sokcevic <sokcevic@chromium.org>
Reviewed-by: Takuto Ikuta <tikuta@chromium.org>
Reviewed-by: Ben Segall <bentekkie@google.com>
Auto-Submit: Junji Watanabe <jwata@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>

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
 

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__":

ninja_reclient.py

@@ -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))

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

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

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([

tests/ninja_reclient_test.py → 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()