'bsalomon@google.com',
'djsollen@chromium.org',
'djsollen@google.com',
+ 'hcm@chromium.org',
+ 'hcm@google.com',
)
AUTHORS_FILE_NAME = 'AUTHORS'
DOCS_PREVIEW_URL = 'https://skia.org/?cl='
+GOLD_TRYBOT_URL = 'https://gold.skia.org/search?issue='
-# Path to CQ bots feature is described in skbug.com/4364
+# Path to CQ bots feature is described in https://bug.skia.org/4364
PATH_PREFIX_TO_EXTRA_TRYBOTS = {
- # pylint: disable=line-too-long
- 'cmake/': 'client.skia.compile:Build-Mac10.9-Clang-x86_64-Release-CMake-Trybot,Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot',
- # pylint: disable=line-too-long
- 'src/opts/': 'client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot',
+ 'src/opts/':
+ 'skia.primary:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD',
+ 'include/private/SkAtomics.h': ('skia.primary:'
+ 'Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Release-TSAN,'
+ 'Test-Ubuntu-Clang-Golo-GPU-GT610-x86_64-Release-TSAN-Trybot'
+ ),
# Below are examples to show what is possible with this feature.
# 'src/svg/': 'master1:abc;master2:def',
'R0201', # Method could be a function.
'E1003', # Using class name in super.
'W0613', # Unused argument.
+ 'W0105', # String statement has no effect.
)
# Run Pylint on only the modified python files. Unfortunately it still runs
# Pylint on the whole file instead of just the modified lines.
results.append(
output_api.PresubmitError(
'The following files have #if or #ifdef before includes:\n%s\n\n'
- 'See skbug.com/3362 for why this should be fixed.' %
+ 'See https://bug.skia.org/3362 for why this should be fixed.' %
'\n'.join(failing_files)))
return results
return results
+def _InfraTests(input_api, output_api):
+ """Run the infra tests."""
+ results = []
+ if not any(f.LocalPath().startswith('infra')
+ for f in input_api.AffectedFiles()):
+ return results
+
+ cmd = ['python', os.path.join('infra', 'bots', 'infra_tests.py')]
+ try:
+ subprocess.check_output(cmd)
+ except subprocess.CalledProcessError as e:
+ results.append(output_api.PresubmitError(
+ '`%s` failed:\n%s' % (' '.join(cmd), e.output)))
+ return results
+
+
+def _CheckGNFormatted(input_api, output_api):
+ """Make sure any .gn files we're changing have been formatted."""
+ results = []
+ for f in input_api.AffectedFiles():
+ if (not f.LocalPath().endswith('.gn') and
+ not f.LocalPath().endswith('.gni')):
+ continue
+
+ gn = 'gn.bat' if 'win32' in sys.platform else 'gn'
+ cmd = [gn, 'format', '--dry-run', f.LocalPath()]
+ try:
+ subprocess.check_output(cmd)
+ except subprocess.CalledProcessError:
+ fix = 'gn format ' + f.LocalPath()
+ results.append(output_api.PresubmitError(
+ '`%s` failed, try\n\t%s' % (' '.join(cmd), fix)))
+ return results
+
+
def _CommonChecks(input_api, output_api):
"""Presubmit checks common to upload and commit."""
results = []
sources = lambda x: (x.LocalPath().endswith('.h') or
- x.LocalPath().endswith('.gypi') or
- x.LocalPath().endswith('.gyp') or
x.LocalPath().endswith('.py') or
x.LocalPath().endswith('.sh') or
x.LocalPath().endswith('.m') or
"""
results = []
results.extend(_CommonChecks(input_api, output_api))
+ # Run on upload, not commit, since the presubmit bot apparently doesn't have
+ # coverage or Go installed.
+ results.extend(_InfraTests(input_api, output_api))
+
+ results.extend(_CheckGNFormatted(input_api, output_api))
return results
return tree_status_results
+class CodeReview(object):
+ """Abstracts which codereview tool is used for the specified issue."""
+
+ def __init__(self, input_api):
+ self._issue = input_api.change.issue
+ self._gerrit = input_api.gerrit
+ self._rietveld_properties = None
+ if not self._gerrit:
+ self._rietveld_properties = input_api.rietveld.get_issue_properties(
+ issue=int(self._issue), messages=True)
+
+ def GetOwnerEmail(self):
+ if self._gerrit:
+ return self._gerrit.GetChangeOwner(self._issue)
+ else:
+ return self._rietveld_properties['owner_email']
+
+ def GetSubject(self):
+ if self._gerrit:
+ return self._gerrit.GetChangeInfo(self._issue)['subject']
+ else:
+ return self._rietveld_properties['subject']
+
+ def GetDescription(self):
+ if self._gerrit:
+ return self._gerrit.GetChangeDescription(self._issue)
+ else:
+ return self._rietveld_properties['description']
+
+ def IsDryRun(self):
+ if self._gerrit:
+ return self._gerrit.GetChangeInfo(
+ self._issue)['labels']['Commit-Queue'].get('value', 0) == 1
+ else:
+ return self._rietveld_properties['cq_dry_run']
+
+ def GetReviewers(self):
+ if self._gerrit:
+ code_review_label = (
+ self._gerrit.GetChangeInfo(self._issue)['labels']['Code-Review'])
+ return [r['email'] for r in code_review_label.get('all', [])]
+ else:
+ return self._rietveld_properties['reviewers']
+
+ def GetApprovers(self):
+ approvers = []
+ if self._gerrit:
+ code_review_label = (
+ self._gerrit.GetChangeInfo(self._issue)['labels']['Code-Review'])
+ for m in code_review_label.get('all', []):
+ if m.get("value") == 1:
+ approvers.append(m["email"])
+ else:
+ for m in self._rietveld_properties.get('messages', []):
+ if 'lgtm' in m['text'].lower():
+ approvers.append(m['sender'])
+ return approvers
+
+
def _CheckOwnerIsInAuthorsFile(input_api, output_api):
results = []
- issue = input_api.change.issue
- if issue and input_api.rietveld:
- issue_properties = input_api.rietveld.get_issue_properties(
- issue=int(issue), messages=False)
- owner_email = issue_properties['owner_email']
+ if input_api.change.issue:
+ cr = CodeReview(input_api)
+ owner_email = cr.GetOwnerEmail()
try:
authors_content = ''
for line in open(AUTHORS_FILE_NAME):
return results
lgtm_from_owner = False
- issue = input_api.change.issue
- if issue and input_api.rietveld:
- issue_properties = input_api.rietveld.get_issue_properties(
- issue=int(issue), messages=True)
- if re.match(REVERT_CL_SUBJECT_PREFIX, issue_properties['subject'], re.I):
+ if input_api.change.issue:
+ cr = CodeReview(input_api)
+
+ if re.match(REVERT_CL_SUBJECT_PREFIX, cr.GetSubject(), re.I):
# It is a revert CL, ignore the public api owners check.
return results
- if issue_properties['cq_dry_run']:
+ if cr.IsDryRun():
# Ignore public api owners check for dry run CLs since they are not
# going to be committed.
return results
- match = re.search(r'^TBR=(.*)$', issue_properties['description'], re.M)
- if match:
- tbr_entries = match.group(1).strip().split(',')
- for owner in PUBLIC_API_OWNERS:
- if owner in tbr_entries or owner.split('@')[0] in tbr_entries:
- # If an owner is specified in the TBR= line then ignore the public
- # api owners check.
+ if input_api.gerrit:
+ for reviewer in cr.GetReviewers():
+ if reviewer in PUBLIC_API_OWNERS:
+ # If an owner is specified as an reviewer in Gerrit then ignore the
+ # public api owners check.
return results
-
- if issue_properties['owner_email'] in PUBLIC_API_OWNERS:
+ else:
+ match = re.search(r'^TBR=(.*)$', cr.GetDescription(), re.M)
+ if match:
+ tbr_section = match.group(1).strip().split(' ')[0]
+ tbr_entries = tbr_section.split(',')
+ for owner in PUBLIC_API_OWNERS:
+ if owner in tbr_entries or owner.split('@')[0] in tbr_entries:
+ # If an owner is specified in the TBR= line then ignore the public
+ # api owners check.
+ return results
+
+ if cr.GetOwnerEmail() in PUBLIC_API_OWNERS:
# An owner created the CL that is an automatic LGTM.
lgtm_from_owner = True
- messages = issue_properties.get('messages')
- if messages:
- for message in messages:
- if (message['sender'] in PUBLIC_API_OWNERS and
- 'lgtm' in message['text'].lower()):
- # Found an lgtm in a message from an owner.
- lgtm_from_owner = True
- break
+ for approver in cr.GetApprovers():
+ if approver in PUBLIC_API_OWNERS:
+ # Found an lgtm in a message from an owner.
+ lgtm_from_owner = True
+ break
if not lgtm_from_owner:
results.append(
output_api.PresubmitError(
"If this CL adds to or changes Skia's public API, you need an LGTM "
"from any of %s. If this CL only removes from or doesn't change "
- "Skia's public API, please add a short note to the CL saying so "
- "and add one of those reviewers on a TBR= line. If you don't know "
- "if this CL affects Skia's public API, treat it like it does."
- % str(PUBLIC_API_OWNERS)))
+ "Skia's public API, please add a short note to the CL saying so. "
+ "Add one of the owners as a reviewer to your CL. For Rietveld CLs "
+ "you also need to add one of those owners on a TBR= line. If you "
+ "don't know if this CL affects Skia's public API, treat it like it "
+ "does." % str(PUBLIC_API_OWNERS)))
return results
break
issue = cl.issue
- rietveld_obj = cl.RpcServer()
- if issue and rietveld_obj:
- original_description = rietveld_obj.get_description(issue)
- new_description = original_description
+ if issue:
+ original_description_lines, footers = cl.GetDescriptionFooters()
+ new_description_lines = list(original_description_lines)
# If the change includes only doc changes then add NOTRY=true in the
# CL's description if it does not exist yet.
- if all_docs_changes and not re.search(
- r'^NOTRY=true$', new_description, re.M | re.I):
- new_description += '\nNOTRY=true'
+ if all_docs_changes and 'NOTRY=true' not in new_description_lines:
+ new_description_lines.append('NOTRY=true')
results.append(
output_api.PresubmitNotifyResult(
'This change has only doc changes. Automatically added '
# If there is atleast one docs change then add preview link in the CL's
# description if it does not already exist there.
- if atleast_one_docs_change and not re.search(
- r'^DOCS_PREVIEW=.*', new_description, re.M | re.I):
+ docs_preview_line = 'DOCS_PREVIEW= %s%s' % (DOCS_PREVIEW_URL, issue)
+ if (atleast_one_docs_change and
+ docs_preview_line not in new_description_lines):
# Automatically add a link to where the docs can be previewed.
- new_description += '\nDOCS_PREVIEW= %s%s' % (DOCS_PREVIEW_URL, issue)
+ new_description_lines.append(docs_preview_line)
results.append(
output_api.PresubmitNotifyResult(
'Automatically added a link to preview the docs changes to the '
# If the target ref is not master then add NOTREECHECKS=true and NOTRY=true
# to the CL's description if it does not already exist there.
- target_ref = rietveld_obj.get_issue_properties(issue, False).get(
- 'target_ref', '')
- if target_ref != 'refs/heads/master':
- if not re.search(
- r'^NOTREECHECKS=true$', new_description, re.M | re.I):
- new_description += "\nNOTREECHECKS=true"
+ target_ref = cl.GetRemoteBranch()[1]
+ if target_ref != 'refs/remotes/origin/master':
+ if 'NOTREECHECKS=true' not in new_description_lines:
+ new_description_lines.append('NOTREECHECKS=true')
results.append(
output_api.PresubmitNotifyResult(
'Branch changes do not need to rely on the master branch\'s '
'tree status. Automatically added \'NOTREECHECKS=true\' to the '
'CL\'s description'))
- if not re.search(
- r'^NOTRY=true$', new_description, re.M | re.I):
- new_description += "\nNOTRY=true"
+ if 'NOTRY=true' not in new_description_lines:
+ new_description_lines.append('NOTRY=true')
results.append(
output_api.PresubmitNotifyResult(
'Trybots do not yet work for non-master branches. '
'Automatically added \'NOTRY=true\' to the CL\'s description'))
- if not re.search(
- r'^NOPRESUBMIT=true$', new_description, re.M | re.I):
- new_description += "\nNOPRESUBMIT=true"
+ if 'NOPRESUBMIT=true' not in new_description_lines:
+ new_description_lines.append('NOPRESUBMIT=true')
results.append(
output_api.PresubmitNotifyResult(
'Branch changes do not run the presubmit checks.'))
- # Automatically set CQ_EXTRA_TRYBOTS if any of the changed files here begin
- # with the paths of interest.
+ # Automatically set CQ_INCLUDE_TRYBOTS if any of the changed files here
+ # begin with the paths of interest.
cq_master_to_trybots = collections.defaultdict(set)
for affected_file in change.AffectedFiles():
affected_file_path = affected_file.LocalPath()
_MergeCQExtraTrybotsMaps(
cq_master_to_trybots, _GetCQExtraTrybotsMap(extra_bots))
if cq_master_to_trybots:
- new_description = _AddCQExtraTrybotsToDesc(
- cq_master_to_trybots, new_description)
+ _AddCQExtraTrybotsToDesc(cq_master_to_trybots, new_description_lines)
# If the description has changed update it.
- if new_description != original_description:
- rietveld_obj.update_description(issue, new_description)
+ if new_description_lines != original_description_lines:
+ # Add a new line separating the new contents from the old contents.
+ new_description_lines.insert(len(original_description_lines), '')
+ cl.UpdateDescriptionFooters(new_description_lines, footers)
return results
-def _AddCQExtraTrybotsToDesc(cq_master_to_trybots, description):
- """Adds the specified master and trybots to the CQ_EXTRA_TRYBOTS keyword.
+def _AddCQExtraTrybotsToDesc(cq_master_to_trybots, description_lines):
+ """Adds the specified master and trybots to the CQ_INCLUDE_TRYBOTS keyword.
If the keyword already exists in the description then it appends to it only
if the specified values do not already exist.
If the keyword does not exist then it creates a new section in the
description.
"""
- match = re.search(r'^CQ_EXTRA_TRYBOTS=(.*)$', description, re.M | re.I)
- if match:
- original_trybots_map = _GetCQExtraTrybotsMap(match.group(1))
+ found = None
+ foundIdx = -1
+ for idx, line in enumerate(description_lines):
+ if line.startswith('CQ_INCLUDE_TRYBOTS'):
+ found = line
+ foundIdx = idx
+
+ if found:
+ original_trybots_map = _GetCQExtraTrybotsMap(found)
_MergeCQExtraTrybotsMaps(cq_master_to_trybots, original_trybots_map)
- new_description = description.replace(
- match.group(0), _GetCQExtraTrybotsStr(cq_master_to_trybots))
+ new_line = _GetCQExtraTrybotsStr(cq_master_to_trybots)
+ if new_line != found:
+ description_lines[foundIdx] = new_line
else:
- new_description = description + "\n%s" % (
- _GetCQExtraTrybotsStr(cq_master_to_trybots))
- return new_description
+ description_lines.append(_GetCQExtraTrybotsStr(cq_master_to_trybots))
def _MergeCQExtraTrybotsMaps(dest_map, map_to_be_consumed):
def _GetCQExtraTrybotsMap(cq_extra_trybots_str):
- """Parses the CQ_EXTRA_TRYBOTS str and returns a map of masters to trybots."""
+ """Parses CQ_INCLUDE_TRYBOTS str and returns a map of masters to trybots."""
cq_master_to_trybots = collections.defaultdict(set)
for section in cq_extra_trybots_str.split(';'):
if section:
def _GetCQExtraTrybotsStr(cq_master_to_trybots):
- """Constructs the CQ_EXTRA_TRYBOTS str from a map of masters to trybots."""
+ """Constructs the CQ_INCLUDE_TRYBOTS str from a map of masters to trybots."""
sections = []
for master, trybots in cq_master_to_trybots.iteritems():
sections.append('%s:%s' % (master, ','.join(trybots)))
- return 'CQ_EXTRA_TRYBOTS=%s' % ';'.join(sections)
+ return 'CQ_INCLUDE_TRYBOTS=%s' % ';'.join(sections)
def CheckChangeOnCommit(input_api, output_api):
projects / platform / upstream / libSkiaSharp.git / blobdiff