From 857749a9ba3619f8190cc8b817d66125cf846c94 Mon Sep 17 00:00:00 2001 From: Tom Scogland Date: Tue, 22 Dec 2020 21:39:10 -0800 Subject: [PATCH] add mypy to style checks; rename `spack flake8` to `spack style` (#20384) I lost my mind a bit after getting the completion stuff working and decided to get Mypy working for spack as well. This adds a `.mypy.ini` that checks all of the spack and llnl modules, though not yet packages, and fixes all of the identified missing types and type issues for the spack library. In addition to these changes, this includes: * rename `spack flake8` to `spack style` Aliases flake8 to style, and just runs flake8 as before, but with a warning. The style command runs both `flake8` and `mypy`, in sequence. Added --no- options to turn off one or the other, they are on by default. Fixed two issues caught by the tools. * stub typing module for python2.x We don't support typing in Spack for python 2.x. To allow 2.x to support `import typing` and `from typing import ...` without a try/except dance to support old versions, this adds a stub module *just* for python 2.x. Doing it this way means we can only reliably use all type hints in python3.7+, and mypi.ini has been updated to reflect that. * add non-default black check to spack style This is a first step to requiring black. It doesn't enforce it by default, but it will check it if requested. Currently enforcing the line length of 79 since that's what flake8 requires, but it's a bit odd for a black formatted project to be quite that narrow. All settings are in the style command since spack has no pyproject.toml and I don't want to add one until more discussion happens. Also re-format `style.py` since it no longer passed the black style check with the new length. * use style check in github action Update the style and docs action to use `spack style`, adding in mypy and black to the action even if it isn't running black right now. --- .flake8 | 12 +- .github/workflows/macos_unit_tests.yaml | 2 +- .github/workflows/style_and_docs.yaml | 12 +- .mypy.ini | 35 ++ bin/spack | 2 + lib/spack/docs/contribution_guide.rst | 22 +- lib/spack/external/py2/typing.py | 84 +++++ lib/spack/llnl/util/tty/log.py | 9 +- lib/spack/spack/architecture.py | 15 +- lib/spack/spack/build_systems/autotools.py | 5 +- lib/spack/spack/build_systems/cmake.py | 3 +- lib/spack/spack/build_systems/makefile.py | 3 +- lib/spack/spack/build_systems/meson.py | 3 +- lib/spack/spack/cmd/flake8.py | 198 +---------- lib/spack/spack/cmd/module.py | 3 +- lib/spack/spack/cmd/style.py | 330 ++++++++++++++++++ lib/spack/spack/compiler.py | 17 +- lib/spack/spack/compilers/__init__.py | 4 +- lib/spack/spack/compilers/nag.py | 6 +- lib/spack/spack/config.py | 3 +- lib/spack/spack/database.py | 12 +- lib/spack/spack/directives.py | 5 +- lib/spack/spack/fetch_strategy.py | 8 +- lib/spack/spack/installer.py | 4 +- lib/spack/spack/mirror.py | 4 +- lib/spack/spack/mixins.py | 16 +- lib/spack/spack/modules/common.py | 3 +- lib/spack/spack/modules/lmod.py | 3 +- lib/spack/spack/modules/tcl.py | 3 +- lib/spack/spack/package.py | 26 +- lib/spack/spack/repo.py | 16 +- lib/spack/spack/schema/environment.py | 7 +- lib/spack/spack/solver/asp.py | 2 +- lib/spack/spack/spec.py | 2 +- lib/spack/spack/stage.py | 3 +- lib/spack/spack/tengine.py | 3 +- lib/spack/spack/test/cmd/flake8.py | 26 +- lib/spack/spack/test/conftest.py | 4 +- lib/spack/spack/test/package_sanity.py | 4 +- lib/spack/spack/test/sbang.py | 2 +- lib/spack/spack/util/crypto.py | 4 +- lib/spack/spack/util/gpg.py | 8 +- lib/spack/spack/util/lock.py | 2 +- lib/spack/spack/util/log_parse.py | 4 +- lib/spack/spack/util/spack_yaml.py | 3 +- lib/spack/spack/util/web.py | 4 +- lib/spack/spack/variant.py | 11 +- .../qa/{run-flake8-tests => run-style-tests} | 4 +- share/spack/qa/setup.sh | 4 + share/spack/spack-completion.bash | 13 +- 50 files changed, 661 insertions(+), 317 deletions(-) create mode 100644 .mypy.ini create mode 100644 lib/spack/external/py2/typing.py create mode 100644 lib/spack/spack/cmd/style.py rename share/spack/qa/{run-flake8-tests => run-style-tests} (92%) diff --git a/.flake8 b/.flake8 index 04ca460eeb..a4c8aa2fbc 100644 --- a/.flake8 +++ b/.flake8 @@ -27,9 +27,17 @@ # - N813: camelcase imported as lowercase # - N814: camelcase imported as constant # +# F4: pyflakes import checks, these are now checked by mypy more precisely +# - F403: from module import * +# - F405: undefined name or from * +# +# Black ignores, these are incompatible with black style and do not follow PEP-8 +# - E203: white space around slice operators can be required, ignore : warn +# - W503: see above, already ignored for line-breaks +# [flake8] -ignore = E129,E221,E241,E272,E731,W503,W504,F999,N801,N813,N814 -max-line-length = 79 +ignore = E129,E221,E241,E272,E731,W503,W504,F999,N801,N813,N814,F403,F405 +max-line-length = 88 # F4: Import # - F405: `name` may be undefined, or undefined from star imports: `module` diff --git a/.github/workflows/macos_unit_tests.yaml b/.github/workflows/macos_unit_tests.yaml index 29caaa2e08..e5918968d2 100644 --- a/.github/workflows/macos_unit_tests.yaml +++ b/.github/workflows/macos_unit_tests.yaml @@ -26,7 +26,7 @@ jobs: run: | pip install --upgrade pip six setuptools pip install --upgrade codecov coverage - pip install --upgrade flake8 pep8-naming + pip install --upgrade flake8 pep8-naming mypy - name: Setup Homebrew packages run: | brew install dash fish gcc gnupg2 kcov diff --git a/.github/workflows/style_and_docs.yaml b/.github/workflows/style_and_docs.yaml index 5abedab784..5a940da99e 100644 --- a/.github/workflows/style_and_docs.yaml +++ b/.github/workflows/style_and_docs.yaml @@ -22,10 +22,10 @@ jobs: pip install --upgrade pip pip install --upgrade vermin - name: Minimum Version (Spack's Core) - run: vermin --backport argparse -t=2.6- -t=3.5- -v lib/spack/spack/ lib/spack/llnl/ bin/ + run: vermin --backport argparse --backport typing -t=2.6- -t=3.5- -v lib/spack/spack/ lib/spack/llnl/ bin/ - name: Minimum Version (Repositories) - run: vermin --backport argparse -t=2.6- -t=3.5- -v var/spack/repos - flake8: + run: vermin --backport argparse --backport typing -t=2.6- -t=3.5- -v var/spack/repos + style: runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 @@ -36,15 +36,15 @@ jobs: python-version: 3.9 - name: Install Python packages run: | - pip install --upgrade pip six setuptools flake8 + pip install --upgrade pip six setuptools flake8 mypy black - name: Setup git configuration run: | # Need this for the git tests to succeed. git --version . .github/workflows/setup_git.sh - - name: Run flake8 tests + - name: Run style tests run: | - share/spack/qa/run-flake8-tests + share/spack/qa/run-style-tests documentation: runs-on: ubuntu-latest steps: diff --git a/.mypy.ini b/.mypy.ini new file mode 100644 index 0000000000..4257b1bd3f --- /dev/null +++ b/.mypy.ini @@ -0,0 +1,35 @@ +[mypy] +python_version = 3.7 +files=lib/spack/llnl/**/*.py,lib/spack/spack/**/*.py +mypy_path=bin,lib/spack,lib/spack/external,var/spack/repos/builtin +# This and a generated import file allows supporting packages +namespace_packages=True +# To avoid re-factoring all the externals, ignore errors and missing imports +# globally, then turn back on in spack and spack submodules +ignore_errors=True +ignore_missing_imports=True + +[mypy-spack.*] +ignore_errors=False +ignore_missing_imports=False + +[mypy-packages.*] +ignore_errors=False +ignore_missing_imports=False + +[mypy-llnl.*] +ignore_errors=False +ignore_missing_imports=False + +[mypy-spack.test.packages] +ignore_errors=True + +# ignore errors in fake import path for packages +[mypy-spack.pkg.*] +ignore_errors=True +ignore_missing_imports=True + +# jinja has syntax in it that requires python3 and causes a parse error +# skip importing it +[mypy-jinja2] +follow_imports=skip diff --git a/bin/spack b/bin/spack index 7e2e7e06de..1f9e257b8d 100755 --- a/bin/spack +++ b/bin/spack @@ -42,6 +42,8 @@ sys.path.insert(0, spack_lib_path) # Add external libs spack_external_libs = os.path.join(spack_lib_path, "external") +if sys.version_info[:2] <= (2, 7): + sys.path.insert(0, os.path.join(spack_external_libs, 'py2')) if sys.version_info[:2] == (2, 6): sys.path.insert(0, os.path.join(spack_external_libs, 'py26')) diff --git a/lib/spack/docs/contribution_guide.rst b/lib/spack/docs/contribution_guide.rst index 8df8ad65ba..f554a76d29 100644 --- a/lib/spack/docs/contribution_guide.rst +++ b/lib/spack/docs/contribution_guide.rst @@ -179,24 +179,26 @@ how to write tests! run the unit tests yourself, we suggest you use ``spack unit-test``. ^^^^^^^^^^^^ -Flake8 Tests +Style Tests ^^^^^^^^^^^^ Spack uses `Flake8 `_ to test for -`PEP 8 `_ conformance. PEP 8 is +`PEP 8 `_ conformance and +`mypy ` for type checking. PEP 8 is a series of style guides for Python that provide suggestions for everything from variable naming to indentation. In order to limit the number of PRs that were mostly style changes, we decided to enforce PEP 8 conformance. Your PR -needs to comply with PEP 8 in order to be accepted. +needs to comply with PEP 8 in order to be accepted, and if it modifies the +spack library it needs to successfully type-check with mypy as well. -Testing for PEP 8 compliance is easy. Simply run the ``spack flake8`` +Testing for compliance with spack's style is easy. Simply run the ``spack style`` command: .. code-block:: console - $ spack flake8 + $ spack style -``spack flake8`` has a couple advantages over running ``flake8`` by hand: +``spack style`` has a couple advantages over running the tools by hand: #. It only tests files that you have modified since branching off of ``develop``. @@ -207,7 +209,9 @@ command: checks. For example, URLs are often longer than 80 characters, so we exempt them from line length checks. We also exempt lines that start with "homepage", "url", "version", "variant", "depends_on", and - "extends" in ``package.py`` files. + "extends" in ``package.py`` files. This is now also possible when directly + running flake8 if you can use the ``spack`` formatter plugin included with + spack. More approved flake8 exemptions can be found `here `_. @@ -240,13 +244,13 @@ However, if you aren't compliant with PEP 8, flake8 will complain: Most of the error messages are straightforward, but if you don't understand what they mean, just ask questions about them when you submit your PR. The line numbers -will change if you add or delete lines, so simply run ``spack flake8`` again +will change if you add or delete lines, so simply run ``spack style`` again to update them. .. tip:: Try fixing flake8 errors in reverse order. This eliminates the need for - multiple runs of ``spack flake8`` just to re-compute line numbers and + multiple runs of ``spack style`` just to re-compute line numbers and makes it much easier to fix errors directly off of the CI output. .. warning:: diff --git a/lib/spack/external/py2/typing.py b/lib/spack/external/py2/typing.py new file mode 100644 index 0000000000..2b06797660 --- /dev/null +++ b/lib/spack/external/py2/typing.py @@ -0,0 +1,84 @@ +# Copyright 2013-2020 Lawrence Livermore National Security, LLC and other +# Spack Project Developers. See the top-level COPYRIGHT file for details. +# +# SPDX-License-Identifier: (Apache-2.0 OR MIT) +""" +This is a fake set of symbols to allow spack to import typing in python +versions where we do not support type checking (<3) +""" +Annotated = None +Any = None +Callable = None +ForwardRef = None +Generic = None +Literal = None +Optional = None +Tuple = None +TypeVar = None +Union = None +AbstractSet = None +ByteString = None +Container = None +Hashable = None +ItemsView = None +Iterable = None +Iterator = None +KeysView = None +Mapping = None +MappingView = None +MutableMapping = None +MutableSequence = None +MutableSet = None +Sequence = None +Sized = None +ValuesView = None +Awaitable = None +AsyncIterator = None +AsyncIterable = None +Coroutine = None +Collection = None +AsyncGenerator = None +AsyncContextManager = None +Reversible = None +SupportsAbs = None +SupportsBytes = None +SupportsComplex = None +SupportsFloat = None +SupportsInt = None +SupportsRound = None +ChainMap = None +Dict = None +List = None +OrderedDict = None +Set = None +FrozenSet = None +NamedTuple = None +Generator = None +AnyStr = None +cast = None +get_args = None +get_origin = None +get_type_hints = None +no_type_check = None +no_type_check_decorator = None +NoReturn = None + +# these are the typing extension symbols +ClassVar = None +Final = None +Protocol = None +Type = None +TypedDict = None +ContextManager = None +Counter = None +Deque = None +DefaultDict = None +SupportsIndex = None +final = None +IntVar = None +Literal = None +NewType = None +overload = None +runtime_checkable = None +Text = None +TYPE_CHECKING = None diff --git a/lib/spack/llnl/util/tty/log.py b/lib/spack/llnl/util/tty/log.py index 658de5c596..4f6acd7a1e 100644 --- a/lib/spack/llnl/util/tty/log.py +++ b/lib/spack/llnl/util/tty/log.py @@ -20,12 +20,17 @@ from six import string_types from six import StringIO +from typing import Optional # novm +from types import ModuleType # novm + import llnl.util.tty as tty +termios = None # type: Optional[ModuleType] try: - import termios + import termios as term_mod + termios = term_mod except ImportError: - termios = None + pass # Use this to strip escape sequences diff --git a/lib/spack/spack/architecture.py b/lib/spack/spack/architecture.py index c0930fe2d0..6377477822 100644 --- a/lib/spack/spack/architecture.py +++ b/lib/spack/spack/architecture.py @@ -235,18 +235,19 @@ class Platform(object): Will return a instance of it once it is returned. """ - priority = None # Subclass sets number. Controls detection order + # Subclass sets number. Controls detection order + priority = None # type: int #: binary formats used on this platform; used by relocation logic binary_formats = ['elf'] - front_end = None - back_end = None - default = None # The default back end target. + front_end = None # type: str + back_end = None # type: str + default = None # type: str # The default back end target. - front_os = None - back_os = None - default_os = None + front_os = None # type: str + back_os = None # type: str + default_os = None # type: str reserved_targets = ['default_target', 'frontend', 'fe', 'backend', 'be'] reserved_oss = ['default_os', 'frontend', 'fe', 'backend', 'be'] diff --git a/lib/spack/spack/build_systems/autotools.py b/lib/spack/spack/build_systems/autotools.py index ee9fb6884e..2f9245638b 100644 --- a/lib/spack/spack/build_systems/autotools.py +++ b/lib/spack/spack/build_systems/autotools.py @@ -8,6 +8,7 @@ import os.path from subprocess import PIPE from subprocess import check_call +from typing import List # novm import llnl.util.tty as tty import llnl.util.filesystem as fs @@ -61,7 +62,7 @@ class AutotoolsPackage(PackageBase): #: Targets for ``make`` during the :py:meth:`~.AutotoolsPackage.build` #: phase - build_targets = [] + build_targets = [] # type: List[str] #: Targets for ``make`` during the :py:meth:`~.AutotoolsPackage.install` #: phase install_targets = ['install'] @@ -75,7 +76,7 @@ class AutotoolsPackage(PackageBase): #: Set to true to force the autoreconf step even if configure is present force_autoreconf = False #: Options to be passed to autoreconf when using the default implementation - autoreconf_extra_args = [] + autoreconf_extra_args = [] # type: List[str] #: If False deletes all the .la files in the prefix folder #: after the installation. If True instead it installs them. diff --git a/lib/spack/spack/build_systems/cmake.py b/lib/spack/spack/build_systems/cmake.py index 1336069846..fd55040652 100644 --- a/lib/spack/spack/build_systems/cmake.py +++ b/lib/spack/spack/build_systems/cmake.py @@ -8,6 +8,7 @@ import os import platform import re +from typing import List # novm import spack.build_environment from llnl.util.filesystem import working_dir @@ -74,7 +75,7 @@ class CMakePackage(PackageBase): #: system base class build_system_class = 'CMakePackage' - build_targets = [] + build_targets = [] # type: List[str] install_targets = ['install'] build_time_test_callbacks = ['check'] diff --git a/lib/spack/spack/build_systems/makefile.py b/lib/spack/spack/build_systems/makefile.py index 6cd05c7ad9..c09971c45f 100644 --- a/lib/spack/spack/build_systems/makefile.py +++ b/lib/spack/spack/build_systems/makefile.py @@ -5,6 +5,7 @@ import inspect +from typing import List # novm import llnl.util.tty as tty from llnl.util.filesystem import working_dir @@ -48,7 +49,7 @@ class MakefilePackage(PackageBase): #: Targets for ``make`` during the :py:meth:`~.MakefilePackage.build` #: phase - build_targets = [] + build_targets = [] # type: List[str] #: Targets for ``make`` during the :py:meth:`~.MakefilePackage.install` #: phase install_targets = ['install'] diff --git a/lib/spack/spack/build_systems/meson.py b/lib/spack/spack/build_systems/meson.py index 825b4c98c3..ee56f50861 100644 --- a/lib/spack/spack/build_systems/meson.py +++ b/lib/spack/spack/build_systems/meson.py @@ -6,6 +6,7 @@ import inspect import os +from typing import List # novm from llnl.util.filesystem import working_dir from spack.directives import depends_on, variant @@ -46,7 +47,7 @@ class MesonPackage(PackageBase): #: system base class build_system_class = 'MesonPackage' - build_targets = [] + build_targets = [] # type: List[str] install_targets = ['install'] build_time_test_callbacks = ['check'] diff --git a/lib/spack/spack/cmd/flake8.py b/lib/spack/spack/cmd/flake8.py index ce6dbfed20..5d2d97143e 100644 --- a/lib/spack/spack/cmd/flake8.py +++ b/lib/spack/spack/cmd/flake8.py @@ -5,200 +5,22 @@ from __future__ import print_function -try: - from itertools import zip_longest # novm -except ImportError: - from itertools import izip_longest # novm +import llnl.util.tty as tty - zip_longest = izip_longest - -import re -import os -import sys -import argparse - -from llnl.util.filesystem import working_dir - -import spack.paths -from spack.util.executable import which +import spack.cmd.style -description = "runs source code style checks on Spack. requires flake8" -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. - - We run flake8 with different exceptions for the core and for - packages, since we allow `from spack import *` and poking globals - into packages. - """ - return f.startswith("var/spack/repos/") or "docs/tutorial/examples" in f - - -#: List of directories to exclude from checks. -exclude_directories = [spack.paths.external_path] - -#: max line length we're enforcing (note: this duplicates what's in .flake8) -max_line_length = 79 - - -def changed_files(base=None, untracked=True, all_files=False): - """Get list of changed files in the Spack repository.""" - - git = which("git", required=True) - - if base is None: - 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], - # Add changed files that have been staged but not yet committed - ["diff", "--name-only", "--diff-filter=ACMR", "--cached"], - # Add changed files that are unstaged - ["diff", "--name-only", "--diff-filter=ACMR"], - ] - - # Add new files that are untracked - if untracked: - 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"]) - - 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") - - for f in files: - # Ignore non-Python files - if not (f.endswith(".py") or f == "bin/spack"): - continue - - # Ignore files in the exclude locations - if any(os.path.realpath(f).startswith(e) for e in excludes): - continue - - changed.add(f) - - return sorted(changed) +description = "alias for spack style (deprecated)" +section = spack.cmd.style.section +level = spack.cmd.style.level def setup_parser(subparser): - subparser.add_argument( - "-b", - "--base", - action="store", - default=None, - help="select base branch for collecting list of modified files", - ) - subparser.add_argument( - "-a", - "--all", - action="store_true", - help="check all files, not just changed files", - ) - subparser.add_argument( - "-o", - "--output", - action="store_true", - help="send filtered files to stdout as well as temp files", - ) - subparser.add_argument( - "-r", - "--root-relative", - action="store_true", - default=False, - help="print root-relative paths (default: cwd-relative)", - ) - 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" - ) + spack.cmd.style.setup_parser(subparser) def flake8(parser, args): - 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 - ) - - file_list = [prefix_relative(p) for p in file_list] - - 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("Modified files:") - for filename in file_list: - 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.") + tty.warn( + "spack flake8 is deprecated", "please use `spack style` to run style checks" + ) + return spack.cmd.style.style(parser, args) diff --git a/lib/spack/spack/cmd/module.py b/lib/spack/spack/cmd/module.py index f86f3e5f25..459fb3748c 100644 --- a/lib/spack/spack/cmd/module.py +++ b/lib/spack/spack/cmd/module.py @@ -4,6 +4,7 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) import argparse +from typing import Dict, Callable # novm import llnl.util.tty as tty @@ -15,7 +16,7 @@ level = "short" -_subcommands = {} +_subcommands = {} # type: Dict[str, Callable] _deprecated_commands = ('refresh', 'find', 'rm', 'loads') diff --git a/lib/spack/spack/cmd/style.py b/lib/spack/spack/cmd/style.py new file mode 100644 index 0000000000..62a7b72d8e --- /dev/null +++ b/lib/spack/spack/cmd/style.py @@ -0,0 +1,330 @@ +# Copyright 2013-2020 Lawrence Livermore National Security, LLC and other +# Spack Project Developers. See the top-level COPYRIGHT file for details. +# +# SPDX-License-Identifier: (Apache-2.0 OR MIT) + +from __future__ import print_function + +import re +import os +import sys +import argparse + +if sys.version_info < (3, 0): + from itertools import izip_longest # novm + + zip_longest = izip_longest +else: + from itertools import zip_longest # novm + +from llnl.util.filesystem import working_dir +import llnl.util.tty as tty + +import spack.paths +from spack.util.executable import which + + +description = ( + "runs source code style checks on Spack. Requires flake8, mypy, black for " + + "their respective checks" +) +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) + + +#: List of directories to exclude from checks. +exclude_directories = [spack.paths.external_path] + +#: max line length we're enforcing (note: this duplicates what's in .flake8) +max_line_length = 79 + + +def is_package(f): + """Whether flake8 should consider a file as a core file or a package. + + We run flake8 with different exceptions for the core and for + packages, since we allow `from spack import *` and poking globals + into packages. + """ + return f.startswith("var/spack/repos/") or "docs/tutorial/examples" in f + + +def changed_files(base=None, untracked=True, all_files=False): + """Get list of changed files in the Spack repository.""" + + git = which("git", required=True) + + if base is None: + 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], + # Add changed files that have been staged but not yet committed + ["diff", "--name-only", "--diff-filter=ACMR", "--cached"], + # Add changed files that are unstaged + ["diff", "--name-only", "--diff-filter=ACMR"], + ] + + # Add new files that are untracked + if untracked: + 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"]) + + 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") + + for f in files: + # Ignore non-Python files + if not (f.endswith(".py") or f == "bin/spack"): + continue + + # Ignore files in the exclude locations + if any(os.path.realpath(f).startswith(e) for e in excludes): + continue + + changed.add(f) + + return sorted(changed) + + +def setup_parser(subparser): + subparser.add_argument( + "-b", + "--base", + action="store", + default=None, + help="select base branch for collecting list of modified files", + ) + subparser.add_argument( + "-a", + "--all", + action="store_true", + help="check all files, not just changed files", + ) + subparser.add_argument( + "-o", + "--output", + action="store_true", + help="send filtered files to stdout as well as temp files", + ) + subparser.add_argument( + "-r", + "--root-relative", + action="store_true", + default=False, + help="print root-relative paths (default: cwd-relative)", + ) + subparser.add_argument( + "-U", + "--no-untracked", + dest="untracked", + action="store_false", + default=True, + help="exclude untracked files from checks", + ) + subparser.add_argument( + "--no-flake8", + dest="flake8", + action="store_false", + help="Do not run flake8, default is run flake8", + ) + subparser.add_argument( + "--no-mypy", + dest="mypy", + action="store_false", + help="Do not run mypy, default is run mypy if available", + ) + subparser.add_argument( + "--black", + dest="black", + action="store_true", + help="Run black checks, default is skip", + ) + subparser.add_argument( + "files", nargs=argparse.REMAINDER, help="specific files to check" + ) + + +def rewrite_and_print_output( + output, + args, + re_obj=re.compile(r"^(.+):([0-9]+):"), + replacement=r"{0}:{1}:", +): + """rewrite ouput with :: format to respect path args""" + if args.root_relative or re_obj is None: + # print results relative to repo root. + print(output) + else: + # print results relative to current working directory + def cwd_relative(path): + return replacement.format( + os.path.relpath( + os.path.join(spack.paths.prefix, path.group(1)), + os.getcwd(), + ), + *list(path.groups()[1:]) + ) + + for line in output.split("\n"): + print( + re_obj.sub( + cwd_relative, + line, + ) + ) + + +def print_style_header(file_list, args): + tty.msg("style: running code checks on spack.") + tools = [] + if args.flake8: + tools.append("flake8") + if args.mypy: + tools.append("mypy") + if args.black: + tools.append("black") + tty.msg("style: tools selected: " + ", ".join(tools)) + tty.msg("Modified files:", *[filename.strip() for filename in file_list]) + sys.stdout.flush() + + +def print_tool_header(tool): + sys.stdout.flush() + tty.msg("style: running %s checks on spack." % tool) + sys.stdout.flush() + + +def run_flake8(file_list, args): + returncode = 0 + print_tool_header("flake8") + flake8_cmd = which("flake8", required=True) + + 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): + 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 + + rewrite_and_print_output(output, args) + + if returncode == 0: + tty.msg("Flake8 style checks were clean") + else: + tty.error("Flake8 style checks found errors") + return returncode + + +def run_mypy(file_list, args): + mypy_cmd = which("mypy") + if mypy_cmd is None: + tty.error("style: mypy is not available in path, skipping") + return 1 + + print_tool_header("mypy") + + returncode = 0 + 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): + chunk = filter(lambda e: e is not None, chunk) + + output = mypy_cmd(*chunk, fail_on_error=False, output=str) + returncode |= mypy_cmd.returncode + + rewrite_and_print_output(output, args) + + if returncode == 0: + tty.msg("mypy checks were clean") + else: + tty.error("mypy checks found errors") + return returncode + + +def run_black(file_list, args): + black_cmd = which("black") + if black_cmd is None: + tty.error("style: black is not available in path, skipping") + return 1 + + print_tool_header("black") + + pat = re.compile("would reformat +(.*)") + replacement = "would reformat {0}" + returncode = 0 + 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): + chunk = filter(lambda e: e is not None, chunk) + + output = black_cmd( + "--check", "--diff", *chunk, fail_on_error=False, output=str, error=str + ) + returncode |= black_cmd.returncode + + rewrite_and_print_output(output, args, pat, replacement) + + if returncode == 0: + tty.msg("black style checks were clean") + else: + tty.error("black checks found errors") + return returncode + + +def style(parser, args): + 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 + ) + + file_list = [prefix_relative(p) for p in file_list] + + returncode = 0 + with working_dir(spack.paths.prefix): + if not file_list: + file_list = changed_files(args.base, args.untracked, args.all) + print_style_header(file_list, args) + if args.flake8: + returncode = run_flake8(file_list, args) + if args.mypy: + returncode |= run_mypy(file_list, args) + if args.black: + returncode |= run_black(file_list, args) + + if returncode != 0: + print("spack style found errors.") + sys.exit(1) + else: + print("spack style checks were clean.") diff --git a/lib/spack/spack/compiler.py b/lib/spack/spack/compiler.py index d498803633..cc5a6c7d5f 100644 --- a/lib/spack/spack/compiler.py +++ b/lib/spack/spack/compiler.py @@ -10,6 +10,7 @@ import itertools import shutil import tempfile +from typing import Sequence, List # novm import llnl.util.lang from llnl.util.filesystem import ( @@ -190,20 +191,20 @@ class Compiler(object): and how to identify the particular type of compiler.""" # Subclasses use possible names of C compiler - cc_names = [] + cc_names = [] # type: List[str] # Subclasses use possible names of C++ compiler - cxx_names = [] + cxx_names = [] # type: List[str] # Subclasses use possible names of Fortran 77 compiler - f77_names = [] + f77_names = [] # type: List[str] # Subclasses use possible names of Fortran 90 compiler - fc_names = [] + fc_names = [] # type: List[str] # Optional prefix regexes for searching for this type of compiler. # Prefixes are sometimes used for toolchains - prefixes = [] + prefixes = [] # type: List[str] # Optional suffix regexes for searching for this type of compiler. # Suffixes are used by some frameworks, e.g. macports uses an '-mp-X.Y' @@ -214,7 +215,7 @@ class Compiler(object): version_argument = '-dumpversion' #: Return values to ignore when invoking the compiler to get its version - ignore_version_errors = () + ignore_version_errors = () # type: Sequence[int] #: Regex used to extract version from compiler's output version_regex = '(.*)' @@ -266,9 +267,9 @@ def opt_flags(self): return ['-O', '-O0', '-O1', '-O2', '-O3'] # Cray PrgEnv name that can be used to load this compiler - PrgEnv = None + PrgEnv = None # type: str # Name of module used to switch versions of this compiler - PrgEnv_compiler = None + PrgEnv_compiler = None # type: str def __init__(self, cspec, operating_system, target, paths, modules=None, alias=None, environment=None, diff --git a/lib/spack/spack/compilers/__init__.py b/lib/spack/spack/compilers/__init__.py index 44a0f90371..2392340570 100644 --- a/lib/spack/spack/compilers/__init__.py +++ b/lib/spack/spack/compilers/__init__.py @@ -11,6 +11,7 @@ import multiprocessing.pool import os import six +from typing import Dict # novm import llnl.util.lang import llnl.util.filesystem as fs @@ -21,6 +22,7 @@ import spack.error import spack.spec import spack.config +import spack.compiler import spack.architecture import spack.util.imp as simp from spack.util.environment import get_path @@ -36,7 +38,7 @@ # TODO: Caches at module level make it difficult to mock configurations in # TODO: unit tests. It might be worth reworking their implementation. #: cache of compilers constructed from config data, keyed by config entry id. -_compiler_cache = {} +_compiler_cache = {} # type: Dict[str, spack.compiler.Compiler] _compiler_to_pkg = { 'clang': 'llvm+clang' diff --git a/lib/spack/spack/compilers/nag.py b/lib/spack/spack/compilers/nag.py index 503a31e404..7d9f0e8245 100644 --- a/lib/spack/spack/compilers/nag.py +++ b/lib/spack/spack/compilers/nag.py @@ -3,15 +3,17 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) +from typing import List # novm + import spack.compiler class Nag(spack.compiler.Compiler): # Subclasses use possible names of C compiler - cc_names = [] + cc_names = [] # type: List[str] # Subclasses use possible names of C++ compiler - cxx_names = [] + cxx_names = [] # type: List[str] # Subclasses use possible names of Fortran 77 compiler f77_names = ['nagfor'] diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py index 84d8e8ca3f..27309be75b 100644 --- a/lib/spack/spack/config.py +++ b/lib/spack/spack/config.py @@ -38,6 +38,7 @@ from contextlib import contextmanager from six import iteritems from ordereddict_backport import OrderedDict +from typing import List # novm import ruamel.yaml as yaml from ruamel.yaml.error import MarkedYAMLError @@ -735,7 +736,7 @@ def override(path_or_scope, value=None): #: configuration scopes added on the command line #: set by ``spack.main.main()``. -command_line_scopes = [] +command_line_scopes = [] # type: List[str] def _add_platform_scope(cfg, scope_type, name, path): diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py index db1e6a636b..ed1bb2f381 100644 --- a/lib/spack/spack/database.py +++ b/lib/spack/spack/database.py @@ -27,6 +27,8 @@ import socket import sys import time +from typing import Dict # novm + try: import uuid _use_uuid = True @@ -289,10 +291,10 @@ def __getattribute__(self, name): class Database(object): """Per-process lock objects for each install prefix.""" - _prefix_locks = {} + _prefix_locks = {} # type: Dict[str, lk.Lock] """Per-process failure (lock) objects for each install prefix.""" - _prefix_failures = {} + _prefix_failures = {} # type: Dict[str, lk.Lock] def __init__(self, root, db_dir=None, upstream_dbs=None, is_upstream=False, enable_transaction_locking=True, @@ -1464,6 +1466,8 @@ def _query( return results + if _query.__doc__ is None: + _query.__doc__ = "" _query.__doc__ += _query_docstring def query_local(self, *args, **kwargs): @@ -1471,6 +1475,8 @@ def query_local(self, *args, **kwargs): with self.read_transaction(): return sorted(self._query(*args, **kwargs)) + if query_local.__doc__ is None: + query_local.__doc__ = "" query_local.__doc__ += _query_docstring def query(self, *args, **kwargs): @@ -1489,6 +1495,8 @@ def query(self, *args, **kwargs): return sorted(results) + if query.__doc__ is None: + query.__doc__ = "" query.__doc__ += _query_docstring def query_one(self, query_spec, known=any, installed=True): diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py index 41276b5b48..65c355edce 100644 --- a/lib/spack/spack/directives.py +++ b/lib/spack/spack/directives.py @@ -33,6 +33,7 @@ class OpenMpi(Package): import os.path import re from six import string_types +from typing import Set, List # novm import llnl.util.lang import llnl.util.tty.color @@ -103,8 +104,8 @@ class DirectiveMeta(type): """ # Set of all known directives - _directive_names = set() - _directives_to_be_executed = [] + _directive_names = set() # type: Set[str] + _directives_to_be_executed = [] # type: List[str] def __new__(cls, name, bases, attr_dict): # Initialize the attribute containing the list of directives diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index f075b893ae..4452a5898f 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -29,6 +29,7 @@ import re import shutil import sys +from typing import Optional, List # novm import llnl.util.tty as tty import six @@ -92,11 +93,12 @@ class FetchStrategy(object): #: The URL attribute must be specified either at the package class #: level, or as a keyword argument to ``version()``. It is used to #: distinguish fetchers for different versions in the package DSL. - url_attr = None + url_attr = None # type: Optional[str] #: Optional attributes can be used to distinguish fetchers when : #: classes have multiple ``url_attrs`` at the top-level. - optional_attrs = [] # optional attributes in version() args. + # optional attributes in version() args. + optional_attrs = [] # type: List[str] def __init__(self, **kwargs): # The stage is initialized late, so that fetch strategies can be @@ -420,7 +422,7 @@ def _fetch_from_url(self, url): warn_content_type_mismatch(self.archive_file or "the archive") return partial_file, save_file - @property + @property # type: ignore # decorated properties unsupported in mypy @_needs_stage def archive_file(self): """Path to the source archive within this stage directory.""" diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py index c19d9018b2..5199adde68 100644 --- a/lib/spack/spack/installer.py +++ b/lib/spack/spack/installer.py @@ -1542,7 +1542,7 @@ def install(self): (stop_before_phase is None and last_phase is None) except spack.directory_layout.InstallDirectoryAlreadyExistsError \ - as err: + as exc: tty.debug('Install prefix for {0} exists, keeping {1} in ' 'place.'.format(pkg.name, pkg.prefix)) self._update_installed(task) @@ -1553,7 +1553,7 @@ def install(self): raise if task.explicit: - exists_errors.append((pkg_id, str(err))) + exists_errors.append((pkg_id, str(exc))) except KeyboardInterrupt as exc: # The build has been terminated with a Ctrl-C so terminate diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py index 045ca5ffec..a4c9fce2f0 100644 --- a/lib/spack/spack/mirror.py +++ b/lib/spack/spack/mirror.py @@ -23,9 +23,9 @@ from ordereddict_backport import OrderedDict -try: +if sys.version_info >= (3, 5): from collections.abc import Mapping # novm -except ImportError: +else: from collections import Mapping import llnl.util.tty as tty diff --git a/lib/spack/spack/mixins.py b/lib/spack/spack/mixins.py index 6da12e4dce..63abcb561e 100644 --- a/lib/spack/spack/mixins.py +++ b/lib/spack/spack/mixins.py @@ -8,6 +8,12 @@ """ import collections import os +import sys +from typing import Callable, DefaultDict, Dict, List # novm +if sys.version_info >= (3, 5): + CallbackDict = DefaultDict[str, List[Callable]] +else: + CallbackDict = None import llnl.util.filesystem @@ -26,12 +32,12 @@ class PackageMixinsMeta(type): gets implicitly attached to the package class by calling the mixin. """ - _methods_to_be_added = {} - _add_method_before = collections.defaultdict(list) - _add_method_after = collections.defaultdict(list) + _methods_to_be_added = {} # type: Dict[str, Callable] + _add_method_before = collections.defaultdict(list) # type: CallbackDict + _add_method_after = collections.defaultdict(list) # type: CallbackDict @staticmethod - def register_method_before(fn, phase): + def register_method_before(fn, phase): # type: (Callable, str) -> None """Registers a method to be run before a certain phase. Args: @@ -42,7 +48,7 @@ def register_method_before(fn, phase): PackageMixinsMeta._add_method_before[phase].append(fn) @staticmethod - def register_method_after(fn, phase): + def register_method_after(fn, phase): # type: (Callable, str) -> None """Registers a method to be run after a certain phase. Args: diff --git a/lib/spack/spack/modules/common.py b/lib/spack/spack/modules/common.py index cbc064520c..fb688e4f1f 100644 --- a/lib/spack/spack/modules/common.py +++ b/lib/spack/spack/modules/common.py @@ -34,6 +34,7 @@ import inspect import os.path import re +from typing import Optional # novm import llnl.util.filesystem from llnl.util.lang import dedupe @@ -540,7 +541,7 @@ class BaseFileLayout(object): """ #: This needs to be redefined - extension = None + extension = None # type: Optional[str] def __init__(self, configuration): self.conf = configuration diff --git a/lib/spack/spack/modules/lmod.py b/lib/spack/spack/modules/lmod.py index 80f6933063..d9d1bca5d7 100644 --- a/lib/spack/spack/modules/lmod.py +++ b/lib/spack/spack/modules/lmod.py @@ -8,6 +8,7 @@ import llnl.util.lang as lang import itertools import collections +from typing import Dict, Any # novm import spack.config import spack.compilers @@ -26,7 +27,7 @@ def configuration(): #: Caches the configuration {spec_hash: configuration} -configuration_registry = {} +configuration_registry = {} # type: Dict[str, Any] def make_configuration(spec): diff --git a/lib/spack/spack/modules/tcl.py b/lib/spack/spack/modules/tcl.py index 0efc6332fe..65cdf5d862 100644 --- a/lib/spack/spack/modules/tcl.py +++ b/lib/spack/spack/modules/tcl.py @@ -8,6 +8,7 @@ """ import os.path import string +from typing import Dict, Any # novm import llnl.util.tty as tty @@ -24,7 +25,7 @@ def configuration(): #: Caches the configuration {spec_hash: configuration} -configuration_registry = {} +configuration_registry = {} # type: Dict[str, Any] def make_configuration(spec): diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index 531f4aadee..1046b2d5eb 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -26,6 +26,7 @@ import traceback import six import types +from typing import Optional, List, Dict, Any, Callable # novm import llnl.util.filesystem as fsys import llnl.util.tty as tty @@ -253,8 +254,9 @@ class PackageMeta( """ phase_fmt = '_InstallPhase_{0}' - _InstallPhase_run_before = {} - _InstallPhase_run_after = {} + # These are accessed only through getattr, by name + _InstallPhase_run_before = {} # type: Dict[str, List[Callable]] + _InstallPhase_run_after = {} # type: Dict[str, List[Callable]] def __new__(cls, name, bases, attr_dict): """ @@ -555,7 +557,7 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)): #: A list or set of build time test functions to be called when tests #: are executed or 'None' if there are no such test functions. - build_time_test_callbacks = None + build_time_test_callbacks = None # type: Optional[List[str]] #: By default, packages are not virtual #: Virtual packages override this attribute @@ -567,7 +569,7 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)): #: A list or set of install time test functions to be called when tests #: are executed or 'None' if there are no such test functions. - install_time_test_callbacks = None + install_time_test_callbacks = None # type: Optional[List[str]] #: By default we build in parallel. Subclasses can override this. parallel = True @@ -589,19 +591,19 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)): #: List of prefix-relative file paths (or a single path). If these do #: not exist after install, or if they exist but are not files, #: sanity checks fail. - sanity_check_is_file = [] + sanity_check_is_file = [] # type: List[str] #: List of prefix-relative directory paths (or a single path). If #: these do not exist after install, or if they exist but are not #: directories, sanity checks will fail. - sanity_check_is_dir = [] + sanity_check_is_dir = [] # type: List[str] #: List of glob expressions. Each expression must either be #: absolute or relative to the package source path. #: Matching artifacts found at the end of the build process will be #: copied in the same directory tree as _spack_build_logfile and #: _spack_build_envfile. - archive_files = [] + archive_files = [] # type: List[str] #: Boolean. Set to ``True`` for packages that require a manual download. #: This is currently used by package sanity tests and generation of a @@ -609,7 +611,7 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)): manual_download = False #: Set of additional options used when fetching package versions. - fetch_options = {} + fetch_options = {} # type: Dict[str, Any] # # Set default licensing information @@ -627,12 +629,12 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)): #: looking for a license. All file paths must be relative to the #: installation directory. More complex packages like Intel may require #: multiple licenses for individual components. Defaults to the empty list. - license_files = [] + license_files = [] # type: List[str] #: List of strings. Environment variables that can be set to tell the #: software where to look for a license if it is not in the usual location. #: Defaults to the empty list. - license_vars = [] + license_vars = [] # type: List[str] #: String. A URL pointing to license setup instructions for the software. #: Defaults to the empty string. @@ -646,7 +648,7 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)): #: List of strings which contains GitHub usernames of package maintainers. #: Do not include @ here in order not to unnecessarily ping the users. - maintainers = [] + maintainers = [] # type: List[str] #: List of attributes to be excluded from a package's hash. metadata_attrs = ['homepage', 'url', 'urls', 'list_url', 'extendable', @@ -2591,7 +2593,7 @@ class BundlePackage(PackageBase): """General purpose bundle, or no-code, package class.""" #: There are no phases by default but the property is required to support #: post-install hooks (e.g., for module generation). - phases = [] + phases = [] # type: List[str] #: This attribute is used in UI queries that require to know which #: build-system class we are using build_system_class = 'BundlePackage' diff --git a/lib/spack/spack/repo.py b/lib/spack/spack/repo.py index 4af7b382f0..12ce448504 100644 --- a/lib/spack/spack/repo.py +++ b/lib/spack/spack/repo.py @@ -13,18 +13,18 @@ import os import re import shutil +import six import stat import sys import traceback import types +from typing import Dict # novm -try: +if sys.version_info >= (3, 5): from collections.abc import Mapping # novm -except ImportError: +else: from collections import Mapping -import six - import ruamel.yaml as yaml import llnl.util.lang @@ -131,7 +131,7 @@ class FastPackageChecker(Mapping): during instance initialization. """ #: Global cache, reused by every instance - _paths_cache = {} + _paths_cache = {} # type: Dict[str, Dict[str, os.stat_result]] def __init__(self, packages_path): # The path of the repository managed by this instance @@ -149,7 +149,7 @@ def invalidate(self): self._paths_cache[self.packages_path] = self._create_new_cache() self._packages_to_stats = self._paths_cache[self.packages_path] - def _create_new_cache(self): + def _create_new_cache(self): # type: () -> Dict[str, os.stat_result] """Create a new cache for packages in a repo. The implementation here should try to minimize filesystem @@ -159,7 +159,7 @@ def _create_new_cache(self): """ # Create a dictionary that will store the mapping between a # package name and its stat info - cache = {} + cache = {} # type: Dict[str, os.stat_result] for pkg_name in os.listdir(self.packages_path): # Skip non-directories in the package root. pkg_dir = os.path.join(self.packages_path, pkg_name) @@ -341,7 +341,7 @@ class PatchIndexer(Indexer): def _create(self): return spack.patch.PatchCache() - def needs_update(): + def needs_update(self): # TODO: patches can change under a package and we should handle # TODO: it, but we currently punt. This should be refactored to # TODO: check whether patches changed each time a package loads, diff --git a/lib/spack/spack/schema/environment.py b/lib/spack/spack/schema/environment.py index d251aeba96..4d0d3c780e 100644 --- a/lib/spack/spack/schema/environment.py +++ b/lib/spack/spack/schema/environment.py @@ -38,11 +38,12 @@ def parse(config_obj): config_obj: a configuration dictionary conforming to the schema definition for environment modifications """ + import sys import spack.util.environment as ev - try: - from collections import Sequence # novm - except ImportError: + if sys.version_info >= (3, 5): from collections.abc import Sequence # novm + else: + from collections import Sequence # novm env = ev.EnvironmentModifications() for command, variable in config_obj.items(): diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index 555b77090f..2322260369 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -19,7 +19,7 @@ try: import clingo except ImportError: - clingo = None + clingo = None # type: ignore import llnl.util.lang import llnl.util.tty as tty diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index f0ae6a1431..012b725adc 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -3269,7 +3269,7 @@ def virtual_dependencies(self): """Return list of any virtual deps in this spec.""" return [spec for spec in self.traverse() if spec.virtual] - @property + @property # type: ignore[misc] # decorated prop not supported in mypy @lang.memoized def patches(self): """Return patch objects for any patch sha256 sums on this Spec. diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index 719d408d84..275a70c227 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -16,6 +16,7 @@ import tempfile from six import string_types from six import iteritems +from typing import Dict # novm import llnl.util.tty as tty from llnl.util.filesystem import mkdirp, can_access, install, install_tree @@ -221,7 +222,7 @@ class Stage(object): """ """Shared dict of all stage locks.""" - stage_locks = {} + stage_locks = {} # type: Dict[str, spack.util.lock.Lock] """Most staging is managed by Spack. DIYStage is one exception.""" managed_by_spack = True diff --git a/lib/spack/spack/tengine.py b/lib/spack/spack/tengine.py index 15268e682d..41f8d913ce 100644 --- a/lib/spack/spack/tengine.py +++ b/lib/spack/spack/tengine.py @@ -4,6 +4,7 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) import itertools import textwrap +from typing import List # novm import llnl.util.lang import six @@ -18,7 +19,7 @@ class ContextMeta(type): """ #: Keeps track of the context properties that have been added #: by the class that is being defined - _new_context_properties = [] + _new_context_properties = [] # type: List[str] def __new__(cls, name, bases, attr_dict): # Merge all the context properties that are coming from base classes diff --git a/lib/spack/spack/test/cmd/flake8.py b/lib/spack/spack/test/cmd/flake8.py index d4bfc5618d..8e3619ca4b 100644 --- a/lib/spack/spack/test/cmd/flake8.py +++ b/lib/spack/spack/test/cmd/flake8.py @@ -11,12 +11,12 @@ from llnl.util.filesystem import FileFilter import spack.paths -from spack.cmd.flake8 import flake8, setup_parser, changed_files +from spack.cmd.style import style, setup_parser, changed_files from spack.repo import Repo from spack.util.executable import which -@pytest.fixture(scope='module') +@pytest.fixture(scope="module") def parser(): """Returns the parser for the ``flake8`` command""" parser = argparse.ArgumentParser() @@ -24,7 +24,7 @@ def parser(): return parser -@pytest.fixture(scope='module') +@pytest.fixture(scope="module") def flake8_package(): """Flake8 only checks files that have been modified. This fixture makes a small change to the ``flake8`` @@ -32,7 +32,7 @@ def flake8_package(): change on cleanup. """ repo = Repo(spack.paths.mock_packages_path) - filename = repo.filename_for_package_name('flake8') + filename = repo.filename_for_package_name("flake8") package = FileFilter(filename) # Make the change @@ -60,16 +60,18 @@ def test_changed_files(parser, flake8_package): # As of flake8 3.0.0, Python 2.6 and 3.3 are no longer supported # http://flake8.pycqa.org/en/latest/release-notes/3.0.0.html @pytest.mark.skipif( - sys.version_info[:2] <= (2, 6) or - (3, 0) <= sys.version_info[:2] <= (3, 3), - reason='flake8 no longer supports Python 2.6 or 3.3 and older') -@pytest.mark.skipif(not which('flake8'), reason='flake8 is not installed.') + sys.version_info[:2] <= (2, 6) or (3, 0) <= sys.version_info[:2] <= (3, 3), + reason="flake8 no longer supports Python 2.6 or 3.3 and older", +) +@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.") def test_flake8(parser, flake8_package): # Only test the flake8_package that we modified # Otherwise, the unit tests would fail every time # the flake8 tests fail - args = parser.parse_args([flake8_package]) - flake8(parser, args) + args = parser.parse_args(["--no-mypy", flake8_package]) + style(parser, args) # Get even more coverage - args = parser.parse_args(['--output', '--root-relative', flake8_package]) - flake8(parser, args) + args = parser.parse_args( + ["--no-mypy", "--output", "--root-relative", flake8_package] + ) + style(parser, args) diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index 8615b14abe..7701b243b0 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -318,7 +318,7 @@ def _skip_if_missing_executables(request): # FIXME: The lines below should better be added to a fixture with # FIXME: session-scope. Anyhow doing it is not easy, as it seems # FIXME: there's some weird interaction with compilers during concretization. -spack.architecture.real_platform = spack.architecture.platform +spack.architecture.real_platform = spack.architecture.platform # type: ignore def test_platform(): @@ -1238,7 +1238,7 @@ def mock_test_repo(tmpdir_factory): class MockBundle(object): has_code = False name = 'mock-bundle' - versions = {} + versions = {} # type: ignore @pytest.fixture diff --git a/lib/spack/spack/test/package_sanity.py b/lib/spack/spack/test/package_sanity.py index d50169a1a4..039297381f 100644 --- a/lib/spack/spack/test/package_sanity.py +++ b/lib/spack/spack/test/package_sanity.py @@ -16,7 +16,7 @@ import spack.variant # A few functions from this module are used to # do sanity checks only on packagess modified by a PR -import spack.cmd.flake8 as flake8 +import spack.cmd.style as style import spack.util.crypto as crypto import pickle @@ -207,7 +207,7 @@ def test_prs_update_old_api(): deprecated calls to any method. """ changed_package_files = [ - x for x in flake8.changed_files() if flake8.is_package(x) + x for x in style.changed_files() if style.is_package(x) ] failing = [] for file in changed_package_files: diff --git a/lib/spack/spack/test/sbang.py b/lib/spack/spack/test/sbang.py index 9307d2bc4b..44d5b5b868 100644 --- a/lib/spack/spack/test/sbang.py +++ b/lib/spack/spack/test/sbang.py @@ -44,7 +44,7 @@ last_line = "last!\n" -@pytest.fixture +@pytest.fixture # type: ignore[no-redef] def sbang_line(): yield '#!/bin/sh %s/bin/sbang\n' % spack.store.layout.root diff --git a/lib/spack/spack/util/crypto.py b/lib/spack/spack/util/crypto.py index 7e264ff0c7..5a2329ad32 100644 --- a/lib/spack/spack/util/crypto.py +++ b/lib/spack/spack/util/crypto.py @@ -5,10 +5,10 @@ import sys import hashlib +from typing import Dict, Callable, Any # novm import llnl.util.tty as tty - #: Set of hash algorithms that Spack can use, mapped to digest size in bytes hashes = { 'md5': 16, @@ -30,7 +30,7 @@ #: cache of hash functions generated -_hash_functions = {} +_hash_functions = {} # type: Dict[str, Callable[[], Any]] class DeprecatedHash(object): diff --git a/lib/spack/spack/util/gpg.py b/lib/spack/spack/util/gpg.py index eaa199e417..9d53ed1815 100644 --- a/lib/spack/spack/util/gpg.py +++ b/lib/spack/spack/util/gpg.py @@ -115,7 +115,7 @@ def result(self): return result -class GpgConstants(object): +class _GpgConstants(object): @cached_property def target_version(self): return spack.version.Version('2') @@ -189,7 +189,7 @@ def user_run_dir(self): # "file-name-too-long" errors in gpg. try: - has_suitable_gpgconf = bool(GpgConstants.gpgconf_string) + has_suitable_gpgconf = bool(_GpgConstants.gpgconf_string) except SpackGPGError: has_suitable_gpgconf = False @@ -244,7 +244,7 @@ def clear(self): pass -GpgConstants = GpgConstants() +GpgConstants = _GpgConstants() def ensure_gpg(reevaluate=False): @@ -382,7 +382,7 @@ def gpg(*args, **kwargs): return get_global_gpg_instance()(*args, **kwargs) -gpg.name = 'gpg' +gpg.name = 'gpg' # type: ignore[attr-defined] @functools.wraps(Gpg.create) diff --git a/lib/spack/spack/util/lock.py b/lib/spack/spack/util/lock.py index b748aa056a..3703965238 100644 --- a/lib/spack/spack/util/lock.py +++ b/lib/spack/spack/util/lock.py @@ -15,7 +15,7 @@ import spack.paths -class Lock(llnl.util.lock.Lock): +class Lock(llnl.util.lock.Lock): # type: ignore[no-redef] """Lock that can be disabled. This overrides the ``_lock()`` and ``_unlock()`` methods from diff --git a/lib/spack/spack/util/log_parse.py b/lib/spack/spack/util/log_parse.py index 4b01c18c91..26cd295e2b 100644 --- a/lib/spack/spack/util/log_parse.py +++ b/lib/spack/spack/util/log_parse.py @@ -43,7 +43,7 @@ def parse_log_events(stream, context=6, jobs=None, profile=False): #: lazily constructed CTest log parser -parse_log_events.ctest_parser = None +parse_log_events.ctest_parser = None # type: ignore[attr-defined] def _wrap(text, width): @@ -107,7 +107,7 @@ def make_log_context(log_events, width=None): for i in range(start, event.end): # wrap to width lines = _wrap(event[i], wrap_width) - lines[1:] = [indent + l for l in lines[1:]] + lines[1:] = [indent + ln for ln in lines[1:]] wrapped_line = line_fmt % (i, '\n'.join(lines)) if i in error_lines: diff --git a/lib/spack/spack/util/spack_yaml.py b/lib/spack/spack/util/spack_yaml.py index 40e66ee62c..3d2374e18f 100644 --- a/lib/spack/spack/util/spack_yaml.py +++ b/lib/spack/spack/util/spack_yaml.py @@ -14,6 +14,7 @@ """ import ctypes import collections +from typing import List # novm from ordereddict_backport import OrderedDict from six import string_types, StringIO @@ -219,7 +220,7 @@ def file_line(mark): #: This is nasty but YAML doesn't give us many ways to pass arguments -- #: yaml.dump() takes a class (not an instance) and instantiates the dumper #: itself, so we can't just pass an instance -_annotations = [] +_annotations = [] # type: List[str] class LineAnnotationDumper(OrderedLineDumper): diff --git a/lib/spack/spack/util/web.py b/lib/spack/spack/util/web.py index 1aad85550a..47010fdbfd 100644 --- a/lib/spack/spack/util/web.py +++ b/lib/spack/spack/util/web.py @@ -20,10 +20,10 @@ from six.moves.urllib.error import URLError from six.moves.urllib.request import urlopen, Request -try: +if sys.version_info < (3, 0): # Python 2 had these in the HTMLParser package. from HTMLParser import HTMLParser, HTMLParseError # novm -except ImportError: +else: # In Python 3, things moved to html.parser from html.parser import HTMLParser diff --git a/lib/spack/spack/variant.py b/lib/spack/spack/variant.py index 8fa52f9738..aa36b9b4ad 100644 --- a/lib/spack/spack/variant.py +++ b/lib/spack/spack/variant.py @@ -12,6 +12,12 @@ import itertools import re from six import StringIO +import sys + +if sys.version_info >= (3, 5): + from collections.abc import Sequence # novm +else: + from collections import Sequence import llnl.util.tty.color import llnl.util.lang as lang @@ -20,11 +26,6 @@ import spack.directives import spack.error as error -try: - from collections.abc import Sequence # novm -except ImportError: - from collections import Sequence - special_variant_values = [None, 'none', '*'] diff --git a/share/spack/qa/run-flake8-tests b/share/spack/qa/run-style-tests similarity index 92% rename from share/spack/qa/run-flake8-tests rename to share/spack/qa/run-style-tests index d6077fceb8..b91834f9d8 100755 --- a/share/spack/qa/run-flake8-tests +++ b/share/spack/qa/run-style-tests @@ -15,10 +15,10 @@ # run-flake8-tests # . "$(dirname $0)/setup.sh" -check_dependencies flake8 +check_dependencies flake8 mypy # verify that the code style is correct -spack flake8 +spack style # verify that the license headers are present spack license verify diff --git a/share/spack/qa/setup.sh b/share/spack/qa/setup.sh index e614bae909..3940d497c2 100755 --- a/share/spack/qa/setup.sh +++ b/share/spack/qa/setup.sh @@ -66,6 +66,10 @@ check_dependencies() { spack_package=py-flake8 pip_package=flake8 ;; + mypy) + spack_package=py-mypy + pip_package=mypy + ;; dot) spack_package=graphviz ;; diff --git a/share/spack/spack-completion.bash b/share/spack/spack-completion.bash index cbe59bc90a..36880dfdef 100755 --- a/share/spack/spack-completion.bash +++ b/share/spack/spack-completion.bash @@ -333,7 +333,7 @@ _spack() { then SPACK_COMPREPLY="-h --help -H --all-help --color -C --config-scope -d --debug --timestamp --pdb -e --env -D --env-dir -E --no-env --use-env-repo -k --insecure -l --enable-locks -L --disable-locks -m --mock -p --profile --sorted-profile --lines -v --verbose --stacktrace -V --version --print-shell-vars" else - SPACK_COMPREPLY="activate add arch blame build-env buildcache cd checksum ci clean clone commands compiler compilers concretize config containerize create deactivate debug dependencies dependents deprecate dev-build develop docs edit env extensions external fetch find flake8 gc gpg graph help info install license list load location log-parse maintainers mark mirror module patch pkg providers pydoc python reindex remove rm repo resource restage setup solve spec stage test test-env tutorial undevelop uninstall unit-test unload url verify versions view" + SPACK_COMPREPLY="activate add arch blame build-env buildcache cd checksum ci clean clone commands compiler compilers concretize config containerize create deactivate debug dependencies dependents deprecate dev-build develop docs edit env extensions external fetch find flake8 gc gpg graph help info install license list load location log-parse maintainers mark mirror module patch pkg providers pydoc python reindex remove rm repo resource restage setup solve spec stage style test test-env tutorial undevelop uninstall unit-test unload url verify versions view" fi } @@ -913,7 +913,7 @@ _spack_find() { _spack_flake8() { if $list_options then - SPACK_COMPREPLY="-h --help -b --base -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 --no-flake8 --no-mypy --black" else SPACK_COMPREPLY="" fi @@ -1513,6 +1513,15 @@ _spack_stage() { fi } +_spack_style() { + if $list_options + then + SPACK_COMPREPLY="-h --help -b --base -a --all -o --output -r --root-relative -U --no-untracked --no-flake8 --no-mypy --black" + else + SPACK_COMPREPLY="" + fi +} + _spack_test() { if $list_options then