[git-cl][presubmit] Remove references to LUCI_OMIT_PYTHON2.

This changes all references to the LUCI_OMIT_PYTHON2 environment
variable to instead assume that it is `'true'`.

R=sokcevic, tikuta

Bug: 1441784
Recipe-Nontrivial-Roll: build
Change-Id: Iea161a372adb68cdcb330d131df2c19ca2fe7b37
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4522480
Reviewed-by: Takuto Ikuta <tikuta@chromium.org>
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>

git_cl.py

@@ -822,9 +822,6 @@ class Settings(object):
   def GetDefaultCCList(self):
     return self._GetConfig('rietveld.cc')
 
-  def GetUsePython3(self):
-    return self._GetConfig('rietveld.use-python3')
-
   def GetSquashGerritUploads(self):
     """Returns True if uploads to Gerrit should be squashed by default."""
     if self.squash_gerrit_uploads is None:
@@ -1259,9 +1256,6 @@ class Changelist(object):
       server = _KNOWN_GERRIT_TO_SHORT_URLS.get(server, server)
     return '%s/%s' % (server, issue)
 
-  def GetUsePython3(self):
-    return settings.GetUsePython3()
-
   def FetchDescription(self, pretty=False):
     assert self.GetIssue(), 'issue is required to query Gerrit'
 
@@ -1432,42 +1426,27 @@ class Changelist(object):
             ' was not specified. To enable ResultDB, please run the command'
             ' again with the --realm argument to specify the LUCI 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,
+    return self._RunPresubmit(args,
                               description,
-                                     use_python3=False,
                               resultdb=resultdb,
                               realm=realm)
-    return self._MergePresubmitResults(py2_results, py3_results)
 
   def _RunPresubmit(self,
                     args,
                     description,
-                    use_python3,
                     resultdb=None,
                     realm=None):
     # type: (Sequence[str], str, bool, Optional[bool], Optional[str]
     #    ) -> Mapping[str, Any]
     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)
         args.extend(['--json_output', json_output])
         args.extend(['--description_file', description_file])
-        if self.GetUsePython3():
-          args.append('--use-python3')
         start = time_time()
-        cmd = [vpython, PRESUBMIT_SUPPORT] + args
+        cmd = ['vpython3', PRESUBMIT_SUPPORT] + args
         if resultdb and realm:
           cmd = ['rdb', 'stream', '-new', '-realm', realm, '--'] + cmd
 
@@ -1486,34 +1465,14 @@ 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):
+  def RunPostUploadHook(self, verbose, upstream, description):
     args = self._GetCommonPresubmitArgs(verbose, upstream)
     args.append('--post_upload')
 
     with gclient_utils.temporary_file() as description_file:
       gclient_utils.FileWrite(description_file, description)
       args.extend(['--description_file', description_file])
-      run_py2 = not py3_only and os.getenv('LUCI_OMIT_PYTHON2') != 'true'
-      if run_py2:
-        p_py2 = subprocess2.Popen(['vpython', PRESUBMIT_SUPPORT] + args)
-      p_py3 = subprocess2.Popen(['vpython3', PRESUBMIT_SUPPORT] + args +
-                                ['--use-python3'])
-      if run_py2:
-        p_py2.wait()
-      p_py3.wait()
+      subprocess2.Popen(['vpython3', PRESUBMIT_SUPPORT] + args).wait()
 
   def _GetDescriptionForUpload(self, options, git_diff_args, files):
     # type: (optparse.Values, Sequence[str], Sequence[str]
@@ -1839,8 +1798,7 @@ class Changelist(object):
 
     if settings.GetRunPostUploadHook():
       self.RunPostUploadHook(options.verbose, new_upload.parent,
-                             new_upload.change_desc.description,
-                             options.no_python2_post_upload_hooks)
+                             new_upload.change_desc.description)
 
     if new_upload.reviewers or new_upload.ccs:
       gerrit_util.AddReviewers(self.GetGerritHost(),
@@ -1896,8 +1854,7 @@ class Changelist(object):
       # Run post upload hooks, if specified.
       if settings.GetRunPostUploadHook():
         self.RunPostUploadHook(options.verbose, base_branch,
-                               change_desc.description,
-                               options.no_python2_post_upload_hooks)
+                               change_desc.description)
 
       # Upload all dependencies if specified.
       if options.dependencies:
@@ -3429,7 +3386,6 @@ def LoadCodereviewSettingsFromFile(fileobj):
               unset_error_ok=True)
   SetProperty(
       'format-full-by-default', 'FORMAT_FULL_BY_DEFAULT', unset_error_ok=True)
-  SetProperty('use-python3', 'USE_PYTHON3', unset_error_ok=True)
 
   if 'GERRIT_HOST' in keyvals:
     RunGit(['config', 'gerrit.host', keyvals['GERRIT_HOST']])
@@ -4744,9 +4700,6 @@ def CMDupload(parser, args):
                     action='store_true',
                     dest='no_add_changeid',
                     help='Do not add change-ids to messages.')
-  parser.add_option('--no-python2-post-upload-hooks',
-                    action='store_true',
-                    help='Only run post-upload hooks in Python 3.')
   parser.add_option('--cherry-pick-stacked',
                     '--cp',
                     dest='cherry_pick_stacked',

presubmit_support.py

@@ -310,32 +310,6 @@ def prompt_should_continue(prompt_string):
   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):
@@ -1412,17 +1386,14 @@ def ListRelevantPresubmitFiles(files, root):
 
 
 class GetPostUploadExecuter(object):
-  def __init__(self, change, gerrit_obj, use_python3):
+  def __init__(self, change, gerrit_obj):
     """
     Args:
       change: The Change object.
       gerrit_obj: provides basic Gerrit codereview functionality.
-      use_python3: if true, will use python3 instead of python2 by default
-                if USE_PYTHON3 is not specified.
     """
     self.change = change
     self.gerrit = gerrit_obj
-    self.use_python3 = use_python3
 
   def ExecPresubmitScript(self, script_text, presubmit_path):
     """Executes PostUploadHook() from a single presubmit script.
@@ -1479,15 +1450,13 @@ def _MergeMasters(masters1, masters2):
   return result
 
 
-def DoPostUploadExecuter(change, gerrit_obj, verbose, use_python3=False):
+def DoPostUploadExecuter(change, gerrit_obj, verbose):
   """Execute the post upload hook.
 
   Args:
     change: The Change object.
     gerrit_obj: The GerritAccessor object.
     verbose: Prints debug info.
-    use_python3: if true, default to using Python3 for presubmit checks
-                 rather than Python2.
   """
   python_version = 'Python %s' % sys.version_info.major
   sys.stdout.write('Running %s post upload checks ...\n' % python_version)
@@ -1496,7 +1465,7 @@ def DoPostUploadExecuter(change, gerrit_obj, verbose, use_python3=False):
   if not presubmit_files and verbose:
     sys.stdout.write('Warning, no PRESUBMIT.py found.\n')
   results = []
-  executer = GetPostUploadExecuter(change, gerrit_obj, use_python3)
+  executer = GetPostUploadExecuter(change, gerrit_obj)
   # The root presubmit file should be executed after the ones in subdirectories.
   # i.e. the specific post upload hooks should run before the general ones.
   # Thus, reverse the order provided by ListRelevantPresubmitFiles.
@@ -1506,12 +1475,7 @@ 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 _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:
+    if verbose:
       sys.stdout.write('Running %s\n' % filename)
     results.extend(executer.ExecPresubmitScript(presubmit_script, filename))
 
@@ -1532,8 +1496,7 @@ def DoPostUploadExecuter(change, gerrit_obj, verbose, use_python3=False):
 
 class PresubmitExecuter(object):
   def __init__(self, change, committing, verbose, gerrit_obj, dry_run=None,
-               thread_pool=None, parallel=False, use_python3=False,
-               no_diffs=False):
+               thread_pool=None, parallel=False, no_diffs=False):
     """
     Args:
       change: The Change object.
@@ -1542,8 +1505,6 @@ class PresubmitExecuter(object):
       dry_run: if true, some Checks will be skipped.
       parallel: if true, all tests reported via input_api.RunTests for all
                 PRESUBMIT files will be run in parallel.
-      use_python3: if true, will use python3 instead of python2 by default
-                if USE_PYTHON3 is not specified.
       no_diffs: if true, implies that --files or --all was specified so some
                 checks can be skipped, and some errors will be messages.
     """
@@ -1555,7 +1516,6 @@ class PresubmitExecuter(object):
     self.more_cc = []
     self.thread_pool = thread_pool
     self.parallel = parallel
-    self.use_python3 = use_python3
     self.no_diffs = no_diffs
 
   def ExecPresubmitScript(self, script_text, presubmit_path):
@@ -1728,7 +1688,6 @@ def DoPresubmitChecks(change,
                       dry_run=None,
                       parallel=False,
                       json_output=None,
-                      use_python3=False,
                       no_diffs=False):
   """Runs all presubmit checks that apply to the files in the change.
 
@@ -1750,8 +1709,6 @@ def DoPresubmitChecks(change,
     dry_run: if true, some Checks will be skipped.
     parallel: if true, all tests specified by input_api.RunTests in all
               PRESUBMIT files will be run in parallel.
-    use_python3: if true, default to using Python3 for presubmit checks
-                 rather than Python2.
     no_diffs: if true, implies that --files or --all was specified so some
               checks can be skipped, and some errors will be messages.
   Return:
@@ -1785,31 +1742,19 @@ def DoPresubmitChecks(change,
       os.remove(python2_usage_log_file)
     thread_pool = ThreadPool()
     executer = PresubmitExecuter(change, committing, verbose, gerrit_obj,
-                                 dry_run, thread_pool, parallel, use_python3,
-                                 no_diffs)
-    skipped_count = 0;
+                                 dry_run, thread_pool, parallel, no_diffs)
     if default_presubmit:
       if verbose:
         sys.stdout.write('Running default presubmit script.\n')
       fake_path = os.path.join(change.RepositoryRoot(), 'PRESUBMIT.py')
-      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 _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:
+      if verbose:
         sys.stdout.write('Running %s\n' % filename)
       results += executer.ExecPresubmitScript(presubmit_script, filename)
-      else:
-        skipped_count += 1
 
     results += thread_pool.RunAsync()
 
@@ -1880,7 +1825,6 @@ def DoPresubmitChecks(change,
             for warning in messages.get('Warnings', [])
         ],
         'more_cc': executer.more_cc,
-        'skipped_presubmits': skipped_count,
       }
 
       gclient_utils.FileWrite(
@@ -2079,8 +2023,6 @@ def main(argv=None):
                       'wildcards can also be used.')
   parser.add_argument('--source_controlled_only', action='store_true',
                       help='Constrain \'files\' to those in source control.')
-  parser.add_argument('--use-python3', action='store_true',
-                      help='Use python3 for presubmit checks by default')
   parser.add_argument('--no_diffs', action='store_true',
                       help='Assume that all "modified" files have no diffs.')
   options = parser.parse_args(argv)
@@ -2107,8 +2049,7 @@ def main(argv=None):
 
   try:
     if options.post_upload:
-      return DoPostUploadExecuter(change, gerrit_obj, options.verbose,
-                                  options.use_python3)
+      return DoPostUploadExecuter(change, gerrit_obj, options.verbose)
     with canned_check_filter(options.skip_canned):
       return DoPresubmitChecks(
           change,
@@ -2120,7 +2061,6 @@ def main(argv=None):
           options.dry_run,
           options.parallel,
           options.json_output,
-          options.use_python3,
           options.no_diffs)
   except PresubmitFailure as e:
     import utils

recipes/recipe_modules/presubmit/api.py

@@ -30,7 +30,7 @@ class PresubmitApi(recipe_api.RecipeApi):
 
     name = kwargs.pop('name', 'presubmit')
     with self.m.depot_tools.on_path():
-      cmd = ['vpython3', self.presubmit_support_path, '--use-python3']
+      cmd = ['vpython3', self.presubmit_support_path]
       cmd.extend(args)
       cmd.extend(['--json_output', self.m.json.output()])
       if self.m.resultdb.enabled:

recipes/recipe_modules/presubmit/examples/full.expected/basic.json

@@ -3,7 +3,6 @@
     "cmd": [
       "vpython3",
       "RECIPE_REPO[depot_tools]/presubmit_support.py",
-      "--use-python3",
       "--json_output",
       "/path/to/tmp/json"
     ],

tests/git_cl_test.py

@@ -91,9 +91,6 @@ class ChangelistMock(object):
   def GetRemoteBranch(self):
     return ('origin', 'refs/remotes/origin/main')
 
-  def GetUsePython3(self):
-    return self._use_python3
-
 class GitMocks(object):
   def __init__(self, config=None, branchref=None):
     self.branchref = branchref or 'refs/heads/main'
@@ -724,8 +721,6 @@ class TestGitCl(unittest.TestCase):
         CERR1),
       ((['git', 'config', '--unset-all', 'rietveld.format-full-by-default'],),
         CERR1),
-      ((['git', 'config', '--unset-all', 'rietveld.use-python3'],),
-        CERR1),
       ((['git', 'config', 'gerrit.host', 'true'],), ''),
     ]
     self.assertIsNone(git_cl.LoadCodereviewSettingsFromFile(codereview_file))
@@ -3642,7 +3637,6 @@ class ChangelistTest(unittest.TestCase):
     mock.patch('git_cl.Changelist.GetAuthor', return_value='author').start()
     mock.patch('git_cl.Changelist.GetIssue', return_value=123456).start()
     mock.patch('git_cl.Changelist.GetPatchset', return_value=7).start()
-    mock.patch('git_cl.Changelist.GetUsePython3', return_value=False).start()
     mock.patch(
         'git_cl.Changelist.GetRemoteBranch',
         return_value=('origin', 'refs/remotes/origin/main')).start()
@@ -3875,7 +3869,7 @@ class ChangelistTest(unittest.TestCase):
 
   def testRunPostUploadHook(self):
     cl = git_cl.Changelist()
-    cl.RunPostUploadHook(2, 'upstream', 'description', False)
+    cl.RunPostUploadHook(2, 'upstream', 'description')
 
     subprocess2.Popen.assert_called_with([
         'vpython3',
@@ -3901,7 +3895,6 @@ class ChangelistTest(unittest.TestCase):
         '--post_upload',
         '--description_file',
         '/tmp/fake-temp1',
-        '--use-python3',
     ])
 
     gclient_utils.FileWrite.assert_called_once_with(
@@ -3909,7 +3902,7 @@ class ChangelistTest(unittest.TestCase):
 
   def testRunPostUploadHookPy3Only(self):
     cl = git_cl.Changelist()
-    cl.RunPostUploadHook(2, 'upstream', 'description', True)
+    cl.RunPostUploadHook(2, 'upstream', 'description')
 
     subprocess2.Popen.assert_called_once_with([
         'vpython3',
@@ -3935,7 +3928,6 @@ class ChangelistTest(unittest.TestCase):
         '--post_upload',
         '--description_file',
         '/tmp/fake-temp1',
-        '--use-python3',
     ])
 
     gclient_utils.FileWrite.assert_called_once_with('/tmp/fake-temp1',
@@ -4224,7 +4216,7 @@ class ChangelistTest(unittest.TestCase):
                                              ccs=ccs,
                                              notify=False)
     mockRunPostHook.assert_called_once_with(True, 'parent-commit',
-                                            change_desc.description, True)
+                                            change_desc.description)
 
 
 class CMDTestCaseBase(unittest.TestCase):

tests/presubmit_unittest.py

@@ -642,8 +642,7 @@ class PresubmitUnittest(PresubmitTestsBase):
     change = self.ExampleChange()
 
     executer = presubmit.GetPostUploadExecuter(change,
-                                               presubmit.GerritAccessor(),
-                                               False)
+                                               presubmit.GerritAccessor())
 
     executer.ExecPresubmitScript(self.presubmit_text, fake_presubmit)
 
@@ -806,7 +805,6 @@ def CheckChangeOnCommit(input_api, output_api):
           }
         ],
         'more_cc': ['me@example.com'],
-        'skipped_presubmits': 0,
     }
 
     fake_result_json = json.dumps(fake_result, sort_keys=True)