for more details about the presubmit API built into gcl.
"""
+import collections
+import csv
import fnmatch
import os
import re
+import subprocess
import sys
import traceback
'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):
"""Checks that files end with atleast one \n (LF)."""
'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.
white_list=affected_python_files)
+def _IfDefChecks(input_api, output_api):
+ """Ensures if/ifdef are not before includes. See skbug/3362 for details."""
+ comment_block_start_pattern = re.compile('^\s*\/\*.*$')
+ comment_block_middle_pattern = re.compile('^\s+\*.*')
+ comment_block_end_pattern = re.compile('^\s+\*\/.*$')
+ single_line_comment_pattern = re.compile('^\s*//.*$')
+ def is_comment(line):
+ return (comment_block_start_pattern.match(line) or
+ comment_block_middle_pattern.match(line) or
+ comment_block_end_pattern.match(line) or
+ single_line_comment_pattern.match(line))
+
+ empty_line_pattern = re.compile('^\s*$')
+ def is_empty_line(line):
+ return empty_line_pattern.match(line)
+
+ failing_files = []
+ for affected_file in input_api.AffectedSourceFiles(None):
+ affected_file_path = affected_file.LocalPath()
+ if affected_file_path.endswith('.cpp') or affected_file_path.endswith('.h'):
+ f = open(affected_file_path)
+ for line in f.xreadlines():
+ if is_comment(line) or is_empty_line(line):
+ continue
+ # The below will be the first real line after comments and newlines.
+ if line.startswith('#if 0 '):
+ pass
+ elif line.startswith('#if ') or line.startswith('#ifdef '):
+ failing_files.append(affected_file_path)
+ break
+
+ results = []
+ if failing_files:
+ results.append(
+ output_api.PresubmitError(
+ 'The following files have #if or #ifdef before includes:\n%s\n\n'
+ 'See https://bug.skia.org/3362 for why this should be fixed.' %
+ '\n'.join(failing_files)))
+ return results
+
+
+def _CopyrightChecks(input_api, output_api, source_file_filter=None):
+ results = []
+ year_pattern = r'\d{4}'
+ year_range_pattern = r'%s(-%s)?' % (year_pattern, year_pattern)
+ years_pattern = r'%s(,%s)*,?' % (year_range_pattern, year_range_pattern)
+ copyright_pattern = (
+ r'Copyright (\([cC]\) )?%s \w+' % years_pattern)
+
+ for affected_file in input_api.AffectedSourceFiles(source_file_filter):
+ if 'third_party' in affected_file.LocalPath():
+ continue
+ contents = input_api.ReadFile(affected_file, 'rb')
+ if not re.search(copyright_pattern, contents):
+ results.append(output_api.PresubmitError(
+ '%s is missing a correct copyright header.' % affected_file))
+ return results
+
+
+def _ToolFlags(input_api, output_api):
+ """Make sure `{dm,nanobench}_flags.py test` passes if modified."""
+ results = []
+ sources = lambda x: ('dm_flags.py' in x.LocalPath() or
+ 'nanobench_flags.py' in x.LocalPath())
+ for f in input_api.AffectedSourceFiles(sources):
+ if 0 != subprocess.call(['python', f.LocalPath(), 'test']):
+ results.append(output_api.PresubmitError('`python %s test` failed' % f))
+ 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
+ x.LocalPath().endswith('.mm') or
+ x.LocalPath().endswith('.go') or
+ x.LocalPath().endswith('.c') or
+ x.LocalPath().endswith('.cc') or
x.LocalPath().endswith('.cpp'))
results.extend(
_CheckChangeHasEol(
input_api, output_api, source_file_filter=sources))
results.extend(_PythonChecks(input_api, output_api))
+ results.extend(_IfDefChecks(input_api, output_api))
+ results.extend(_CopyrightChecks(input_api, output_api,
+ source_file_filter=sources))
+ results.extend(_ToolFlags(input_api, output_api))
return results
"""
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.
"""
results = []
requires_owner_check = False
- for affected_svn_file in input_api.AffectedFiles():
- affected_file_path = affected_svn_file.AbsoluteLocalPath()
+ for affected_file in input_api.AffectedFiles():
+ 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 include dir.
- if file_ext == '.h' and 'include' in file_path.split(os.path.sep):
+ # We only care about files that end in .h and are under the top-level
+ # 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
- 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.
- return results
+ if cr.IsDryRun():
+ # Ignore public api owners check for dry run CLs since they are not
+ # going to be committed.
+ return results
- if issue_properties['owner_email'] in PUBLIC_API_OWNERS:
+ 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
+ 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
+def PostUploadHook(cl, change, output_api):
+ """git cl upload will call this hook after the issue is created/modified.
+
+ This hook does the following:
+ * Adds a link to preview docs changes if there are any docs changes in the CL.
+ * Adds 'NOTRY=true' if the CL contains only docs changes.
+ * Adds 'NOTREECHECKS=true' for non master branch changes since they do not
+ need to be gated on the master branch's tree.
+ * Adds 'NOTRY=true' for non master branch changes since trybots do not yet
+ 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 = []
+ atleast_one_docs_change = False
+ all_docs_changes = True
+ for affected_file in change.AffectedFiles():
+ affected_file_path = affected_file.LocalPath()
+ file_path, _ = os.path.splitext(affected_file_path)
+ if 'site' == file_path.split(os.path.sep)[0]:
+ atleast_one_docs_change = True
+ else:
+ all_docs_changes = False
+ if atleast_one_docs_change and not all_docs_changes:
+ break
+
+ issue = cl.issue
+ 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 '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 '
+ '\'NOTRY=true\' to the CL\'s description'))
+
+ # If there is atleast one docs change then add preview link in the CL's
+ # description if it does not already exist there.
+ 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_lines.append(docs_preview_line)
+ results.append(
+ output_api.PresubmitNotifyResult(
+ 'Automatically added a link to preview the docs changes to the '
+ 'CL\'s description'))
+
+ # 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 = 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 '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 '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_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_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_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):
"""Presubmit checks for the change on commit.
projects / platform / upstream / libSkiaSharp.git / blobdiff