From 2d59cfb99222f6099eb4bcc633fa13424cf86df9 Mon Sep 17 00:00:00 2001 From: Aurimas Liutikas Date: Fri, 22 Jan 2016 13:29:56 -0800 Subject: [PATCH] Add support for running Checkstyle on a given SHA-1. Usage e.g.: ./checkstyle.py -s 3aca02ca91816a86febb5ed6e380ec2122adea47 Additionally this CL adds a pre-push script that can be used a git hook to run Checkstyle on 'repo upload' Bug: 25852971 Change-Id: Ia00d48df80b2b024de0899d9590868016c5b7bf0 --- tools/checkstyle/checkstyle.py | 207 ++++++++++++++++++++++++-------- tools/checkstyle/gitlint/git.py | 7 +- tools/checkstyle/pre-push.py | 48 ++++++++ tools/checkstyle/tests.py | 30 ++--- 4 files changed, 225 insertions(+), 67 deletions(-) create mode 100755 tools/checkstyle/pre-push.py diff --git a/tools/checkstyle/checkstyle.py b/tools/checkstyle/checkstyle.py index 3fa0ba00c..014ff634c 100755 --- a/tools/checkstyle/checkstyle.py +++ b/tools/checkstyle/checkstyle.py @@ -21,8 +21,10 @@ import argparse import errno import os +import shutil import subprocess import sys +import tempfile import xml.dom.minidom import gitlint.git as git @@ -35,46 +37,94 @@ FORCED_RULES = ['com.puppycrawl.tools.checkstyle.checks.imports.ImportOrderCheck ERROR_UNCOMMITTED = 'You need to commit all modified files before running Checkstyle\n' ERROR_UNTRACKED = 'You have untracked java files that are not being checked:\n' -def RunCheckstyle(java_files): - if not os.path.exists(CHECKSTYLE_STYLE): - print 'Java checkstyle configuration file is missing' - sys.exit(1) - if java_files: - # Files to check were specified via command line. - print 'Running Checkstyle on inputted files' - sha = '' - last_commit_modified_files = {} - java_files = map(os.path.abspath, java_files) - else: - # Files where not specified exclicitly. Let's use last commit files. - sha, last_commit_modified_files = _GetModifiedFiles() - print 'Running Checkstyle on %s commit' % sha - java_files = last_commit_modified_files.keys() +def RunCheckstyleOnFiles(java_files): + """Runs Checkstyle checks on a given set of java_files. - if not java_files: - print 'No files to check' - return + Args: + java_files: A list of files to check. + Returns: + A tuple of errors and warnings. + """ + print 'Running Checkstyle on inputted files' + java_files = map(os.path.abspath, java_files) stdout = _ExecuteCheckstyle(java_files) - (errors, warnings) = _ParseAndFilterOutput(stdout, - sha, - last_commit_modified_files) + (errors, warnings) = _ParseAndFilterOutput(stdout) + _PrintErrorsAndWarnings(errors, warnings) + return errors, warnings + +def RunCheckstyleOnACommit(commit): + """Runs Checkstyle checks on a given commit. + + It will run Checkstyle on the changed Java files in a specified commit SHA-1 + and if that is None it will fallback to check the latest commit of the + currently checked out branch. + + Args: + commit: A full 40 character SHA-1 of a commit to check. + + Returns: + A tuple of errors and warnings. + """ + if not commit: + _WarnIfUntrackedFiles() + commit = git.last_commit() + print 'Running Checkstyle on %s commit' % commit + commit_modified_files = _GetModifiedFiles(commit) + if not commit_modified_files.keys(): + print 'No Java files to check' + return [], [] + + (tmp_dir, tmp_file_map) = _GetTempFilesForCommit( + commit_modified_files.keys(), commit) + + java_files = tmp_file_map.keys() + stdout = _ExecuteCheckstyle(java_files) + + # Remove all the temporary files. + shutil.rmtree(tmp_dir) + + (errors, warnings) = _ParseAndFilterOutput(stdout, + commit, + commit_modified_files, + tmp_file_map) + _PrintErrorsAndWarnings(errors, warnings) + return errors, warnings + + +def _WarnIfUntrackedFiles(out=sys.stdout): + """Prints a warning and a list of untracked files if needed.""" + root = git.repository_root() + untracked_files = git.modified_files(root, False) + untracked_files = {f for f in untracked_files if f.endswith('.java')} + if untracked_files: + out.write(ERROR_UNTRACKED) + for untracked_file in untracked_files: + out.write(untracked_file + '\n') + out.write('\n') + + +def _PrintErrorsAndWarnings(errors, warnings): + """Prints given errors and warnings.""" if errors: print 'ERRORS:' print '\n'.join(errors) if warnings: print 'WARNINGS:' print '\n'.join(warnings) - if errors or warnings: - sys.exit(1) - - print 'SUCCESS! NO ISSUES FOUND' - sys.exit(0) def _ExecuteCheckstyle(java_files): + """Runs Checkstyle to check give Java files for style errors. + + Args: + java_files: A list of Java files that needs to be checked. + + Returns: + Checkstyle output in XML format. + """ # Run checkstyle checkstyle_env = os.environ.copy() checkstyle_env['JAVA_CMD'] = 'java' @@ -90,32 +140,38 @@ def _ExecuteCheckstyle(java_files): print 'Error running Checkstyle!' sys.exit(1) - # Work-around for Checkstyle printing error count to stdio. + # A work-around for Checkstyle printing error count to stdio. if 'Checkstyle ends with' in stdout.splitlines()[-1]: stdout = '\n'.join(stdout.splitlines()[:-1]) return stdout -def _ParseAndFilterOutput(stdout, sha, last_commit_modified_files): + +def _ParseAndFilterOutput(stdout, + sha=None, + commit_modified_files=None, + tmp_file_map=None): result_errors = [] result_warnings = [] root = xml.dom.minidom.parseString(stdout) for file_element in root.getElementsByTagName('file'): file_name = file_element.attributes['name'].value - if last_commit_modified_files: + if tmp_file_map: + file_name = tmp_file_map[file_name] + if commit_modified_files: modified_lines = git.modified_lines(file_name, - last_commit_modified_files[file_name], + commit_modified_files[file_name], sha) file_name = os.path.relpath(file_name) errors = file_element.getElementsByTagName('error') for error in errors: line = int(error.attributes['line'].value) rule = error.attributes['source'].value - if last_commit_modified_files and _ShouldSkip(modified_lines, line, rule): - continue + if commit_modified_files and _ShouldSkip(modified_lines, line, rule): + continue column = '' if error.hasAttribute('column'): - column = '%s:' % (error.attributes['column'].value) + column = '%s:' % error.attributes['column'].value message = error.attributes['message'].value result = ' %s:%s:%s %s' % (file_name, line, column, message) @@ -124,44 +180,99 @@ def _ParseAndFilterOutput(stdout, sha, last_commit_modified_files): result_errors.append(result) elif severity == 'warning': result_warnings.append(result) - return (result_errors, result_warnings) + return result_errors, result_warnings -# Returns whether an error on a given line should be skipped -# based on the modified_lines list and the rule. def _ShouldSkip(modified_lines, line, rule): + """Returns whether an error on a given line should be skipped. + + Args: + modified_lines: A list of lines that has been modified. + line: The line that has a rule violation. + rule: The type of rule that a given line is violating. + + Returns: + A boolean whether a given line should be skipped in the reporting. + """ # None modified_lines means checked file is new and nothing should be skipped. if modified_lines is None: return False return line not in modified_lines and rule not in FORCED_RULES -def _GetModifiedFiles(out = sys.stdout): +def _GetModifiedFiles(commit, out=sys.stdout): root = git.repository_root() - sha = git.last_commit() pending_files = git.modified_files(root, True) if pending_files: out.write(ERROR_UNCOMMITTED) sys.exit(1) - untracked_files = git.modified_files(root, False) - untracked_files = {f for f in untracked_files if f.endswith('.java')} - if untracked_files: - out.write(ERROR_UNTRACKED) - for untracked_file in untracked_files: - out.write(untracked_file + '\n') - out.write('\n') - modified_files = git.modified_files(root, True, sha) + modified_files = git.modified_files(root, True, commit) modified_files = {f: modified_files[f] for f in modified_files if f.endswith('.java')} - return (sha, modified_files) + return modified_files + + +def _GetTempFilesForCommit(file_names, commit): + """Creates a temporary snapshot of the files in at a commit. + + Retrieves the state of every file in file_names at a given commit and writes + them all out to a temporary directory. + + Args: + file_names: A list of files that need to be retrieved. + commit: A full 40 character SHA-1 of a commit. + + Returns: + A tuple of temprorary directory name and a directionary of + temp_file_name: filename. For example: + + ('/tmp/random/', {'/tmp/random/blarg.java': 'real/path/to/file.java' } + """ + tmp_dir_name = tempfile.mkdtemp() + tmp_file_names = {} + for file_name in file_names: + content = subprocess.check_output( + ['git', 'show', commit + ':' + os.path.relpath(file_name)]) + (fd, tmp_file_name) = tempfile.mkstemp(suffix='.java', + dir=tmp_dir_name, + text=True) + tmp_file = os.fdopen(fd, 'w') + tmp_file.write(content) + tmp_file.close() + tmp_file_names[tmp_file_name] = file_name + return tmp_dir_name, tmp_file_names def main(args=None): + """Runs Checkstyle checks on a given set of java files or a commit. + + It will run Checkstyle on the list of java files first, if unspecified, + then the check will be run on a specified commit SHA-1 and if that + is None it will fallback to check the latest commit of the currently checked + out branch. + """ parser = argparse.ArgumentParser() parser.add_argument('--file', '-f', nargs='+') + parser.add_argument('--sha', '-s') args = parser.parse_args() - RunCheckstyle(args.file) + + if not os.path.exists(CHECKSTYLE_STYLE): + print 'Java checkstyle configuration file is missing' + sys.exit(1) + + if args.file: + # Files to check were specified via command line. + (errors, warnings) = RunCheckstyleOnFiles(args.file) + else: + (errors, warnings) = RunCheckstyleOnACommit(args.sha) + + if errors or warnings: + sys.exit(1) + + print 'SUCCESS! NO ISSUES FOUND' + sys.exit(0) + if __name__ == '__main__': main() diff --git a/tools/checkstyle/gitlint/git.py b/tools/checkstyle/gitlint/git.py index 5d59a75c4..9d796f639 100644 --- a/tools/checkstyle/gitlint/git.py +++ b/tools/checkstyle/gitlint/git.py @@ -110,10 +110,7 @@ def modified_lines(filename, extra_data, commit=None): filename: the file to check. extra_data: is the extra_data returned by modified_files. Additionally, a value of None means that the file was not modified. - commit: the complete sha1 (40 chars) of the commit. Note that specifying - this value will only work (100%) when commit == last_commit (with - respect to the currently checked out revision), otherwise, we could miss - some lines. + commit: the complete sha1 (40 chars) of the commit. Returns: a list of lines that were modified, or None in case all lines are new. @@ -129,7 +126,7 @@ def modified_lines(filename, extra_data, commit=None): # Split as bytes, as the output may have some non unicode characters. blame_lines = subprocess.check_output( - ['git', 'blame', '--porcelain', filename]).split( + ['git', 'blame', commit, '--porcelain', '--', filename]).split( os.linesep.encode('utf-8')) modified_line_numbers = utils.filter_lines( blame_lines, diff --git a/tools/checkstyle/pre-push.py b/tools/checkstyle/pre-push.py new file mode 100755 index 000000000..7f8aec7f0 --- /dev/null +++ b/tools/checkstyle/pre-push.py @@ -0,0 +1,48 @@ +#!/usr/bin/python + +# +# Copyright 2016, The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +"""Pre-push hook to run Checkstyle on changes that are about to be uploaded. + + Usage: add a symbolic link from /path/to/git/repo/.git/hooks/pre-push to + this file. +""" + +import sys +import checkstyle + + +def main(): + print '\nStarting Checkstyle!\n' + sys.stdout.flush() + + ins = raw_input() + (errors, warnings) = checkstyle.RunCheckstyleOnACommit(ins.split(' ')[1]) + + if errors or warnings: + print 'Upload anyway (y/N)?:' + sys.stdin = open('/dev/tty') + sys.stdout.flush() + answer = raw_input() + if 'y' == answer.lower(): + sys.exit(0) + else: + sys.exit(1) + + +if __name__ == '__main__': + main() diff --git a/tools/checkstyle/tests.py b/tools/checkstyle/tests.py index d87fa76d0..1a34e6053 100755 --- a/tools/checkstyle/tests.py +++ b/tools/checkstyle/tests.py @@ -89,37 +89,39 @@ class TestCheckstyle(unittest.TestCase): def test_GetModifiedFiles(self): checkstyle.git.modified_files = mock_modified_files_good out = StringIO() - sha, files = checkstyle._GetModifiedFiles(out) + files = checkstyle._GetModifiedFiles(mock_last_commit(), out=out) output = out.getvalue() self.assertEqual(output, '') - self.assertEqual(sha, TEST_SHA) self.assertEqual(files, {TEST_FILE1: FILE_MODIFIED, TEST_FILE2: FILE_ADDED}) - def test_GetModifiedFilesUntracked(self): - checkstyle.git.modified_files = mock_modified_files_untracked - out = StringIO() - sha, files = checkstyle._GetModifiedFiles(out) - output = out.getvalue() - self.assertEqual(output, checkstyle.ERROR_UNTRACKED + TEST_FILE1 + '\n\n') - self.assertEqual(sha, TEST_SHA) - self.assertEqual(files, {TEST_FILE2: FILE_ADDED}) - def test_GetModifiedFilesUncommitted(self): checkstyle.git.modified_files = mock_modified_files_uncommitted with self.assertRaises(SystemExit): out = StringIO() - checkstyle._GetModifiedFiles(out) + checkstyle._GetModifiedFiles(mock_last_commit(), out=out) self.assertEqual(out.getvalue(), checkstyle.ERROR_UNCOMMITTED) def test_GetModifiedFilesNonJava(self): checkstyle.git.modified_files = mock_modified_files_non_java out = StringIO() - sha, files = checkstyle._GetModifiedFiles(out) + files = checkstyle._GetModifiedFiles(mock_last_commit(), out=out) output = out.getvalue() self.assertEqual(output, '') - self.assertEqual(sha, TEST_SHA) self.assertEqual(files, {TEST_FILE1: FILE_MODIFIED}) + def test_WarnIfUntrackedFiles(self): + checkstyle.git.modified_files = mock_modified_files_untracked + out = StringIO() + checkstyle._WarnIfUntrackedFiles(out=out) + output = out.getvalue() + self.assertEqual(output, checkstyle.ERROR_UNTRACKED + TEST_FILE1 + '\n\n') + + def test_WarnIfUntrackedFilesNoUntracked(self): + checkstyle.git.modified_files = mock_modified_files_good + out = StringIO() + checkstyle._WarnIfUntrackedFiles(out=out) + output = out.getvalue() + self.assertEqual(output, '') if __name__ == '__main__': unittest.main()