diff --git a/git_cl.py b/git_cl.py index 9bf60ea29..bf949133c 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1317,7 +1317,8 @@ class Changelist(object): return presubmit_support.DoPresubmitChecks(change, committing, verbose=verbose, output_stream=sys.stdout, input_stream=sys.stdin, default_presubmit=None, may_prompt=may_prompt, - rietveld_obj=self._codereview_impl.GetRieveldObjForPresubmit()) + rietveld_obj=self._codereview_impl.GetRieveldObjForPresubmit(), + gerrit_obj=self._codereview_impl.GetGerritObjForPresubmit()) except presubmit_support.PresubmitFailure, e: DieWithError( ('%s\nMaybe your depot_tools is out of date?\n' @@ -1501,6 +1502,10 @@ class _ChangelistCodereviewBase(object): # For non-Rietveld codereviews, this probably should return a dummy object. raise NotImplementedError() + def GetGerritObjForPresubmit(self): + # None is valid return value, otherwise presubmit_support.GerritAccessor. + return None + def UpdateDescriptionRemote(self, description): """Update the description on codereview site.""" raise NotImplementedError() @@ -2107,6 +2112,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): raise NotImplementedError() return ThisIsNotRietveldIssue() + def GetGerritObjForPresubmit(self): + return presubmit_support.GerritAccessor(self._GetGerritHost()) + def GetStatus(self): """Apply a rough heuristic to give a simple summary of an issue's review or CQ status, assuming adherence to a common workflow. diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index e21d08675..1e16d8fff 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -875,8 +875,7 @@ def CheckOwners(input_api, output_api, source_file_filter=None): input_api.change.AffectedFiles(file_filter=source_file_filter)]) owners_db = input_api.owners_db - # TODO(tandrii): this will always return None, set() in case of Gerrit. - owner_email, reviewers = _RietveldOwnerAndReviewers( + owner_email, reviewers = _CodereviewOwnersAndReviewers( input_api, owners_db.email_regexp, approval_needed=input_api.is_committing) @@ -905,6 +904,26 @@ def CheckOwners(input_api, output_api, source_file_filter=None): return [output('Missing LGTM from someone other than %s' % owner_email)] return [] +def _CodereviewOwnersAndReviewers(input_api, email_regexp, approval_needed): + """Return the owner and reviewers of a change, if any. + + If approval_needed is True, only reviewers who have approved the change + will be returned. + """ + if input_api.change.issue: + if input_api.gerrit: + res = _GerritOwnerAndReviewers(input_api, email_regexp, approval_needed) + else: + # Rietveld is default. + res = _RietveldOwnerAndReviewers(input_api, email_regexp, approval_needed) + if res: + return res + + reviewers = set() + if not approval_needed: + reviewers = _ReviewersFromChange(input_api.change) + return None, reviewers + def _GetRietveldIssueProps(input_api, messages): """Gets the issue properties from rietveld.""" @@ -926,35 +945,51 @@ def _ReviewersFromChange(change): return set(reviewer for reviewer in reviewers if '@' in reviewer) +def _match_reviewer_email(r, owner_email, email_regexp): + return email_regexp.match(r) and r != owner_email + def _RietveldOwnerAndReviewers(input_api, email_regexp, approval_needed=False): """Return the owner and reviewers of a change, if any. If approval_needed is True, only reviewers who have approved the change will be returned. + Returns None if can't fetch issue properties from codereview. """ issue_props = _GetRietveldIssueProps(input_api, True) if not issue_props: - reviewers = set() - if not approval_needed: - reviewers = _ReviewersFromChange(input_api.change) - return None, reviewers + return None if not approval_needed: return issue_props['owner_email'], set(issue_props['reviewers']) owner_email = issue_props['owner_email'] - def match_reviewer(r): - return email_regexp.match(r) and r != owner_email - messages = issue_props.get('messages', []) approvers = set( m['sender'] for m in messages - if m.get('approval') and match_reviewer(m['sender'])) - + if m.get('approval') and _match_reviewer_email(m['sender'], owner_email, + email_regexp)) return owner_email, approvers +def _GerritOwnerAndReviewers(input_api, email_regexp, approval_needed=False): + """Return the owner and reviewers of a change, if any. + + If approval_needed is True, only reviewers who have approved the change + will be returned. + Returns None if can't fetch issue properties from codereview. + """ + issue = input_api.change.issue + if not issue: + return None + + owner_email = input_api.gerrit.GetChangeOwner(issue) + reviewers = set( + r for r in input_api.gerrit.GetChangeReviewers(issue, approval_needed) + if _match_reviewer_email(r, owner_email, email_regexp)) + return owner_email, reviewers + + def _CheckConstNSObject(input_api, output_api, source_file_filter): """Checks to make sure no objective-c files have |const NSSomeClass*|.""" pattern = input_api.re.compile( diff --git a/presubmit_support.py b/presubmit_support.py index 07cf7b24b..57cea9b83 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -197,6 +197,73 @@ class _MailTextResult(_PresubmitResult): super(_MailTextResult, self).__init__() raise NotImplementedError() +class GerritAccessor(object): + """Limited Gerrit functionality for canned presubmit checks to work. + + To avoid excessive Gerrit calls, caches the results. + """ + + def __init__(self, host): + self.host = host + self.cache = {} + + def _FetchChangeDetail(self, issue): + # Separate function to be easily mocked in tests. + return gerrit_util.GetChangeDetail( + self.host, str(issue), + ['ALL_REVISIONS', 'DETAILED_LABELS']) + + def GetChangeInfo(self, issue): + """Returns labels and all revisions (patchsets) for this issue. + + The result is a dictionary according to Gerrit REST Api. + https://gerrit-review.googlesource.com/Documentation/rest-api.html + + However, API isn't very clear what's inside, so see tests for example. + """ + assert issue + cache_key = int(issue) + if cache_key not in self.cache: + self.cache[cache_key] = self._FetchChangeDetail(issue) + return self.cache[cache_key] + + def GetChangeDescription(self, issue, patchset=None): + """If patchset is none, fetches current patchset.""" + info = self.GetChangeInfo(issue) + # info is a reference to cache. We'll modify it here adding description to + # it to the right patchset, if it is not yet there. + + # Find revision info for the patchset we want. + if patchset is not None: + for rev, rev_info in info['revisions'].iteritems(): + if str(rev_info['_number']) == str(patchset): + break + else: + raise Exception('patchset %s doesn\'t exist in issue %s' % ( + patchset, issue)) + else: + rev = info['current_revision'] + rev_info = info['revisions'][rev] + + # Updates revision info, which is part of cached issue info. + if 'real_description' not in rev_info: + rev_info['real_description'] = ( + gerrit_util.GetChangeDescriptionFromGitiles( + rev_info['fetch']['http']['url'], rev)) + return rev_info['real_description'] + + def GetChangeOwner(self, issue): + return self.GetChangeInfo(issue)['owner']['email'] + + def GetChangeReviewers(self, issue, approving_only=True): + # Gerrit has 'approved' sub-section, but it only lists 1 approver. + # So, if we look only for approvers, we have to look at all anyway. + # Also, assume LGTM means Code-Review label == 2. Other configurations + # aren't supported. + return [r['email'] + for r in self.GetChangeInfo(issue)['labels']['Code-Review']['all'] + if not approving_only or '2' == str(r.get('value', 0))] + class OutputApi(object): """An instance of OutputApi gets passed to presubmit scripts so that they @@ -265,7 +332,7 @@ class InputApi(object): ) def __init__(self, change, presubmit_path, is_committing, - rietveld_obj, verbose, dry_run=None): + rietveld_obj, verbose, gerrit_obj=None, dry_run=None): """Builds an InputApi object. Args: @@ -273,12 +340,15 @@ class InputApi(object): presubmit_path: The path to the presubmit script being processed. is_committing: True if the change is about to be committed. rietveld_obj: rietveld.Rietveld client object + gerrit_obj: provides basic Gerrit codereview functionality. + dry_run: if true, some Checks will be skipped. """ # Version number of the presubmit_support script. self.version = [int(x) for x in __version__.split('.')] self.change = change self.is_committing = is_committing self.rietveld = rietveld_obj + self.gerrit = gerrit_obj self.dry_run = dry_run # TBD self.host_url = 'http://codereview.chromium.org' @@ -1349,16 +1419,19 @@ def DoPostUploadExecuter(change, class PresubmitExecuter(object): def __init__(self, change, committing, rietveld_obj, verbose, - dry_run=None): + gerrit_obj=None, dry_run=None): """ Args: change: The Change object. committing: True if 'gcl commit' is running, False if 'gcl upload' is. rietveld_obj: rietveld.Rietveld client object. + gerrit_obj: provides basic Gerrit codereview functionality. + dry_run: if true, some Checks will be skipped. """ self.change = change self.committing = committing self.rietveld = rietveld_obj + self.gerrit = gerrit_obj self.verbose = verbose self.dry_run = dry_run @@ -1381,7 +1454,7 @@ class PresubmitExecuter(object): # Load the presubmit script into context. input_api = InputApi(self.change, presubmit_path, self.committing, self.rietveld, self.verbose, - dry_run=self.dry_run) + self.dry_run, self.gerrit) context = {} try: exec script_text in context @@ -1424,6 +1497,7 @@ def DoPresubmitChecks(change, default_presubmit, may_prompt, rietveld_obj, + gerrit_obj=None, dry_run=None): """Runs all presubmit checks that apply to the files in the change. @@ -1443,6 +1517,7 @@ def DoPresubmitChecks(change, default_presubmit: A default presubmit script to execute in any case. may_prompt: Enable (y/n) questions on warning or error. rietveld_obj: rietveld.Rietveld object. + gerrit_obj: provides basic Gerrit codereview functionality. dry_run: if true, some Checks will be skipped. Warning: @@ -1471,7 +1546,7 @@ def DoPresubmitChecks(change, output.write("Warning, no PRESUBMIT.py found.\n") results = [] executer = PresubmitExecuter(change, committing, rietveld_obj, verbose, - dry_run=dry_run) + gerrit_obj, dry_run) if default_presubmit: if verbose: output.write("Running default presubmit script.\n") @@ -1696,7 +1771,8 @@ def main(argv=None): parser.error('For unversioned directory, is not optional.') logging.info('Found %d file(s).' % len(files)) - rietveld_obj = None + rietveld_obj, gerrit_obj = None, None + if options.rietveld_url: # The empty password is permitted: '' is not None. if options.rietveld_private_key_file: @@ -1718,21 +1794,11 @@ def main(argv=None): logging.info('Got description: """\n%s\n"""', options.description) if options.gerrit_url and options.gerrit_fetch: - rietveld_obj = None assert options.issue and options.patchset - props = gerrit_util.GetChangeDetail( - urlparse.urlparse(options.gerrit_url).netloc, str(options.issue), - ['ALL_REVISIONS']) - options.author = props['owner']['email'] - for rev, rev_info in props['revisions'].iteritems(): - if str(rev_info['_number']) == str(options.patchset): - options.description = gerrit_util.GetChangeDescriptionFromGitiles( - rev_info['fetch']['http']['url'], rev) - break - else: - print >> sys.stderr, ('Patchset %d was not found in Gerrit issue %d' % - options.patchset, options.issue) - return 2 + rietveld_obj = None + gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc) + options.author = gerrit_obj.GetChangeOwner(options.issue) + options.description = gerrit_obj.GetChangeDescription(options.patchset) logging.info('Got author: "%s"', options.author) logging.info('Got description: """\n%s\n"""', options.description) @@ -1754,6 +1820,7 @@ def main(argv=None): options.default_presubmit, options.may_prompt, rietveld_obj, + gerrit_obj, options.dry_run) return not results.should_continue() except NonexistantCannedCheckFilter, e: diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index d0c9c10ed..256ed5016 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -184,6 +184,7 @@ class PresubmitUnittest(PresubmitTestsBase): 'subprocess', 'sys', 'tempfile', 'time', 'traceback', 'types', 'unittest', 'urllib2', 'warn', 'multiprocessing', 'DoGetTryMasters', 'GetTryMastersExecuter', 'itertools', 'urlparse', 'gerrit_util', + 'GerritAccessor', ] # If this test fails, you should add the relevant test. self.compareMembers(presubmit, members) @@ -828,7 +829,8 @@ def CheckChangeOnCommit(input_api, output_api): 0, None) output = presubmit.DoPresubmitChecks( - change, False, True, None, input_buf, DEFAULT_SCRIPT, False, None) + change, False, True, None, input_buf, DEFAULT_SCRIPT, False, None, None, + None) self.failIf(output.should_continue()) text = ( 'Running presubmit upload checks ...\n' @@ -910,7 +912,8 @@ def CheckChangeOnCommit(input_api, output_api): 0, None) self.failUnless(presubmit.DoPresubmitChecks( - change, False, True, output, input_buf, DEFAULT_SCRIPT, False, None)) + change, False, True, output, input_buf, DEFAULT_SCRIPT, False, None, + None)) self.assertEquals(output.getvalue(), ('Running presubmit upload checks ...\n' 'Warning, no PRESUBMIT.py found.\n' @@ -1153,7 +1156,7 @@ def CheckChangeOnCommit(input_api, output_api): presubmit.DoPresubmitChecks(mox.IgnoreArg(), False, False, mox.IgnoreArg(), mox.IgnoreArg(), - None, False, None, None).AndReturn(output) + None, False, None, None, None).AndReturn(output) self.mox.ReplayAll() self.assertEquals( @@ -1197,7 +1200,7 @@ class InputApiUnittest(PresubmitTestsBase): 'os_walk', 'os_path', 'os_stat', 'owners_db', 'pickle', 'platform', 'python_executable', 're', 'rietveld', 'subprocess', 'tbr', 'tempfile', 'time', 'traceback', 'unittest', 'urllib2', 'version', 'verbose', - 'dry_run', + 'dry_run', 'gerrit', ] # If this test fails, you should add the relevant test. self.compareMembers( @@ -1839,6 +1842,7 @@ class CannedChecksUnittest(PresubmitTestsBase): input_api.os_path = presubmit.os.path input_api.re = presubmit.re input_api.rietveld = self.mox.CreateMock(rietveld.Rietveld) + input_api.gerrit = None input_api.traceback = presubmit.traceback input_api.urllib2 = self.mox.CreateMock(presubmit.urllib2) input_api.unittest = unittest @@ -2561,7 +2565,8 @@ class CannedChecksUnittest(PresubmitTestsBase): presubmit.OutputApi.PresubmitNotifyResult) def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None, - reviewers=None, is_committing=True, rietveld_response=None, + reviewers=None, is_committing=True, + rietveld_response=None, gerrit_response=None, uncovered_files=None, expected_output='', manually_specified_reviewers=None, dry_run=None): if approvers is None: @@ -2584,6 +2589,11 @@ class CannedChecksUnittest(PresubmitTestsBase): change.TBR = '' affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile) input_api = self.MockInputApi(change, False) + if gerrit_response: + assert not rietveld_response + input_api.rietveld = None + input_api.gerrit = presubmit.GerritAccessor('host') + fake_db = self.mox.CreateMock(owners.Database) fake_db.email_regexp = input_api.re.compile(owners.BASIC_EMAIL_REGEXP) input_api.owners_db = fake_db @@ -2595,7 +2605,7 @@ class CannedChecksUnittest(PresubmitTestsBase): if not dry_run: affected_file.LocalPath().AndReturn('foo/xyz.cc') change.AffectedFiles(file_filter=None).AndReturn([affected_file]) - if issue and not rietveld_response: + if issue and not rietveld_response and not gerrit_response: rietveld_response = { "owner_email": change.author_email, "messages": [ @@ -2612,9 +2622,12 @@ class CannedChecksUnittest(PresubmitTestsBase): if not dry_run: if issue: - input_api.rietveld.get_issue_properties( - issue=int(input_api.change.issue), messages=True).AndReturn( - rietveld_response) + if rietveld_response: + input_api.rietveld.get_issue_properties( + issue=int(input_api.change.issue), messages=True).AndReturn( + rietveld_response) + elif gerrit_response: + input_api.gerrit._FetchChangeDetail = lambda _: gerrit_response people.add(change.author_email) fake_db.files_not_covered_by(set(['foo/xyz.cc']), @@ -2647,6 +2660,39 @@ class CannedChecksUnittest(PresubmitTestsBase): rietveld_response=response, expected_output='') + def testCannedCheckOwners_Approved_Gerrit(self): + response = { + "owner": {"email": "john@example.com"}, + "labels": {"Code-Review": { + u'all': [ + { + u'email': u'john@example.com', # self +1 :) + u'value': 1 + }, + { + u'email': u'ben@example.com', + u'value': 2 + }, + ], + u'approved': {u'email': u'ben@example.org'}, + u'default_value': 0, + u'values': {u' 0': u'No score', + u'+1': u'Looks good to me, but someone else must approve', + u'+2': u'Looks good to me, approved', + u'-1': u"I would prefer that you didn't submit this", + u'-2': u'Do not submit'} + }}, + } + self.AssertOwnersWorks(approvers=set(['ben@example.com']), + gerrit_response=response, + is_committing=True, + expected_output='') + + self.AssertOwnersWorks(approvers=set(['ben@example.com']), + is_committing=False, + gerrit_response=response, + expected_output='') + def testCannedCheckOwners_Approved(self): response = { "owner_email": "john@example.com",