Use same PRESUBMIT.py as master branch.
It was very out of date in this branch.
Bug: skia:9962
Change-Id: I78be24686a44c9c9729bd75036656bbe373af7a0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/275678
Commit-Queue: Ravi Mistry <rmistry@google.com>
Reviewed-by: Eric Boren <borenet@google.com>
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index 0b7e31f..6660744 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -21,11 +21,8 @@
REVERT_CL_SUBJECT_PREFIX = 'Revert '
-SKIA_TREE_STATUS_URL = 'http://skia-tree-status.appspot.com'
-
# Please add the complete email address here (and not just 'xyz@' or 'xyz').
PUBLIC_API_OWNERS = (
- 'mtklein@chromium.org',
'mtklein@google.com',
'reed@chromium.org',
'reed@google.com',
@@ -40,7 +37,7 @@
AUTHORS_FILE_NAME = 'AUTHORS'
RELEASE_NOTES_FILE_NAME = 'RELEASE_NOTES.txt'
-DOCS_PREVIEW_URL = 'https://skia.org/?cl='
+DOCS_PREVIEW_URL = 'https://skia.org/?cl={issue}'
GOLD_TRYBOT_URL = 'https://gold.skia.org/search?issue='
SERVICE_ACCOUNT_SUFFIX = [
@@ -50,11 +47,11 @@
def _CheckChangeHasEol(input_api, output_api, source_file_filter=None):
- """Checks that files end with atleast one \n (LF)."""
+ """Checks that files end with at least one \n (LF)."""
eof_files = []
for f in input_api.AffectedSourceFiles(source_file_filter):
contents = input_api.ReadFile(f, 'rb')
- # Check that the file ends in atleast one newline character.
+ # Check that the file ends in at least one newline character.
if len(contents) > 1 and contents[-1:] != '\n':
eof_files.append(f.LocalPath())
@@ -147,17 +144,6 @@
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 = []
@@ -176,18 +162,30 @@
def _CheckGNFormatted(input_api, output_api):
"""Make sure any .gn files we're changing have been formatted."""
- results = []
+ files = []
for f in input_api.AffectedFiles():
- if (not f.LocalPath().endswith('.gn') and
- not f.LocalPath().endswith('.gni')):
- continue
+ if (f.LocalPath().endswith('.gn') or
+ f.LocalPath().endswith('.gni')):
+ files.append(f)
+ if not files:
+ return []
- gn = 'gn.bat' if 'win32' in sys.platform else 'gn'
+ cmd = ['python', os.path.join('bin', 'fetch-gn')]
+ try:
+ subprocess.check_output(cmd)
+ except subprocess.CalledProcessError as e:
+ return [output_api.PresubmitError(
+ '`%s` failed:\n%s' % (' '.join(cmd), e.output))]
+
+ results = []
+ for f in files:
+ gn = 'gn.exe' if 'win32' in sys.platform else 'gn'
+ gn = os.path.join(input_api.PresubmitLocalPath(), 'bin', gn)
cmd = [gn, 'format', '--dry-run', f.LocalPath()]
try:
subprocess.check_output(cmd)
except subprocess.CalledProcessError:
- fix = 'gn format ' + f.LocalPath()
+ fix = 'bin/gn format ' + f.LocalPath()
results.append(output_api.PresubmitError(
'`%s` failed, try\n\t%s' % (' '.join(cmd), fix)))
return results
@@ -277,7 +275,6 @@
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))
results.extend(_CheckCompileIsolate(input_api, output_api))
results.extend(_CheckDEPSValid(input_api, output_api))
results.extend(_CheckIncludesFormatted(input_api, output_api))
@@ -285,11 +282,7 @@
def CheckChangeOnUpload(input_api, output_api):
- """Presubmit checks for the change on upload.
-
- The following are the presubmit checks:
- * Check change has one and only one EOL.
- """
+ """Presubmit checks for the change on upload."""
results = []
results.extend(_CommonChecks(input_api, output_api))
# Run on upload, not commit, since the presubmit bot apparently doesn't have
@@ -301,44 +294,6 @@
return results
-def _CheckTreeStatus(input_api, output_api, json_url):
- """Check whether to allow commit.
-
- Args:
- input_api: input related apis.
- output_api: output related apis.
- json_url: url to download json style status.
- """
- tree_status_results = input_api.canned_checks.CheckTreeIsOpen(
- input_api, output_api, json_url=json_url)
- if not tree_status_results:
- # Check for caution state only if tree is not closed.
- connection = input_api.urllib_request.urlopen(json_url)
- status = input_api.json.loads(connection.read())
- connection.close()
- if ('caution' in status['message'].lower() and
- os.isatty(sys.stdout.fileno())):
- # Display a prompt only if we are in an interactive shell. Without this
- # check the commit queue behaves incorrectly because it considers
- # prompts to be failures.
- short_text = 'Tree state is: ' + status['general_state']
- long_text = status['message'] + '\n' + json_url
- tree_status_results.append(
- output_api.PresubmitPromptWarning(
- message=short_text, long_text=long_text))
- else:
- # Tree status is closed. Put in message about contacting sheriff.
- connection = input_api.urllib_request.urlopen(
- SKIA_TREE_STATUS_URL + '/current-sheriff')
- sheriff_details = input_api.json.loads(connection.read())
- if sheriff_details:
- tree_status_results[0]._message += (
- '\n\nPlease contact the current Skia sheriff (%s) if you are trying '
- 'to submit a build fix\nand do not know how to submit because the '
- 'tree is closed') % sheriff_details['username']
- return tree_status_results
-
-
class CodeReview(object):
"""Abstracts which codereview tool is used for the specified issue."""
@@ -355,10 +310,6 @@
def GetDescription(self):
return self._gerrit.GetChangeDescription(self._issue)
- def IsDryRun(self):
- return self._gerrit.GetChangeInfo(
- self._issue)['labels']['Commit-Queue'].get('value', 0) == 1
-
def GetReviewers(self):
code_review_label = (
self._gerrit.GetChangeInfo(self._issue)['labels']['Code-Review'])
@@ -465,11 +416,6 @@
# It is a revert CL, 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 input_api.gerrit:
for reviewer in cr.GetReviewers():
if reviewer in PUBLIC_API_OWNERS:
@@ -509,91 +455,74 @@
return results
-def _FooterExists(footers, key, value):
- for k, v in footers:
- if k == key and v == value:
- return True
- return False
-
-
-def PostUploadHook(cl, change, output_api):
+def PostUploadHook(gerrit, 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 'No-Try: true' if the CL contains only docs changes.
"""
+ if not change.issue:
+ return []
+
+ # Skip PostUploadHooks for all auto-commit service account bots. New
+ # patchsets (caused due to PostUploadHooks) invalidates the CQ+2 vote from
+ # the "--use-commit-queue" flag to "git cl upload".
+ for suffix in SERVICE_ACCOUNT_SUFFIX:
+ if change.author_email.endswith(suffix):
+ return []
results = []
- atleast_one_docs_change = False
+ at_least_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
+ at_least_one_docs_change = True
else:
all_docs_changes = False
- if atleast_one_docs_change and not all_docs_changes:
+ if at_least_one_docs_change and not all_docs_changes:
break
- issue = cl.issue
- if issue:
- # Skip PostUploadHooks for all auto-commit service account bots. New
- # patchsets (caused due to PostUploadHooks) invalidates the CQ+2 vote from
- # the "--use-commit-queue" flag to "git cl upload".
- for suffix in SERVICE_ACCOUNT_SUFFIX:
- if cl.GetIssueOwner().endswith(suffix):
- return results
+ footers = change.GitFootersFromDescription()
+ description_changed = False
- original_description_lines, footers = cl.GetDescriptionFooters()
- new_description_lines = list(original_description_lines)
+ # If the change includes only doc changes then add No-Try: true in the
+ # CL's description if it does not exist yet.
+ if all_docs_changes and 'true' not in footers.get('No-Try', []):
+ description_changed = True
+ change.AddDescriptionFooter('No-Try', 'true')
+ results.append(
+ output_api.PresubmitNotifyResult(
+ 'This change has only doc changes. Automatically added '
+ '\'No-Try: true\' to the CL\'s description'))
- # If the change includes only doc changes then add No-Try: true in the
- # CL's description if it does not exist yet.
- if all_docs_changes and not _FooterExists(footers, 'No-Try', 'true'):
- new_description_lines.append('No-Try: true')
- results.append(
- output_api.PresubmitNotifyResult(
- 'This change has only doc changes. Automatically added '
- '\'No-Try: true\' to the CL\'s description'))
+ # If there is at least one docs change then add preview link in the CL's
+ # description if it does not already exist there.
+ docs_preview_link = DOCS_PREVIEW_URL.format(issue=change.issue)
+ if (at_least_one_docs_change
+ and docs_preview_link not in footers.get('Docs-Preview', [])):
+ # Automatically add a link to where the docs can be previewed.
+ description_changed = True
+ change.AddDescriptionFooter('Docs-Preview', docs_preview_link)
+ results.append(
+ output_api.PresubmitNotifyResult(
+ 'Automatically added a link to preview the docs changes 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_link = '%s%s' % (DOCS_PREVIEW_URL, issue)
- docs_preview_line = 'Docs-Preview: %s' % docs_preview_link
- if (atleast_one_docs_change and
- not _FooterExists(footers, 'Docs-Preview', docs_preview_link)):
- # 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 description has changed update it.
+ if description_changed:
+ gerrit.UpdateDescription(
+ change.FullDescriptionText(), change.issue)
- # 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
+ return results
def CheckChangeOnCommit(input_api, output_api):
- """Presubmit checks for the change on commit.
-
- The following are the presubmit checks:
- * Check change has one and only one EOL.
- * Ensures that the Skia tree is open in
- http://skia-tree-status.appspot.com/. Shows a warning if it is in 'Caution'
- state and an error if it is in 'Closed' state.
- """
+ """Presubmit checks for the change on commit."""
results = []
results.extend(_CommonChecks(input_api, output_api))
- results.extend(
- _CheckTreeStatus(input_api, output_api, json_url=(
- SKIA_TREE_STATUS_URL + '/banner-status?format=json')))
results.extend(_CheckLGTMsForPublicAPI(input_api, output_api))
results.extend(_CheckOwnerIsInAuthorsFile(input_api, output_api))
# Checks for the presence of 'DO NOT''SUBMIT' in CL description and in
