for more details about the presubmit API built into gcl.
"""
+import collections
import csv
import fnmatch
import os
SKIA_TREE_STATUS_URL = 'http://skia-tree-status.appspot.com'
-CQ_KEYWORDS_THAT_NEED_APPENDING = ('CQ_INCLUDE_TRYBOTS', 'CQ_EXTRA_TRYBOTS',
- 'CQ_EXCLUDE_TRYBOTS', 'CQ_TRYBOTS')
-
# Please add the complete email address here (and not just 'xyz@' or 'xyz').
PUBLIC_API_OWNERS = (
'reed@chromium.org',
'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 https://bug.skia.org/4364
+PATH_PREFIX_TO_EXTRA_TRYBOTS = {
+ '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',
+ # 'src/svg/parser/': 'master3:ghi,jkl;master4:mno',
+ # 'src/image/SkImage_Base.h': 'master5:pqr,stu;master1:abc1;master2:def',
+}
def _CheckChangeHasEol(input_api, output_api, source_file_filter=None):
'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):
# Found a match, the user is in the AUTHORS file break out of the loop
break
else:
- # TODO(rmistry): Remove the below CLA messaging once a CLA checker has
- # been added to the CQ.
results.append(
output_api.PresubmitError(
'The email %s is not in Skia\'s AUTHORS file.\n'
'Issue owner, this CL must include an addition to the Skia AUTHORS '
- 'file.\n'
- 'Googler reviewers, please check that the AUTHORS entry '
- 'corresponds to an email address in http://goto/cla-signers. If it '
- 'does not then ask the issue owner to sign the CLA at '
- 'https://developers.google.com/open-source/cla/individual '
- '(individual) or '
- 'https://developers.google.com/open-source/cla/corporate '
- '(corporate).'
+ 'file.'
% owner_email))
except IOError:
# Do not fail if authors file cannot be found.
affected_file_path = affected_file.LocalPath()
file_path, file_ext = os.path.splitext(affected_file_path)
# We only care about files that end in .h and are under the top-level
- # include dir.
- if file_ext == '.h' and 'include' == file_path.split(os.path.sep)[0]:
+ # include dir, but not include/private.
+ if (file_ext == '.h' and
+ 'include' == file_path.split(os.path.sep)[0] and
+ 'private' not in file_path):
requires_owner_check = True
if not requires_owner_check:
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
- # TODO(rmistry): Stop checking for COMMIT=false once crbug/470609 is
- # resolved.
- if issue_properties['cq_dry_run'] or re.search(
- r'^COMMIT=false$', issue_properties['description'], re.M):
+ 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(
- 'Since the CL is editing public API, you must have an LGTM from '
- 'one of: %s' % str(PUBLIC_API_OWNERS)))
+ "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. "
+ "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
work on them.
* Adds 'NOPRESUBMIT=true' for non master branch changes since those don't
run the presubmit checks.
+ * Adds extra trybots for the paths defined in PATH_TO_EXTRA_TRYBOTS.
"""
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.'))
- # Read and process the HASHTAGS file.
- hashtags_fullpath = os.path.join(change._local_root, 'HASHTAGS')
- with open(hashtags_fullpath, 'rb') as hashtags_csv:
- hashtags_reader = csv.reader(hashtags_csv, delimiter=',')
- for row in hashtags_reader:
- if not row or row[0].startswith('#'):
- # Ignore empty lines and comments
- continue
- hashtag = row[0]
- # Search for the hashtag in the description.
- if re.search('#%s' % hashtag, new_description, re.M | re.I):
- for mapped_text in row[1:]:
- # Special case handling for CQ_KEYWORDS_THAT_NEED_APPENDING.
- appended_description = _HandleAppendingCQKeywords(
- hashtag, mapped_text, new_description, results, output_api)
- if appended_description:
- new_description = appended_description
- continue
-
- # Add the mapped text if it does not already exist in the
- # CL's description.
- if not re.search(
- r'^%s$' % mapped_text, new_description, re.M | re.I):
- new_description += '\n%s' % mapped_text
- results.append(
- output_api.PresubmitNotifyResult(
- 'Found \'#%s\', automatically added \'%s\' to the CL\'s '
- 'description' % (hashtag, mapped_text)))
+ # 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()
+ for path_prefix, extra_bots in PATH_PREFIX_TO_EXTRA_TRYBOTS.iteritems():
+ if affected_file_path.startswith(path_prefix):
+ results.append(
+ output_api.PresubmitNotifyResult(
+ 'Your CL modifies the path %s.\nAutomatically adding %s to '
+ 'the CL description.' % (affected_file_path, extra_bots)))
+ _MergeCQExtraTrybotsMaps(
+ cq_master_to_trybots, _GetCQExtraTrybotsMap(extra_bots))
+ if cq_master_to_trybots:
+ _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 _HandleAppendingCQKeywords(hashtag, keyword_and_value, description,
- results, output_api):
- """Handles the CQ keywords that need appending if specified in hashtags."""
- keyword = keyword_and_value.split('=')[0]
- if keyword in CQ_KEYWORDS_THAT_NEED_APPENDING:
- # If the keyword is already in the description then append to it.
- match = re.search(
- r'^%s=(.*)$' % keyword, description, re.M | re.I)
- if match:
- old_values = match.group(1).split(';')
- new_value = keyword_and_value.split('=')[1]
- if new_value in old_values:
- # Do not need to do anything here.
- return description
- # Update the description with the new values.
- new_description = description.replace(
- match.group(0), "%s;%s" % (match.group(0), new_value))
- results.append(
- output_api.PresubmitNotifyResult(
- 'Found \'#%s\', automatically appended \'%s\' to %s in '
- 'the CL\'s description' % (hashtag, new_value, keyword)))
- return new_description
- return None
+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.
+ """
+ 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_line = _GetCQExtraTrybotsStr(cq_master_to_trybots)
+ if new_line != found:
+ description_lines[foundIdx] = new_line
+ else:
+ description_lines.append(_GetCQExtraTrybotsStr(cq_master_to_trybots))
+
+
+def _MergeCQExtraTrybotsMaps(dest_map, map_to_be_consumed):
+ """Merges two maps of masters to trybots into one."""
+ for master, trybots in map_to_be_consumed.iteritems():
+ dest_map[master].update(trybots)
+ return dest_map
+
+
+def _GetCQExtraTrybotsMap(cq_extra_trybots_str):
+ """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:
+ master, bots = section.split(':')
+ cq_master_to_trybots[master].update(bots.split(','))
+ return cq_master_to_trybots
+
+
+def _GetCQExtraTrybotsStr(cq_master_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_INCLUDE_TRYBOTS=%s' % ';'.join(sections)
def CheckChangeOnCommit(input_api, output_api):
projects / platform / upstream / libSkiaSharp.git / blobdiff