git-cl: Invoke presubmit checks via subprocess execution instead of via module.

PRESUBMIT.py scripts are executed in presubmit_support.py using exec().
Since PRESUBMIT.py scripts might not be yet compatible with python 3, we
have to execute presubmit_support.py using python 2.

git_cl.py imports presubmit_support.py, and executes presubmit checks using
presubmit_support as a module. This forces git_cl.py to be executed using
python 2 to maintain compatibility for PRESUBMIT.py scripts.

This change allows git_cl.py to be executed using python 3, while
presubmit_support.py is executed using python 2.

Similar changes for post-submit hooks and git-cl try masters will follow.

Bug: 1042324
Change-Id: Ic3bb1c2985459baf6aa04d0cc65017a1c2578153
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2068945Reviewed-by: Anthony Polito <apolito@google.com>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

git_cl.py

@@ -68,6 +68,7 @@ __version__ = '2.0'
 # depot_tools checkout.
 DEPOT_TOOLS = os.path.dirname(os.path.abspath(__file__))
 TRACES_DIR = os.path.join(DEPOT_TOOLS, 'traces')
+PRESUBMIT_SUPPORT = os.path.join(DEPOT_TOOLS, 'presubmit_support.py')
 
 # When collecting traces, Git hashes will be reduced to 6 characters to reduce
 # the size after compression.
@@ -1422,23 +1423,57 @@ class Changelist(object):
 
     self.description = description
 
-  def RunHook(self, committing, may_prompt, verbose, change, parallel):
+  def RunHook(
+      self, committing, may_prompt, verbose, parallel, upstream, description,
+      all_files):
     """Calls sys.exit() if the hook fails; returns a HookResults otherwise."""
-    try:
-      start = time_time()
-      result = presubmit_support.DoPresubmitChecks(change, committing,
-          verbose=verbose, output_stream=sys.stdout, input_stream=sys.stdin,
-          default_presubmit=None, may_prompt=may_prompt,
-          gerrit_obj=self.GetGerritObjForPresubmit(),
-          parallel=parallel)
-      metrics.collector.add_repeated('sub_commands', {
-        'command': 'presubmit',
-        'execution_time': time_time() - start,
-        'exit_code': 0 if result.should_continue() else 1,
-      })
-      return result
-    except presubmit_support.PresubmitFailure as e:
-      DieWithError('%s\nMaybe your depot_tools is out of date?' % e)
+    args = [
+        '--commit' if committing else '--upload',
+        '--author', self.GetAuthor(),
+        '--root', settings.GetRoot(),
+        '--upstream', upstream,
+    ]
+
+    args.extend(['--verbose'] * verbose)
+
+    issue = self.GetIssue()
+    patchset = self.GetPatchset()
+    gerrit_url = self.GetCodereviewServer()
+    if issue:
+      args.extend(['--issue', str(issue)])
+    if patchset:
+      args.extend(['--patchset', str(patchset)])
+    if gerrit_url:
+      args.extend(['--gerrit_url', gerrit_url])
+
+    if may_prompt:
+      args.append('--may_prompt')
+    if parallel:
+      args.append('--parallel')
+    if all_files:
+      args.append('--all_files')
+
+    with gclient_utils.temporary_file() as description_file:
+      with gclient_utils.temporary_file() as json_output:
+        gclient_utils.FileWrite(
+            description_file, description.encode('utf-8'), mode='wb')
+        args.extend(['--json_output', json_output])
+        args.extend(['--description_file', description_file])
+
+        start = time_time()
+        p = subprocess2.Popen(['vpython', PRESUBMIT_SUPPORT] + args)
+        exit_code = p.wait()
+        metrics.collector.add_repeated('sub_commands', {
+          'command': 'presubmit',
+          'execution_time': time_time() - start,
+          'exit_code': exit_code,
+        })
+
+        if exit_code:
+          sys.exit(exit_code)
+
+        json_results = gclient_utils.FileRead(json_output)
+        return json.loads(json_results)
 
   def CMDUpload(self, options, git_diff_args, orig_args):
     """Uploads a change to codereview."""
@@ -1467,21 +1502,23 @@ class Changelist(object):
       self.ExtendCC(watchlist.GetWatchersForPaths(files))
 
     if not options.bypass_hooks:
+      description = change.FullDescriptionText()
       if options.reviewers or options.tbrs or options.add_owners_to:
         # Set the reviewer list now so that presubmit checks can access it.
-        change_description = ChangeDescription(change.FullDescriptionText())
+        change_description = ChangeDescription(description)
         change_description.update_reviewers(options.reviewers,
                                             options.tbrs,
                                             options.add_owners_to,
                                             change)
-        change.SetDescriptionText(change_description.description)
+        description = change_description.description
       hook_results = self.RunHook(committing=False,
                                   may_prompt=not options.force,
                                   verbose=options.verbose,
-                                  change=change, parallel=options.parallel)
-      if not hook_results.should_continue():
-        return 1
-      self.ExtendCC(hook_results.more_cc)
+                                  parallel=options.parallel,
+                                  upstream=base_branch,
+                                  description=description,
+                                  all_files=False)
+      self.ExtendCC(hook_results['more_cc'])
 
     print_stats(git_diff_args)
     ret = self.CMDUploadChange(options, git_diff_args, custom_cl_base, change)
@@ -1982,14 +2019,19 @@ class Changelist(object):
             action='submit')
       print('WARNING: Bypassing hooks and submitting latest uploaded patchset.')
     elif not bypass_hooks:
-      hook_results = self.RunHook(
+      upstream = self.GetCommonAncestorWithUpstream()
+      if self.GetIssue():
+        description = self.FetchDescription()
+      else:
+        description = self.GetDescription(upstream)
+      self.RunHook(
           committing=True,
           may_prompt=not force,
           verbose=verbose,
-          change=self.GetChange(self.GetCommonAncestorWithUpstream()),
-          parallel=parallel)
-      if not hook_results.should_continue():
-        return 1
+          parallel=parallel,
+          upstream=upstream,
+          description=description,
+          all_files=False)
 
     self.SubmitIssue(wait_for_merge=True)
     print('Issue %s has been submitted.' % self.GetIssueURL())
@@ -4085,27 +4127,19 @@ def CMDpresubmit(parser, args):
     # Default to diffing against the common ancestor of the upstream branch.
     base_branch = cl.GetCommonAncestorWithUpstream()
 
-  if options.all:
-    base_change = cl.GetChange(base_branch)
-    files = [('M', f) for f in base_change.AllFiles()]
-    change = presubmit_support.GitChange(
-        base_change.Name(),
-        base_change.FullDescriptionText(),
-        base_change.RepositoryRoot(),
-        files,
-        base_change.issue,
-        base_change.patchset,
-        base_change.author_email,
-        base_change._upstream)
+  if self.GetIssue():
+    description = self.FetchDescription()
   else:
-    change = cl.GetChange(base_branch)
+    description = self.GetDescription(base_branch)
 
   cl.RunHook(
       committing=not options.upload,
       may_prompt=False,
       verbose=options.verbose,
-      change=change,
-      parallel=options.parallel)
+      parallel=options.parallel,
+      upstream=base_branch,
+      description=description,
+      all_files=options.all)
   return 0
 
 

presubmit_support.py

@@ -1706,7 +1706,6 @@ def DoPresubmitChecks(change,
         'warnings': [
             warning.json_format() for warning in warnings
         ],
-        'should_continue': output.should_continue(),
         'more_cc': output.more_cc,
       }
 
@@ -1850,7 +1849,10 @@ def main(argv=None):
                       help='Use 2 times for more debug info.')
   parser.add_argument('--name', default='no name')
   parser.add_argument('--author')
-  parser.add_argument('--description', default='')
+  desc = parser.add_mutually_exclusive_group()
+  desc.add_argument('--description', default='', help='The change description.')
+  desc.add_argument('--description_file',
+                    help='File to read change description from.')
   parser.add_argument('--issue', type=int, default=0)
   parser.add_argument('--patchset', type=int, default=0)
   parser.add_argument('--root', default=os.getcwd(),
@@ -1892,6 +1894,8 @@ def main(argv=None):
   else:
     logging.basicConfig(level=logging.ERROR)
 
+  if options.description_file:
+    options.description = gclient_utils.FileRead(options.description_file)
   gerrit_obj = _parse_gerrit_options(parser, options)
   change = _parse_change(parser, options)
 

tests/git_cl_test.py

@@ -33,8 +33,8 @@ import metrics
 # We have to disable monitoring before importing git_cl.
 metrics.DISABLE_METRICS_COLLECTION = True
 
-import contextlib
 import clang_format
+import contextlib
 import gclient_utils
 import gerrit_util
 import git_cl
@@ -82,16 +82,6 @@ class ChangelistMock(object):
     ChangelistMock.desc = desc
 
 
-class PresubmitMock(object):
-  def __init__(self, *args, **kwargs):
-    self.reviewers = []
-    self.more_cc = ['chromium-reviews+test-more-cc@chromium.org']
-
-  @staticmethod
-  def should_continue():
-    return True
-
-
 class GitMocks(object):
   def __init__(self, config=None, branchref=None):
     self.branchref = branchref or 'refs/heads/master'
@@ -657,7 +647,8 @@ class TestGitCl(unittest.TestCase):
         'git_cl.write_json',
         lambda *a: self._mocked_call('write_json', *a)).start()
     mock.patch(
-        'git_cl.presubmit_support.DoPresubmitChecks', PresubmitMock).start()
+        'git_cl.Changelist.RunHook',
+        return_value={'more_cc': ['test-more-cc@chromium.org']}).start()
     mock.patch('git_cl.watchlists.Watchlists', WatchlistsMock).start()
     mock.patch('git_cl.auth.Authenticator', AuthenticatorMock).start()
     mock.patch('gerrit_util.GetChangeDetail').start()
@@ -819,13 +810,6 @@ class TestGitCl(unittest.TestCase):
     ]
 
     calls += [
-      (('time.time',), 1000,),
-      (('time.time',), 3000,),
-      (('add_repeated', 'sub_commands', {
-          'execution_time': 2000,
-          'command': 'presubmit',
-          'exit_code': 0
-      }), None,),
       ((['git', 'diff', '--no-ext-diff', '--stat', '-l100000', '-C50'] +
          ([custom_cl_base] if custom_cl_base else
           [ancestor_revision, 'HEAD']),),
@@ -959,7 +943,7 @@ class TestGitCl(unittest.TestCase):
         ref_suffix += ',r=%s' % r
         metrics_arguments.append('r')
       if issue is None:
-        cc += ['chromium-reviews+test-more-cc@chromium.org', 'joe@example.com']
+        cc += ['test-more-cc@chromium.org', 'joe@example.com']
       for c in sorted(cc):
         ref_suffix += ',cc=%s' % c
         metrics_arguments.append('cc')
@@ -969,7 +953,7 @@ class TestGitCl(unittest.TestCase):
       calls += [
         (('ValidAccounts', '%s-review.googlesource.com' % short_hostname,
           sorted(reviewers) + ['joe@example.com',
-          'chromium-reviews+test-more-cc@chromium.org'] + cc),
+          'test-more-cc@chromium.org'] + cc),
          {
            e: {'email': e}
            for e in (reviewers + ['joe@example.com'] + cc)
@@ -1106,7 +1090,7 @@ class TestGitCl(unittest.TestCase):
           (('AddReviewers',
             'chromium-review.googlesource.com', 'my%2Frepo~123456',
             sorted(reviewers),
-            cc + ['chromium-reviews+test-more-cc@chromium.org'],
+            cc + ['test-more-cc@chromium.org'],
             notify),
            ''),
       ]
@@ -2779,6 +2763,25 @@ class TestGitCl(unittest.TestCase):
 
 
 class ChangelistTest(unittest.TestCase):
+  def setUp(self):
+    super(ChangelistTest, self).setUp()
+    mock.patch('gclient_utils.FileRead').start()
+    mock.patch('gclient_utils.FileWrite').start()
+    mock.patch('gclient_utils.temporary_file', TemporaryFileMock()).start()
+    mock.patch(
+        'git_cl.Changelist.GetCodereviewServer',
+        return_value='https://chromium-review.googlesource.com').start()
+    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.PRESUBMIT_SUPPORT', 'PRESUBMIT_SUPPORT').start()
+    mock.patch('git_cl.Settings.GetRoot', return_value='root').start()
+    mock.patch('git_cl.time_time').start()
+    mock.patch('metrics.collector').start()
+    mock.patch('subprocess2.Popen').start()
+    self.addCleanup(mock.patch.stopall)
+    self.temp_count = 0
+
   @mock.patch('git_cl.RunGitWithCode')
   def testGetLocalDescription(self, _mock):
     git_cl.RunGitWithCode.return_value = (0, 'description')
@@ -2788,6 +2791,72 @@ class ChangelistTest(unittest.TestCase):
     git_cl.RunGitWithCode.assert_called_once_with(
         ['log', '--pretty=format:%s%n%n%b', 'branch...'])
 
+  def testRunHook(self):
+    expected_results = {
+        'more_cc': ['more@example.com', 'cc@example.com'],
+        'should_continue': True,
+    }
+    gclient_utils.FileRead.return_value = json.dumps(expected_results)
+    git_cl.time_time.side_effect = [100, 200]
+    mockProcess = mock.Mock()
+    mockProcess.wait.return_value = 0
+    subprocess2.Popen.return_value = mockProcess
+
+    cl = git_cl.Changelist()
+    results = cl.RunHook(
+        committing=True,
+        may_prompt=True,
+        verbose=2,
+        parallel=True,
+        upstream='upstream',
+        description='description',
+        all_files=True)
+
+    self.assertEqual(expected_results, results)
+    subprocess2.Popen.assert_called_once_with([
+        'vpython', 'PRESUBMIT_SUPPORT',
+        '--commit',
+        '--author', 'author',
+        '--root', 'root',
+        '--upstream', 'upstream',
+        '--verbose', '--verbose',
+        '--issue', '123456',
+        '--patchset', '7',
+        '--gerrit_url', 'https://chromium-review.googlesource.com',
+        '--may_prompt',
+        '--parallel',
+        '--all_files',
+        '--json_output', '/tmp/fake-temp2',
+        '--description_file', '/tmp/fake-temp1',
+    ])
+    gclient_utils.FileWrite.assert_called_once_with(
+        '/tmp/fake-temp1', b'description', mode='wb')
+    metrics.collector.add_repeated('sub_commands', {
+      'command': 'presubmit',
+      'execution_time': 100,
+      'exit_code': 0,
+    })
+
+  @mock.patch('sys.exit', side_effect=SystemExitMock)
+  def testRunHook_Failure(self, _mock):
+    git_cl.time_time.side_effect = [100, 200]
+    mockProcess = mock.Mock()
+    mockProcess.wait.return_value = 2
+    subprocess2.Popen.return_value = mockProcess
+
+    cl = git_cl.Changelist()
+    with self.assertRaises(SystemExitMock):
+      cl.RunHook(
+          committing=True,
+          may_prompt=True,
+          verbose=2,
+          parallel=True,
+          upstream='upstream',
+          description='description',
+          all_files=True)
+
+    sys.exit.assert_called_once_with(2)
+
 
 class CMDTestCaseBase(unittest.TestCase):
   _STATUSES = [

tests/presubmit_unittest.py

@@ -648,7 +648,6 @@ def CheckChangeOnCommit(input_api, output_api):
             'long_text': fake_warning_long_text
           }
         ],
-        'should_continue': False,
         'more_cc': ['me@example.com'],
     }