From 48d8e90bd45c46e2e859497fa5e427434d7bbe34 Mon Sep 17 00:00:00 2001 From: Josip Sokcevic Date: Thu, 9 Mar 2023 02:38:26 +0000 Subject: [PATCH] Revert "Reland "Remove Python 2 support for PRESUBMIT.py"" This reverts commit e9ece0f581d5cb89de5b396ee02def97ca919ee2. Reason for revert: broke infra presubmits: https://ci.chromium.org/p/infra/builders/try/infra-try-presubmit Original change's description: > Reland "Remove Python 2 support for PRESUBMIT.py" > > This is a reland of commit 8454fc2458b2421e0e339714c6ff7e6fffb70dc4 > > Original change's description: > > Remove Python 2 support for PRESUBMIT.py > > > > The presubmit system still supports invoking PRESUBMIT.py files using > > Python 2. This has recently been turned off on the bots so this change > > removes support more completely. > > > > There are still some python3 parameters being passed around - it seemed > > better to do the simplest possible removal now, with a follow-up change > > to remove more support code after this has sat for a while. > > > > Tests run from PRESUBMIT.py files could still be run using Python 2, but > > those should also have been addressed already. Removing support for that > > will be done in a subsequent change. > > > > Bug: 1207012 > > Change-Id: Id244d547a04438f83734dba269c3cc180c148b37 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4315183 > > Reviewed-by: Aravind Vasudevan > > Commit-Queue: Bruce Dawson > > Bug: 1207012 > Change-Id: If542cac21d8ec8704b28d03fd8407c5c2899ca2c > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4317177 > Reviewed-by: Aravind Vasudevan > Commit-Queue: Aravind Vasudevan > Auto-Submit: Josip Sokcevic Bug: 1207012 Change-Id: Iebf76d9e2580761fc773791bac07439160503c98 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4323198 Commit-Queue: Rubber Stamper Auto-Submit: Josip Sokcevic Bot-Commit: Rubber Stamper --- git_cl.py | 42 ++++++++++++++++++++++----- presubmit_support.py | 58 ++++++++++++++++++++++++++++++++----- tests/PRESUBMIT.py | 27 +++++++++++++++++ tests/git_cl_test.py | 44 ++++++++++++++++++++++++++++ tests/presubmit_unittest.py | 36 +++++++++++++++++++---- 5 files changed, 188 insertions(+), 19 deletions(-) create mode 100644 tests/PRESUBMIT.py diff --git a/git_cl.py b/git_cl.py index dae123dec..51a57085f 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1435,11 +1435,21 @@ class Changelist(object): ' was not specified. To enable ResultDB, please run the command' ' again with the --realm argument to specify the LUCI realm.') - return self._RunPresubmit(args, - description, - use_python3=True, - resultdb=resultdb, - realm=realm) + py3_results = self._RunPresubmit(args, + description, + use_python3=True, + resultdb=resultdb, + realm=realm) + 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, + description, + use_python3=False, + resultdb=resultdb, + realm=realm) + return self._MergePresubmitResults(py2_results, py3_results) def _RunPresubmit(self, args, @@ -1479,6 +1489,19 @@ class Changelist(object): json_results = gclient_utils.FileRead(json_output) return json.loads(json_results) + def _MergePresubmitResults(self, py2_results, py3_results): + return { + 'more_cc': sorted(set(py2_results.get('more_cc', []) + + py3_results.get('more_cc', []))), + 'errors': ( + py2_results.get('errors', []) + py3_results.get('errors', [])), + 'notifications': ( + py2_results.get('notifications', []) + + py3_results.get('notifications', [])), + 'warnings': ( + py2_results.get('warnings', []) + py3_results.get('warnings', [])) + } + def RunPostUploadHook(self, verbose, upstream, description, py3_only): args = self._GetCommonPresubmitArgs(verbose, upstream) args.append('--post_upload') @@ -1486,8 +1509,13 @@ class Changelist(object): with gclient_utils.temporary_file() as description_file: gclient_utils.FileWrite(description_file, description) args.extend(['--description_file', description_file]) - subprocess2.Popen(['vpython3', PRESUBMIT_SUPPORT] + args + - ['--use-python3']).wait() + if not py3_only: + p_py2 = subprocess2.Popen(['vpython', PRESUBMIT_SUPPORT] + args) + p_py3 = subprocess2.Popen(['vpython3', PRESUBMIT_SUPPORT] + args + + ['--use-python3']) + if not py3_only: + p_py2.wait() + p_py3.wait() def _GetDescriptionForUpload(self, options, git_diff_args, files): # type: (optparse.Values, Sequence[str], Sequence[str] diff --git a/presubmit_support.py b/presubmit_support.py index 43d815163..fe6e819aa 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -303,6 +303,33 @@ def prompt_should_continue(prompt_string): response = sys.stdin.readline().strip().lower() return response in ('y', 'yes') + +def _ShouldRunPresubmit(script_text, use_python3): + """Try to figure out whether these presubmit checks should be run under + python2 or python3. We need to do this without actually trying to + compile the text, since the text might compile in one but not the + other. + + Args: + script_text: The text of the presubmit script. + use_python3: if true, will use python3 instead of python2 by default + if USE_PYTHON3 is not specified. + + Return: + A boolean if presubmit should be executed + """ + if os.getenv('LUCI_OMIT_PYTHON2') == 'true': + # If LUCI omits python2, run all presubmits with python3, regardless of + # USE_PYTHON3 variable. + return True + + m = re.search('^USE_PYTHON3 = (True|False)$', script_text, flags=re.MULTILINE) + if m: + use_python3 = m.group(1) == 'True' + + return ((sys.version_info.major == 2) and not use_python3) or \ + ((sys.version_info.major == 3) and use_python3) + # Top level object so multiprocessing can pickle # Public access through OutputApi object. class _PresubmitResult(object): @@ -1423,9 +1450,14 @@ def DoPostUploadExecuter(change, gerrit_obj, verbose, use_python3=False): filename = os.path.abspath(filename) # Accept CRLF presubmit script. presubmit_script = gclient_utils.FileRead(filename).replace('\r\n', '\n') - if verbose: - sys.stdout.write('Running %s\n' % filename) - results.extend(executer.ExecPresubmitScript(presubmit_script, filename)) + if _ShouldRunPresubmit(presubmit_script, use_python3): + if sys.version_info[0] == 2: + sys.stdout.write( + 'Running %s under Python 2. Add USE_PYTHON3 = True to prevent ' + 'this.\n' % filename) + elif verbose: + sys.stdout.write('Running %s\n' % filename) + results.extend(executer.ExecPresubmitScript(presubmit_script, filename)) if not results: return 0 @@ -1700,18 +1732,29 @@ def DoPresubmitChecks(change, executer = PresubmitExecuter(change, committing, verbose, gerrit_obj, dry_run, thread_pool, parallel, use_python3, no_diffs) + 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) # Accept CRLF presubmit script. presubmit_script = gclient_utils.FileRead(filename).replace('\r\n', '\n') - if verbose: - sys.stdout.write('Running %s\n' % filename) - results += executer.ExecPresubmitScript(presubmit_script, filename) + if _ShouldRunPresubmit(presubmit_script, use_python3): + if sys.version_info[0] == 2: + sys.stdout.write( + 'Running %s under Python 2. Add USE_PYTHON3 = True to prevent ' + 'this.\n' % filename) + elif verbose: + sys.stdout.write('Running %s\n' % filename) + results += executer.ExecPresubmitScript(presubmit_script, filename) + else: + skipped_count += 1 results += thread_pool.RunAsync() @@ -1782,6 +1825,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/PRESUBMIT.py b/tests/PRESUBMIT.py new file mode 100644 index 000000000..5c0a3bd4b --- /dev/null +++ b/tests/PRESUBMIT.py @@ -0,0 +1,27 @@ +# Copyright (c) 2021 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 sys + +PRESUBMIT_VERSION = '2.0.0' + +# This file can be removed once py2 presubmit is no longer supported. This is +# an integration test to ensure py2 presubmit still works. + + +def CheckPythonVersion(input_api, output_api): + # The tests here are assuming this is not defined, so raise an error + # if it is. + if 'USE_PYTHON3' in globals(): + return [ + output_api.PresubmitError( + 'USE_PYTHON3 is defined; update the tests in //PRESUBMIT.py and ' + '//tests/PRESUBMIT.py.') + ] + if sys.version_info.major != 2: + return [ + output_api.PresubmitError( + 'Did not use Python2 for //PRESUBMIT.py by default.') + ] + return [] diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index b01d05b95..a2ce1054e 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -3690,6 +3690,25 @@ class ChangelistTest(unittest.TestCase): '--json_output', '/tmp/fake-temp2', '--description_file', '/tmp/fake-temp1', ]) + subprocess2.Popen.assert_any_call([ + 'vpython', 'PRESUBMIT_SUPPORT', + '--root', 'root', + '--upstream', 'upstream', + '--verbose', '--verbose', + '--gerrit_url', 'https://chromium-review.googlesource.com', + '--gerrit_project', 'project', + '--gerrit_branch', 'refs/heads/main', + '--author', 'author', + '--issue', '123456', + '--patchset', '7', + '--commit', + '--may_prompt', + '--parallel', + '--all_files', + '--no_diffs', + '--json_output', '/tmp/fake-temp4', + '--description_file', '/tmp/fake-temp3', + ]) gclient_utils.FileWrite.assert_any_call( '/tmp/fake-temp1', 'description') metrics.collector.add_repeated('sub_commands', { @@ -3861,6 +3880,31 @@ class ChangelistTest(unittest.TestCase): cl = git_cl.Changelist() cl.RunPostUploadHook(2, 'upstream', 'description', False) + subprocess2.Popen.assert_any_call([ + 'vpython', + 'PRESUBMIT_SUPPORT', + '--root', + 'root', + '--upstream', + 'upstream', + '--verbose', + '--verbose', + '--gerrit_url', + 'https://chromium-review.googlesource.com', + '--gerrit_project', + 'project', + '--gerrit_branch', + 'refs/heads/main', + '--author', + 'author', + '--issue', + '123456', + '--patchset', + '7', + '--post_upload', + '--description_file', + '/tmp/fake-temp1', + ]) subprocess2.Popen.assert_called_with([ 'vpython3', 'PRESUBMIT_SUPPORT', diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 2583a5070..5a1ee9dfb 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -164,7 +164,10 @@ index fe3de7b..54ae6e1 100755 # limit set. self.maxDiff = None - self.presubmit_text = self.presubmit_text + # TODO: remove once py2 no longer supported + self.presubmit_text_prefix = ('USE_PYTHON3 = ' + + str(sys.version_info.major == 3) + '\n') + self.presubmit_text = self.presubmit_text_prefix + self.presubmit_text class FakeChange(object): def __init__(self, obj): @@ -440,6 +443,7 @@ class PresubmitUnittest(PresubmitTestsBase): # No error if no on-upload entry point self.assertFalse( executer.ExecPresubmitScript( + self.presubmit_text_prefix + ('def CheckChangeOnCommit(input_api, output_api):\n' ' return (output_api.PresubmitError("!!"))\n'), fake_presubmit)) @@ -448,11 +452,13 @@ class PresubmitUnittest(PresubmitTestsBase): # No error if no on-commit entry point self.assertFalse( executer.ExecPresubmitScript( + self.presubmit_text_prefix + ('def CheckChangeOnUpload(input_api, output_api):\n' ' return (output_api.PresubmitError("!!"))\n'), fake_presubmit)) self.assertFalse( executer.ExecPresubmitScript( + self.presubmit_text_prefix + ('def CheckChangeOnUpload(input_api, output_api):\n' ' if not input_api.change.BugsFromDescription():\n' ' return (output_api.PresubmitError("!!"))\n' @@ -461,6 +467,7 @@ class PresubmitUnittest(PresubmitTestsBase): self.assertFalse( executer.ExecPresubmitScript( + self.presubmit_text_prefix + 'def CheckChangeOnCommit(input_api, output_api):\n' ' results = []\n' ' results.extend(input_api.canned_checks.CheckChangeHasBugField(\n' @@ -485,6 +492,7 @@ class PresubmitUnittest(PresubmitTestsBase): # STATUS_PASS on success executer.ExecPresubmitScript( + self.presubmit_text_prefix + 'def CheckChangeOnCommit(input_api, output_api):\n' ' return [output_api.PresubmitResult("test")]\n', fake_presubmit) sink.report.assert_called_with('CheckChangeOnCommit', @@ -493,6 +501,7 @@ class PresubmitUnittest(PresubmitTestsBase): # STATUS_FAIL on fatal error sink.reset_mock() executer.ExecPresubmitScript( + self.presubmit_text_prefix + 'def CheckChangeOnCommit(input_api, output_api):\n' ' return [output_api.PresubmitError("error")]\n', fake_presubmit) sink.report.assert_called_with('CheckChangeOnCommit', @@ -510,12 +519,14 @@ class PresubmitUnittest(PresubmitTestsBase): self.assertEqual([], executer.ExecPresubmitScript( + self.presubmit_text_prefix + ('def CheckChangeOnUpload(input_api, output_api):\n' ' if len(input_api._named_temporary_files):\n' ' return (output_api.PresubmitError("!!"),)\n' ' return ()\n'), fake_presubmit)) result = executer.ExecPresubmitScript( + self.presubmit_text_prefix + ('def CheckChangeOnUpload(input_api, output_api):\n' ' with input_api.CreateTemporaryFile():\n' ' pass\n' @@ -541,7 +552,7 @@ class PresubmitUnittest(PresubmitTestsBase): executer = presubmit.PresubmitExecuter(change, False, None, presubmit.GerritAccessor()) - executer.ExecPresubmitScript('', fake_presubmit) + executer.ExecPresubmitScript(self.presubmit_text_prefix, fake_presubmit) # Check that the executer switched to the directory of the script and back. self.assertEqual(os.chdir.call_args_list, [ @@ -581,6 +592,11 @@ class PresubmitUnittest(PresubmitTestsBase): change=change, gerrit_obj=None, verbose=False)) expected = (r'Running Python ' + str(sys.version_info.major) + r' post upload checks \.\.\.\n') + if sys.version_info[0] == 2: + expected += ('Running .*PRESUBMIT.py under Python 2. Add USE_PYTHON3 = ' + 'True to prevent this.\n') + expected += ('Running .*PRESUBMIT.py under Python 2. Add USE_PYTHON3 = ' + 'True to prevent this.\n') self.assertRegexpMatches(sys.stdout.getvalue(), expected) def testDoPostUploadExecuterWarning(self): @@ -612,12 +628,17 @@ class PresubmitUnittest(PresubmitTestsBase): presubmit.DoPostUploadExecuter( change=change, gerrit_obj=None, verbose=False)) + extra = '' + if sys.version_info[0] == 2: + extra = ('Running .*PRESUBMIT.py under Python 2. Add USE_PYTHON3 = True ' + 'to prevent this.\n') expected = ('Running Python ' + str(sys.version_info.major) + ' ' 'post upload checks \.\.\.\n' + '%s' '\n' '\*\* Post Upload Hook Messages \*\*\n' '!!\n' - '\n') + '\n' % extra) self.assertRegexpMatches(sys.stdout.getvalue(), expected) def testDoPresubmitChecksNoWarningsOrErrors(self): @@ -656,7 +677,8 @@ class PresubmitUnittest(PresubmitTestsBase): fake_notify = 'This is a dry run' fake_notify_items = '["N"]' fake_notify_long_text = 'Notification long text...' - always_fail_presubmit_script = ("""\n + always_fail_presubmit_script = ('USE_PYTHON3 = ' + + str(sys.version_info.major == 3) + """\n def CheckChangeOnUpload(input_api, output_api): output_api.more_cc = ['me@example.com'] return [ @@ -712,6 +734,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) @@ -811,7 +834,8 @@ def CheckChangeOnCommit(input_api, output_api): self.assertEqual(sys.stdout.getvalue().count(RUNNING_PY_CHECKS_TEXT), 1) def testDoDefaultPresubmitChecksAndFeedback(self): - always_fail_presubmit_script = ("""\n + always_fail_presubmit_script = ('USE_PYTHON3 = ' + + str(sys.version_info.major == 3) + """\n def CheckChangeOnUpload(input_api, output_api): return [output_api.PresubmitError("!!")] def CheckChangeOnCommit(input_api, output_api): @@ -888,6 +912,7 @@ def CheckChangeOnCommit(input_api, output_api): os.path.isfile.side_effect = lambda f: 'PRESUBMIT.py' in f os.listdir.return_value = ['PRESUBMIT.py'] gclient_utils.FileRead.return_value = ( + 'USE_PYTHON3 = ' + str(sys.version_info.major == 3) + '\n' 'def PostUploadHook(gerrit, change, output_api):\n' ' return ()\n') scm.determine_scm.return_value = None @@ -912,6 +937,7 @@ def CheckChangeOnCommit(input_api, output_api): @mock.patch('lib.utils.ListRelevantFilesInSourceCheckout') def testMainUnversionedChecksFail(self, *_mocks): gclient_utils.FileRead.return_value = ( + 'USE_PYTHON3 = ' + str(sys.version_info.major == 3) + '\n' 'def CheckChangeOnUpload(input_api, output_api):\n' ' return [output_api.PresubmitError("!!")]\n') scm.determine_scm.return_value = None