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 <dpranke@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>

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))

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')

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
-

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): ')

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',

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'