Commit aa6ddc60 authored by Aaron Gable's avatar Aaron Gable Committed by Commit Bot

Revert "Remove Rietveld support from presubmit"

This reverts commit f3eed001.

Reason for revert: Some PRESUBMIT checks still expect 
CheckRietveldTryJobExecution to exist.

Original change's description:
> Remove Rietveld support from presubmit
> 
> Since no one can upload or land changes from Rietveld anymore,
> PRESUBMIT and its support files no longer need to be able to
> support it.
> 
> R=​tandrii@chromium.org
> 
> Bug: 770408
> Change-Id: I749092b66fdca16d5cef77e8cefc905aa5375b50
> Reviewed-on: https://chromium-review.googlesource.com/693380
> Commit-Queue: Aaron Gable <agable@chromium.org>
> Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

TBR=agable@chromium.org,tandrii@chromium.org

Change-Id: I72e29bd8a9739406f29190adbeb7eb7718ed21cd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 770408
Reviewed-on: https://chromium-review.googlesource.com/991012
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: 's avatarAaron Gable <agable@chromium.org>
parent d69c5dd5
...@@ -1525,6 +1525,7 @@ class Changelist(object): ...@@ -1525,6 +1525,7 @@ class Changelist(object):
return presubmit_support.DoPresubmitChecks(change, committing, return presubmit_support.DoPresubmitChecks(change, committing,
verbose=verbose, output_stream=sys.stdout, input_stream=sys.stdin, verbose=verbose, output_stream=sys.stdout, input_stream=sys.stdin,
default_presubmit=None, may_prompt=may_prompt, default_presubmit=None, may_prompt=may_prompt,
rietveld_obj=self._codereview_impl.GetRietveldObjForPresubmit(),
gerrit_obj=self._codereview_impl.GetGerritObjForPresubmit()) gerrit_obj=self._codereview_impl.GetGerritObjForPresubmit())
except presubmit_support.PresubmitFailure as e: except presubmit_support.PresubmitFailure as e:
DieWithError('%s\nMaybe your depot_tools is out of date?' % e) DieWithError('%s\nMaybe your depot_tools is out of date?' % e)
...@@ -1806,6 +1807,11 @@ class _ChangelistCodereviewBase(object): ...@@ -1806,6 +1807,11 @@ class _ChangelistCodereviewBase(object):
"""Which branch-specific properties to erase when unsetting issue.""" """Which branch-specific properties to erase when unsetting issue."""
return [] return []
def GetRietveldObjForPresubmit(self):
# This is an unfortunate Rietveld-embeddedness in presubmit.
# For non-Rietveld code reviews, this probably should return a dummy object.
raise NotImplementedError()
def GetGerritObjForPresubmit(self): def GetGerritObjForPresubmit(self):
# None is valid return value, otherwise presubmit_support.GerritAccessor. # None is valid return value, otherwise presubmit_support.GerritAccessor.
return None return None
...@@ -2123,6 +2129,9 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): ...@@ -2123,6 +2129,9 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
def CodereviewServerConfigKey(cls): def CodereviewServerConfigKey(cls):
return 'rietveldserver' return 'rietveldserver'
def GetRietveldObjForPresubmit(self):
return self.RpcServer()
def SetLabels(self, enable_auto_submit, use_commit_queue, cq_dry_run): def SetLabels(self, enable_auto_submit, use_commit_queue, cq_dry_run):
raise NotImplementedError() raise NotImplementedError()
...@@ -2495,6 +2504,24 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): ...@@ -2495,6 +2504,24 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
"""Which branch-specific properties to erase when unsetting issue.""" """Which branch-specific properties to erase when unsetting issue."""
return ['gerritsquashhash'] return ['gerritsquashhash']
def GetRietveldObjForPresubmit(self):
class ThisIsNotRietveldIssue(object):
def __nonzero__(self):
# This is a hack to make presubmit_support think that rietveld is not
# defined, yet still ensure that calls directly result in a decent
# exception message below.
return False
def __getattr__(self, attr):
print(
'You aren\'t using Rietveld at the moment, but Gerrit.\n'
'Using Rietveld in your PRESUBMIT scripts won\'t work.\n'
'Please, either change your PRESUBMIT to not use rietveld_obj.%s,\n'
'or use Rietveld for codereview.\n'
'See also http://crbug.com/579160.' % attr)
raise NotImplementedError()
return ThisIsNotRietveldIssue()
def GetGerritObjForPresubmit(self): def GetGerritObjForPresubmit(self):
return presubmit_support.GerritAccessor(self._GetGerritHost()) return presubmit_support.GerritAccessor(self._GetGerritHost())
......
...@@ -793,6 +793,15 @@ def RunPylint(input_api, *args, **kwargs): ...@@ -793,6 +793,15 @@ def RunPylint(input_api, *args, **kwargs):
return input_api.RunTests(GetPylint(input_api, *args, **kwargs), False) return input_api.RunTests(GetPylint(input_api, *args, **kwargs), False)
def CheckRietveldTryJobExecution(dummy_input_api, output_api,
dummy_host_url, dummy_platforms,
dummy_owner):
return [
output_api.PresubmitNotifyResult(
'CheckRietveldTryJobExecution is deprecated, please remove it.')
]
def CheckBuildbotPendingBuilds(input_api, output_api, url, max_pendings, def CheckBuildbotPendingBuilds(input_api, output_api, url, max_pendings,
ignored): ignored):
try: try:
...@@ -886,25 +895,25 @@ def CheckOwners(input_api, output_api, source_file_filter=None): ...@@ -886,25 +895,25 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
return [output_fn('Missing LGTM from someone other than %s' % owner_email)] return [output_fn('Missing LGTM from someone other than %s' % owner_email)]
return [] return []
def GetCodereviewOwnerAndReviewers(input_api, email_regexp, approval_needed): def GetCodereviewOwnerAndReviewers(input_api, email_regexp, approval_needed):
"""Return the owner and reviewers of a change, if any. """Return the owner and reviewers of a change, if any.
If approval_needed is True, only reviewers who have approved the change If approval_needed is True, only reviewers who have approved the change
will be returned. will be returned.
""" """
issue = input_api.change.issue # Rietveld is default.
if not issue: func = _RietveldOwnerAndReviewers
return None, (set() if approval_needed else if input_api.gerrit:
_ReviewersFromChange(input_api.change)) func = _GerritOwnerAndReviewers
return func(input_api, email_regexp, approval_needed)
owner_email = input_api.gerrit.GetChangeOwner(issue)
reviewers = set( def _GetRietveldIssueProps(input_api, messages):
r for r in input_api.gerrit.GetChangeReviewers(issue, approval_needed) """Gets the issue properties from rietveld."""
if _match_reviewer_email(r, owner_email, email_regexp)) issue = input_api.change.issue
input_api.logging.debug('owner: %s; approvals given by: %s', if issue and input_api.rietveld:
owner_email, ', '.join(sorted(reviewers))) return input_api.rietveld.get_issue_properties(
return owner_email, reviewers issue=int(issue), messages=messages)
def _ReviewersFromChange(change): def _ReviewersFromChange(change):
...@@ -920,6 +929,49 @@ def _ReviewersFromChange(change): ...@@ -920,6 +929,49 @@ def _ReviewersFromChange(change):
def _match_reviewer_email(r, owner_email, email_regexp): def _match_reviewer_email(r, owner_email, email_regexp):
return email_regexp.match(r) and r != owner_email 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.
"""
issue_props = _GetRietveldIssueProps(input_api, True)
if not issue_props:
return None, (set() if approval_needed else
_ReviewersFromChange(input_api.change))
if not approval_needed:
return issue_props['owner_email'], set(issue_props['reviewers'])
owner_email = issue_props['owner_email']
messages = issue_props.get('messages', [])
approvers = set(
m['sender'] for m in messages
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.
"""
issue = input_api.change.issue
if not issue:
return None, (set() if approval_needed else
_ReviewersFromChange(input_api.change))
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))
input_api.logging.debug('owner: %s; approvals given by: %s',
owner_email, ', '.join(sorted(reviewers)))
return owner_email, reviewers
def CheckSingletonInHeaders(input_api, output_api, source_file_filter=None): def CheckSingletonInHeaders(input_api, output_api, source_file_filter=None):
"""Deprecated, must be removed.""" """Deprecated, must be removed."""
......
...@@ -41,6 +41,7 @@ import urlparse ...@@ -41,6 +41,7 @@ import urlparse
from warnings import warn from warnings import warn
# Local imports. # Local imports.
import auth
import fix_encoding import fix_encoding
import gclient_utils import gclient_utils
import git_footers import git_footers
...@@ -48,6 +49,7 @@ import gerrit_util ...@@ -48,6 +49,7 @@ import gerrit_util
import owners import owners
import owners_finder import owners_finder
import presubmit_canned_checks import presubmit_canned_checks
import rietveld
import scm import scm
import subprocess2 as subprocess # Exposed through the API. import subprocess2 as subprocess # Exposed through the API.
...@@ -388,13 +390,14 @@ class InputApi(object): ...@@ -388,13 +390,14 @@ class InputApi(object):
) )
def __init__(self, change, presubmit_path, is_committing, def __init__(self, change, presubmit_path, is_committing,
verbose, gerrit_obj, dry_run=None): rietveld_obj, verbose, gerrit_obj=None, dry_run=None):
"""Builds an InputApi object. """Builds an InputApi object.
Args: Args:
change: A presubmit.Change object. change: A presubmit.Change object.
presubmit_path: The path to the presubmit script being processed. presubmit_path: The path to the presubmit script being processed.
is_committing: True if the change is about to be committed. is_committing: True if the change is about to be committed.
rietveld_obj: rietveld.Rietveld client object
gerrit_obj: provides basic Gerrit codereview functionality. gerrit_obj: provides basic Gerrit codereview functionality.
dry_run: if true, some Checks will be skipped. dry_run: if true, some Checks will be skipped.
""" """
...@@ -402,8 +405,13 @@ class InputApi(object): ...@@ -402,8 +405,13 @@ class InputApi(object):
self.version = [int(x) for x in __version__.split('.')] self.version = [int(x) for x in __version__.split('.')]
self.change = change self.change = change
self.is_committing = is_committing self.is_committing = is_committing
self.rietveld = rietveld_obj
self.gerrit = gerrit_obj self.gerrit = gerrit_obj
self.dry_run = dry_run self.dry_run = dry_run
# TBD
self.host_url = 'http://codereview.chromium.org'
if self.rietveld:
self.host_url = self.rietveld.url
# We expose various modules and functions as attributes of the input_api # We expose various modules and functions as attributes of the input_api
# so that presubmit scripts don't have to import them. # so that presubmit scripts don't have to import them.
...@@ -1264,17 +1272,19 @@ def DoPostUploadExecuter(change, ...@@ -1264,17 +1272,19 @@ def DoPostUploadExecuter(change,
class PresubmitExecuter(object): class PresubmitExecuter(object):
def __init__(self, change, committing, verbose, def __init__(self, change, committing, rietveld_obj, verbose,
gerrit_obj, dry_run=None): gerrit_obj=None, dry_run=None):
""" """
Args: Args:
change: The Change object. change: The Change object.
committing: True if 'git cl land' is running, False if 'git cl upload' is. committing: True if 'git cl land' is running, False if 'git cl upload' is.
rietveld_obj: rietveld.Rietveld client object.
gerrit_obj: provides basic Gerrit codereview functionality. gerrit_obj: provides basic Gerrit codereview functionality.
dry_run: if true, some Checks will be skipped. dry_run: if true, some Checks will be skipped.
""" """
self.change = change self.change = change
self.committing = committing self.committing = committing
self.rietveld = rietveld_obj
self.gerrit = gerrit_obj self.gerrit = gerrit_obj
self.verbose = verbose self.verbose = verbose
self.dry_run = dry_run self.dry_run = dry_run
...@@ -1298,7 +1308,7 @@ class PresubmitExecuter(object): ...@@ -1298,7 +1308,7 @@ class PresubmitExecuter(object):
# Load the presubmit script into context. # Load the presubmit script into context.
input_api = InputApi(self.change, presubmit_path, self.committing, input_api = InputApi(self.change, presubmit_path, self.committing,
self.verbose, self.rietveld, self.verbose,
gerrit_obj=self.gerrit, dry_run=self.dry_run) gerrit_obj=self.gerrit, dry_run=self.dry_run)
output_api = OutputApi(self.committing) output_api = OutputApi(self.committing)
context = {} context = {}
...@@ -1347,7 +1357,8 @@ def DoPresubmitChecks(change, ...@@ -1347,7 +1357,8 @@ def DoPresubmitChecks(change,
input_stream, input_stream,
default_presubmit, default_presubmit,
may_prompt, may_prompt,
gerrit_obj, rietveld_obj,
gerrit_obj=None,
dry_run=None): dry_run=None):
"""Runs all presubmit checks that apply to the files in the change. """Runs all presubmit checks that apply to the files in the change.
...@@ -1367,6 +1378,7 @@ def DoPresubmitChecks(change, ...@@ -1367,6 +1378,7 @@ def DoPresubmitChecks(change,
default_presubmit: A default presubmit script to execute in any case. default_presubmit: A default presubmit script to execute in any case.
may_prompt: Enable (y/n) questions on warning or error. If False, may_prompt: Enable (y/n) questions on warning or error. If False,
any questions are answered with yes by default. any questions are answered with yes by default.
rietveld_obj: rietveld.Rietveld object.
gerrit_obj: provides basic Gerrit codereview functionality. gerrit_obj: provides basic Gerrit codereview functionality.
dry_run: if true, some Checks will be skipped. dry_run: if true, some Checks will be skipped.
...@@ -1395,7 +1407,7 @@ def DoPresubmitChecks(change, ...@@ -1395,7 +1407,7 @@ def DoPresubmitChecks(change,
if not presubmit_files and verbose: if not presubmit_files and verbose:
output.write("Warning, no PRESUBMIT.py found.\n") output.write("Warning, no PRESUBMIT.py found.\n")
results = [] results = []
executer = PresubmitExecuter(change, committing, verbose, executer = PresubmitExecuter(change, committing, rietveld_obj, verbose,
gerrit_obj, dry_run) gerrit_obj, dry_run)
if default_presubmit: if default_presubmit:
if verbose: if verbose:
...@@ -1590,8 +1602,17 @@ def main(argv=None): ...@@ -1590,8 +1602,17 @@ def main(argv=None):
parser.add_option("--gerrit_url", help=optparse.SUPPRESS_HELP) parser.add_option("--gerrit_url", help=optparse.SUPPRESS_HELP)
parser.add_option("--gerrit_fetch", action='store_true', parser.add_option("--gerrit_fetch", action='store_true',
help=optparse.SUPPRESS_HELP) help=optparse.SUPPRESS_HELP)
parser.add_option("--rietveld_url", help=optparse.SUPPRESS_HELP)
parser.add_option("--rietveld_email", help=optparse.SUPPRESS_HELP)
parser.add_option("--rietveld_fetch", action='store_true', default=False,
help=optparse.SUPPRESS_HELP)
# These are for OAuth2 authentication for bots. See also apply_issue.py
parser.add_option("--rietveld_email_file", help=optparse.SUPPRESS_HELP)
parser.add_option("--rietveld_private_key_file", help=optparse.SUPPRESS_HELP)
auth.add_auth_options(parser)
options, args = parser.parse_args(argv) options, args = parser.parse_args(argv)
auth_config = auth.extract_auth_config_from_options(options)
if options.verbose >= 2: if options.verbose >= 2:
logging.basicConfig(level=logging.DEBUG) logging.basicConfig(level=logging.DEBUG)
...@@ -1600,14 +1621,49 @@ def main(argv=None): ...@@ -1600,14 +1621,49 @@ def main(argv=None):
else: else:
logging.basicConfig(level=logging.ERROR) logging.basicConfig(level=logging.ERROR)
if (any((options.rietveld_url, options.rietveld_email_file,
options.rietveld_fetch, options.rietveld_private_key_file))
and any((options.gerrit_url, options.gerrit_fetch))):
parser.error('Options for only codereview --rietveld_* or --gerrit_* '
'allowed')
if options.rietveld_email and options.rietveld_email_file:
parser.error("Only one of --rietveld_email or --rietveld_email_file "
"can be passed to this program.")
if options.rietveld_email_file:
with open(options.rietveld_email_file, "rb") as f:
options.rietveld_email = f.read().strip()
change_class, files = load_files(options, args) change_class, files = load_files(options, args)
if not change_class: if not change_class:
parser.error('For unversioned directory, <files> is not optional.') parser.error('For unversioned directory, <files> is not optional.')
logging.info('Found %d file(s).', len(files)) logging.info('Found %d file(s).', len(files))
gerrit_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:
rietveld_obj = rietveld.JwtOAuth2Rietveld(
options.rietveld_url,
options.rietveld_email,
options.rietveld_private_key_file)
else:
rietveld_obj = rietveld.CachingRietveld(
options.rietveld_url,
auth_config,
options.rietveld_email)
if options.rietveld_fetch:
assert options.issue
props = rietveld_obj.get_issue_properties(options.issue, False)
options.author = props['owner_email']
options.description = props['description']
logging.info('Got author: "%s"', options.author)
logging.info('Got description: """\n%s\n"""', options.description)
if options.gerrit_url and options.gerrit_fetch: if options.gerrit_url and options.gerrit_fetch:
assert options.issue and options.patchset assert options.issue and options.patchset
rietveld_obj = None
gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc) gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc)
options.author = gerrit_obj.GetChangeOwner(options.issue) options.author = gerrit_obj.GetChangeOwner(options.issue)
options.description = gerrit_obj.GetChangeDescription(options.issue, options.description = gerrit_obj.GetChangeDescription(options.issue,
...@@ -1632,6 +1688,7 @@ def main(argv=None): ...@@ -1632,6 +1688,7 @@ def main(argv=None):
sys.stdin, sys.stdin,
options.default_presubmit, options.default_presubmit,
options.may_prompt, options.may_prompt,
rietveld_obj,
gerrit_obj, gerrit_obj,
options.dry_run) options.dry_run)
return not results.should_continue() return not results.should_continue()
......
This diff is collapsed.
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment