[{"id":31854,"web_url":"https://patchwork.libcamera.org/comment/31854/","msgid":"<172951507349.3353069.9565804108071377644@ping.linuxembedded.co.uk>","date":"2024-10-21T12:51:13","subject":"Re: [PATCH 4/4] utils: checkstyle.py: Centralize dependency handling\n\tfor checkers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-10-18 20:32:46)\n> The checkstyle.py script depends on external tools. Those dependencies\n> are handled in different ways in different parts of the code. Centralize\n> the management of checker-specific dependencies to simplify the checkers\n> and output more consistent error messages.\n> \n> This fixes a crash in the Pep8Formatter class when the autopep8\n> dependency is not found.\n> \n> Fixes: 8ffaf376bb53 (\"utils: checkstyle: Add a python formatter\")\n> Signed-off-by: Laurent Pinchart \n> ---\n> utils/checkstyle.py | 81 ++++++++++++++++++++++++++++++---------------\n> 1 file changed, 55 insertions(+), 26 deletions(-)\n> \n> diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> index 3f841a54d54a..f6229bbd86ce 100755\n> --- a/utils/checkstyle.py\n> +++ b/utils/checkstyle.py\n> @@ -23,7 +23,6 @@ import subprocess\n> import sys\n> \n> dependencies = {\n> - 'clang-format': True,\n> 'git': True,\n> }\n> \n> @@ -346,9 +345,6 @@ class ClassRegistry(type):\n> \n> \n> class CheckerBase(metaclass=ClassRegistry):\n> - #\n> - # Class methods\n> - #\n> @classmethod\n> def instances(cls, obj, names):\n> for instance in cls.subclasses:\n> @@ -378,6 +374,22 @@ class CheckerBase(metaclass=ClassRegistry):\n> \n> return patterns\n> \n> + @classmethod\n> + def check_dependencies(cls):\n> + if not hasattr(cls, 'dependencies'):\n> + return []\n> +\n> + issues = []\n> +\n> + for command in cls.dependencies:\n> + if command not in dependencies:\n> + dependencies[command] = shutil.which(command)\n> +\n> + if not dependencies[command]:\n> + issues.append(CommitIssue(f'Missing {command} to run {cls.__name__}'))\n> +\n> + return issues\n> +\n> \n> # ------------------------------------------------------------------------------\n> # Commit Checkers\n> @@ -710,6 +722,7 @@ class MesonChecker(StyleChecker):\n> \n> \n> class ShellChecker(StyleChecker):\n> + dependencies = ('shellcheck',)\n> patterns = ('*.sh',)\n> results_line_regex = re.compile(r'In - line ([0-9]+):')\n> \n> @@ -718,12 +731,8 @@ class ShellChecker(StyleChecker):\n> issues = []\n> data = ''.join(content).encode('utf-8')\n> \n> - try:\n> - ret = subprocess.run(['shellcheck', '-Cnever', '-'],\n> - input=data, stdout=subprocess.PIPE)\n> - except FileNotFoundError:\n> - issues.append(StyleIssue(0, None, None, 'Please install shellcheck to validate shell script additions'))\n> - return issues\n> + ret = subprocess.run(['shellcheck', '-Cnever', '-'],\n> + input=data, stdout=subprocess.PIPE)\n> \n> results = ret.stdout.decode('utf-8').splitlines()\n> for nr, item in enumerate(results):\n> @@ -750,6 +759,7 @@ class Formatter(CheckerBase):\n> \n> \n> class CLangFormatter(Formatter):\n> + dependencies = ('clang-format',)\n> patterns = ('*.c', '*.cpp', '*.h')\n> priority = -1\n> \n> @@ -879,17 +889,13 @@ class IncludeOrderFormatter(Formatter):\n> \n> \n> class Pep8Formatter(Formatter):\n> + dependencies = ('autopep8',)\n> patterns = ('*.py',)\n> \n> @classmethod\n> def format(cls, filename, data):\n> - try:\n> - ret = subprocess.run(['autopep8', '--ignore=E501', '-'],\n> - input=data.encode('utf-8'), stdout=subprocess.PIPE)\n> - except FileNotFoundError:\n> - issues.append(StyleIssue(0, None, None, 'Please install autopep8 to format python additions'))\n> - return issues\n> -\n> + ret = subprocess.run(['autopep8', '--ignore=E501', '-'],\n> + input=data.encode('utf-8'), stdout=subprocess.PIPE)\n> return ret.stdout.decode('utf-8')\n> \n> \n> @@ -908,6 +914,24 @@ class StripTrailingSpaceFormatter(Formatter):\n> # Style checking\n> #\n> \n> +def check_commit(top_level, commit, checkers):\n> + issues = []\n> +\n> + # Apply the commit checkers first.\n> + for checker in CommitChecker.instances(commit, checkers):\n> + issues_ = checker.check_dependencies()\n> + if issues_:\n> + issues += issues_\n> + continue\n> +\n> + issues += checker.check(commit, top_level)\n\nNice. Looks simple, and that's the goal, and now reports dependency\nissues ... as an issue!\n\n\nReviewed-by: Kieran Bingham \n\n> +\n> + for issue in issues:\n> + print(issue)\n> +\n> + return len(issues)\n> +\n> +\n> def check_file(top_level, commit, filename, checkers):\n> # Extract the line numbers touched by the commit.\n> commit_diff = commit.get_diff(top_level, filename)\n> @@ -923,9 +947,15 @@ def check_file(top_level, commit, filename, checkers):\n> # Format the file after the commit with all formatters and compute the diff\n> # between the unformatted and formatted contents.\n> after = commit.get_file(filename)\n> + issues = []\n> \n> formatted = after\n> for formatter in Formatter.instances(filename, checkers):\n> + issues_ = formatter.check_dependencies()\n> + if issues_:\n> + issues += issues_\n> + continue\n> +\n> formatted = formatter.format(filename, formatted)\n> \n> after = after.splitlines(True)\n> @@ -938,8 +968,12 @@ def check_file(top_level, commit, filename, checkers):\n> formatted_diff = [hunk for hunk in formatted_diff if hunk.intersects(lines)]\n> \n> # Check for code issues not related to formatting.\n> - issues = []\n> for checker in StyleChecker.instances(filename, checkers):\n> + issues_ = checker.check_dependencies()\n> + if issues_:\n> + issues += issues_\n> + continue\n> +\n> for hunk in commit_diff:\n> issues += checker.check(after, hunk.side('to').touched)\n> \n> @@ -955,7 +989,7 @@ def check_file(top_level, commit, filename, checkers):\n> print(hunk)\n> \n> if len(issues):\n> - issues = sorted(issues, key=lambda i: i.line_number)\n> + issues = sorted(issues, key=lambda i: getattr(i, 'line_number', -1))\n> for issue in issues:\n> print(issue)\n> \n> @@ -969,13 +1003,8 @@ def check_style(top_level, commit, checkers):\n> print(title)\n> print(separator)\n> \n> - issues = 0\n> -\n> # Apply the commit checkers first.\n> - for checker in CommitChecker.instances(commit, checkers):\n> - for issue in checker.check(commit, top_level):\n> - print(issue)\n> - issues += 1\n> + issues = check_commit(top_level, commit, checkers)\n> \n> # Filter out files we have no checker for.\n> patterns = set()\n> @@ -1047,7 +1076,7 @@ def main(argv):\n> if args.checkers:\n> args.checkers = args.checkers.split(',')\n> \n> - # Check for required dependencies.\n> + # Check for required common dependencies.\n> for command, mandatory in dependencies.items():\n> found = shutil.which(command)\n> if mandatory and not found:\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 20879C32A3\n\tfor ;\n\tMon, 21 Oct 2024 12:51:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E8C365391;\n\tMon, 21 Oct 2024 14:51:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE7A465391\n\tfor ;\n\tMon, 21 Oct 2024 14:51:16 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E10D2316;\n\tMon, 21 Oct 2024 14:49:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AjHQTlQS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729514971;\n\tbh=N0lRTXV50cWbHyQxUae/xSbX604X/b+UEe000eW26FY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=AjHQTlQSb1kzki55ENTzBLTKTgdzjAjlOZTOnwM4Ktuwe1UuEK/ByUSsg4Cz3Q1q0\n\tbaJWv+AUY5dHJoBaInOJQvNAcDiPyH+UmjeZoLgDzLwsmU114n8qHdHIXX+BHo23cM\n\tVGZd2qWU0QN9n4I/Mcc9M/NIUNajCamalNKzhBus=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241018193246.805-5-laurent.pinchart@ideasonboard.com>","References":"<20241018193246.805-1-laurent.pinchart@ideasonboard.com>\n\t<20241018193246.805-5-laurent.pinchart@ideasonboard.com>","Subject":"Re: [PATCH 4/4] utils: checkstyle.py: Centralize dependency handling\n\tfor checkers","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 21 Oct 2024 13:51:13 +0100","Message-ID":"<172951507349.3353069.9565804108071377644@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]