From 632bbc0cb1ea9d5f0d46291c1f5cc735393321f6 Mon Sep 17 00:00:00 2001 From: Josip Sokcevic Date: Thu, 19 May 2022 05:32:50 +0000 Subject: [PATCH] Skip Python 2 presubmit step when unneeded All of the PRESUBMIT.py files in the Chromium repo are running under Python 3. However "git cl presubmit" also works with other repos where some PRESUBMIT.py scripts still run under Python 2. This means that the Python 2 presubmit commit checks step cannot simply be disabled. That meant that Chromium was paying up to a one-minute cost just to setup for and look for Python 2 scripts that it doesn't run. This change runs the Python 3 PRESUBMIT.py scripts first, and keeps track of whether any were skipped. If none were skipped then the Python 2 PRESUBMIT.py stage can be skipped. Note that the child scripts of PRESUBMIT.py scripts may still be run under Python 2, but that is orthogonal to this change. Bug: 1313804 Change-Id: Ib65838223f232f1e78058d6a08ea15a89f442310 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3614453 Reviewed-by: Josip Sokcevic Reviewed-by: Bruce Dawson Commit-Queue: Bruce Dawson --- git_cl.py | 8 ++++++-- presubmit_support.py | 28 ++++++++++++++++++---------- tests/git_cl_test.py | 8 ++++---- tests/presubmit_unittest.py | 1 + 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/git_cl.py b/git_cl.py index 29674c9e81..eebf734e65 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1370,10 +1370,14 @@ class Changelist(object): ' was not specified. To enable ResultDB, please run the command' ' again with the --realm argument to specify the LUCI realm.') - py2_results = self._RunPresubmit(args, resultdb, realm, description, - use_python3=False) py3_results = self._RunPresubmit(args, resultdb, realm, description, use_python3=True) + if py3_results.get('skipped_presubmits', 1) == 0: + print('No more presubmits to run - skipping Python 2 presubmits.') + return py3_results + + py2_results = self._RunPresubmit(args, resultdb, realm, description, + use_python3=False) return self._MergePresubmitResults(py2_results, py3_results) def _RunPresubmit(self, args, resultdb, realm, description, use_python3): diff --git a/presubmit_support.py b/presubmit_support.py index dbfcc80af8..d9ae4cae9b 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -1402,6 +1402,8 @@ class GetPostUploadExecuter(object): def ExecPresubmitScript(self, script_text, presubmit_path, gerrit_obj, change): """Executes PostUploadHook() from a single presubmit script. + Caller is responsible for validating whether the hook should be executed + and should only call this function if it should be. Args: script_text: The text of the presubmit script. @@ -1412,9 +1414,6 @@ class GetPostUploadExecuter(object): Return: A list of results objects. """ - if not _ShouldRunPresubmit(script_text, self.use_python3): - return {} - context = {} try: exec(compile(script_text, 'PRESUBMIT.py', 'exec', dont_inherit=True), @@ -1473,8 +1472,9 @@ def DoPostUploadExecuter(change, gerrit_obj, verbose, use_python3=False): sys.stdout.write('Running %s\n' % filename) # Accept CRLF presubmit script. presubmit_script = gclient_utils.FileRead(filename, 'rU') - results.extend(executer.ExecPresubmitScript( - presubmit_script, filename, gerrit_obj, change)) + if _ShouldRunPresubmit(presubmit_script, use_python3): + results.extend(executer.ExecPresubmitScript( + presubmit_script, filename, gerrit_obj, change)) if not results: return 0 @@ -1517,6 +1517,8 @@ class PresubmitExecuter(object): def ExecPresubmitScript(self, script_text, presubmit_path): """Executes a single presubmit script. + Caller is responsible for validating whether the hook should be executed + and should only call this function if it should be. Args: script_text: The text of the presubmit script. @@ -1526,9 +1528,6 @@ class PresubmitExecuter(object): Return: A list of result objects, empty if no problems. """ - if not _ShouldRunPresubmit(script_text, self.use_python3): - return [] - # Change to the presubmit file's directory to support local imports. main_path = os.getcwd() presubmit_dir = os.path.dirname(presubmit_path) @@ -1720,18 +1719,26 @@ def DoPresubmitChecks(change, thread_pool = ThreadPool() executer = PresubmitExecuter(change, committing, verbose, gerrit_obj, dry_run, thread_pool, parallel, use_python3) + skipped_count = 0; if default_presubmit: if verbose: sys.stdout.write('Running default presubmit script.\n') fake_path = os.path.join(change.RepositoryRoot(), 'PRESUBMIT.py') - results += executer.ExecPresubmitScript(default_presubmit, fake_path) + if _ShouldRunPresubmit(default_presubmit, use_python3): + results += executer.ExecPresubmitScript(default_presubmit, fake_path) + else: + skipped_count += 1 for filename in presubmit_files: filename = os.path.abspath(filename) if verbose: sys.stdout.write('Running %s\n' % filename) # Accept CRLF presubmit script. presubmit_script = gclient_utils.FileRead(filename, 'rU') - results += executer.ExecPresubmitScript(presubmit_script, filename) + if _ShouldRunPresubmit(presubmit_script, use_python3): + results += executer.ExecPresubmitScript(presubmit_script, filename) + else: + skipped_count += 1 + results += thread_pool.RunAsync() messages = {} @@ -1784,6 +1791,7 @@ def DoPresubmitChecks(change, for warning in messages.get('Warnings', []) ], 'more_cc': executer.more_cc, + 'skipped_presubmits': skipped_count, } gclient_utils.FileWrite( diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 2ff2d90a95..6d9fa12e7a 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -3095,7 +3095,7 @@ class ChangelistTest(unittest.TestCase): self.assertEqual(expected_results, results) subprocess2.Popen.assert_any_call([ - 'vpython', 'PRESUBMIT_SUPPORT', + 'vpython3', 'PRESUBMIT_SUPPORT', '--root', 'root', '--upstream', 'upstream', '--verbose', '--verbose', @@ -3113,7 +3113,7 @@ class ChangelistTest(unittest.TestCase): '--description_file', '/tmp/fake-temp1', ]) subprocess2.Popen.assert_any_call([ - 'vpython3', 'PRESUBMIT_SUPPORT', + 'vpython', 'PRESUBMIT_SUPPORT', '--root', 'root', '--upstream', 'upstream', '--verbose', '--verbose', @@ -3168,7 +3168,7 @@ class ChangelistTest(unittest.TestCase): self.assertEqual(expected_results, results) subprocess2.Popen.assert_any_call([ - 'vpython', 'PRESUBMIT_SUPPORT', + 'vpython3', 'PRESUBMIT_SUPPORT', '--root', 'root', '--upstream', 'upstream', '--gerrit_url', 'https://chromium-review.googlesource.com', @@ -3218,7 +3218,7 @@ class ChangelistTest(unittest.TestCase): self.assertEqual(expected_results, results) subprocess2.Popen.assert_any_call([ 'rdb', 'stream', '-new', '-realm', 'chromium:public', '--', - 'vpython', 'PRESUBMIT_SUPPORT', + 'vpython3', 'PRESUBMIT_SUPPORT', '--root', 'root', '--upstream', 'upstream', '--gerrit_url', 'https://chromium-review.googlesource.com', diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 088f592d69..4f313c7582 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -778,6 +778,7 @@ def CheckChangeOnCommit(input_api, output_api): } ], 'more_cc': ['me@example.com'], + 'skipped_presubmits': 0, } fake_result_json = json.dumps(fake_result, sort_keys=True)