Remove Python 2 support for presubmit Commands

PRESUBMIT.py files can invoke other scripts, and those scripts may be
run under Python 2 or Python 3. There are multiple mechanisms that
govern which version will be used, sometimes requiring several flags
to be passed.

This change removes support for Python 2 from this system, thus making
it simpler to invoke Python 3 scripts.

The function parameters that are used to select Python versions are
passed in from multiple places so they still need to be supported, but
they are now ignored. The parameters are deleted to prevent accidental
use.

This change was tested by running this command in a Chromium repo:

  git cl presubmit --force --all

Bug: 1207012
Change-Id: I4adc7164417e155ff80d3a039cf36ed030756456
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4328470
Reviewed-by: Aravind Vasudevan <aravindvasudev@google.com>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>

presubmit_canned_checks.py

@@ -10,7 +10,6 @@ import io as _io
 import os as _os
 import zlib
 
-from warnings import warn
 _HERE = _os.path.dirname(_os.path.abspath(__file__))
 
 # These filters will be disabled if callers do not explicitly supply a
@@ -837,16 +836,22 @@ def GetUnitTestsInDirectory(input_api,
                             files_to_check=None,
                             files_to_skip=None,
                             env=None,
-                            run_on_python2=True,
+                            run_on_python2=False,
                             run_on_python3=True,
-                            skip_shebang_check=False,
+                            skip_shebang_check=True,
                             allowlist=None,
                             blocklist=None):
   """Lists all files in a directory and runs them. Doesn't recurse.
 
   It's mainly a wrapper for RunUnitTests. Use allowlist and blocklist to filter
-  tests accordingly.
+  tests accordingly. run_on_python2, run_on_python3, and skip_shebang_check are
+  no longer used but have to be retained because of the many callers in other
+  repos that pass them in.
   """
+  del run_on_python2
+  del run_on_python3
+  del skip_shebang_check
+
   unit_tests = []
   test_path = input_api.os_path.abspath(
       input_api.os_path.join(input_api.PresubmitLocalPath(), directory))
@@ -874,27 +879,26 @@ def GetUnitTestsInDirectory(input_api,
           'Out of %d files, found none that matched c=%r, s=%r in directory %s'
           % (found, files_to_check, files_to_skip, directory))
     ]
-  return GetUnitTests(input_api, output_api, unit_tests, env, run_on_python2,
-                      run_on_python3, skip_shebang_check)
+  return GetUnitTests(input_api, output_api, unit_tests, env)
 
 
 def GetUnitTests(input_api,
                  output_api,
                  unit_tests,
                  env=None,
-                 run_on_python2=True,
+                 run_on_python2=False,
                  run_on_python3=True,
-                 skip_shebang_check=False):
+                 skip_shebang_check=True):
   """Runs all unit tests in a directory.
 
   On Windows, sys.executable is used for unit tests ending with ".py".
+  run_on_python2, run_on_python3, and skip_shebang_check are no longer used but
+  have to be retained because of the many callers in other repos that pass them
+  in.
   """
-  assert run_on_python3 or run_on_python2, (
-      'At least one of "run_on_python2" or "run_on_python3" must be set.')
-  def has_py3_shebang(test):
-    with _io.open(test, encoding='utf-8') as f:
-      maybe_shebang = f.readline()
-    return maybe_shebang.startswith('#!') and 'python3' in maybe_shebang
+  del run_on_python2
+  del run_on_python3
+  del skip_shebang_check
 
   # We don't want to hinder users from uploading incomplete patches, but we do
   # want to report errors as errors when doing presubmit --all testing.
@@ -918,31 +922,11 @@ def GetUnitTests(input_api,
           kwargs=kwargs,
           message=message_type))
     else:
-      test_run = False
-      # TODO(crbug.com/1223478): The intent for this line was to run the test
-      # on python3 if the file has a shebang OR if it was explicitly requested
-      # to run on python3. Since tests have been broken since this landed, we
-      # introduced the |skip_shebang_check| argument to work around the issue
-      # until every caller in Chromium has been fixed.
-      if (skip_shebang_check or has_py3_shebang(unit_test)) and run_on_python3:
-        results.append(input_api.Command(
-            name=unit_test,
-            cmd=cmd,
-            kwargs=kwargs,
-            message=message_type,
-            python3=True))
-        test_run = True
-      if run_on_python2:
-        results.append(input_api.Command(
-            name=unit_test,
-            cmd=cmd,
-            kwargs=kwargs,
-            message=message_type))
-        test_run = True
-      if not test_run:
-        results.append(output_api.PresubmitError(
-            "The %s test was not run. You may need to add\n"
-            "skip_shebang_check=True for python3 tests." % unit_test))
+      results.append(
+          input_api.Command(name=unit_test,
+                            cmd=cmd,
+                            kwargs=kwargs,
+                            message=message_type))
   return results
 
 
@@ -951,14 +935,20 @@ def GetUnitTestsRecursively(input_api,
                             directory,
                             files_to_check,
                             files_to_skip,
-                            run_on_python2=True,
+                            run_on_python2=False,
                             run_on_python3=True,
-                            skip_shebang_check=False):
+                            skip_shebang_check=True):
   """Gets all files in the directory tree (git repo) that match files_to_check.
 
   Restricts itself to only find files within the Change's source repo, not
-  dependencies.
+  dependencies. run_on_python2, run_on_python3, and skip_shebang_check are no
+  longer used but have to be retained because of the many callers in other repos
+  that pass them in.
   """
+  del run_on_python2
+  del run_on_python3
+  del skip_shebang_check
+
   def check(filename):
     return (any(input_api.re.match(f, filename) for f in files_to_check) and
             not any(input_api.re.match(f, filename) for f in files_to_skip))
@@ -979,12 +969,7 @@ def GetUnitTestsRecursively(input_api,
           % (found, files_to_check, files_to_skip, directory))
     ]
 
-  return GetUnitTests(input_api,
-                      output_api,
-                      tests,
-                      run_on_python2=run_on_python2,
-                      run_on_python3=run_on_python3,
-                      skip_shebang_check=skip_shebang_check)
+  return GetUnitTests(input_api, output_api, tests)
 
 
 def GetPythonUnitTests(input_api, output_api, unit_tests, python3=False):
@@ -1026,10 +1011,7 @@ def GetPythonUnitTests(input_api, output_api, unit_tests, python3=False):
         backpath.append(env.get('PYTHONPATH'))
       env['PYTHONPATH'] = input_api.os_path.pathsep.join((backpath))
       env.pop('VPYTHON_CLEAR_PYTHONPATH', None)
-    if python3:
-      cmd = [input_api.python3_executable, '-m', '%s' % unit_test]
-    else:
-      cmd = [input_api.python_executable, '-m', '%s' % unit_test]
+    cmd = [input_api.python3_executable, '-m', '%s' % unit_test]
     results.append(input_api.Command(
         name=unit_test_name,
         cmd=cmd,
@@ -1116,8 +1098,7 @@ def GetPylint(input_api,
   files_to_skip = tuple(files_to_skip or input_api.DEFAULT_FILES_TO_SKIP)
   extra_paths_list = extra_paths_list or []
 
-  assert version in ('2.6', '2.7'), \
-      'Unsupported pylint version: %s' % version
+  assert version in ('2.6', '2.7'), 'Unsupported pylint version: %s' % version
 
   if input_api.is_committing or input_api.no_diffs:
     error_type = output_api.PresubmitError

presubmit_support.py

@@ -81,7 +81,10 @@ class PresubmitFailure(Exception):
 
 
 class CommandData(object):
-  def __init__(self, name, cmd, kwargs, message, python3=False):
+  def __init__(self, name, cmd, kwargs, message, python3=True):
+    # The python3 argument is ignored but has to be retained because of the many
+    # callers in other repos that pass it in.
+    del python3
     self.name = name
     self.cmd = cmd
     self.stdin = kwargs.get('stdin', None)
@@ -91,7 +94,7 @@ class CommandData(object):
     self.kwargs['stdin'] = subprocess.PIPE
     self.message = message
     self.info = None
-    self.python3 = python3
+
 
 
 # Adapted from
@@ -184,9 +187,7 @@ class ThreadPool(object):
     self._nonparallel_tests = []
 
   def _GetCommand(self, test):
-    vpython = 'vpython'
-    if test.python3:
-      vpython += '3'
+    vpython = 'vpython3'
     if sys.platform == 'win32':
       vpython += '.bat'
 
@@ -638,10 +639,10 @@ class InputApi(object):
 
     self.is_windows = sys.platform == 'win32'
 
-    # Set python_executable to 'vpython' in order to allow scripts in other
+    # Set python_executable to 'vpython3' in order to allow scripts in other
     # repos (e.g. src.git) to automatically pick up that repo's .vpython file,
     # instead of inheriting the one in depot_tools.
-    self.python_executable = 'vpython'
+    self.python_executable = 'vpython3'
     # Offer a python 3 executable for use during the migration off of python 2.
     self.python3_executable = 'vpython3'
     self.environ = os.environ
@@ -1744,13 +1745,6 @@ def DoPresubmitChecks(change,
     if not presubmit_files and verbose:
       sys.stdout.write('Warning, no PRESUBMIT.py found.\n')
     results = []
-    if sys.platform == 'win32':
-      temp = os.environ['TEMP']
-    else:
-      temp = '/tmp'
-    python2_usage_log_file = os.path.join(temp, 'python2_usage.txt')
-    if os.path.exists(python2_usage_log_file):
-      os.remove(python2_usage_log_file)
     thread_pool = ThreadPool()
     executer = PresubmitExecuter(change, committing, verbose, gerrit_obj,
                                  dry_run, thread_pool, parallel, no_diffs)
@@ -1769,17 +1763,6 @@ def DoPresubmitChecks(change,
 
     results += thread_pool.RunAsync()
 
-    if os.path.exists(python2_usage_log_file):
-      with open(python2_usage_log_file) as f:
-        python2_usage = [x.strip() for x in f.readlines()]
-        results.append(
-            OutputApi(committing).PresubmitPromptWarning(
-                'Python 2 scripts were run during %s presubmits. Please see '
-                'https://bugs.chromium.org/p/chromium/issues/detail?id=1313804'
-                '#c61 for tips on resolving this.'
-                % python_version,
-                items=python2_usage))
-
     messages = {}
     should_prompt = False
     presubmits_failed = False

tests/presubmit_unittest.py

@@ -2548,7 +2548,7 @@ the current line as well!
     self.assertEqual(results[0].__class__,
                       presubmit.OutputApi.PresubmitNotifyResult)
     self.assertEqual(
-        'test_module\npyyyyython -m test_module (0.00s) failed\nfoo',
+        'test_module\npyyyyython3 -m test_module (0.00s) failed\nfoo',
         results[0]._message)
 
   def testRunPythonUnitTestsFailureCommitting(self):
@@ -2561,7 +2561,7 @@ the current line as well!
     self.assertEqual(len(results), 1)
     self.assertEqual(results[0].__class__, presubmit.OutputApi.PresubmitError)
     self.assertEqual(
-        'test_module\npyyyyython -m test_module (0.00s) failed\nfoo',
+        'test_module\npyyyyython3 -m test_module (0.00s) failed\nfoo',
         results[0]._message)
 
   def testRunPythonUnitTestsSuccess(self):
@@ -2830,9 +2830,9 @@ the current line as well!
 
     cmd = ['bar.py', '--verbose']
     if input_api.platform == 'win32':
-      cmd.insert(0, 'vpython.bat')
+      cmd.insert(0, 'vpython3.bat')
     else:
-      cmd.insert(0, 'vpython')
+      cmd.insert(0, 'vpython3')
     self.assertEqual(subprocess.Popen.mock_calls, [
         mock.call(
             cmd, cwd=self.fake_root_dir, stdout=subprocess.PIPE,
@@ -2932,20 +2932,14 @@ the current line as well!
     self.assertEqual([result.__class__ for result in results], [
         presubmit.OutputApi.PresubmitPromptWarning,
         presubmit.OutputApi.PresubmitNotifyResult,
-        presubmit.OutputApi.PresubmitNotifyResult,
     ])
 
     cmd = ['bar.py', '--verbose']
-    vpython = 'vpython'
     vpython3 = 'vpython3'
     if input_api.platform == 'win32':
-      vpython += '.bat'
       vpython3 += '.bat'
 
     self.assertEqual(subprocess.Popen.mock_calls, [
-        mock.call(
-            [vpython] + cmd, cwd=self.fake_root_dir, stdout=subprocess.PIPE,
-            stderr=subprocess.STDOUT, stdin=subprocess.PIPE),
         mock.call(
             [vpython3] + cmd, cwd=self.fake_root_dir, stdout=subprocess.PIPE,
             stderr=subprocess.STDOUT, stdin=subprocess.PIPE),
@@ -2958,7 +2952,6 @@ the current line as well!
     self.assertEqual(presubmit.sigint_handler.wait.mock_calls, [
         mock.call(subprocesses[0], None),
         mock.call(subprocesses[1], None),
-        mock.call(subprocesses[2], None),
     ])
 
     self.checkstdout('')
@@ -3007,50 +3000,6 @@ the current line as well!
 
     self.checkstdout('')
 
-  @mock.patch('io.open', mock.mock_open())
-  def testCannedRunUnitTestsDontRunOnPython3(self):
-    io.open().readline.return_value = '#!/usr/bin/env python3'
-    change = presubmit.Change(
-        'foo1', 'description1', self.fake_root_dir, None, 0, 0, None)
-    input_api = self.MockInputApi(change, False)
-    input_api.verbose = True
-    input_api.PresubmitLocalPath.return_value = self.fake_root_dir
-    presubmit.sigint_handler.wait.return_value = (b'', None)
-
-    subprocess.Popen.side_effect = [
-        mock.Mock(returncode=1),
-        mock.Mock(returncode=0),
-        mock.Mock(returncode=0),
-    ]
-
-    unit_tests = ['allo', 'bar.py']
-    results = presubmit_canned_checks.RunUnitTests(
-        input_api,
-        presubmit.OutputApi,
-        unit_tests,
-        run_on_python3=False)
-    self.assertEqual([result.__class__ for result in results], [
-        presubmit.OutputApi.PresubmitPromptWarning,
-        presubmit.OutputApi.PresubmitNotifyResult,
-    ])
-
-    cmd = ['bar.py', '--verbose']
-    vpython = 'vpython'
-    if input_api.platform == 'win32':
-      vpython += '.bat'
-
-    self.assertEqual(subprocess.Popen.mock_calls, [
-        mock.call(
-            [vpython] + cmd, cwd=self.fake_root_dir, stdout=subprocess.PIPE,
-            stderr=subprocess.STDOUT, stdin=subprocess.PIPE),
-        mock.call(
-            ['allo', '--verbose'], cwd=self.fake_root_dir,
-            stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
-            stdin=subprocess.PIPE),
-    ])
-
-    self.checkstdout('')
-
   def testCannedRunUnitTestsInDirectory(self):
     change = presubmit.Change(
         'foo1', 'description1', self.fake_root_dir, None, 0, 0, None)

vpython

@@ -41,8 +41,6 @@ base_dir=$(dirname "$0")
 source "$base_dir/cipd_bin_setup.sh"
 cipd_bin_setup &> /dev/null
 
-echo $@ from $(pwd) >> "/tmp/python2_usage.txt"
-
 if [[ $(uname -s) = MINGW* || $(uname -s) = CYGWIN* ]]; then
   cmd.exe //c $0.bat "$@"
 else

vpython.bat

@@ -6,5 +6,4 @@
 :: See revert instructions in cipd_manifest.txt
 
 call "%~dp0\cipd_bin_setup.bat" > nul 2>&1
-echo %* from %cd% >> "%TEMP%\python2_usage.txt"
 "%~dp0\.cipd_bin\vpython.exe" %*