diff options
| -rw-r--r-- | .flake8 | 33 | ||||
| -rw-r--r-- | .flake8_packages | 24 | ||||
| -rw-r--r-- | lib/spack/spack/cmd/flake8.py | 341 | ||||
| -rw-r--r-- | share/spack/qa/flake8_formatter.py | 146 | ||||
| -rwxr-xr-x | share/spack/spack-completion.bash | 2 |
5 files changed, 292 insertions, 254 deletions
@@ -30,3 +30,36 @@ [flake8] ignore = E129,E221,E241,E272,E731,W503,W504,F999,N801,N813,N814 max-line-length = 79 + +# F4: Import +# - F405: `name` may be undefined, or undefined from star imports: `module` +# +# F8: Name +# - F821: undefined name `name` +# +per-file-ignores = + var/spack/repos/*/package.py:F405,F821 + +# exclude things we usually do not want linting for. +# These still get linted when passed explicitly, as when spack flake8 passes +# them on the command line. +exclude = + .git + etc/ + opt/ + share/ + var/spack/cache/ + var/spack/gpg*/ + var/spack/junit-report/ + var/spack/mock-configs/ + lib/spack/external + __pycache__ + var + +format = spack + +[flake8:local-plugins] +report = + spack = flake8_formatter:SpackFormatter +paths = + ./share/spack/qa/ diff --git a/.flake8_packages b/.flake8_packages deleted file mode 100644 index fc2dbf98af..0000000000 --- a/.flake8_packages +++ /dev/null @@ -1,24 +0,0 @@ -# -*- conf -*- -# flake8 settings for Spack package files. -# -# This should include all the same exceptions that we use for core files. -# -# In Spack packages, we also allow the single `from spack import *` -# wildcard import and dependencies can set globals for their -# dependents. So we add exceptions for checks related to undefined names. -# -# Note that we also add *per-line* exemptions for certain patterns in the -# `spack flake8` command. This is where F403 for `from spack import *` -# is added (because we *only* allow that wildcard). -# -# See .flake8 for regular exceptions. -# -# F4: Import -# - F405: `name` may be undefined, or undefined from star imports: `module` -# -# F8: Name -# - F821: undefined name `name` -# -[flake8] -ignore = E129,E221,E241,E272,E731,W503,W504,F405,F821,F999,N801,N813,N814 -max-line-length = 79 diff --git a/lib/spack/spack/cmd/flake8.py b/lib/spack/spack/cmd/flake8.py index cdfce2cab2..ce6dbfed20 100644 --- a/lib/spack/spack/cmd/flake8.py +++ b/lib/spack/spack/cmd/flake8.py @@ -5,14 +5,19 @@ from __future__ import print_function +try: + from itertools import zip_longest # novm +except ImportError: + from itertools import izip_longest # novm + + zip_longest = izip_longest + import re import os import sys -import shutil -import tempfile import argparse -from llnl.util.filesystem import working_dir, mkdirp +from llnl.util.filesystem import working_dir import spack.paths from spack.util.executable import which @@ -23,6 +28,13 @@ section = "developer" level = "long" +def grouper(iterable, n, fillvalue=None): + "Collect data into fixed-length chunks or blocks" + # grouper('ABCDEFG', 3, 'x') --> ABC DEF Gxx" + args = [iter(iterable)] * n + return zip_longest(*args, fillvalue=fillvalue) + + def is_package(f): """Whether flake8 should consider a file as a core file or a package. @@ -30,7 +42,7 @@ def is_package(f): packages, since we allow `from spack import *` and poking globals into packages. """ - return f.startswith('var/spack/repos/') or 'docs/tutorial/examples' in f + return f.startswith("var/spack/repos/") or "docs/tutorial/examples" in f #: List of directories to exclude from checks. @@ -39,96 +51,43 @@ exclude_directories = [spack.paths.external_path] #: max line length we're enforcing (note: this duplicates what's in .flake8) max_line_length = 79 -#: This is a dict that maps: -#: filename pattern -> -#: flake8 exemption code -> -#: list of patterns, for which matching lines should have codes applied. -#: -#: For each file, if the filename pattern matches, we'll add per-line -#: exemptions if any patterns in the sub-dict match. -pattern_exemptions = { - # exemptions applied only to package.py files. - r'package.py$': { - # Allow 'from spack import *' in packages, but no other wildcards - 'F403': [ - r'^from spack import \*$' - ], - # Exempt lines with urls and descriptions from overlong line errors. - 'E501': [ - r'^\s*homepage\s*=', - r'^\s*url\s*=', - r'^\s*git\s*=', - r'^\s*svn\s*=', - r'^\s*hg\s*=', - r'^\s*list_url\s*=', - r'^\s*version\(', - r'^\s*variant\(', - r'^\s*provides\(', - r'^\s*extends\(', - r'^\s*depends_on\(', - r'^\s*conflicts\(', - r'^\s*resource\(', - r'^\s*patch\(', - ], - # Exempt '@when' decorated functions from redefinition errors. - 'F811': [ - r'^\s*@when\(.*\)', - ], - }, - - # exemptions applied to all files. - r'.py$': { - 'E501': [ - r'(https?|ftp|file)\:', # URLs - r'([\'"])[0-9a-fA-F]{32,}\1', # long hex checksums - ] - }, -} - -# compile all regular expressions. -pattern_exemptions = dict( - (re.compile(file_pattern), - dict((code, [re.compile(p) for p in patterns]) - for code, patterns in error_dict.items())) - for file_pattern, error_dict in pattern_exemptions.items()) - def changed_files(base=None, untracked=True, all_files=False): """Get list of changed files in the Spack repository.""" - git = which('git', required=True) + git = which("git", required=True) if base is None: - base = os.environ.get('TRAVIS_BRANCH', 'develop') + base = os.environ.get("TRAVIS_BRANCH", "develop") range = "{0}...".format(base) git_args = [ # Add changed files committed since branching off of develop - ['diff', '--name-only', '--diff-filter=ACMR', range], + ["diff", "--name-only", "--diff-filter=ACMR", range], # Add changed files that have been staged but not yet committed - ['diff', '--name-only', '--diff-filter=ACMR', '--cached'], + ["diff", "--name-only", "--diff-filter=ACMR", "--cached"], # Add changed files that are unstaged - ['diff', '--name-only', '--diff-filter=ACMR'], + ["diff", "--name-only", "--diff-filter=ACMR"], ] # Add new files that are untracked if untracked: - git_args.append(['ls-files', '--exclude-standard', '--other']) + git_args.append(["ls-files", "--exclude-standard", "--other"]) # add everything if the user asked for it if all_files: - git_args.append(['ls-files', '--exclude-standard']) + git_args.append(["ls-files", "--exclude-standard"]) excludes = [os.path.realpath(f) for f in exclude_directories] changed = set() for arg_list in git_args: - files = git(*arg_list, output=str).split('\n') + files = git(*arg_list, output=str).split("\n") for f in files: # Ignore non-Python files - if not (f.endswith('.py') or f == 'bin/spack'): + if not (f.endswith(".py") or f == "bin/spack"): continue # Ignore files in the exclude locations @@ -140,182 +99,106 @@ def changed_files(base=None, untracked=True, all_files=False): return sorted(changed) -def add_pattern_exemptions(line, codes): - """Add a flake8 exemption to a line.""" - if line.startswith('#'): - return line - - line = line.rstrip('\n') - - # Line is already ignored - if line.endswith('# noqa'): - return line + '\n' - - orig_len = len(line) - codes = set(codes) - - # don't add E501 unless the line is actually too long, as it can mask - # other errors like trailing whitespace - if orig_len <= max_line_length and "E501" in codes: - codes.remove("E501") - if not codes: - return line + "\n" - - exemptions = ','.join(sorted(codes)) - - # append exemption to line - if '# noqa: ' in line: - line += ',{0}'.format(exemptions) - elif line: # ignore noqa on empty lines - line += ' # noqa: {0}'.format(exemptions) - - # if THIS made the line too long, add an exemption for that - if len(line) > max_line_length and orig_len <= max_line_length: - line += ',E501' - - return line + '\n' - - -def filter_file(source, dest, output=False): - """Filter a single file through all the patterns in pattern_exemptions.""" - - # Prior to Python 3.8, `noqa: F811` needed to be placed on the `@when` line - # Starting with Python 3.8, it must be placed on the `def` line - # https://gitlab.com/pycqa/flake8/issues/583 - ignore_f811_on_previous_line = False - - with open(source) as infile: - parent = os.path.dirname(dest) - mkdirp(parent) - - with open(dest, 'w') as outfile: - for line in infile: - line_errors = [] - - # pattern exemptions - for file_pattern, errors in pattern_exemptions.items(): - if not file_pattern.search(source): - continue - - for code, patterns in errors.items(): - for pattern in patterns: - if pattern.search(line): - line_errors.append(code) - break - - if 'F811' in line_errors: - ignore_f811_on_previous_line = True - elif ignore_f811_on_previous_line: - line_errors.append('F811') - ignore_f811_on_previous_line = False - - if line_errors: - line = add_pattern_exemptions(line, line_errors) - - outfile.write(line) - if output: - sys.stdout.write(line) - - def setup_parser(subparser): subparser.add_argument( - '-b', '--base', action='store', default=None, - help="select base branch for collecting list of modified files") + "-b", + "--base", + action="store", + default=None, + help="select base branch for collecting list of modified files", + ) subparser.add_argument( - '-k', '--keep-temp', action='store_true', - help="do not delete temporary directory where flake8 runs. " - "use for debugging, to see filtered files") + "-a", + "--all", + action="store_true", + help="check all files, not just changed files", + ) subparser.add_argument( - '-a', '--all', action='store_true', - help="check all files, not just changed files") + "-o", + "--output", + action="store_true", + help="send filtered files to stdout as well as temp files", + ) subparser.add_argument( - '-o', '--output', action='store_true', - help="send filtered files to stdout as well as temp files") + "-r", + "--root-relative", + action="store_true", + default=False, + help="print root-relative paths (default: cwd-relative)", + ) subparser.add_argument( - '-r', '--root-relative', action='store_true', default=False, - help="print root-relative paths (default: cwd-relative)") + "-U", + "--no-untracked", + dest="untracked", + action="store_false", + default=True, + help="exclude untracked files from checks", + ) subparser.add_argument( - '-U', '--no-untracked', dest='untracked', action='store_false', - default=True, help="exclude untracked files from checks") - subparser.add_argument( - 'files', nargs=argparse.REMAINDER, help="specific files to check") + "files", nargs=argparse.REMAINDER, help="specific files to check" + ) def flake8(parser, args): - flake8 = which('flake8', required=True) + file_list = args.files + if file_list: - temp = tempfile.mkdtemp() - try: - file_list = args.files - if file_list: - def prefix_relative(path): - return os.path.relpath( - os.path.abspath(os.path.realpath(path)), - spack.paths.prefix) + def prefix_relative(path): + return os.path.relpath( + os.path.abspath(os.path.realpath(path)), spack.paths.prefix + ) - file_list = [prefix_relative(p) for p in file_list] + file_list = [prefix_relative(p) for p in file_list] - with working_dir(spack.paths.prefix): - if not file_list: - file_list = changed_files(args.base, args.untracked, args.all) + returncode = 0 + with working_dir(spack.paths.prefix): + if not file_list: + file_list = changed_files(args.base, args.untracked, args.all) - print('=======================================================') - print('flake8: running flake8 code checks on spack.') + print("=======================================================") + print("flake8: running flake8 code checks on spack.") print() - print('Modified files:') - for filename in file_list: - print(' {0}'.format(filename.strip())) - print('=======================================================') - - # filter files into a temporary directory with exemptions added. + print("Modified files:") for filename in file_list: - src_path = os.path.join(spack.paths.prefix, filename) - dest_path = os.path.join(temp, filename) - filter_file(src_path, dest_path, args.output) - - # run flake8 on the temporary tree, once for core, once for pkgs - package_file_list = [f for f in file_list if is_package(f)] - file_list = [f for f in file_list if not is_package(f)] - - returncode = 0 - with working_dir(temp): - output = '' - if file_list: - output += flake8( - '--format', 'pylint', - '--config=%s' % os.path.join(spack.paths.prefix, - '.flake8'), - *file_list, fail_on_error=False, output=str) - returncode |= flake8.returncode - if package_file_list: - output += flake8( - '--format', 'pylint', - '--config=%s' % os.path.join(spack.paths.prefix, - '.flake8_packages'), - *package_file_list, fail_on_error=False, output=str) - returncode |= flake8.returncode - - if args.root_relative: - # print results relative to repo root. - print(output) - else: - # print results relative to current working directory - def cwd_relative(path): - return '{0}: ['.format(os.path.relpath( - os.path.join( - spack.paths.prefix, path.group(1)), os.getcwd())) - - for line in output.split('\n'): - print(re.sub(r'^(.*): \[', cwd_relative, line)) - - if returncode != 0: - print('Flake8 found errors.') - sys.exit(1) - else: - print('Flake8 checks were clean.') - - finally: - if args.keep_temp: - print('Temporary files are in: ', temp) - else: - shutil.rmtree(temp, ignore_errors=True) + print(" {0}".format(filename.strip())) + print("=======================================================") + + output = "" + # run in chunks of 100 at a time to avoid line length limit + # filename parameter in config *does not work* for this reliably + for chunk in grouper(file_list, 100): + flake8_cmd = which("flake8", required=True) + chunk = filter(lambda e: e is not None, chunk) + + output = flake8_cmd( + # use .flake8 implicitly to work around bug in flake8 upstream + # append-config is ignored if `--config` is explicitly listed + # see: https://gitlab.com/pycqa/flake8/-/issues/455 + # "--config=.flake8", + *chunk, + fail_on_error=False, + output=str + ) + returncode |= flake8_cmd.returncode + + if args.root_relative: + # print results relative to repo root. + print(output) + else: + # print results relative to current working directory + def cwd_relative(path): + return "{0}: [".format( + os.path.relpath( + os.path.join(spack.paths.prefix, path.group(1)), + os.getcwd(), + ) + ) + + for line in output.split("\n"): + print(re.sub(r"^(.*): \[", cwd_relative, line)) + + if returncode != 0: + print("Flake8 found errors.") + sys.exit(1) + else: + print("Flake8 checks were clean.") diff --git a/share/spack/qa/flake8_formatter.py b/share/spack/qa/flake8_formatter.py new file mode 100644 index 0000000000..0660a97ccf --- /dev/null +++ b/share/spack/qa/flake8_formatter.py @@ -0,0 +1,146 @@ +import re +import sys +import pycodestyle +from collections import defaultdict +from flake8.formatting.default import Pylint +from flake8.style_guide import Violation + +#: This is a dict that maps: +#: filename pattern -> +#: flake8 exemption code -> +#: list of patterns, for which matching lines should have codes applied. +#: +#: For each file, if the filename pattern matches, we'll add per-line +#: exemptions if any patterns in the sub-dict match. +pattern_exemptions = { + # exemptions applied only to package.py files. + r"package.py$": { + # Allow 'from spack import *' in packages, but no other wildcards + "F403": [ + r"^from spack import \*$", + r"^from spack.pkgkit import \*$", + ], + # Exempt lines with urls and descriptions from overlong line errors. + "E501": [ + r"^\s*homepage\s*=", + r"^\s*url\s*=", + r"^\s*git\s*=", + r"^\s*svn\s*=", + r"^\s*hg\s*=", + r"^\s*list_url\s*=", + r"^\s*version\(", + r"^\s*variant\(", + r"^\s*provides\(", + r"^\s*extends\(", + r"^\s*depends_on\(", + r"^\s*conflicts\(", + r"^\s*resource\(", + r"^\s*patch\(", + ], + # Exempt '@when' decorated functions from redefinition errors. + "F811": [ + r"^\s*@when\(.*\)", + ], + }, + # exemptions applied to all files. + r".py$": { + "E501": [ + r"(https?|ftp|file)\:", # URLs + r'([\'"])[0-9a-fA-F]{32,}\1', # long hex checksums + ] + }, +} + + +# compile all regular expressions. +pattern_exemptions = dict( + ( + re.compile(file_pattern), + dict( + (code, [re.compile(p) for p in patterns]) + for code, patterns in error_dict.items() + ), + ) + for file_pattern, error_dict in pattern_exemptions.items() +) + + +class SpackFormatter(Pylint): + def __init__(self, options): + self.spack_errors = {} + self.error_seen = False + super().__init__(options) + + def after_init(self): # type: () -> None + """Overriding to keep format string from being unset in Default""" + pass + + def beginning(self, filename): + self.filename = filename + self.file_lines = None + self.spack_errors = defaultdict(list) + for file_pattern, errors in pattern_exemptions.items(): + if file_pattern.search(filename): + for code, pat_arr in errors.items(): + self.spack_errors[code].extend(pat_arr) + + def handle(self, error): # type: (Violation) -> None + """Handle an error reported by Flake8. + + This defaults to calling :meth:`format`, :meth:`show_source`, and + then :meth:`write`. This version implements the pattern-based ignore + behavior from `spack flake8` as a native flake8 plugin. + + :param error: + This will be an instance of + :class:`~flake8.style_guide.Violation`. + :type error: + flake8.style_guide.Violation + """ + + # print(error.code) + # print(error.physical_line) + # get list of patterns for this error code + pats = self.spack_errors.get(error.code, None) + # if any pattern matches, skip line + if pats is not None and any( + (pat.search(error.physical_line) for pat in pats) + ): + return + + # Special F811 handling + # Prior to Python 3.8, `noqa: F811` needed to be placed on the `@when` + # line + # Starting with Python 3.8, it must be placed on the `def` line + # https://gitlab.com/pycqa/flake8/issues/583 + # we can only determine if F811 should be ignored given the previous + # line, so get the previous line and check it + if ( + self.spack_errors.get("F811", False) + and error.code == "F811" + and error.line_number > 1 + ): + if self.file_lines is None: + if self.filename in {"stdin", "-", "(none)", None}: + self.file_lines = pycodestyle.stdin_get_value().splitlines( + True + ) + else: + self.file_lines = pycodestyle.readlines(self.filename) + for pat in self.spack_errors["F811"]: + if pat.search(self.file_lines[error.line_number - 2]): + return + + self.error_seen = True + line = self.format(error) + source = self.show_source(error) + self.write(line, source) + + def stop(self): + """Override stop to check whether any errors we consider to be errors + were reported. + + This is a hack, but it makes flake8 behave the desired way. + """ + if not self.error_seen: + sys.exit(0) diff --git a/share/spack/spack-completion.bash b/share/spack/spack-completion.bash index 0143d305ca..cbe59bc90a 100755 --- a/share/spack/spack-completion.bash +++ b/share/spack/spack-completion.bash @@ -913,7 +913,7 @@ _spack_find() { _spack_flake8() { if $list_options then - SPACK_COMPREPLY="-h --help -b --base -k --keep-temp -a --all -o --output -r --root-relative -U --no-untracked" + SPACK_COMPREPLY="-h --help -b --base -a --all -o --output -r --root-relative -U --no-untracked" else SPACK_COMPREPLY="" fi |