[{"id":49,"web_url":"https://patchwork.libcamera.org/comment/49/","msgid":"<3683023.JIi4j7INfm@avalon>","date":"2018-12-13T20:37:21","subject":"Re: [libcamera-devel] [PATCH v4 2/2] Documentation: Add style\n\tchecker tool","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wednesday, 12 December 2018 13:09:36 EET Jacopo Mondi wrote:\n> Add the style checker tool 'checkstyle.sh' and add tool documentation\n> section to 'coding-style.rst'.\n> \n> The script is in a very early development stage, and it has been tested\n> locally only. Use this a starting point, as we might later consider\n> re-implementing it in something that is not shell scripting (as Python, in\n> example).\n\nAnother example of a problem that I would have sworn was solved already, but\ndoesn't seem to be :-( If you had asked me a few weeks ago, I would have said\nthere must be an existing tool to do this.\n\n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  Documentation/coding-style.rst |  45 +++++++++++++\n>  utils/checkstyle.sh            | 141 ++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 186 insertions(+)\n>  create mode 100755 utils/checkstyle.sh\n> \n> diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst\n> index 4747927..c2f95c7 100644\n> --- a/Documentation/coding-style.rst\n> +++ b/Documentation/coding-style.rst\n> @@ -78,3 +78,48 @@ C++-11-specific features:\n>    * General-purpose smart pointers (std::unique_ptr), deprecating\n> std::auto_ptr Smart pointers, as well as shared pointers and weak pointers,\n> shall not be overused.\n> +\n> +\n> +Tools\n> +-----\n> +\n> +Libcamera provides a style checker scripts that uses 'astyle', to ease\n> +identification of style errors before patches gets submitted for review.\n> +\n> +Right now, these are the basic astyle options used by the project's code\n> base:\n> +\n> +| --style=linux\n> +| --indent=force-tab=8\n> +| --attach-namespaces\n> +| --attach-extern-c\n> +| --pad-oper\n> +| --align-pointer=name\n> +| --align-reference=name\n> +| --max-code-length=120\n> +\n> +Astyle works on full files, and modifies files in place unless instructed\n> to +do differently. It can't serve directly as a style validator by\n> operating +directly on patches. The libcamera project thus provides a\n> 'checkstyle.sh' +script that wraps around git and astyle to get the changes\n> recorded in the +top-most commit in the working tree and detect style\n> errors.\n> +\n> +Here is a simple usage example:\n> +\n> +  * Do your file editing, then \"git add\" and \"git commit\" as usual.\n> +  * Run 'checkstyle.sh' on your latest commit: be aware that\n> 'checkstyle.sh' +    works on commits, so make sure your index is clean.\n> +\n> +To use the script simply run:\n> +\n> +.. code-block:: bash\n> +\n> +        $ ./utils/checkstyle.sh\n> +\n> +The tool outputs the differences between the code added by the last commit\n> +and its astyled version, for all source files modified by the commit.\n> +\n> +Once the script doesn't report any difference, or when the reported\n> +differences are false positives according to your best judgment, the\n> patches +are ready to be submitted for review.\n> +\n> +Happy hacking, libcamera awaits your patches!\n> diff --git a/utils/checkstyle.sh b/utils/checkstyle.sh\n> new file mode 100755\n> index 0000000..7f52975\n> --- /dev/null\n> +++ b/utils/checkstyle.sh\n> @@ -0,0 +1,141 @@\n> +#!/bin/bash\n> +# SPDX-License-Identifier: GPL-2.0-or-later\n> +# Copyright (C) 2018, Google Inc.\n> +#\n> +# Author: Jacopo Mondi <jacopo@jmondi.org>\n> +#\n> +# checkstyle.sh - A style checker script using astyle for the libcamera\n> project +#\n> +# The scripts makes use of the following tools, which are expected to be\n> +# found in $PATH:\n> +# - astyle\n> +# - git\n> +# - diff\n> +# - colordiff\n> +\n> +ASTYLE=astyle\n> +ASTYLE_OPT='-n --style=linux --indent=force-tab=8 --attach-namespaces\n> +--attach-extern-c --pad-oper --align-pointer=name --align-reference=name\n> +--max-code-length=120'\n> +EXTDIFF=colordiff\n> +INTDIFF=diff\n> +GIT=git\n> +TMP=/tmp/\n> +\n> +# Check for tools to be installed and available in $PATH\n> +TOOL_LIST=\"$ASTYLE $EXTDIFF $INTDIFF $GIT\"\n> +for T in $TOOL_LIST; do\n> +\tif [ _$(which $T) = '_' ]; then\n> +\t\techo $T \"missing or not in \\$PATH; please install it\"\n> +\t\texit 1\n> +\tfi\n> +done\n> +\n> +COMMIT_MSG=$($GIT log --format=%s -n1)\n> +FLIST=$($GIT diff-index --name-only HEAD^)\n> +\n> +echo \"Running $0 on commit: \\\"$COMMIT_MSG\\\"\"\n> +echo\n> +echo \"The commit modifies the following files:\"\n> +for F in $FLIST; do echo $F; done\n> +echo\n> +\n> +# Loop on every file modified by the last commit\n> +for F in $FLIST; do\n> +\trm $TMP/chstyle.* &> /dev/null\n> +\tBASENAME=$(basename $F)\n> +\tDIRNAME=$(dirname $F)\n> +\n> +\techo\n> +\t# Skip style check on meson files\n> +\tif [[ $BASENAME == \"meson.build\" ]]; then\n> +\t\techo \"=================================================================\"\n> +\t\techo \"skip checks on:\" $F\n> +\t\techo \"it's a meson build file\"\n> +\t\techo \"=================================================================\"\n> +\t\tcontinue;\n> +\tfi\n> +\n> +\t# Skip style check on hidden files\n> +\tif [[ $BASENAME == '.'* ]]; then\n> +\t\techo \"=================================================================\"\n> +\t\techo \"skip checks on:\" $F\n> +\t\techo \"it's an hidden file\"\n> +\t\techo \"=================================================================\"\n> +\t\tcontinue;\n> +\tfi\n> +\n> +\t# Skip Documentation patches\n> +\tif [[ $DIRNAME == 'Documentation' ]]; then\n> +\t\techo \"=================================================================\"\n> +\t\techo \"skip checks on:\" $F\n> +\t\techo \"it's a Documentation file\"\n> +\t\techo \"=================================================================\"\n> +\t\tcontinue;\n> +\tfi\n> +\n> +\t# Skip patches on utils/\n> +\tif [[ $DIRNAME == 'utils' ]]; then\n> +\t\techo \"=================================================================\"\n> +\t\techo \"skip checks on:\" $F\n> +\t\techo \"it's a utils/ script\"\n> +\t\techo \"=================================================================\"\n> +\t\tcontinue;\n> +\tfi\n> +\n> +\techo \"=================================================================\"\n> +\techo \"Checking style on file: \" $F\n> +\n> +\t$GIT show $F > $TMP/chstyle.patch\n> +\tpatch -p1 -R < $TMP/chstyle.patch > /dev/null\n> +\tif [[ ! -f \"$F\" ]]; then\n> +\t\t# If the file has been added by the last commit, run astyle\n> +\t\t# on it and report the differences\n> +\t\techo\n> +\t\techo \"Is $BASENAME introduced by this commit? It seems so...\"\n> +\t\techo \"Now running astyle on the whole file: $BASENAME\"\n> +\t\techo\n> +\t\tpatch -p1 < $TMP/chstyle.patch > /dev/null\n> +\t\t$ASTYLE $ASTYLE_OPT < $F > $TMP/chstyle.astylepre\n> +\t\techo \"-----------------------------------------------------------------\"\n> +\t\t$EXTDIFF $F $TMP/chstyle.astylepre\n> +\t\techo \"-----------------------------------------------------------------\"\n> +\t\techo\n> +\t\techo \"If you see nothing here, it means the patch on this file is good!\"\n> +\t\techo \"Otherwise, you may want to check what's wrong with this change\"\n> +\t\techo \"=================================================================\"\n> +\t\tcontinue\n> +\tfi\n> +\n> +\t# Run astyle on the file -before- this commit, and then -after-\n> +\t# Record the differences between the two to have the astyled version\n> +\t# of the latest changes\n> +\t$ASTYLE $ASTYLE_OPT < $F > $TMP/chstyle.astylepre\n> +\tpatch -p1 < $TMP/chstyle.patch > /dev/null\n> +\t$ASTYLE $ASTYLE_OPT < $F > $TMP/chstyle.astylepost\n> +\t$INTDIFF $TMP/chstyle.astylepre $TMP/chstyle.astylepost \\\n> +\t\t| grep -e '^[\\>\\<]' \\\n> +\t\t| sed s/^[\\>\\<]\\ // > $TMP/chstyle.astylediff\n> +\n> +\t# Sanitize the content of the patch as recorded in the commit\n> +\tgrep ^[+-] $TMP/chstyle.patch \\\n> +\t\t| sed s/^[+-]// | sed /^[+-]\\.*/d > $TMP/chstyle.patchpost\n> +\n> +\t# Now compare the two: raw commit and its astyled version\n> +\techo \"-----------------------------------------------------------------\"\n> +\t$EXTDIFF -u $TMP/chstyle.patchpost /tmp/chstyle.astylediff\n> +\techo \"-----------------------------------------------------------------\"\n> +\techo\n> +\techo \"If you see nothing here, it means the patch on this file is good!\"\n> +\techo \"Otherwise, you may want to check what's wrong with this change\"\n> +\techo \"=================================================================\"\n> +done\n> +\n> +cat << _END_MSG\n> +\n> +-----------------------------------------------------------------\n> +If checkstyle reports any difference you may want to check what's\n> +wrong in your patch before submitting it, otherwise the patch\n> +is ready to be sent out!\n> +-----------------------------------------------------------------\n> +_END_MSG\n\nI really like the idea, but I think we can do better by using a more powerful\nprogramming language. I came up with the following, what do you think ? It's\nalso work in progress and needs, among other things, a --help option, more\ncomments, and processing by smaller hunks (splitting hunks with a diff -u 1\noption, but retaining 3 lines of context for display).\n\n#!/usr/bin/python3\n# SPDX-License-Identifier: GPL-2.0-or-later\n# Copyright (C) 2018, Google Inc.\n#\n# Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n#\n# checkstyle.py - A patch style checker script based on astyle\n\nimport difflib\nimport os\nimport re\nimport shutil\nimport subprocess\nimport sys\n\nastyle_options = (\n    '-n',\n    '--style=linux',\n    '--indent=force-tab=8',\n    '--attach-namespaces',\n    '--attach-extern-c',\n    '--pad-oper',\n    '--align-pointer=name',\n    '--align-reference=name',\n    '--max-code-length=120'\n)\n\nsource_extensions = (\n    '.c',\n    '.cpp',\n    '.h'\n)\n\nclass DiffHunkSide(object):\n    \"\"\"A side of a diff hunk, recording line numbers\"\"\"\n    def __init__(self, start):\n        self.start = start\n        self.touched = []\n        self.untouched = []\n\n    def __len__(self):\n        return len(self.touched) + len(self.untouched)\n\n\nclass DiffHunk(object):\n    diff_header_regex = re.compile('@@ -([0-9]+),([0-9]+) \\+([0-9]+),([0-9]+) @@')\n\n    def __init__(self, line):\n        match = DiffHunk.diff_header_regex.match(line)\n        if not match:\n            raise RuntimeError(\"Malformed diff hunk header '%s'\" % line)\n\n        self.__from_line = int(match.group(1))\n        self.__to_line = int(match.group(3))\n        self.__from = DiffHunkSide(self.__from_line)\n        self.__to = DiffHunkSide(self.__to_line)\n\n        self.lines = []\n\n    def __repr__(self):\n        s = '\\033[0;36m@@ -%u,%u +%u,%u @@\\n' % \\\n                (self.__from.start, len(self.__from),\n                 self.__to.start, len(self.__to))\n\n        for line in self.lines:\n            if line[0] == '-':\n                s += '\\033[0;31m'\n            elif line[0] == '+':\n                s += '\\033[0;32m'\n            else:\n                s += '\\033[0;0m'\n            s += line\n\n        s += '\\033[0;0m'\n        return s\n\n    def append(self, line):\n        if line[0] == ' ':\n            self.__from.untouched.append(self.__from_line)\n            self.__from_line += 1\n            self.__to.untouched.append(self.__to_line)\n            self.__to_line += 1\n        elif line[0] == '-':\n            self.__from.touched.append(self.__from_line)\n            self.__from_line += 1\n        elif line[0] == '+':\n            self.__to.touched.append(self.__to_line)\n            self.__to_line += 1\n\n        self.lines.append(line)\n\n    def intersects(self, lines):\n        for line in lines:\n            if line in self.__from.touched:\n                return True\n        return False\n\n    def side(self, side):\n        if side == 'from':\n            return self.__from\n        else:\n            return self.__to\n\n\ndef parse_diff(diff):\n    hunks = []\n    hunk = None\n    for line in diff:\n        if line.startswith('@@'):\n            if hunk:\n                hunks.append(hunk)\n            hunk = DiffHunk(line)\n\n        elif hunk is not None:\n            hunk.append(line)\n\n    if hunk:\n        hunks.append(hunk)\n\n    return hunks\n\n\ndef check_file(commit, filename):\n    # Extract the line numbers touched by the commit.\n    diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--', filename],\n                            stdout=subprocess.PIPE).stdout\n    diff = diff.decode('utf-8').splitlines(True)\n    commit_diff = parse_diff(diff)\n\n    lines = []\n    for hunk in commit_diff:\n        lines.extend(hunk.side('to').touched)\n\n    # Skip commits that don't add any line.\n    if len(lines) == 0:\n        return 0\n\n    # Apply astyle on the file after the commit and compute the diff between\n    # the two files.\n    after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n                           stdout=subprocess.PIPE).stdout\n    astyle = subprocess.run(['astyle', *astyle_options],\n                            input=after, stdout=subprocess.PIPE).stdout\n\n    after = after.decode('utf-8').splitlines(True)\n    astyle = astyle.decode('utf-8').splitlines(True)\n\n    diff = difflib.unified_diff(after, astyle)\n\n    # Split the diff in hunks, recording line number ranges for each hunk.\n    astyle_diff = parse_diff(diff)\n\n    # Filter out hunks that are not touched by the commit.\n    astyle_diff = [hunk for hunk in astyle_diff if hunk.intersects(lines)]\n    if len(astyle_diff) == 0:\n        return 0\n\n    print('\\033[0;31m---', filename)\n    print('\\033[0;32m+++', filename)\n    for hunk in astyle_diff:\n        print(hunk)\n\n    return len(astyle_diff)\n\n\ndef check_style(commit):\n    # Get the commit title and list of files.\n    ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],\n                         stdout=subprocess.PIPE)\n    files = ret.stdout.decode('ascii').splitlines()\n    title = files[0]\n    files = files[1:]\n\n    separator = '-' * len(title)\n    print(separator)\n    print(title)\n    print(separator)\n\n    # Filter out non C/C++ files.\n    files = [f for f in files if f.endswith(source_extensions)]\n    if len(files) == 0:\n        print(\"Commit doesn't touch source files, skipping\")\n        return\n\n    issues = 0\n    for f in files:\n        issues += check_file(commit, f)\n\n    if issues == 0:\n        print(\"No style issue detected\")\n    else:\n        print('---')\n        print(\"%u potential style %s detected, please review\" % \\\n                (issues, 'issue' if issues == 1 else 'issues'))\n\n\ndef extract_revlist(revs):\n    \"\"\"Extract a list of commits on which to operate from a revision or revision\n    range.\n    \"\"\"\n    ret = subprocess.run(['git', 'rev-parse', revs], stdout=subprocess.PIPE)\n    revlist = ret.stdout.decode('ascii').splitlines()\n\n    # If the revlist contains more than one item, pass it to git rev-list to list\n    # each commit individually.\n    if len(revlist) > 1:\n        ret = subprocess.run(['git', 'rev-list', *revlist], stdout=subprocess.PIPE)\n        revlist = ret.stdout.decode('ascii').splitlines()\n\n    return revlist\n\n\ndef main(argv):\n\n    # Check for required dependencies.\n    dependencies = ('astyle', 'git')\n\n    for dependency in dependencies:\n        if not shutil.which(dependency):\n            print(\"Executable %s not found\" % dependency)\n            return 1\n\n    # Extract the commits on which to operate.\n    if len(sys.argv) >= 2:\n        revs = sys.argv[1]\n    else:\n        revs = 'HEAD'\n\n    revlist = extract_revlist(revs)\n\n    for commit in revlist:\n        check_style(commit)\n        print('')\n\n\nif __name__ == '__main__':\n    sys.exit(main(sys.argv))\n\n\nIt should be quite trivial to add support for clang-format if we want to.","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 828F460B0B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 13 Dec 2018 21:36:34 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D4819549;\n\tThu, 13 Dec 2018 21:36:33 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1544733394;\n\tbh=hWwkrg8NlgCCpc/8cy061+ReVuvypmvMkiBxQKxJSjA=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=AoIiItYc2UU+1haoL5kDCTDVfILhGvq7KYuJZscopwwBemL2K9xi48/HpHvD62kyS\n\th9AJXPYOtPpYncHQeuSPnbnVAR/HMHa594jf0eDE6BNKsVXFTo7yifh6f/VgrcxIos\n\tzjTFjOxYipNlsI7SOqfcwaykj21kzCiIvphWg8vI=","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"libcamera-devel@lists.libcamera.org","Date":"Thu, 13 Dec 2018 22:37:21 +0200","Message-ID":"<3683023.JIi4j7INfm@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<1544612976-27101-3-git-send-email-jacopo@jmondi.org>","References":"<1544612976-27101-1-git-send-email-jacopo@jmondi.org>\n\t<1544612976-27101-3-git-send-email-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH v4 2/2] Documentation: Add style\n\tchecker tool","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 13 Dec 2018 20:36:34 -0000"}}]