Execute Ninja / Siso directly from autoninja.py.

Instead of printing a command-line, we just directly call into the respective main functions from Python. This saves spawning another interpreter and prevents things that can go wrong from having to quote, unquote, split and tunnel arguments through shells.

Part of my bigger auto{ninja,siso} refactoring.

Tested:
- Handling of the ^^ suffix on Windows still works correctly.
- Handling of error codes - i.e.; making sure
  "autoninja base_unittests && base_unittests.exe" behaves properly
  in the success/failure case.
- Make sure the command prompt title is reliably reset on exit.

I tested autoninja with all combinations of these:
- Host platform: Linux, macOS, Windows
- Remote GN args: <none>, use_goma=true, use_remoteexec=true
- Siso GN args: <none>, use_siso=true
- Targets: base, ../../base/types/expected_macros_unittest.cc^ (on Linux) and ../../base/types/expected_macros_unittest.cc^^ (on Windows)

R=brucedawson@chromium.org, jwata@google.com, tikuta@chromium.org

Bug: b/293657720
Change-Id: I275a775fdc5abb6555f79d4beab76cd0914d4bd6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4924185
Reviewed-by: Takuto Ikuta <tikuta@chromium.org>
Reviewed-by: Junji Watanabe <jwata@google.com>
Commit-Queue: Philipp Wollermann <philwo@chromium.org>

autoninja

@@ -20,23 +20,16 @@ fi
 
 # Execute whatever is printed by autoninja.py.
 # Also print it to reassure that the right settings are being used.
-command=$(python3 "$(dirname -- "$0")/autoninja.py" "$@")
-if [ "$NINJA_SUMMARIZE_BUILD" == "1" ]; then
-  echo "$command"
-fi
-if eval "$command"; then
-  if [ "$NINJA_SUMMARIZE_BUILD" == "1" ]; then
-    python3 "$(dirname -- "$0")/post_build_ninja_summary.py" "$@"
-  fi
-
-  # Collect ninjalog from googler.
-  python3 "$(dirname -- "$0")/ninjalog_uploader_wrapper.py" --cmd $command
-  exit
+python3 "$(dirname -- "$0")/autoninja.py" "$@"
+retval=$?
+
+if [ "$retval" == "0" ] && [ "$NINJA_SUMMARIZE_BUILD" == "1" ]; then
+  python3 "$(dirname -- "$0")/post_build_ninja_summary.py" "$@"
 fi
 
 # Collect ninjalog from googler.
-python3 "$(dirname -- "$0")/ninjalog_uploader_wrapper.py" --cmd $command
+python3 "$(dirname -- "$0")/ninjalog_uploader_wrapper.py" --cmdline "$@"
 
-# Return an error code of 1 so that if a developer types:
+# Pass-through autoninja's error code so that if a developer types:
 # "autoninja chrome && chrome" then chrome won't run if the build fails.
-exit 1
+exit $retval

autoninja.bat

@@ -24,28 +24,13 @@ if not defined AUTONINJA_BUILD_ID (
 :: Windows. The trailing space is intentional.
 if "%NINJA_SUMMARIZE_BUILD%" == "1" set "NINJA_STATUS=[%%r processes, %%f/%%t @ %%o/s : %%es ] "
 
-:loop
-IF NOT "%1"=="" (
-    :: Tell goma or reclient to not do network compiles.
-    IF "%1"=="--offline" (
-        SET GOMA_DISABLED=1
-        SET RBE_remote_disabled=1
-    )
-    IF "%1"=="-o" (
-        SET GOMA_DISABLED=1
-        SET RBE_remote_disabled=1
-    )
-    SHIFT
-    GOTO :loop
-)
-
-:: Execute whatever is printed by autoninja.py.
-:: Also print it to reassure that the right settings are being used.
+:: Execute autoninja.py and pass all arguments to it.
 :: Don't use vpython - it is too slow to start.
 :: Don't use python3 because it doesn't work in git bash on Windows and we
 :: should be consistent between autoninja.bat and the autoninja script used by
 :: git bash.
-FOR /f "usebackq tokens=*" %%a in (`%scriptdir%python-bin\python3.bat %scriptdir%autoninja.py "%*"`) do echo %%a & %%a
+
+@call %scriptdir%python-bin\python3.bat %scriptdir%autoninja.py "%%*"
 @if errorlevel 1 goto buildfailure
 
 :: Use call to invoke python script here, because we use python via python3.bat.

autoninja.py

@@ -18,14 +18,51 @@ import multiprocessing
 import os
 import platform
 import re
+import shlex
 import subprocess
 import sys
 
+import autosiso
+import ninja
+import ninja_reclient
+import siso
+
 if sys.platform in ['darwin', 'linux']:
     import resource
 
 SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
 
+# See [1] and [2] for the painful details of this next section, which handles
+# escaping command lines so that they can be copied and pasted into a cmd
+# window.
+#
+# pylint: disable=line-too-long
+# [1] https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way # noqa
+# [2] https://web.archive.org/web/20150815000000*/https://www.microsoft.com/resources/documentation/windows/xp/all/proddocs/en-us/set.mspx # noqa
+_UNSAFE_FOR_CMD = set('^<>&|()%')
+_ALL_META_CHARS = _UNSAFE_FOR_CMD.union(set('"'))
+
+
+def _quote_for_cmd(arg):
+    # First, escape the arg so that CommandLineToArgvW will parse it properly.
+    if arg == '' or ' ' in arg or '"' in arg:
+        quote_re = re.compile(r'(\\*)"')
+        arg = '"%s"' % (quote_re.sub(lambda mo: 2 * mo.group(1) + '\\"', arg))
+
+    # Then check to see if the arg contains any metacharacters other than
+    # double quotes; if it does, quote everything (including the double
+    # quotes) for safety.
+    if any(a in _UNSAFE_FOR_CMD for a in arg):
+        arg = ''.join('^' + a if a in _ALL_META_CHARS else a for a in arg)
+    return arg
+
+
+def _print_cmd(cmd):
+    shell_quoter = shlex.quote
+    if sys.platform.startswith('win'):
+        shell_quoter = _quote_for_cmd
+    print(*[shell_quoter(arg) for arg in cmd], file=sys.stderr)
+
 
 def _gn_lines(output_dir, path):
     """
@@ -58,6 +95,7 @@ def main(args):
     offline = False
     output_dir = '.'
     input_args = args
+    summarize_build = os.environ.get('NINJA_SUMMARIZE_BUILD') == '1'
     # On Windows the autoninja.bat script passes along the arguments enclosed in
     # double quotes. This prevents multiple levels of parsing of the special '^'
     # characters needed when compiling a single file but means that this script
@@ -134,21 +172,19 @@ def main(args):
             # if there is a .ninja_log but no .siso_deps then that implies a
             # ninja build.
             if os.path.exists(ninja_marker) and not os.path.exists(siso_marker):
-                return (
-                    'echo Run gn clean before switching from ninja to siso in '
-                    '%s' % output_dir)
-            siso = ['autosiso'] if use_remoteexec else ['siso', 'ninja']
-            if sys.platform.startswith('win'):
-                # An explicit 'call' is needed to make sure the invocation of
-                # autosiso returns to autoninja.bat, and the command prompt
-                # title gets reset.
-                siso = ['call'] + siso
-            return ' '.join(siso + input_args[1:])
+                print('Run gn clean before switching from ninja to siso in %s' %
+                      output_dir,
+                      file=sys.stderr)
+                return 1
+            if use_remoteexec:
+                return autosiso.main(['autosiso'] + input_args[1:])
+            return siso.main(['siso', 'ninja'] + input_args[1:])
 
         if os.path.exists(siso_marker):
-            return (
-                'echo Run gn clean before switching from siso to ninja in %s' %
-                output_dir)
+            print('Run gn clean before switching from siso to ninja in %s' %
+                  output_dir,
+                  file=sys.stderr)
+            return 1
 
     else:
         for relative_path in [
@@ -207,19 +243,20 @@ def main(args):
                 sys.exit(1)
 
     # A large build (with or without goma) tends to hog all system resources.
-    # Launching the ninja process with 'nice' priorities improves this
-    # situation.
-    prefix_args = []
-    if (sys.platform.startswith('linux')
-            and os.environ.get('NINJA_BUILD_IN_BACKGROUND', '0') == '1'):
-        # nice -10 is process priority 10 lower than default 0
-        # ionice -c 3 is IO priority IDLE
-        prefix_args = ['nice'] + ['-10']
-
-    # Tell goma or reclient to do local compiles. On Windows these environment
-    # variables are set by the wrapper batch file.
-    offline_env = ['RBE_remote_disabled=1', 'GOMA_DISABLED=1'
-                   ] if offline and not sys.platform.startswith('win') else []
+    # Depending on the operating system, we might have mechanisms available
+    # to run at a lower priority, which improves this situation.
+    if os.environ.get('NINJA_BUILD_IN_BACKGROUND') == '1':
+        if sys.platform in ['darwin', 'linux']:
+            # nice-level 10 is usually considered a good default for background
+            # tasks. The niceness is inherited by child processes, so we can
+            # just set it here for us and it'll apply to the build tool we
+            # spawn later.
+            os.nice(10)
+
+    # Tell goma or reclient to do local compiles.
+    if offline:
+        os.environ['RBE_remote_disabled'] = '1'
+        os.environ['GOMA_DISABLED'] = '1'
 
     # On macOS and most Linux distributions, the default limit of open file
     # descriptors is too low (256 and 1024, respectively).
@@ -227,7 +264,6 @@ def main(args):
     # Check whether the limit can be raised to a large enough value. If yes,
     # use `ulimit -n .... &&` as a prefix to increase the limit when running
     # ninja.
-    prepend_command = []
     if sys.platform in ['darwin', 'linux']:
         # Increase the number of allowed open file descriptors to the maximum.
         fileno_limit, hard_limit = resource.getrlimit(resource.RLIMIT_NOFILE)
@@ -235,12 +271,10 @@ def main(args):
             try:
                 resource.setrlimit(resource.RLIMIT_NOFILE,
                                    (hard_limit, hard_limit))
-            except Exception as _:
+            except Exception:
                 pass
             fileno_limit, hard_limit = resource.getrlimit(
                 resource.RLIMIT_NOFILE)
-            if fileno_limit == hard_limit:
-                prepend_command = ['ulimit', '-n', f'{fileno_limit}', '&&']
 
     # Call ninja.py so that it can find ninja binary installed by DEPS or one in
     # PATH.
@@ -250,9 +284,7 @@ def main(args):
     if use_remoteexec:
         ninja_path = os.path.join(SCRIPT_DIR, 'ninja_reclient.py')
 
-    args = prepend_command + offline_env + prefix_args + [
-        sys.executable, ninja_path
-    ] + input_args[1:]
+    args = [sys.executable, ninja_path] + input_args[1:]
 
     num_cores = multiprocessing.cpu_count()
     if not j_specified and not t_specified:
@@ -291,20 +323,20 @@ def main(args):
             args.append('-j')
             args.append('%d' % j_value)
 
-    # On Windows, fully quote the path so that the command processor doesn't
-    # think the whole output is the command. On Linux and Mac, if people put
-    # depot_tools in directories with ' ', shell would misunderstand ' ' as a
-    # path separation. TODO(yyanagisawa): provide proper quoting for Windows.
-    # see https://cs.chromium.org/chromium/src/tools/mb/mb.py
-    for i in range(len(args)):
-        if (i == 0 and sys.platform.startswith('win')) or ' ' in args[i]:
-            args[i] = '"%s"' % args[i].replace('"', '\\"')
-
-    if os.environ.get('NINJA_SUMMARIZE_BUILD', '0') == '1':
+    if summarize_build:
+        # Enable statistics collection in Ninja.
         args += ['-d', 'stats']
+        # Print the command-line to reassure the user that the right settings
+        # are being used.
+        _print_cmd(args)
 
-    return ' '.join(args)
+    if use_remoteexec:
+        return ninja_reclient.main(args[1:])
+    return ninja.main(args[1:])
 
 
 if __name__ == '__main__':
-    print(main(sys.argv))
+    try:
+        sys.exit(main(sys.argv))
+    except KeyboardInterrupt:
+        sys.exit(1)

tests/autoninja_test.py

@@ -6,9 +6,11 @@
 import multiprocessing
 import os
 import os.path
+import io
 import sys
 import unittest
-import unittest.mock
+import contextlib
+from unittest import mock
 
 ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
 sys.path.insert(0, ROOT_DIR)
@@ -38,42 +40,117 @@ class AutoninjaTest(trial_dir.TestCase):
         super(AutoninjaTest, self).tearDown()
 
     def test_autoninja(self):
-        autoninja.main([])
+        """Test that by default (= no GN args) autoninja delegates to ninja."""
+        with mock.patch('ninja.main', return_value=0) as ninja_main:
+            out_dir = os.path.join('out', 'dir')
+            write(os.path.join(out_dir, 'args.gn'), '')
+            autoninja.main(['autoninja.py', '-C', out_dir])
+            ninja_main.assert_called_once()
+            args = ninja_main.call_args.args[0]
+        self.assertIn('-C', args)
+        self.assertEqual(args[args.index('-C') + 1], out_dir)
+
+    @mock.patch('sys.platform', 'win32')
+    def test_autoninja_splits_args_on_windows(self):
+        """
+        Test that autoninja correctly handles the special case of being
+        passed its arguments as a quoted, whitespace-delimited string on
+        Windows.
+        """
+        with mock.patch('ninja.main', return_value=0) as ninja_main:
+            out_dir = os.path.join('out', 'dir')
+            write(os.path.join(out_dir, 'args.gn'), '')
+            autoninja.main([
+                'autoninja.py',
+                '-C {} base'.format(out_dir),
+            ])
+            ninja_main.assert_called_once()
+            args = ninja_main.call_args.args[0]
+        self.assertIn('-C', args)
+        self.assertEqual(args[args.index('-C') + 1], out_dir)
+        self.assertIn('base', args)
 
     def test_autoninja_goma(self):
-        with unittest.mock.patch(
-                'subprocess.call',
-                return_value=0) as mock_call, unittest.mock.patch.dict(
-                    os.environ,
-                    {"GOMA_DIR": os.path.join(self.root_dir, 'goma_dir')}):
+        """
+        Test that when specifying use_goma=true, autoninja verifies that Goma
+        is running and then delegates to ninja.
+        """
+        goma_dir = os.path.join(self.root_dir, 'goma_dir')
+        with mock.patch('subprocess.call', return_value=0), \
+             mock.patch('ninja.main', return_value=0) as ninja_main, \
+             mock.patch.dict(os.environ, {"GOMA_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()
-
+            autoninja.main(['autoninja.py', '-C', out_dir])
+            ninja_main.assert_called_once()
+            args = ninja_main.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
+        # as required for remote execution, instead of using the value for
+        # local execution.
         self.assertIn('-j', args)
         parallel_j = int(args[args.index('-j') + 1])
         self.assertGreater(parallel_j, multiprocessing.cpu_count() * 2)
-        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(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()
-
+        """
+        Test that when specifying use_remoteexec=true, autoninja delegates to
+        ninja_reclient.
+        """
+        with mock.patch('ninja_reclient.main',
+                        return_value=0) as ninja_reclient_main:
+            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]
+        self.assertIn('-C', args)
+        self.assertEqual(args[args.index('-C') + 1], out_dir)
+        # Check that autoninja correctly calculated the number of jobs to use
+        # as required for remote execution, instead of using the value for
+        # local execution.
         self.assertIn('-j', args)
         parallel_j = int(args[args.index('-j') + 1])
         self.assertGreater(parallel_j, multiprocessing.cpu_count() * 2)
-        self.assertIn(os.path.join(autoninja.SCRIPT_DIR, 'ninja_reclient.py'),
-                      args)
+
+    def test_autoninja_siso(self):
+        """
+        Test that when specifying use_siso=true, autoninja delegates to siso.
+        """
+        with mock.patch('siso.main', return_value=0) as siso_main:
+            out_dir = os.path.join('out', 'dir')
+            write(os.path.join(out_dir, 'args.gn'), 'use_siso=true')
+            autoninja.main(['autoninja.py', '-C', out_dir])
+            siso_main.assert_called_once()
+            args = siso_main.call_args.args[0]
+        self.assertIn('-C', args)
+        self.assertEqual(args[args.index('-C') + 1], out_dir)
+
+    def test_autoninja_siso_reclient(self):
+        """
+        Test that when specifying use_siso=true and use_remoteexec=true,
+        autoninja delegates to autosiso.
+        """
+        with mock.patch('autosiso.main', return_value=0) as autosiso_main:
+            out_dir = os.path.join('out', 'dir')
+            write(os.path.join(out_dir, 'args.gn'),
+                  'use_siso=true\nuse_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])
+            autosiso_main.assert_called_once()
+            args = autosiso_main.call_args.args[0]
+        self.assertIn('-C', args)
+        self.assertEqual(args[args.index('-C') + 1], out_dir)
 
     def test_gn_lines(self):
         out_dir = os.path.join('out', 'dir')
@@ -92,6 +169,34 @@ class AutoninjaTest(trial_dir.TestCase):
             'use_remoteexec=true',
         ])
 
+    @mock.patch('sys.platform', 'win32')
+    def test_print_cmd_windows(self):
+        args = [
+            'C:\\Program Files\\Python 3\\bin\\python3.exe', 'ninja.py', '-C',
+            'out\\direc tory\\',
+            '../../base/types/expected_macros_unittest.cc^', '-j', '140'
+        ]
+        with contextlib.redirect_stderr(io.StringIO()) as f:
+            autoninja._print_cmd(args)
+            self.assertEqual(
+                f.getvalue(),
+                '"C:\\Program Files\\Python 3\\bin\\python3.exe" ninja.py -C ' +
+                '"out\\direc tory\\" ' +
+                '../../base/types/expected_macros_unittest.cc^^ -j 140\n')
+
+    @mock.patch('sys.platform', 'linux')
+    def test_print_cmd_linux(self):
+        args = [
+            '/home/user name/bin/python3', 'ninja.py', '-C', 'out/direc tory/',
+            '../../base/types/expected_macros_unittest.cc^', '-j', '140'
+        ]
+        with contextlib.redirect_stderr(io.StringIO()) as f:
+            autoninja._print_cmd(args)
+            self.assertEqual(
+                f.getvalue(),
+                "'/home/user name/bin/python3' ninja.py -C 'out/direc tory/' " +
+                "'../../base/types/expected_macros_unittest.cc^' -j 140\n")
+
 
 if __name__ == '__main__':
     unittest.main()