[PATCH 4/4] utils: checkstyle.py: Centralize dependency handling for checkers

[Kieran Bingham]

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 at 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 at 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
>