From 30c1cba02fa10a5d23077e9e037dbcf9c177dc3d Mon Sep 17 00:00:00 2001 From: Bruce Dawson Date: Fri, 15 Sep 2023 18:20:32 +0000 Subject: [PATCH] Handle import statements in args.gn files Did you know that args.gn files can have import statements and conditionals? I did not, but apparently some developers make use of both of these. Supporting import statements is not too hard, so this change adds this support. Supporting conditionals is possible, but risks turning autoninja into a turing complete language which is more than I think we want to do. This doesn't use the similar code in tools/mb/mb.py because that code is complex, and relies on the script location to find the src directory. This change also updates two of the existing test conditionals that were not quite sufficient - ninja/autoninja default to num-cores plus 2 so > cpu_count() is actually not sufficient to prove anything. Bug: 1482404 Change-Id: I0539d8068af59d11927cbdad260278a24ab912e6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4864898 Reviewed-by: Takuto Ikuta Commit-Queue: Bruce Dawson --- autoninja.py | 78 ++++++++++++++++++++++++++--------------- tests/autoninja_test.py | 27 ++++++++++++-- 2 files changed, 75 insertions(+), 30 deletions(-) diff --git a/autoninja.py b/autoninja.py index 8a2c2672a..34db3c359 100755 --- a/autoninja.py +++ b/autoninja.py @@ -8,6 +8,10 @@ whether a build is accelerated using a service like goma. If so, it runs with a large -j value, and otherwise it chooses a small one. This auto-adjustment makes using remote build acceleration simpler and safer, and avoids errors that can cause slow goma builds or swap-storms on unaccelerated builds. + +autoninja tries to detect relevant build settings such as use_remoteexec, and it +does handle import statements, but it can't handle conditional setting of build +settings. """ import multiprocessing @@ -23,6 +27,25 @@ if sys.platform in ['darwin', 'linux']: SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) +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: + import_path = os.path.normpath( + os.path.join(output_dir, '..', '..', + match.groups()[0][2:])) + for import_line in _gn_lines(output_dir, import_path): + yield import_line + else: + yield line + + def main(args): # The -t tools are incompatible with -j t_specified = False @@ -71,33 +94,32 @@ def main(args): # 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')): - with open(os.path.join(output_dir, 'args.gn')) as file_handle: - for line in file_handle: - # use_goma, use_remoteexec, or use_rbe will activate build - # acceleration. - # - # This test can match multi-argument lines. Examples of this - # are: is_debug=false use_goma=true is_official_build=false - # use_goma=false# use_goma=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_goma)\s*=\s*true($|\s)', - line_without_comment): - use_goma = True - continue - if re.search(r'(^|\s)(use_remoteexec)\s*=\s*true($|\s)', - line_without_comment): - use_remoteexec = True - continue - if re.search(r'(^|\s)(use_rbe)\s*=\s*true($|\s)', - line_without_comment): - use_rbe = True - continue - if re.search(r'(^|\s)(use_siso)\s*=\s*true($|\s)', - line_without_comment): - use_siso = True - continue + for line in _gn_lines(output_dir, os.path.join(output_dir, 'args.gn')): + # use_goma, use_remoteexec, or use_rbe will activate build + # acceleration. + # + # This test can match multi-argument lines. Examples of this + # are: is_debug=false use_goma=true is_official_build=false + # use_goma=false# use_goma=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_goma)\s*=\s*true($|\s)', + line_without_comment): + use_goma = True + continue + if re.search(r'(^|\s)(use_remoteexec)\s*=\s*true($|\s)', + line_without_comment): + use_remoteexec = True + continue + if re.search(r'(^|\s)(use_rbe)\s*=\s*true($|\s)', + line_without_comment): + use_rbe = True + continue + if re.search(r'(^|\s)(use_siso)\s*=\s*true($|\s)', + line_without_comment): + use_siso = True + continue siso_marker = os.path.join(output_dir, '.siso_deps') if use_siso: @@ -130,7 +152,7 @@ def main(args): ]: path = os.path.join(output_dir, relative_path, 'rules.ninja') if os.path.exists(path): - with open(path) as file_handle: + with open(path, encoding='utf-8') as file_handle: for line in file_handle: if re.match(r'^\s*command\s*=\s*\S+gomacc', line): use_goma = True diff --git a/tests/autoninja_test.py b/tests/autoninja_test.py index af2fdb833..37f303b2e 100755 --- a/tests/autoninja_test.py +++ b/tests/autoninja_test.py @@ -57,7 +57,7 @@ class AutoninjaTest(trial_dir.TestCase): self.assertIn('-j', args) parallel_j = int(args[args.index('-j') + 1]) - self.assertGreater(parallel_j, multiprocessing.cpu_count()) + self.assertGreater(parallel_j, multiprocessing.cpu_count() * 2) self.assertIn(os.path.join(autoninja.SCRIPT_DIR, 'ninja.py'), args) def test_autoninja_reclient(self): @@ -73,7 +73,30 @@ class AutoninjaTest(trial_dir.TestCase): self.assertIn('-j', args) parallel_j = int(args[args.index('-j') + 1]) - self.assertGreater(parallel_j, multiprocessing.cpu_count()) + self.assertGreater(parallel_j, multiprocessing.cpu_count() * 2) + self.assertIn(os.path.join(autoninja.SCRIPT_DIR, 'ninja_reclient.py'), + args) + + def test_autoninja_import(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("//out/common_2.gni")') + # The test will only pass if both imports work and + # 'use_remoteexec=true' is seen. + write(os.path.join('out', 'common_2.gni'), '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() * 2) self.assertIn(os.path.join(autoninja.SCRIPT_DIR, 'ninja_reclient.py'), args)