| Message ID | 20241018193246.805-5-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 7ff6fd9774e5697eebddc28170d8cb36b1ad4c37 |
| Headers | show |
| Series |
|
| Related | show |
Quoting Laurent Pinchart (2024-10-18 20:32:46) > The checkstyle.py script depends on external tools. Those dependencies > are handled in different ways in different parts of the code. Centralize > the management of checker-specific dependencies to simplify the checkers > and output more consistent error messages. > > This fixes a crash in the Pep8Formatter class when the autopep8 > dependency is not found. > > Fixes: 8ffaf376bb53 ("utils: checkstyle: Add a python formatter") > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > utils/checkstyle.py | 81 ++++++++++++++++++++++++++++++--------------- > 1 file changed, 55 insertions(+), 26 deletions(-) > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py > index 3f841a54d54a..f6229bbd86ce 100755 > --- a/utils/checkstyle.py > +++ b/utils/checkstyle.py > @@ -23,7 +23,6 @@ import subprocess > import sys > > dependencies = { > - 'clang-format': True, > 'git': True, > } > > @@ -346,9 +345,6 @@ class ClassRegistry(type): > > > class CheckerBase(metaclass=ClassRegistry): > - # > - # Class methods > - # > @classmethod > def instances(cls, obj, names): > for instance in cls.subclasses: > @@ -378,6 +374,22 @@ class CheckerBase(metaclass=ClassRegistry): > > return patterns > > + @classmethod > + def check_dependencies(cls): > + if not hasattr(cls, 'dependencies'): > + return [] > + > + issues = [] > + > + for command in cls.dependencies: > + if command not in dependencies: > + dependencies[command] = shutil.which(command) > + > + if not dependencies[command]: > + issues.append(CommitIssue(f'Missing {command} to run {cls.__name__}')) > + > + return issues > + > > # ------------------------------------------------------------------------------ > # Commit Checkers > @@ -710,6 +722,7 @@ class MesonChecker(StyleChecker): > > > class ShellChecker(StyleChecker): > + dependencies = ('shellcheck',) > patterns = ('*.sh',) > results_line_regex = re.compile(r'In - line ([0-9]+):') > > @@ -718,12 +731,8 @@ class ShellChecker(StyleChecker): > issues = [] > data = ''.join(content).encode('utf-8') > > - try: > - ret = subprocess.run(['shellcheck', '-Cnever', '-'], > - input=data, stdout=subprocess.PIPE) > - except FileNotFoundError: > - issues.append(StyleIssue(0, None, None, 'Please install shellcheck to validate shell script additions')) > - return issues > + ret = subprocess.run(['shellcheck', '-Cnever', '-'], > + input=data, stdout=subprocess.PIPE) > > results = ret.stdout.decode('utf-8').splitlines() > for nr, item in enumerate(results): > @@ -750,6 +759,7 @@ class Formatter(CheckerBase): > > > class CLangFormatter(Formatter): > + dependencies = ('clang-format',) > patterns = ('*.c', '*.cpp', '*.h') > priority = -1 > > @@ -879,17 +889,13 @@ class IncludeOrderFormatter(Formatter): > > > class Pep8Formatter(Formatter): > + dependencies = ('autopep8',) > patterns = ('*.py',) > > @classmethod > def format(cls, filename, data): > - try: > - ret = subprocess.run(['autopep8', '--ignore=E501', '-'], > - input=data.encode('utf-8'), stdout=subprocess.PIPE) > - except FileNotFoundError: > - issues.append(StyleIssue(0, None, None, 'Please install autopep8 to format python additions')) > - return issues > - > + ret = subprocess.run(['autopep8', '--ignore=E501', '-'], > + input=data.encode('utf-8'), stdout=subprocess.PIPE) > return ret.stdout.decode('utf-8') > > > @@ -908,6 +914,24 @@ class StripTrailingSpaceFormatter(Formatter): > # Style checking > # > > +def check_commit(top_level, commit, checkers): > + issues = [] > + > + # Apply the commit checkers first. > + for checker in CommitChecker.instances(commit, checkers): > + issues_ = checker.check_dependencies() > + if issues_: > + issues += issues_ > + continue > + > + issues += checker.check(commit, top_level) Nice. Looks simple, and that's the goal, and now reports dependency issues ... as an issue! Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + > + for issue in issues: > + print(issue) > + > + return len(issues) > + > + > def check_file(top_level, commit, filename, checkers): > # Extract the line numbers touched by the commit. > commit_diff = commit.get_diff(top_level, filename) > @@ -923,9 +947,15 @@ def check_file(top_level, commit, filename, checkers): > # Format the file after the commit with all formatters and compute the diff > # between the unformatted and formatted contents. > after = commit.get_file(filename) > + issues = [] > > formatted = after > for formatter in Formatter.instances(filename, checkers): > + issues_ = formatter.check_dependencies() > + if issues_: > + issues += issues_ > + continue > + > formatted = formatter.format(filename, formatted) > > after = after.splitlines(True) > @@ -938,8 +968,12 @@ def check_file(top_level, commit, filename, checkers): > formatted_diff = [hunk for hunk in formatted_diff if hunk.intersects(lines)] > > # Check for code issues not related to formatting. > - issues = [] > for checker in StyleChecker.instances(filename, checkers): > + issues_ = checker.check_dependencies() > + if issues_: > + issues += issues_ > + continue > + > for hunk in commit_diff: > issues += checker.check(after, hunk.side('to').touched) > > @@ -955,7 +989,7 @@ def check_file(top_level, commit, filename, checkers): > print(hunk) > > if len(issues): > - issues = sorted(issues, key=lambda i: i.line_number) > + issues = sorted(issues, key=lambda i: getattr(i, 'line_number', -1)) > for issue in issues: > print(issue) > > @@ -969,13 +1003,8 @@ def check_style(top_level, commit, checkers): > print(title) > print(separator) > > - issues = 0 > - > # Apply the commit checkers first. > - for checker in CommitChecker.instances(commit, checkers): > - for issue in checker.check(commit, top_level): > - print(issue) > - issues += 1 > + issues = check_commit(top_level, commit, checkers) > > # Filter out files we have no checker for. > patterns = set() > @@ -1047,7 +1076,7 @@ def main(argv): > if args.checkers: > args.checkers = args.checkers.split(',') > > - # Check for required dependencies. > + # Check for required common dependencies. > for command, mandatory in dependencies.items(): > found = shutil.which(command) > if mandatory and not found: > -- > Regards, > > Laurent Pinchart >
diff --git a/utils/checkstyle.py b/utils/checkstyle.py index 3f841a54d54a..f6229bbd86ce 100755 --- a/utils/checkstyle.py +++ b/utils/checkstyle.py @@ -23,7 +23,6 @@ import subprocess import sys dependencies = { - 'clang-format': True, 'git': True, } @@ -346,9 +345,6 @@ class ClassRegistry(type): class CheckerBase(metaclass=ClassRegistry): - # - # Class methods - # @classmethod def instances(cls, obj, names): for instance in cls.subclasses: @@ -378,6 +374,22 @@ class CheckerBase(metaclass=ClassRegistry): return patterns + @classmethod + def check_dependencies(cls): + if not hasattr(cls, 'dependencies'): + return [] + + issues = [] + + for command in cls.dependencies: + if command not in dependencies: + dependencies[command] = shutil.which(command) + + if not dependencies[command]: + issues.append(CommitIssue(f'Missing {command} to run {cls.__name__}')) + + return issues + # ------------------------------------------------------------------------------ # Commit Checkers @@ -710,6 +722,7 @@ class MesonChecker(StyleChecker): class ShellChecker(StyleChecker): + dependencies = ('shellcheck',) patterns = ('*.sh',) results_line_regex = re.compile(r'In - line ([0-9]+):') @@ -718,12 +731,8 @@ class ShellChecker(StyleChecker): issues = [] data = ''.join(content).encode('utf-8') - try: - ret = subprocess.run(['shellcheck', '-Cnever', '-'], - input=data, stdout=subprocess.PIPE) - except FileNotFoundError: - issues.append(StyleIssue(0, None, None, 'Please install shellcheck to validate shell script additions')) - return issues + ret = subprocess.run(['shellcheck', '-Cnever', '-'], + input=data, stdout=subprocess.PIPE) results = ret.stdout.decode('utf-8').splitlines() for nr, item in enumerate(results): @@ -750,6 +759,7 @@ class Formatter(CheckerBase): class CLangFormatter(Formatter): + dependencies = ('clang-format',) patterns = ('*.c', '*.cpp', '*.h') priority = -1 @@ -879,17 +889,13 @@ class IncludeOrderFormatter(Formatter): class Pep8Formatter(Formatter): + dependencies = ('autopep8',) patterns = ('*.py',) @classmethod def format(cls, filename, data): - try: - ret = subprocess.run(['autopep8', '--ignore=E501', '-'], - input=data.encode('utf-8'), stdout=subprocess.PIPE) - except FileNotFoundError: - issues.append(StyleIssue(0, None, None, 'Please install autopep8 to format python additions')) - return issues - + ret = subprocess.run(['autopep8', '--ignore=E501', '-'], + input=data.encode('utf-8'), stdout=subprocess.PIPE) return ret.stdout.decode('utf-8') @@ -908,6 +914,24 @@ class StripTrailingSpaceFormatter(Formatter): # Style checking # +def check_commit(top_level, commit, checkers): + issues = [] + + # Apply the commit checkers first. + for checker in CommitChecker.instances(commit, checkers): + issues_ = checker.check_dependencies() + if issues_: + issues += issues_ + continue + + issues += checker.check(commit, top_level) + + for issue in issues: + print(issue) + + return len(issues) + + def check_file(top_level, commit, filename, checkers): # Extract the line numbers touched by the commit. commit_diff = commit.get_diff(top_level, filename) @@ -923,9 +947,15 @@ def check_file(top_level, commit, filename, checkers): # Format the file after the commit with all formatters and compute the diff # between the unformatted and formatted contents. after = commit.get_file(filename) + issues = [] formatted = after for formatter in Formatter.instances(filename, checkers): + issues_ = formatter.check_dependencies() + if issues_: + issues += issues_ + continue + formatted = formatter.format(filename, formatted) after = after.splitlines(True) @@ -938,8 +968,12 @@ def check_file(top_level, commit, filename, checkers): formatted_diff = [hunk for hunk in formatted_diff if hunk.intersects(lines)] # Check for code issues not related to formatting. - issues = [] for checker in StyleChecker.instances(filename, checkers): + issues_ = checker.check_dependencies() + if issues_: + issues += issues_ + continue + for hunk in commit_diff: issues += checker.check(after, hunk.side('to').touched) @@ -955,7 +989,7 @@ def check_file(top_level, commit, filename, checkers): print(hunk) if len(issues): - issues = sorted(issues, key=lambda i: i.line_number) + issues = sorted(issues, key=lambda i: getattr(i, 'line_number', -1)) for issue in issues: print(issue) @@ -969,13 +1003,8 @@ def check_style(top_level, commit, checkers): print(title) print(separator) - issues = 0 - # Apply the commit checkers first. - for checker in CommitChecker.instances(commit, checkers): - for issue in checker.check(commit, top_level): - print(issue) - issues += 1 + issues = check_commit(top_level, commit, checkers) # Filter out files we have no checker for. patterns = set() @@ -1047,7 +1076,7 @@ def main(argv): if args.checkers: args.checkers = args.checkers.split(',') - # Check for required dependencies. + # Check for required common dependencies. for command, mandatory in dependencies.items(): found = shutil.which(command) if mandatory and not found:
The checkstyle.py script depends on external tools. Those dependencies are handled in different ways in different parts of the code. Centralize the management of checker-specific dependencies to simplify the checkers and output more consistent error messages. This fixes a crash in the Pep8Formatter class when the autopep8 dependency is not found. Fixes: 8ffaf376bb53 ("utils: checkstyle: Add a python formatter") Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- utils/checkstyle.py | 81 ++++++++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 26 deletions(-)