hlrs-software-stack/spack
7
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Refactor flake8 handling and tool compatibility (#20376)

Browse source 
This PR does three related things to try to improve developer tooling quality of life:

1. Adds new options to `.flake8` so it applies the rules of both `.flake8` and `.flake_package` based on paths in the repository.
2. Adds a re-factoring of the `spack flake8` logic into a flake8 plugin so using flake8 directly, or through editor or language server integration, only reports errors that `spack flake8` would.
3. Allows star import of `spack.pkgkit` in packages, since this is now the thing that needs to be imported for completion to work correctly in package files, it's nice to be able to do that.

I'm sorely tempted to sed over the whole repository and put `from spack.pkgkit import *` in every package, but at least being allowed to do it on a per-package basis helps.

As an example of what the result of this is:

```
~/Workspace/Projects/spack/spack develop* ⇣
❯ flake8 --format=pylint ./var/spack/repos/builtin/packages/kripke/package.py
./var/spack/repos/builtin/packages/kripke/package.py:6: [F403] 'from spack.pkgkit import *' used; unable to detect undefined names
./var/spack/repos/builtin/packages/kripke/package.py:25: [E501] line too long (88 > 79 characters)

~/Workspace/Projects/spack/spack refactor-flake8*
1 ❯ flake8 --format=spack ./var/spack/repos/builtin/packages/kripke/package.py

~/Workspace/Projects/spack/spack refactor-flake8*
❯ flake8 ./var/spack/repos/builtin/packages/kripke/package.py
```

* qa/flake8: update .flake8, spack formatter plugin

Adds:
* Modern flake8 settings for per-path/glob error ignores, allows
  packages to use the same `.flake8` as the rest of spack
* A spack formatter plugin to flake8 that implements the behavior of
  `spack flake8` for direct invocations.  Makes integration with
  developer tooling nicer, linting with flake8 reports only errors that
  `spack flake8` would report.  Using pyls and pyls-flake8, or any other
  non-format-dependent flake8 integration, now works with spack's rules.

* qa/flake8: allow star import of spack.pkgkit

To get working completion of directives and spack components it's
necessary to import the contents of spack.pkgkit.  At the moment doing
this makes flake8 displeased.  For now, allow spack.pkgkit and spack
both, next step is to ban spack * and require spack.pkgkit *.

* first cut at refactoring spack flake8

This version still copies all of the files to be checked as befire, and
some other things that probably aren't necessary, but it relies on the
spack formatter plugin to implement the ignore logic.

* keep flake8 from rejecting itself

* remove separate packages flake8 config

* fix failures from too many files

I ran into this in the PR converting pkgkit to std.  The solution in
that branch does not work in all cases as it turns out, and all the
workarounds I tried to use generated configs to get a single invocation
of flake8 with a filename optoion to work failed.  It's an astonishingly
frustrating config option.

Regardless, this removes all temporary file creation from the command
and relies on the plugin instead.  To work around the huge number of
files in spack and still allow the command to control what gets checked,
it scans files in batches of 100.  This is a completely arbitrary number
but was chosen to be safely under common line-length limits.  One
side-effect of this is that every 100 files the command will produce
output, rather than only at the end, which doesn't seem like a terrible
thing.
This commit is contained in:
Tom Scogland 2020-12-22 09:28:46 -08:00 committed by GitHub
parent cde6ffe369
commit c1e4f3e131
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 287 additions and 249 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

33
.flake8
View file

@ -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/

24
.flake8_packages
View file

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

331
lib/spack/spack/cmd/flake8.py
View file

@ -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 @@
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 @@ def is_package(f):
#: 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:')
print("Modified files:")
for filename in file_list:
print(' {0}'.format(filename.strip()))
print('=======================================================')
print(" {0}".format(filename.strip()))
print("=======================================================")
# filter files into a temporary directory with exemptions added.
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)
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)
# 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)]
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
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(),
)
)
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))
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)
if returncode != 0:
print("Flake8 found errors.")
sys.exit(1)
else:
print("Flake8 checks were clean.")

146
share/spack/qa/flake8_formatter.py Normal file
View file

@ -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)

2
share/spack/spack-completion.bash
View file

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