black: configuration

This adds necessary configuration for flake8 and black to work together.

This also sets the line length to 99, per the data here:

* https://github.com/spack/spack/pull/24718#issuecomment-876933636

Given the data and the spirit of black's 88-character limit, we set the limit to 99
characters for all of Spack, because:

* 99 is one less than 100, a nice round number, and all lines will fit in a
  100-character wide terminal (even when the text editor puts a \ at EOL).
* 99 is just past the knee the file size curve for packages, and it means that packages
  remain readable and not significantly longer than they are now.
* It doesn't seem to hurt core -- files in core might change length by a few percent but
  seem like they'll be mostly the same as before -- just a bit more roomy.

- [x] set line length to 99
- [x] remove most exceptions from `.flake8` and add the ones black cares about
- [x] add `[tool.black]` to `pyproject.toml`
- [x] make `black` run if available in `spack style --fix`

Co-Authored-By: Tom Scogland <tscogland@llnl.gov>

.flake8

@@ -1,43 +1,25 @@
 # -*- conf -*-
-# flake8 settings for Spack core files.
+# flake8 settings for Spack.
 #
 # These exceptions are for Spack core files. We're slightly more lenient
 # with packages.  See .flake8_packages for that.
 #
-# E1: Indentation
-# - E129: visually indented line with same indent as next logical line
-#
-# E2: Whitespace
-# - E221: multiple spaces before operator
-# - E241: multiple spaces after ','
-# - E272: multiple spaces before keyword
-#
-# E7: Statement
+# This is the only flake8 rule Spack violates somewhat flagrantly
 # - E731: do not assign a lambda expression, use a def
 #
-# W5: Line break warning
-# - W503: line break before binary operator
-# - W504: line break after binary operator
-#
-# These are required to get the package.py files to test clean:
-# - F999: syntax error in doctest
-#
-# N8: PEP8-naming
-# - N801: class names should use CapWords convention
-# - 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
+# This is the only flake8 exception needed when using Black.
 # - E203: white space around slice operators can be required, ignore : warn
-# - W503: see above, already ignored for line-breaks
+#
+# We still allow these in packages (Would like to get rid of them or rely on mypy
+# in the future)
+# - F403: from/import * used; unable to detect undefined names
+# - F405: undefined name or from *
+# - F821: undefined name (needed with from/import *)
 #
 [flake8]
-ignore = E129,E221,E241,E272,E731,W503,W504,F999,N801,N813,N814,F403,F405
-max-line-length = 88
+#ignore = E129,,W503,W504,F999,N801,N813,N814,F403,F405,E203
+extend-ignore = E731,E203
+max-line-length = 99
 
 # F4: Import
 # - F405: `name` may be undefined, or undefined from star imports: `module`
@@ -46,7 +28,7 @@ max-line-length = 88
 # - F821: undefined name `name`
 #
 per-file-ignores =
-  var/spack/repos/*/package.py:F405,F821
+  var/spack/repos/*/package.py:F403,F405,F821
 
 # exclude things we usually do not want linting for.
 # These still get linted when passed explicitly, as when spack flake8 passes

.github/workflows/unit_tests.yaml

@@ -130,7 +130,7 @@ jobs:
           # ensure style checks are not skipped in unit tests for python >= 3.6
           # note that true/false (i.e., 1/0) are opposite in conditions in python and bash
           if python -c 'import sys; sys.exit(not sys.version_info >= (3, 6))'; then
-              pip install --upgrade flake8 isort>=4.3.5 mypy>=0.900 black
+              pip install --upgrade flake8 "isort>=4.3.5" "mypy>=0.900" "click==8.0.4" "black<=21.12b0"
           fi
     - name: Pin pathlib for Python 2.7
       if: ${{ matrix.python-version == 2.7 }}

.github/workflows/windows_python.yml

@@ -46,7 +46,7 @@ jobs:
         python-version: 3.9
     - name: Install Python packages
       run: |
-        python -m pip install --upgrade pip six setuptools flake8 isort>=4.3.5 mypy>=0.800 black pywin32 types-python-dateutil
+        python -m pip install --upgrade pip six setuptools flake8 "isort>=4.3.5" "mypy>=0.800" "click==8.0.4" "black<=21.12b0" pywin32 types-python-dateutil
     - name: Create local develop
       run: |
         .\spack\.github\workflows\setup_git.ps1

lib/spack/spack/bootstrap.py

@@ -845,11 +845,13 @@ def ensure_mypy_in_path_or_raise():
 
 
 def black_root_spec():
-    return _root_spec('py-black')
+    # black v21 is the last version to support Python 2.7.
+    # Upgrade when we no longer support Python 2.7
+    return _root_spec('py-black@:21')
 
 
 def ensure_black_in_path_or_raise():
-    """Ensure that isort is in the PATH or raise."""
+    """Ensure that black is in the PATH or raise."""
     executable, root_spec = 'black', black_root_spec()
     return ensure_executables_in_path_or_raise([executable], abstract_spec=root_spec)
 

lib/spack/spack/cmd/style.py

@@ -65,7 +65,7 @@ def is_package(f):
     packages, since we allow `from spack import *` and poking globals
     into packages.
     """
-    return f.startswith("var/spack/repos/") and f.endswith('package.py')
+    return f.startswith("var/spack/repos/") and f.endswith("package.py")
 
 
 #: decorator for adding tools to the list
@@ -94,13 +94,14 @@ def changed_files(base="develop", untracked=True, all_files=False, root=None):
     git = which("git", required=True)
 
     # ensure base is in the repo
-    base_sha = git("rev-parse", "--quiet", "--verify", "--revs-only", base,
-                   fail_on_error=False, output=str)
+    base_sha = git(
+        "rev-parse", "--quiet", "--verify", "--revs-only", base, fail_on_error=False, output=str
+    )
     if git.returncode != 0:
         tty.die(
             "This repository does not have a '%s' revision." % base,
             "spack style needs this branch to determine which files changed.",
-            "Ensure that '%s' exists, or specify files to check explicitly." % base
+            "Ensure that '%s' exists, or specify files to check explicitly." % base,
         )
 
     range = "{0}...".format(base_sha.strip())
@@ -122,10 +123,7 @@ def changed_files(base="develop", untracked=True, all_files=False, root=None):
     if all_files:
         git_args.append(["ls-files", "--exclude-standard"])
 
-    excludes = [
-        os.path.realpath(os.path.join(root, f))
-        for f in exclude_directories
-    ]
+    excludes = [os.path.realpath(os.path.join(root, f)) for f in exclude_directories]
     changed = set()
 
     for arg_list in git_args:
@@ -200,10 +198,10 @@ def setup_parser(subparser):
         help="do not run mypy (default: run mypy if available)",
     )
     subparser.add_argument(
-        "--black",
+        "--no-black",
         dest="black",
-        action="store_true",
-        help="run black if available (default: skip black)",
+        action="store_false",
+        help="run black if available (default: run black if available)",
     )
     subparser.add_argument(
         "--root",
@@ -211,9 +209,7 @@ def setup_parser(subparser):
         default=None,
         help="style check a different spack instance",
     )
-    subparser.add_argument(
-        "files", nargs=argparse.REMAINDER, help="specific files to check"
-    )
+    subparser.add_argument("files", nargs=argparse.REMAINDER, help="specific files to check")
 
 
 def cwd_relative(path, args):
@@ -227,9 +223,7 @@ def rewrite_and_print_output(
     """rewrite ouput with <file>:<line>: format to respect path args"""
     # print results relative to current working directory
     def translate(match):
-        return replacement.format(
-            cwd_relative(match.group(1), args), *list(match.groups()[1:])
-        )
+        return replacement.format(cwd_relative(match.group(1), args), *list(match.groups()[1:]))
 
     for line in output.split("\n"):
         if not line:
@@ -291,18 +285,29 @@ def run_flake8(flake8_cmd, file_list, args):
 def run_mypy(mypy_cmd, file_list, args):
     # always run with config from running spack prefix
     common_mypy_args = [
-        "--config-file", os.path.join(spack.paths.prefix, "pyproject.toml"),
+        "--config-file",
+        os.path.join(spack.paths.prefix, "pyproject.toml"),
         "--show-error-codes",
     ]
-    mypy_arg_sets = [common_mypy_args + [
-        "--package", "spack",
-        "--package", "llnl",
-    ]]
-    if 'SPACK_MYPY_CHECK_PACKAGES' in os.environ:
-        mypy_arg_sets.append(common_mypy_args + [
-            '--package', 'packages',
-            '--disable-error-code', 'no-redef',
-        ])
+    mypy_arg_sets = [
+        common_mypy_args
+        + [
+            "--package",
+            "spack",
+            "--package",
+            "llnl",
+        ]
+    ]
+    if "SPACK_MYPY_CHECK_PACKAGES" in os.environ:
+        mypy_arg_sets.append(
+            common_mypy_args
+            + [
+                "--package",
+                "packages",
+                "--disable-error-code",
+                "no-redef",
+            ]
+        )
 
     returncode = 0
     for mypy_args in mypy_arg_sets:
@@ -334,16 +339,22 @@ def process_files(file_list, is_args):
 
             rewrite_and_print_output(output, args, pat, replacement)
 
-    packages_isort_args = ('--rm', 'spack', '--rm', 'spack.pkgkit', '--rm',
-                           'spack.package_defs', '-a', 'from spack.package import *')
+    packages_isort_args = (
+        "--rm",
+        "spack",
+        "--rm",
+        "spack.pkgkit",
+        "--rm",
+        "spack.package_defs",
+        "-a",
+        "from spack.package import *",
+    )
     packages_isort_args = packages_isort_args + isort_args
 
     # packages
-    process_files(filter(is_package, file_list),
-                  packages_isort_args)
+    process_files(filter(is_package, file_list), packages_isort_args)
     # non-packages
-    process_files(filter(lambda f: not is_package(f), file_list),
-                  isort_args)
+    process_files(filter(lambda f: not is_package(f), file_list), isort_args)
 
     print_tool_result("isort", returncode[0])
     return returncode[0]
@@ -369,6 +380,13 @@ def run_black(black_cmd, file_list, args):
         output = black_cmd(*packed_args, fail_on_error=False, output=str, error=str)
         returncode |= black_cmd.returncode
 
+        # ignore Python 2.7 deprecation error because we already know it's deprecated.
+        output = "\n".join(
+            line
+            for line in output.split("\n")
+            if not "DEPRECATION: Python 2 support will be removed" in line
+        )
+
         rewrite_and_print_output(output, args, pat, replacement)
 
     print_tool_result("black", returncode)
@@ -393,10 +411,7 @@ def style(parser, args):
     args.root = os.path.realpath(args.root) if args.root else spack.paths.prefix
     spack_script = os.path.join(args.root, "bin", "spack")
     if not os.path.exists(spack_script):
-        tty.die(
-            "This does not look like a valid spack root.",
-            "No such file: '%s'" % spack_script
-        )
+        tty.die("This does not look like a valid spack root.", "No such file: '%s'" % spack_script)
 
     file_list = args.files
     if file_list:

pyproject.toml

@@ -1,3 +1,18 @@
+[tool.black]
+line-length = 99
+target-version = ['py27', 'py35', 'py36', 'py37', 'py38', 'py39', 'py310']
+include = '''
+    \.pyi?$
+'''
+extend-exclude = '''
+/(
+    \.git
+  | \.mypy_cache
+  | ^lib/spack/external/
+  | ^opt/
+)/
+'''
+
 [tool.isort]
 profile = "black"
 sections = [

share/spack/spack-completion.bash

@@ -1697,7 +1697,7 @@ _spack_stage() {
 _spack_style() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help -b --base -a --all -r --root-relative -U --no-untracked -f --fix --no-isort --no-flake8 --no-mypy --black --root"
+        SPACK_COMPREPLY="-h --help -b --base -a --all -r --root-relative -U --no-untracked -f --fix --no-isort --no-flake8 --no-mypy --no-black --root"
     else
         SPACK_COMPREPLY=""
     fi

var/spack/repos/builtin/packages/py-black/package.py

@@ -21,6 +21,9 @@ class PyBlack(PythonPackage):
     version('22.3.0', sha256='35020b8886c022ced9282b51b5a875b6d1ab0c387b31a065b84db7c33085ca79')
     version('22.1.0', sha256='a7c0192d35635f6fc1174be575cb7915e92e5dd629ee79fdaf0dcfa41a80afb5')
 
+    # This is the last v21 release, and it's needed to format for Python 2.7
+    version('21.12b0', sha256='77b80f693a569e2e527958459634f18df9b0ba2625ba4e0c2d5da5be42e6f2b3')
+
     variant('d', default=False, description='enable blackd HTTP server')
     variant('colorama', default=False, description='enable colorama support')
     variant('uvloop', default=False, description='enable uvloop support')
@@ -32,10 +35,14 @@ class PyBlack(PythonPackage):
 
     # setup.py
     depends_on('python@3.6.2:', type=('build', 'run'))
+
     depends_on('py-click@8:', type=('build', 'run'))
+    # see: https://github.com/psf/black/issues/2964
+    # note that pip doesn't know this constraint.
+    depends_on("py-click@:8.0", when="@:22.2", type=("build", "run"))
+
     depends_on('py-platformdirs@2:', type=('build', 'run'))
-    depends_on('py-tomli@1.1:', when='@22.3: ^python@:3.10', type=('build', 'run'))
-    depends_on('py-tomli@1.1:', when='@22.1', type=('build', 'run'))
+    depends_on('py-tomli@1.1:', when='@21.7:', type=('build', 'run'))
     depends_on('py-typed-ast@1.4.2:', when='^python@:3.7', type=('build', 'run'))
     depends_on('py-pathspec@0.9:', type=('build', 'run'))
     depends_on('py-dataclasses@0.6:', when='^python@:3.6', type=('build', 'run'))