From 227d5108573318ac6510f17d80c167d94ceae3e1 Mon Sep 17 00:00:00 2001 From: Edward Lemur Date: Tue, 25 Feb 2020 23:45:35 +0000 Subject: [PATCH] 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/+/2068945 Reviewed-by: Anthony Polito Commit-Queue: Edward Lesmes --- git_cl.py | 118 +++++++++++++++++++++++------------- presubmit_support.py | 8 ++- tests/git_cl_test.py | 113 +++++++++++++++++++++++++++------- tests/presubmit_unittest.py | 1 - 4 files changed, 173 insertions(+), 67 deletions(-) diff --git a/git_cl.py b/git_cl.py index 78174df6c0..4d89021f5a 100755 --- a/git_cl.py +++ b/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 diff --git a/presubmit_support.py b/presubmit_support.py index 71264ecd8a..dce82165d2 100755 --- a/presubmit_support.py +++ b/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) diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index ad6adcf9e8..8051de502b 100755 --- a/tests/git_cl_test.py +++ b/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 = [ diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 86c9a58818..c024455a90 100755 --- a/tests/presubmit_unittest.py +++ b/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'], }