Run hooks and show errors for all projects and commits before quitting.
- Add functionality that formats and prints hook errors and context (project, commit info)
- No longer exit on first error. Print errors for all projects/commits, and exit on error at the end.
- No longer throw exceptions for hook failures, instead return HookFailure instance.
- Move error reporting functionality into errors.py module
- Add hints to help user with common tasks (uploading only current project, updating hooks).
BUG=None
TEST=Ran with errors in multiple repos. Ran with no errors.
Change-Id: I0b539a4f9ce31aac6d12e0239f203082439731df
Reviewed-on: http://gerrit.chromium.org/gerrit/558
Reviewed-by: Ryan Cui <rcui@chromium.org>
Tested-by: Ryan Cui <rcui@chromium.org>
diff --git a/errors.py b/errors.py
new file mode 100644
index 0000000..198d0e2
--- /dev/null
+++ b/errors.py
@@ -0,0 +1,102 @@
+# Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import re
+import sys
+
+
+class VerifyException(Exception):
+ pass
+
+
+class HookFailure(object):
+ """Contains an error message and a list of error details."""
+ def __init__(self, msg, items=None):
+ self.msg = msg
+ self.items = items
+
+
+_INDENT = ' ' * 4
+_PROJECT_INFO = 'Errors in PROJECT *%s*!'
+
+def _PrintWithIndent(msg, indent_level):
+ """Print a block of text with a specified indent level to stderr.
+
+ Args:
+ msg: A string to print (may contain newlines).
+ indent_level: The number of indents to prefix each line with. Each indent
+ is four characters wide.
+ """
+ regex = re.compile(r'^', re.M)
+ msg = regex.sub(_INDENT * indent_level, msg)
+ print >> sys.stderr, msg
+
+
+def _FormatCommitDesc(desc):
+ """Returns the properly prefixed commit description."""
+ regex = re.compile(r'^', re.M)
+ return regex.sub('>', desc)
+
+
+def _FormatHookFailure(hook_failure):
+ """Returns the properly formatted VerifyException as a string."""
+ item_prefix = '\n%s* ' % _INDENT
+ formatted_items = ''
+ if hook_failure.items:
+ formatted_items = item_prefix + item_prefix.join(hook_failure.items)
+ return '* ' + hook_failure.msg + formatted_items
+
+
+def PrintErrorForProject(project, error):
+ """Prints the project and its error.
+
+ Args:
+ project: project name
+ error: An instance of the HookFailure class
+ """
+ _PrintWithIndent(_PROJECT_INFO % project, 0)
+ _PrintWithIndent(_FormatHookFailure(error), 1)
+ print >> sys.stderr, ''
+
+
+def PrintErrorsForCommit(project, commit, commit_desc, error_list):
+ """Prints the hook error to stderr with project and commit context
+
+ A sample error output for a project would be:
+ ----------------------------------------------------------------------------
+ Errors in PROJECT *chromiumos/repohooks*!
+ COMMIT 10041758:
+ Description:
+ >staged
+ >
+ >TEST=some
+ >Change-Id: I2c4f545a20a659541c02be16aa9dc440c876a604
+ >
+ Errors:
+ * Changelist description needs BUG field (after first line)
+ * Found line ending with white space in:
+ * src/repohooks/pre-upload.py, line 307
+ * Found lines longer than 80 characters (first 5 shown):
+ * src/repohooks/pre-upload.py, line 335, 85 chars
+ ----------------------------------------------------------------------------
+
+ Args:
+ project: project name
+ commit: the commit hash the errors belong to
+ commit_desc: a string containing the commit message
+ error_list: a list of HookFailure instances
+ """
+ _PrintWithIndent(_PROJECT_INFO % project, 0)
+
+ formatted_desc = _FormatCommitDesc(commit_desc)
+ _PrintWithIndent('COMMIT %s:' % commit[:8], 1)
+ _PrintWithIndent('Description:', 2)
+ _PrintWithIndent(formatted_desc, 3)
+ _PrintWithIndent('Errors:', 2)
+
+ for error in error_list:
+ _PrintWithIndent(_FormatHookFailure(error), 3)
+
+ print >> sys.stderr, ''
+
diff --git a/pre-upload.py b/pre-upload.py
index b33105b..4b4799c 100644
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -8,10 +8,9 @@
import sys
import subprocess
-class VerifyException(Exception):
- pass
+from errors import (VerifyException, HookFailure, PrintErrorForProject,
+ PrintErrorsForCommit)
-# General Helpers
COMMON_INCLUDED_PATHS = [
# C++ and friends
@@ -26,6 +25,7 @@
r".*\.java$", r".*\.mk$", r".*\.am$",
]
+
COMMON_EXCLUDED_PATHS = [
# avoid doing source file checks for kernel
r"/src/third_party/kernel/",
@@ -37,17 +37,21 @@
r".*[\\\/]debian[\\\/]rules$",
]
-MIN_GIT_VERSION = [1, 7, 2]
+
+# General Helpers
+
def _run_command(cmd):
"""Executes the passed in command and returns raw stdout output."""
return subprocess.Popen(cmd, stdout=subprocess.PIPE).communicate()[0]
+
def _get_hooks_dir():
"""Returns the absolute path to the repohooks directory."""
cmd = ['repo', 'forall', 'chromiumos/repohooks', '-c', 'pwd']
return _run_command(cmd).strip()
+
def _match_regex_list(subject, expressions):
"""Try to match a list of regular expressions to a string.
@@ -63,6 +67,7 @@
return True
return False
+
def _filter_files(files, include_list, exclude_list=[]):
"""Filter out files based on the conditions passed in.
@@ -86,22 +91,10 @@
filtered.append(f)
return filtered
-def _report_error(msg, items=None):
- """Raises an exception with the passed in error message.
-
- If extra error detail is passed in, it will be appended to the error message.
-
- Args:
- msg: Error message header.
- items: A list of lines that follow the header that give extra error
- information.
- """
- if items:
- msg += '\n' + '\n'.join(items)
- raise VerifyException(msg)
-
# Git Helpers
+
+
def _get_upstream_branch():
"""Returns the upstream tracking branch of the current branch.
@@ -111,20 +104,22 @@
current_branch = _run_command(['git', 'symbolic-ref', 'HEAD']).strip()
current_branch = current_branch.replace('refs/heads/', '')
if not current_branch:
- _report_error('Need to be on a tracking branch')
+ raise VerifyException('Need to be on a tracking branch')
cfg_option = 'branch.' + current_branch + '.%s'
full_upstream = _run_command(['git', 'config', cfg_option % 'merge']).strip()
remote = _run_command(['git', 'config', cfg_option % 'remote']).strip()
if not remote or not full_upstream:
- _report_error('Need to be on a tracking branch')
+ raise VerifyException('Need to be on a tracking branch')
return full_upstream.replace('heads', 'remotes/' + remote)
+
def _get_diff(commit):
"""Returns the diff for this commit."""
return _run_command(['git', 'show', commit])
+
def _get_file_diff(file, commit):
"""Returns a list of (linenum, lines) tuples that the commit touched."""
output = _run_command(['git', 'show', '-p', '--no-ext-diff', commit, file])
@@ -142,6 +137,7 @@
line_num += 1
return new_lines
+
def _get_affected_files(commit):
"""Returns list of absolute filepaths that were modified/added."""
output = _run_command(['git', 'diff', '--name-status', commit + '^!'])
@@ -154,11 +150,13 @@
files.append(os.path.join(pwd, m.group(2)))
return files
+
def _get_commits():
"""Returns a list of commits for this review."""
cmd = ['git', 'log', '%s..' % _get_upstream_branch(), '--format=%H']
return _run_command(cmd).split()
+
def _get_commit_desc(commit):
"""Returns the full commit message of a commit."""
return _run_command(['git', 'log', '--format=%s%n%n%b', commit + '^!'])
@@ -166,6 +164,7 @@
# Common Hooks
+
def _check_no_long_lines(project, commit):
"""Checks that there aren't any lines longer than maxlen characters in any of
the text files to be submitted.
@@ -195,7 +194,8 @@
if errors:
msg = 'Found lines longer than %s characters (first 5 shown):' % MAX_LEN
- _report_error(msg, errors)
+ return HookFailure(msg, errors)
+
def _check_no_stray_whitespace(project, commit):
"""Checks that there is no stray whitespace at source lines end."""
@@ -209,7 +209,8 @@
if line.rstrip() != line:
errors.append('%s, line %s' % (afile, line_num))
if errors:
- _report_error('Found line ending with white space in:', errors)
+ return HookFailure('Found line ending with white space in:', errors)
+
def _check_no_tabs(project, commit):
"""Checks there are no unexpanded tabs."""
@@ -232,28 +233,34 @@
if '\t' in line:
errors.append('%s, line %s' % (afile, line_num))
if errors:
- _report_error('Found a tab character in:', errors)
+ return HookFailure('Found a tab character in:', errors)
+
def _check_change_has_test_field(project, commit):
"""Check for a non-empty 'TEST=' field in the commit message."""
TEST_RE = r'\n\s*TEST\s*=[^\n]*\S+'
if not re.search(TEST_RE, _get_commit_desc(commit)):
- _report_error('Changelist description needs TEST field (after first line)')
+ msg = 'Changelist description needs TEST field (after first line)'
+ return HookFailure(msg)
+
def _check_change_has_bug_field(project, commit):
"""Check for a non-empty 'BUG=' field in the commit message."""
BUG_RE = r'\n\s*BUG\s*=[^\n]*\S+'
if not re.search(BUG_RE, _get_commit_desc(commit)):
- _report_error('Changelist description needs BUG field (after first line)')
+ msg = 'Changelist description needs BUG field (after first line)'
+ return HookFailure(msg)
+
def _check_change_has_proper_changeid(project, commit):
"""Verify that Change-ID is present in last paragraph of commit message."""
desc = _get_commit_desc(commit)
loc = desc.rfind('\nChange-Id:')
if loc == -1 or re.search('\n\s*\n\s*\S+', desc[loc:]):
- _report_error('Change-Id must be in last paragraph of description.')
+ return HookFailure('Change-Id must be in last paragraph of description.')
+
def _check_license(project, commit):
"""Verifies the license header."""
@@ -278,13 +285,14 @@
if not license_re.search(contents):
bad_files.append(f)
if bad_files:
- _report_error('License must match:\n%s\n' % license_re.pattern +
- 'Found a bad license header in these files:',
- bad_files)
+ return HookFailure('License must match:\n%s\n' % license_re.pattern +
+ 'Found a bad license header in these files:',
+ bad_files)
# Project-specific hooks
+
def _run_checkpatch(project, commit):
"""Runs checkpatch.pl on the given project"""
hooks_dir = _get_hooks_dir()
@@ -292,7 +300,7 @@
p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE)
output = p.communicate(_get_diff(commit))[0]
if p.returncode:
- _report_error('checkpatch.pl errors/warnings\n\n' + output)
+ return HookFailure('checkpatch.pl errors/warnings\n\n' + output)
def _run_json_check(project, commit):
@@ -301,11 +309,12 @@
try:
json.load(open(f))
except Exception, e:
- _report_error('Invalid JSON in %s: %s' % (f, e))
+ return HookFailure('Invalid JSON in %s: %s' % (f, e))
# Base
+
COMMON_HOOKS = [_check_change_has_bug_field,
_check_change_has_test_field,
_check_change_has_proper_changeid,
@@ -314,6 +323,7 @@
_check_license,
_check_no_tabs]
+
def _setup_project_hooks():
"""Returns a dictionay of callbacks: dict[project] = [callback1, callback2]"""
return {
@@ -322,8 +332,17 @@
"chromeos/autotest-tools": [_run_json_check],
}
+
def _run_project_hooks(project, hooks):
- """For each project run its project specific hook from the hooks dictionary"""
+ """For each project run its project specific hook from the hooks dictionary.
+
+ Args:
+ project: name of project to run hooks for.
+ hooks: a dictionary of hooks indexed by project name
+
+ Returns:
+ Boolean value of whether any errors were ecountered while running the hooks.
+ """
proj_dir = _run_command(['repo', 'forall', project, '-c', 'pwd']).strip()
pwd = os.getcwd()
# hooks assume they are run from the root of the project
@@ -336,39 +355,45 @@
try:
commit_list = _get_commits()
except VerifyException as e:
- print >> sys.stderr, "ERROR: project *%s*" % project
- print >> sys.stderr, e
- raise
+ PrintErrorForProject(project, HookFailure(str(e)))
+ os.chdir(pwd)
+ return True
+ error_found = False
for commit in commit_list:
- try:
- for hook in COMMON_HOOKS + project_specific_hooks:
- hook(project, commit)
- except VerifyException as e:
- msg = 'ERROR: pre-upload failed: commit=%s, project=%s' % (commit[:8],
- project)
-
- print >> sys.stderr, msg
- print >> sys.stderr
- print >> sys.stderr, _get_commit_desc(commit)
- print >> sys.stderr
- print >> sys.stderr, e
-
- raise
+ error_list = []
+ for hook in COMMON_HOOKS + project_specific_hooks:
+ hook_error = hook(project, commit)
+ if hook_error:
+ error_list.append(hook_error)
+ error_found = True
+ if error_list:
+ PrintErrorsForCommit(project, commit, _get_commit_desc(commit),
+ error_list)
os.chdir(pwd)
+ return error_found
# Main
+
def main(project_list, **kwargs):
hooks = _setup_project_hooks()
- try:
- for project in project_list:
- _run_project_hooks(project, hooks)
- except VerifyException as e:
+ found_error = False
+ for project in project_list:
+ if _run_project_hooks(project, hooks):
+ found_error = True
+
+ if (found_error):
+ msg = ('Preupload failed due to errors in project(s). HINTS:\n'
+ '- To upload only current project, run \'repo upload .\'\n'
+ '- Errors may also be due to old upload hooks. Please run '
+ '\'repo sync chromiumos/repohooks\' to update.')
+ print >> sys.stderr, msg
sys.exit(1)
+
if __name__ == '__main__':
main()