[libcamera-devel] [PATCH v4 2/2] Documentation: Add style checker tool
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Dec 13 21:37:21 CET 2018
Hi Jacopo,
Thank you for the patch.
On Wednesday, 12 December 2018 13:09:36 EET Jacopo Mondi wrote:
> Add the style checker tool 'checkstyle.sh' and add tool documentation
> section to 'coding-style.rst'.
>
> The script is in a very early development stage, and it has been tested
> locally only. Use this a starting point, as we might later consider
> re-implementing it in something that is not shell scripting (as Python, in
> example).
Another example of a problem that I would have sworn was solved already, but
doesn't seem to be :-( If you had asked me a few weeks ago, I would have said
there must be an existing tool to do this.
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> Documentation/coding-style.rst | 45 +++++++++++++
> utils/checkstyle.sh | 141 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 186 insertions(+)
> create mode 100755 utils/checkstyle.sh
>
> diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst
> index 4747927..c2f95c7 100644
> --- a/Documentation/coding-style.rst
> +++ b/Documentation/coding-style.rst
> @@ -78,3 +78,48 @@ C++-11-specific features:
> * General-purpose smart pointers (std::unique_ptr), deprecating
> std::auto_ptr Smart pointers, as well as shared pointers and weak pointers,
> shall not be overused.
> +
> +
> +Tools
> +-----
> +
> +Libcamera provides a style checker scripts that uses 'astyle', to ease
> +identification of style errors before patches gets submitted for review.
> +
> +Right now, these are the basic astyle options used by the project's code
> base:
> +
> +| --style=linux
> +| --indent=force-tab=8
> +| --attach-namespaces
> +| --attach-extern-c
> +| --pad-oper
> +| --align-pointer=name
> +| --align-reference=name
> +| --max-code-length=120
> +
> +Astyle works on full files, and modifies files in place unless instructed
> to +do differently. It can't serve directly as a style validator by
> operating +directly on patches. The libcamera project thus provides a
> 'checkstyle.sh' +script that wraps around git and astyle to get the changes
> recorded in the +top-most commit in the working tree and detect style
> errors.
> +
> +Here is a simple usage example:
> +
> + * Do your file editing, then "git add" and "git commit" as usual.
> + * Run 'checkstyle.sh' on your latest commit: be aware that
> 'checkstyle.sh' + works on commits, so make sure your index is clean.
> +
> +To use the script simply run:
> +
> +.. code-block:: bash
> +
> + $ ./utils/checkstyle.sh
> +
> +The tool outputs the differences between the code added by the last commit
> +and its astyled version, for all source files modified by the commit.
> +
> +Once the script doesn't report any difference, or when the reported
> +differences are false positives according to your best judgment, the
> patches +are ready to be submitted for review.
> +
> +Happy hacking, libcamera awaits your patches!
> diff --git a/utils/checkstyle.sh b/utils/checkstyle.sh
> new file mode 100755
> index 0000000..7f52975
> --- /dev/null
> +++ b/utils/checkstyle.sh
> @@ -0,0 +1,141 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2018, Google Inc.
> +#
> +# Author: Jacopo Mondi <jacopo at jmondi.org>
> +#
> +# checkstyle.sh - A style checker script using astyle for the libcamera
> project +#
> +# The scripts makes use of the following tools, which are expected to be
> +# found in $PATH:
> +# - astyle
> +# - git
> +# - diff
> +# - colordiff
> +
> +ASTYLE=astyle
> +ASTYLE_OPT='-n --style=linux --indent=force-tab=8 --attach-namespaces
> +--attach-extern-c --pad-oper --align-pointer=name --align-reference=name
> +--max-code-length=120'
> +EXTDIFF=colordiff
> +INTDIFF=diff
> +GIT=git
> +TMP=/tmp/
> +
> +# Check for tools to be installed and available in $PATH
> +TOOL_LIST="$ASTYLE $EXTDIFF $INTDIFF $GIT"
> +for T in $TOOL_LIST; do
> + if [ _$(which $T) = '_' ]; then
> + echo $T "missing or not in \$PATH; please install it"
> + exit 1
> + fi
> +done
> +
> +COMMIT_MSG=$($GIT log --format=%s -n1)
> +FLIST=$($GIT diff-index --name-only HEAD^)
> +
> +echo "Running $0 on commit: \"$COMMIT_MSG\""
> +echo
> +echo "The commit modifies the following files:"
> +for F in $FLIST; do echo $F; done
> +echo
> +
> +# Loop on every file modified by the last commit
> +for F in $FLIST; do
> + rm $TMP/chstyle.* &> /dev/null
> + BASENAME=$(basename $F)
> + DIRNAME=$(dirname $F)
> +
> + echo
> + # Skip style check on meson files
> + if [[ $BASENAME == "meson.build" ]]; then
> + echo "================================================================="
> + echo "skip checks on:" $F
> + echo "it's a meson build file"
> + echo "================================================================="
> + continue;
> + fi
> +
> + # Skip style check on hidden files
> + if [[ $BASENAME == '.'* ]]; then
> + echo "================================================================="
> + echo "skip checks on:" $F
> + echo "it's an hidden file"
> + echo "================================================================="
> + continue;
> + fi
> +
> + # Skip Documentation patches
> + if [[ $DIRNAME == 'Documentation' ]]; then
> + echo "================================================================="
> + echo "skip checks on:" $F
> + echo "it's a Documentation file"
> + echo "================================================================="
> + continue;
> + fi
> +
> + # Skip patches on utils/
> + if [[ $DIRNAME == 'utils' ]]; then
> + echo "================================================================="
> + echo "skip checks on:" $F
> + echo "it's a utils/ script"
> + echo "================================================================="
> + continue;
> + fi
> +
> + echo "================================================================="
> + echo "Checking style on file: " $F
> +
> + $GIT show $F > $TMP/chstyle.patch
> + patch -p1 -R < $TMP/chstyle.patch > /dev/null
> + if [[ ! -f "$F" ]]; then
> + # If the file has been added by the last commit, run astyle
> + # on it and report the differences
> + echo
> + echo "Is $BASENAME introduced by this commit? It seems so..."
> + echo "Now running astyle on the whole file: $BASENAME"
> + echo
> + patch -p1 < $TMP/chstyle.patch > /dev/null
> + $ASTYLE $ASTYLE_OPT < $F > $TMP/chstyle.astylepre
> + echo "-----------------------------------------------------------------"
> + $EXTDIFF $F $TMP/chstyle.astylepre
> + echo "-----------------------------------------------------------------"
> + echo
> + echo "If you see nothing here, it means the patch on this file is good!"
> + echo "Otherwise, you may want to check what's wrong with this change"
> + echo "================================================================="
> + continue
> + fi
> +
> + # Run astyle on the file -before- this commit, and then -after-
> + # Record the differences between the two to have the astyled version
> + # of the latest changes
> + $ASTYLE $ASTYLE_OPT < $F > $TMP/chstyle.astylepre
> + patch -p1 < $TMP/chstyle.patch > /dev/null
> + $ASTYLE $ASTYLE_OPT < $F > $TMP/chstyle.astylepost
> + $INTDIFF $TMP/chstyle.astylepre $TMP/chstyle.astylepost \
> + | grep -e '^[\>\<]' \
> + | sed s/^[\>\<]\ // > $TMP/chstyle.astylediff
> +
> + # Sanitize the content of the patch as recorded in the commit
> + grep ^[+-] $TMP/chstyle.patch \
> + | sed s/^[+-]// | sed /^[+-]\.*/d > $TMP/chstyle.patchpost
> +
> + # Now compare the two: raw commit and its astyled version
> + echo "-----------------------------------------------------------------"
> + $EXTDIFF -u $TMP/chstyle.patchpost /tmp/chstyle.astylediff
> + echo "-----------------------------------------------------------------"
> + echo
> + echo "If you see nothing here, it means the patch on this file is good!"
> + echo "Otherwise, you may want to check what's wrong with this change"
> + echo "================================================================="
> +done
> +
> +cat << _END_MSG
> +
> +-----------------------------------------------------------------
> +If checkstyle reports any difference you may want to check what's
> +wrong in your patch before submitting it, otherwise the patch
> +is ready to be sent out!
> +-----------------------------------------------------------------
> +_END_MSG
I really like the idea, but I think we can do better by using a more powerful
programming language. I came up with the following, what do you think ? It's
also work in progress and needs, among other things, a --help option, more
comments, and processing by smaller hunks (splitting hunks with a diff -u 1
option, but retaining 3 lines of context for display).
#!/usr/bin/python3
# SPDX-License-Identifier: GPL-2.0-or-later
# Copyright (C) 2018, Google Inc.
#
# Author: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
#
# checkstyle.py - A patch style checker script based on astyle
import difflib
import os
import re
import shutil
import subprocess
import sys
astyle_options = (
'-n',
'--style=linux',
'--indent=force-tab=8',
'--attach-namespaces',
'--attach-extern-c',
'--pad-oper',
'--align-pointer=name',
'--align-reference=name',
'--max-code-length=120'
)
source_extensions = (
'.c',
'.cpp',
'.h'
)
class DiffHunkSide(object):
"""A side of a diff hunk, recording line numbers"""
def __init__(self, start):
self.start = start
self.touched = []
self.untouched = []
def __len__(self):
return len(self.touched) + len(self.untouched)
class DiffHunk(object):
diff_header_regex = re.compile('@@ -([0-9]+),([0-9]+) \+([0-9]+),([0-9]+) @@')
def __init__(self, line):
match = DiffHunk.diff_header_regex.match(line)
if not match:
raise RuntimeError("Malformed diff hunk header '%s'" % line)
self.__from_line = int(match.group(1))
self.__to_line = int(match.group(3))
self.__from = DiffHunkSide(self.__from_line)
self.__to = DiffHunkSide(self.__to_line)
self.lines = []
def __repr__(self):
s = '\033[0;36m@@ -%u,%u +%u,%u @@\n' % \
(self.__from.start, len(self.__from),
self.__to.start, len(self.__to))
for line in self.lines:
if line[0] == '-':
s += '\033[0;31m'
elif line[0] == '+':
s += '\033[0;32m'
else:
s += '\033[0;0m'
s += line
s += '\033[0;0m'
return s
def append(self, line):
if line[0] == ' ':
self.__from.untouched.append(self.__from_line)
self.__from_line += 1
self.__to.untouched.append(self.__to_line)
self.__to_line += 1
elif line[0] == '-':
self.__from.touched.append(self.__from_line)
self.__from_line += 1
elif line[0] == '+':
self.__to.touched.append(self.__to_line)
self.__to_line += 1
self.lines.append(line)
def intersects(self, lines):
for line in lines:
if line in self.__from.touched:
return True
return False
def side(self, side):
if side == 'from':
return self.__from
else:
return self.__to
def parse_diff(diff):
hunks = []
hunk = None
for line in diff:
if line.startswith('@@'):
if hunk:
hunks.append(hunk)
hunk = DiffHunk(line)
elif hunk is not None:
hunk.append(line)
if hunk:
hunks.append(hunk)
return hunks
def check_file(commit, filename):
# Extract the line numbers touched by the commit.
diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--', filename],
stdout=subprocess.PIPE).stdout
diff = diff.decode('utf-8').splitlines(True)
commit_diff = parse_diff(diff)
lines = []
for hunk in commit_diff:
lines.extend(hunk.side('to').touched)
# Skip commits that don't add any line.
if len(lines) == 0:
return 0
# Apply astyle on the file after the commit and compute the diff between
# the two files.
after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],
stdout=subprocess.PIPE).stdout
astyle = subprocess.run(['astyle', *astyle_options],
input=after, stdout=subprocess.PIPE).stdout
after = after.decode('utf-8').splitlines(True)
astyle = astyle.decode('utf-8').splitlines(True)
diff = difflib.unified_diff(after, astyle)
# Split the diff in hunks, recording line number ranges for each hunk.
astyle_diff = parse_diff(diff)
# Filter out hunks that are not touched by the commit.
astyle_diff = [hunk for hunk in astyle_diff if hunk.intersects(lines)]
if len(astyle_diff) == 0:
return 0
print('\033[0;31m---', filename)
print('\033[0;32m+++', filename)
for hunk in astyle_diff:
print(hunk)
return len(astyle_diff)
def check_style(commit):
# Get the commit title and list of files.
ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],
stdout=subprocess.PIPE)
files = ret.stdout.decode('ascii').splitlines()
title = files[0]
files = files[1:]
separator = '-' * len(title)
print(separator)
print(title)
print(separator)
# Filter out non C/C++ files.
files = [f for f in files if f.endswith(source_extensions)]
if len(files) == 0:
print("Commit doesn't touch source files, skipping")
return
issues = 0
for f in files:
issues += check_file(commit, f)
if issues == 0:
print("No style issue detected")
else:
print('---')
print("%u potential style %s detected, please review" % \
(issues, 'issue' if issues == 1 else 'issues'))
def extract_revlist(revs):
"""Extract a list of commits on which to operate from a revision or revision
range.
"""
ret = subprocess.run(['git', 'rev-parse', revs], stdout=subprocess.PIPE)
revlist = ret.stdout.decode('ascii').splitlines()
# If the revlist contains more than one item, pass it to git rev-list to list
# each commit individually.
if len(revlist) > 1:
ret = subprocess.run(['git', 'rev-list', *revlist], stdout=subprocess.PIPE)
revlist = ret.stdout.decode('ascii').splitlines()
return revlist
def main(argv):
# Check for required dependencies.
dependencies = ('astyle', 'git')
for dependency in dependencies:
if not shutil.which(dependency):
print("Executable %s not found" % dependency)
return 1
# Extract the commits on which to operate.
if len(sys.argv) >= 2:
revs = sys.argv[1]
else:
revs = 'HEAD'
revlist = extract_revlist(revs)
for commit in revlist:
check_style(commit)
print('')
if __name__ == '__main__':
sys.exit(main(sys.argv))
It should be quite trivial to add support for clang-format if we want to.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list