From 61bf6e8d69c4cb084b1541a996fc3f4990cd2535 Mon Sep 17 00:00:00 2001 From: Dirk Pranke Date: Fri, 23 Apr 2021 00:50:21 +0000 Subject: [PATCH] Add support for running PRESUBMIT.py checks under Python2 or 3. This CL adds support for running PRESUBMIT.py under either Python2 or Python3 as specified in each PRESUBMIT.py file. To run the checks under Python3, the PRESUBMIT.py file must contain a line exactly matching "^USE_PYTHON3 = True$". If the file does not contain this string, the checks will run under Python2. Different PRESUBMIT.py files in a single CL may thus contain a mix of 2- and 3-compatible checks, but each individual file will only be run in one or the other (it doesn't likely make sense to run them in both by default). Bug: 1157663 Change-Id: Ic74977941a6519388089328b6e1dfba2e885924b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2832654 Commit-Queue: Dirk Pranke Reviewed-by: Josip Sokcevic --- PRESUBMIT.py | 27 ++++++++++++++----- git_cl.py | 38 ++++++++++++++++++++++----- presubmit_canned_checks.py | 2 +- presubmit_support.py | 21 ++++++++++++--- tests/git_cl_test.py | 52 +++++++++++++++++++++++++++---------- tests/presubmit_unittest.py | 10 +++---- 6 files changed, 113 insertions(+), 37 deletions(-) diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 83bad90ba4..ad837c9874 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -12,6 +12,12 @@ import fnmatch import os import sys +# Whether to run the checks under Python2 or Python3. +# TODO: Uncomment this to run the checks under Python3, and change the tests +# in _CommonChecks in this file and the values and tests in +# //tests/PRESUBMIT.py as well to keep the test coverage of all three cases +# (true, false, and default/not specified). +# USE_PYTHON3 = False # CIPD ensure manifest for checking CIPD client itself. CIPD_CLIENT_ENSURE_FILE_TEMPLATE = r''' @@ -61,10 +67,21 @@ def DepotToolsPylint(input_api, output_api): disabled_warnings=disabled_warnings) -def CommonChecks(input_api, output_api, tests_to_skip_list, run_on_python3): +def CommonChecks(input_api, output_api, tests_to_skip_list): input_api.SetTimeout(TEST_TIMEOUT_S) results = [] + + # The tests here are assuming this is not defined, so raise an error + # if it is. + if 'USE_PYTHON3' in globals(): + results.append(output_api.PresubmitError( + 'USE_PYTHON3 is defined; update the tests in //PRESUBMIT.py and ' + '//tests/PRESUBMIT.py.')) + elif sys.version_info.major != 2: + results.append(output_api.PresubmitError( + 'Did not use Python2 for //PRESUBMIT.py by default.')) + results.extend(input_api.canned_checks.CheckJsonParses( input_api, output_api)) @@ -90,7 +107,7 @@ def CommonChecks(input_api, output_api, tests_to_skip_list, run_on_python3): 'tests', files_to_check=test_to_run_list, files_to_skip=tests_to_skip_list, - run_on_python3=run_on_python3)) + run_on_python3=False)) # Validate CIPD manifests. root = input_api.os_path.normpath( @@ -138,15 +155,13 @@ def CheckChangeOnUpload(input_api, output_api): input_api, output_api, allow_tbr=False)) results.extend(input_api.canned_checks.CheckOwnersFormat( input_api, output_api)) - # TODO(ehmaldonado): Run Python 3 tests on upload once Python 3 is - # bootstrapped on Linux and Mac. - results.extend(CommonChecks(input_api, output_api, tests_to_skip_list, False)) + results.extend(CommonChecks(input_api, output_api, tests_to_skip_list)) return results def CheckChangeOnCommit(input_api, output_api): output = [] - output.extend(CommonChecks(input_api, output_api, [], True)) + output.extend(CommonChecks(input_api, output_api, [])) output.extend(input_api.canned_checks.CheckDoNotSubmit( input_api, output_api)) diff --git a/git_cl.py b/git_cl.py index 5482382478..87904a900e 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1321,6 +1321,23 @@ class Changelist(object): if all_files: args.append('--all_files') + if resultdb and not realm: + # TODO (crbug.com/1113463): store realm somewhere and look it up so + # it is not required to pass the realm flag + print('Note: ResultDB reporting will NOT be performed because --realm' + ' 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) + return self._MergePresubmitResults(py2_results, py3_results) + + def _RunPresubmit(self, args, resultdb, realm, description, use_python3): + args = args[:] + vpython = 'vpython3' if use_python3 else 'vpython' + with gclient_utils.temporary_file() as description_file: with gclient_utils.temporary_file() as json_output: gclient_utils.FileWrite(description_file, description) @@ -1328,15 +1345,9 @@ class Changelist(object): args.extend(['--description_file', description_file]) start = time_time() - cmd = ['vpython', PRESUBMIT_SUPPORT] + args + cmd = [vpython, PRESUBMIT_SUPPORT] + args if resultdb and realm: cmd = ['rdb', 'stream', '-new', '-realm', realm, '--'] + cmd - elif resultdb: - # TODO (crbug.com/1113463): store realm somewhere and look it up so - # it is not required to pass the realm flag - print('Note: ResultDB reporting will NOT be performed because --realm' - ' was not specified. To enable ResultDB, please run the command' - ' again with the --realm argument to specify the LUCI realm.') p = subprocess2.Popen(cmd) exit_code = p.wait() @@ -1353,6 +1364,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): args = self._GetCommonPresubmitArgs(verbose, upstream) args.append('--post_upload') diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 31d904caa1..be81f6f07c 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -7,6 +7,7 @@ from __future__ import print_function import os as _os + from warnings import warn _HERE = _os.path.dirname(_os.path.abspath(__file__)) @@ -1847,4 +1848,3 @@ def CheckInclusiveLanguage(input_api, output_api, output_api.PresubmitError('Banned non-inclusive language was used.\n' + '\n'.join(errors))) return result - diff --git a/presubmit_support.py b/presubmit_support.py index f5202a3077..e2c279a219 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -1564,6 +1564,16 @@ class PresubmitExecuter(object): output_api = OutputApi(self.committing) context = {} + # 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. + m = re.search('^USE_PYTHON3 = True$', script_text, flags=re.MULTILINE) + use_python3 = m is not None + if (((sys.version_info.major == 2) and use_python3) or + ((sys.version_info.major == 3) and not use_python3)): + return [] + try: exec(compile(script_text, 'PRESUBMIT.py', 'exec', dont_inherit=True), context) @@ -1712,10 +1722,13 @@ def DoPresubmitChecks(change, os.environ = os.environ.copy() os.environ['PYTHONDONTWRITEBYTECODE'] = '1' + python_version = 'Python %s' % sys.version_info.major if committing: - sys.stdout.write('Running presubmit commit checks ...\n') + sys.stdout.write('Running %s presubmit commit checks ...\n' % + python_version) else: - sys.stdout.write('Running presubmit upload checks ...\n') + sys.stdout.write('Running %s presubmit upload checks ...\n' % + python_version) start_time = time_time() presubmit_files = ListRelevantPresubmitFiles( change.AbsoluteLocalPaths(), change.RepositoryRoot()) @@ -1765,9 +1778,9 @@ def DoPresubmitChecks(change, 'Presubmit checks took %.1fs to calculate.\n\n' % total_time) if not should_prompt and not presubmits_failed: - sys.stdout.write('Presubmit checks passed.\n') + sys.stdout.write('%s presubmit checks passed.\n' % python_version) elif should_prompt: - sys.stdout.write('There were presubmit warnings. ') + sys.stdout.write('There were %s presubmit warnings. ' % python_version) if may_prompt: presubmits_failed = not prompt_should_continue( 'Are you sure you wish to continue? (y/N): ') diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 74234cf9fa..623bdccf23 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -3023,11 +3023,13 @@ class ChangelistTest(unittest.TestCase): def testRunHook(self): expected_results = { - 'more_cc': ['more@example.com', 'cc@example.com'], - 'should_continue': True, + 'more_cc': ['cc@example.com', 'more@example.com'], + 'errors': [], + 'notifications': [], + 'warnings': [], } gclient_utils.FileRead.return_value = json.dumps(expected_results) - git_cl.time_time.side_effect = [100, 200] + git_cl.time_time.side_effect = [100, 200, 300, 400] mockProcess = mock.Mock() mockProcess.wait.return_value = 0 subprocess2.Popen.return_value = mockProcess @@ -3044,7 +3046,7 @@ class ChangelistTest(unittest.TestCase): resultdb=False) self.assertEqual(expected_results, results) - subprocess2.Popen.assert_called_once_with([ + subprocess2.Popen.assert_any_call([ 'vpython', 'PRESUBMIT_SUPPORT', '--root', 'root', '--upstream', 'upstream', @@ -3062,7 +3064,25 @@ class ChangelistTest(unittest.TestCase): '--json_output', '/tmp/fake-temp2', '--description_file', '/tmp/fake-temp1', ]) - gclient_utils.FileWrite.assert_called_once_with( + subprocess2.Popen.assert_any_call([ + 'vpython3', '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', + '--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', { 'command': 'presubmit', @@ -3072,11 +3092,13 @@ class ChangelistTest(unittest.TestCase): def testRunHook_FewerOptions(self): expected_results = { - 'more_cc': ['more@example.com', 'cc@example.com'], - 'should_continue': True, + 'more_cc': ['cc@example.com', 'more@example.com'], + 'errors': [], + 'notifications': [], + 'warnings': [], } gclient_utils.FileRead.return_value = json.dumps(expected_results) - git_cl.time_time.side_effect = [100, 200] + git_cl.time_time.side_effect = [100, 200, 300, 400] mockProcess = mock.Mock() mockProcess.wait.return_value = 0 subprocess2.Popen.return_value = mockProcess @@ -3097,7 +3119,7 @@ class ChangelistTest(unittest.TestCase): resultdb=False) self.assertEqual(expected_results, results) - subprocess2.Popen.assert_called_once_with([ + subprocess2.Popen.assert_any_call([ 'vpython', 'PRESUBMIT_SUPPORT', '--root', 'root', '--upstream', 'upstream', @@ -3108,7 +3130,7 @@ class ChangelistTest(unittest.TestCase): '--json_output', '/tmp/fake-temp2', '--description_file', '/tmp/fake-temp1', ]) - gclient_utils.FileWrite.assert_called_once_with( + gclient_utils.FileWrite.assert_any_call( '/tmp/fake-temp1', 'description') metrics.collector.add_repeated('sub_commands', { 'command': 'presubmit', @@ -3118,11 +3140,13 @@ class ChangelistTest(unittest.TestCase): def testRunHook_FewerOptionsResultDB(self): expected_results = { - 'more_cc': ['more@example.com', 'cc@example.com'], - 'should_continue': True, + 'more_cc': ['cc@example.com', 'more@example.com'], + 'errors': [], + 'notifications': [], + 'warnings': [], } gclient_utils.FileRead.return_value = json.dumps(expected_results) - git_cl.time_time.side_effect = [100, 200] + git_cl.time_time.side_effect = [100, 200, 300, 400] mockProcess = mock.Mock() mockProcess.wait.return_value = 0 subprocess2.Popen.return_value = mockProcess @@ -3144,7 +3168,7 @@ class ChangelistTest(unittest.TestCase): realm='chromium:public') self.assertEqual(expected_results, results) - subprocess2.Popen.assert_called_once_with([ + subprocess2.Popen.assert_any_call([ 'rdb', 'stream', '-new', '-realm', 'chromium:public', '--', 'vpython', 'PRESUBMIT_SUPPORT', '--root', 'root', diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 5263f2b39e..0ae7d80f58 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -687,7 +687,7 @@ class PresubmitUnittest(PresubmitTestsBase): self.assertEqual(sys.stdout.getvalue().count('!!'), 0) self.assertEqual(sys.stdout.getvalue().count('??'), 0) self.assertEqual(sys.stdout.getvalue().count( - 'Running presubmit upload checks ...\n'), 1) + 'Running Python 2 presubmit upload checks ...\n'), 1) def testDoPresubmitChecksJsonOutput(self): fake_error = 'Missing LGTM' @@ -808,7 +808,7 @@ def CheckChangeOnCommit(input_api, output_api): gerrit_obj=None, json_output=None)) self.assertEqual(sys.stdout.getvalue().count('??'), 2) self.assertEqual(sys.stdout.getvalue().count( - 'Running presubmit upload checks ...\n'), 1) + 'Running Python 2 presubmit upload checks ...\n'), 1) def testDoPresubmitChecksWithWarningsAndNoPrompt(self): presubmit_path = os.path.join(self.fake_root_dir, 'PRESUBMIT.py') @@ -834,7 +834,7 @@ def CheckChangeOnCommit(input_api, output_api): self.assertEqual(sys.stdout.getvalue().count('??'), 2) self.assertEqual(sys.stdout.getvalue().count('(y/N)'), 0) self.assertEqual(sys.stdout.getvalue().count( - 'Running presubmit upload checks ...\n'), 1) + 'Running Python 2 presubmit upload checks ...\n'), 1) def testDoPresubmitChecksNoWarningPromptIfErrors(self): presubmit_path = os.path.join(self.fake_root_dir, 'PRESUBMIT.py') @@ -858,7 +858,7 @@ def CheckChangeOnCommit(input_api, output_api): self.assertEqual(sys.stdout.getvalue().count('!!'), 2) self.assertEqual(sys.stdout.getvalue().count('(y/N)'), 0) self.assertEqual(sys.stdout.getvalue().count( - 'Running presubmit upload checks ...\n'), 1) + 'Running Python 2 presubmit upload checks ...\n'), 1) def testDoDefaultPresubmitChecksAndFeedback(self): always_fail_presubmit_script = """ @@ -883,7 +883,7 @@ def CheckChangeOnCommit(input_api, output_api): default_presubmit=always_fail_presubmit_script, may_prompt=False, gerrit_obj=None, json_output=None)) text = ( - 'Running presubmit upload checks ...\n' + 'Running Python 2 presubmit upload checks ...\n' 'Warning, no PRESUBMIT.py found.\n' 'Running default presubmit script.\n' '\n'