From 356ef0324e4352bc17733d8aab05ba1f77206b42 Mon Sep 17 00:00:00 2001 From: Fumitoshi Ukai Date: Thu, 27 Jun 2024 14:18:32 +0000 Subject: [PATCH] ninja: error if trying to build for use_remoteexec=true Change-Id: Ia32dd3cba1d1874401c6614f792f212b2cfa60a1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5660200 Commit-Queue: Joanna Wang Reviewed-by: Junji Watanabe Reviewed-by: Takuto Ikuta Auto-Submit: Fumitoshi Ukai Reviewed-by: Joanna Wang --- autoninja.py | 41 ++++------------------- gn_helper.py | 60 ++++++++++++++++++++++++++++++++++ ninja.py | 13 ++++++-- tests/OWNERS | 1 + tests/autoninja_test.py | 17 ---------- tests/gn_helper_test.py | 72 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 150 insertions(+), 54 deletions(-) create mode 100644 gn_helper.py create mode 100644 tests/gn_helper_test.py diff --git a/autoninja.py b/autoninja.py index 597fa18ee..792d95c34 100755 --- a/autoninja.py +++ b/autoninja.py @@ -30,6 +30,7 @@ import warnings import google.auth from google.auth.transport.requests import AuthorizedSession +import gn_helper import ninja import ninja_reclient import reclient_helper @@ -182,30 +183,6 @@ def _print_cmd(cmd): print(*[shell_quoter(arg) for arg in cmd], file=sys.stderr) -def _gn_lines(output_dir, path): - """ - Generator function that returns args.gn lines one at a time, following - import directives as needed. - """ - import_re = re.compile(r'\s*import\("(.*)"\)') - with open(path, encoding="utf-8") as f: - for line in f: - match = import_re.match(line) - if match: - raw_import_path = match.groups()[0] - if raw_import_path[:2] == "//": - import_path = os.path.normpath( - os.path.join(output_dir, "..", "..", - raw_import_path[2:])) - else: - import_path = os.path.normpath( - os.path.join(os.path.dirname(path), raw_import_path)) - for import_line in _gn_lines(output_dir, import_path): - yield import_line - else: - yield line - - def main(args): # if user doesn't set PYTHONPYCACHEPREFIX and PYTHONDONTWRITEBYTECODE # set PYTHONDONTWRITEBYTECODE=1 not to create many *.pyc in workspace @@ -259,8 +236,8 @@ def main(args): # 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. - if os.path.exists(os.path.join(output_dir, "args.gn")): - for line in _gn_lines(output_dir, os.path.join(output_dir, "args.gn")): + if gn_helper.exists(output_dir): + for k, v in gn_helper.args(output_dir): # use_remoteexec will activate build acceleration. # # This test can match multi-argument lines. Examples of this @@ -268,19 +245,13 @@ def main(args): # use_remoteexec=false# use_remoteexec=true This comment is ignored # # Anything after a comment is not consider a valid argument. - line_without_comment = line.split("#")[0] - if re.search( - r"(^|\s)(use_remoteexec)\s*=\s*true($|\s)", - line_without_comment, - ): + if k == "use_remoteexec" and v == "true": use_remoteexec = True continue - if re.search(r"(^|\s)(use_siso)\s*=\s*true($|\s)", - line_without_comment): + if k == "use_siso" and v == "true": use_siso = True continue - if re.search(r"(^|\s)(use_reclient)\s*=\s*false($|\s)", - line_without_comment): + if k == "use_reclient" and v == "false": use_reclient = False continue if use_reclient is None: diff --git a/gn_helper.py b/gn_helper.py new file mode 100644 index 000000000..fedc9e0ef --- /dev/null +++ b/gn_helper.py @@ -0,0 +1,60 @@ +# Copyright 2024 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 provides an easy way to access args.gn.""" + +import os +import re + + +def _gn_lines(output_dir, path): + """ + Generator function that returns args.gn lines one at a time, following + import directives as needed. + """ + import_re = re.compile(r'\s*import\("(.*)"\)') + with open(path, encoding="utf-8") as f: + for line in f: + match = import_re.match(line) + if match: + raw_import_path = match.groups()[0] + if raw_import_path[:2] == "//": + import_path = os.path.normpath( + os.path.join(output_dir, "..", "..", + raw_import_path[2:])) + else: + import_path = os.path.normpath( + os.path.join(os.path.dirname(path), raw_import_path)) + yield from _gn_lines(output_dir, import_path) + else: + yield line + + +def _path(output_dir): + return os.path.join(output_dir, "args.gn") + + +def exists(output_dir): + """Checks args.gn exists in output_dir.""" + return os.path.exists(_path(output_dir)) + + +def lines(output_dir): + """Generator of args.gn lines. comment is removed.""" + if not exists(output_dir): + return + for line in _gn_lines(output_dir, _path(output_dir)): + line_without_comment = line.split("#")[0] + yield line_without_comment + + +_gn_arg_pattern = re.compile(r"(^|\s)([^=\s]*)\s*=\s*(\S*)\s*$") + + +def args(output_dir): + """Generator of args.gn's key,value pair.""" + for line in lines(output_dir): + m = _gn_arg_pattern.match(line) + if not m: + continue + yield (m.group(2), m.group(3)) diff --git a/ninja.py b/ninja.py index 2b296ece4..49aa71b80 100755 --- a/ninja.py +++ b/ninja.py @@ -12,6 +12,7 @@ import subprocess import sys import gclient_paths +import gn_helper def findNinjaInPath(): @@ -29,7 +30,6 @@ def findNinjaInPath(): if os.path.isfile(ninja_path): return ninja_path - def checkOutdir(ninja_args): out_dir = "." tool = "" @@ -54,7 +54,16 @@ def checkOutdir(ninja_args): (out_dir, out_dir), file=sys.stderr) sys.exit(1) - + if os.environ.get("AUTONINJA_BUILD_ID"): + # autoninja sets AUTONINJA_BUILD_ID + return + for k, v in gn_helper.args(out_dir): + if k == "use_remoteexec" and v == "true": + print( + "depot_tools/ninja.py: detect use_remoteexec=true\n" + "Use `autoninja` to choose appropriate build tool\n", + file=sys.stderr) + sys.exit(1) def fallback(ninja_args): # Try to find ninja in PATH. diff --git a/tests/OWNERS b/tests/OWNERS index cd36a2923..9198d2a56 100644 --- a/tests/OWNERS +++ b/tests/OWNERS @@ -1,6 +1,7 @@ per-file autoninja_test.py=brucedawson@chromium.org 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 diff --git a/tests/autoninja_test.py b/tests/autoninja_test.py index 630368dd9..b807ef660 100755 --- a/tests/autoninja_test.py +++ b/tests/autoninja_test.py @@ -177,23 +177,6 @@ class AutoninjaTest(trial_dir.TestCase): autoninja._is_google_corp_machine_using_external_account()), expected) - def test_gn_lines(self): - out_dir = os.path.join('out', 'dir') - # Make sure nested import directives work. This is based on the - # reclient test. - write(os.path.join(out_dir, 'args.gn'), 'import("//out/common.gni")') - write(os.path.join('out', 'common.gni'), 'import("common_2.gni")') - write(os.path.join('out', 'common_2.gni'), 'use_remoteexec=true') - - lines = list( - autoninja._gn_lines(out_dir, os.path.join(out_dir, 'args.gn'))) - - # The test will only pass if both imports work and - # 'use_remoteexec=true' is seen. - self.assertListEqual(lines, [ - 'use_remoteexec=true', - ]) - @mock.patch('sys.platform', 'win32') def test_print_cmd_windows(self): args = [ diff --git a/tests/gn_helper_test.py b/tests/gn_helper_test.py new file mode 100644 index 000000000..9c03f60eb --- /dev/null +++ b/tests/gn_helper_test.py @@ -0,0 +1,72 @@ +#!/usr/bin/env vpython3 +# Copyright (c) 2024 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 sys +import unittest + +ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +sys.path.insert(0, ROOT_DIR) + +import gn_helper +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 GnHelperTest(trial_dir.TestCase): + + def setUp(self): + super().setUp() + self.previous_dir = os.getcwd() + os.chdir(self.root_dir) + + def tearDown(self): + os.chdir(self.previous_dir) + super().tearDown() + + def test_lines(self): + out_dir = os.path.join('out', 'dir') + # Make sure nested import directives work. This is based on the + # reclient test. + write(os.path.join(out_dir, 'args.gn'), 'import("//out/common.gni")') + write(os.path.join('out', 'common.gni'), 'import("common_2.gni")') + write(os.path.join('out', 'common_2.gni'), 'use_remoteexec=true') + + lines = list(gn_helper.lines(out_dir)) + + # The test will only pass if both imports work and + # 'use_remoteexec=true' is seen. + self.assertListEqual(lines, [ + 'use_remoteexec=true', + ]) + + def test_args(self): + out_dir = os.path.join('out', 'dir') + # Make sure nested import directives work. This is based on the + # reclient test. + write(os.path.join(out_dir, 'args.gn'), 'import("//out/common.gni")') + write(os.path.join('out', 'common.gni'), 'import("common_2.gni")') + write(os.path.join('out', 'common_2.gni'), 'use_remoteexec=true') + + lines = list(gn_helper.args(out_dir)) + + # The test will only pass if both imports work and + # 'use_remoteexec=true' is seen. + self.assertListEqual(lines, [ + ('use_remoteexec', 'true'), + ]) + + +if __name__ == '__main__': + unittest.main()