Skip Python 2 presubmit step when unneeded

All of the PRESUBMIT.py files in the Chromium repo are running under
Python 3. However "git cl presubmit" also works with other repos where
some PRESUBMIT.py scripts still run under Python 2. This means that
the Python 2 presubmit commit checks step cannot simply be disabled.
That meant that Chromium was paying up to a one-minute cost just to
setup for and look for Python 2 scripts that it doesn't run.

This change runs the Python 3 PRESUBMIT.py scripts first, and keeps
track of whether any were skipped. If none were skipped then the
Python 2 PRESUBMIT.py stage can be skipped.

Note that the child scripts of PRESUBMIT.py scripts may still be run
under Python 2, but that is orthogonal to this change.

Bug: 1313804
Change-Id: Ib65838223f232f1e78058d6a08ea15a89f442310
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3614453
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>

git_cl.py

@@ -1370,10 +1370,14 @@ class Changelist(object):
             ' 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)
+    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, resultdb, realm, description,
+                                      use_python3=False)
     return self._MergePresubmitResults(py2_results, py3_results)
 
   def _RunPresubmit(self, args, resultdb, realm, description, use_python3):

presubmit_support.py

@@ -1402,6 +1402,8 @@ class GetPostUploadExecuter(object):
   def ExecPresubmitScript(self, script_text, presubmit_path, gerrit_obj,
                           change):
     """Executes PostUploadHook() from a single presubmit script.
+    Caller is responsible for validating whether the hook should be executed
+    and should only call this function if it should be.
 
     Args:
       script_text: The text of the presubmit script.
@@ -1412,9 +1414,6 @@ class GetPostUploadExecuter(object):
     Return:
       A list of results objects.
     """
-    if not _ShouldRunPresubmit(script_text, self.use_python3):
-      return {}
-
     context = {}
     try:
       exec(compile(script_text, 'PRESUBMIT.py', 'exec', dont_inherit=True),
@@ -1473,6 +1472,7 @@ def DoPostUploadExecuter(change, gerrit_obj, verbose, use_python3=False):
       sys.stdout.write('Running %s\n' % filename)
     # Accept CRLF presubmit script.
     presubmit_script = gclient_utils.FileRead(filename, 'rU')
+    if _ShouldRunPresubmit(presubmit_script, use_python3):
       results.extend(executer.ExecPresubmitScript(
           presubmit_script, filename, gerrit_obj, change))
 
@@ -1517,6 +1517,8 @@ class PresubmitExecuter(object):
 
   def ExecPresubmitScript(self, script_text, presubmit_path):
     """Executes a single presubmit script.
+    Caller is responsible for validating whether the hook should be executed
+    and should only call this function if it should be.
 
     Args:
       script_text: The text of the presubmit script.
@@ -1526,9 +1528,6 @@ class PresubmitExecuter(object):
     Return:
       A list of result objects, empty if no problems.
     """
-    if not _ShouldRunPresubmit(script_text, self.use_python3):
-      return []
-
     # Change to the presubmit file's directory to support local imports.
     main_path = os.getcwd()
     presubmit_dir = os.path.dirname(presubmit_path)
@@ -1720,18 +1719,26 @@ def DoPresubmitChecks(change,
     thread_pool = ThreadPool()
     executer = PresubmitExecuter(change, committing, verbose, gerrit_obj,
                                  dry_run, thread_pool, parallel, use_python3)
+    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')
+      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)
       if verbose:
         sys.stdout.write('Running %s\n' % filename)
       # Accept CRLF presubmit script.
       presubmit_script = gclient_utils.FileRead(filename, 'rU')
+      if _ShouldRunPresubmit(presubmit_script, use_python3):
         results += executer.ExecPresubmitScript(presubmit_script, filename)
+      else:
+        skipped_count += 1
+        
     results += thread_pool.RunAsync()
 
     messages = {}
@@ -1784,6 +1791,7 @@ def DoPresubmitChecks(change,
             for warning in messages.get('Warnings', [])
         ],
         'more_cc': executer.more_cc,
+        'skipped_presubmits': skipped_count,
       }
 
       gclient_utils.FileWrite(

tests/git_cl_test.py

@@ -3095,7 +3095,7 @@ class ChangelistTest(unittest.TestCase):
 
     self.assertEqual(expected_results, results)
     subprocess2.Popen.assert_any_call([
-        'vpython', 'PRESUBMIT_SUPPORT',
+        'vpython3', 'PRESUBMIT_SUPPORT',
         '--root', 'root',
         '--upstream', 'upstream',
         '--verbose', '--verbose',
@@ -3113,7 +3113,7 @@ class ChangelistTest(unittest.TestCase):
         '--description_file', '/tmp/fake-temp1',
     ])
     subprocess2.Popen.assert_any_call([
-        'vpython3', 'PRESUBMIT_SUPPORT',
+        'vpython', 'PRESUBMIT_SUPPORT',
         '--root', 'root',
         '--upstream', 'upstream',
         '--verbose', '--verbose',
@@ -3168,7 +3168,7 @@ class ChangelistTest(unittest.TestCase):
 
     self.assertEqual(expected_results, results)
     subprocess2.Popen.assert_any_call([
-        'vpython', 'PRESUBMIT_SUPPORT',
+        'vpython3', 'PRESUBMIT_SUPPORT',
         '--root', 'root',
         '--upstream', 'upstream',
         '--gerrit_url', 'https://chromium-review.googlesource.com',
@@ -3218,7 +3218,7 @@ class ChangelistTest(unittest.TestCase):
     self.assertEqual(expected_results, results)
     subprocess2.Popen.assert_any_call([
         'rdb', 'stream', '-new', '-realm', 'chromium:public', '--',
-        'vpython', 'PRESUBMIT_SUPPORT',
+        'vpython3', 'PRESUBMIT_SUPPORT',
         '--root', 'root',
         '--upstream', 'upstream',
         '--gerrit_url', 'https://chromium-review.googlesource.com',

tests/presubmit_unittest.py

@@ -778,6 +778,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)