Revert "Reland "Remove Python 2 support for PRESUBMIT.py""

This reverts commit e9ece0f581.

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 8454fc2458
>
> 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 <aravindvasudev@google.com>
> > Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
>
> Bug: 1207012
> Change-Id: If542cac21d8ec8704b28d03fd8407c5c2899ca2c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4317177
> Reviewed-by: Aravind Vasudevan <aravindvasudev@google.com>
> Commit-Queue: Aravind Vasudevan <aravindvasudev@google.com>
> Auto-Submit: Josip Sokcevic <sokcevic@chromium.org>

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 <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Josip Sokcevic <sokcevic@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>

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]

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(

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 []

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

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