[libcamera-devel,v4,2/2] Documentation: Add style checker tool

39 diff mbox series

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).

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 Documentation/coding-style.rst |  45 +++++++++++++
 utils/checkstyle.sh            | 141 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 186 insertions(+)
 create mode 100755 utils/checkstyle.sh

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@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@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@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.