spack style: add --root option (#25085)

This adds a `--root` option so that `spack style` can check style for
a spack instance other than its own.

We also change the inner workings of `spack style` so that `--config FILE`
(and similar options for the various tools) options are used. This ensures
that when `spack style` runs, it always uses the config from the running spack,
and does *not* pick up configuration from the external root.

- [x] add `--root` option to `spack style`
- [x] add `--config` (or similar) option when invoking style tools
- [x] add a test that verifies we can check an external instance

.github/workflows/unit_tests.yaml

@@ -138,6 +138,11 @@ jobs:
     - name: Install Python packages
       run: |
           pip install --upgrade pip six setuptools codecov coverage[toml]
+          # 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
+          fi
     - name: Setup git configuration
       run: |
           # Need this for the git tests to succeed.

lib/spack/spack/cmd/style.py

@@ -39,11 +39,10 @@ def grouper(iterable, n, fillvalue=None):
         yield filter(None, group)
 
 
-#: directory where spack style started, for relativizing paths
+#: List of directories to exclude from checks -- relative to spack root
-initial_working_dir = None
+exclude_directories = [
-
+    os.path.relpath(spack.paths.external_path, spack.paths.prefix),
-#: List of directories to exclude from checks.
+]
-exclude_directories = [spack.paths.external_path]
 
 #: Order in which tools should be run. flake8 is last so that it can
 #: double-check the results of other tools (if, e.g., --fix was provided)
@@ -66,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/") or "docs/tutorial/examples" in f
+    return f.startswith("var/spack/repos/")
 
 
 #: decorator for adding tools to the list
@@ -80,8 +79,10 @@ def __call__(self, fun):
         return fun
 
 
-def changed_files(base=None, untracked=True, all_files=False):
+def changed_files(base=None, untracked=True, all_files=False, root=None):
     """Get list of changed files in the Spack repository."""
+    if root is None:
+        root = spack.paths.prefix
 
     git = which("git", required=True)
 
@@ -89,6 +90,16 @@ def changed_files(base=None, untracked=True, all_files=False):
     if base is None:
         base = os.environ.get("GITHUB_BASE_REF", "develop")
 
+    # ensure base is in the repo
+    git("show-ref", "--verify", "--quiet", "refs/heads/%s" % base,
+        fail_on_error=False)
+    if git.returncode != 0:
+        tty.die(
+            "This repository does not have a '%s' branch." % base,
+            "spack style needs this branch to determine which files changed.",
+            "Ensure that '%s' exists, or specify files to check explicitly." % base
+        )
+
     range = "{0}...".format(base)
 
     git_args = [
@@ -108,7 +119,10 @@ def changed_files(base=None, untracked=True, all_files=False):
     if all_files:
         git_args.append(["ls-files", "--exclude-standard"])
 
-    excludes = [os.path.realpath(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:
@@ -188,14 +202,20 @@ def setup_parser(subparser):
         action="store_true",
         help="run black if available (default: skip black)",
     )
+    subparser.add_argument(
+        "--root",
+        action="store",
+        default=None,
+        help="style check a different spack instance",
+    )
     subparser.add_argument(
         "files", nargs=argparse.REMAINDER, help="specific files to check"
     )
 
 
-def cwd_relative(path):
+def cwd_relative(path, args):
     """Translate prefix-relative path to current working directory-relative."""
-    return os.path.relpath(os.path.join(spack.paths.prefix, path), initial_working_dir)
+    return os.path.relpath(os.path.join(args.root, path), args.initial_working_dir)
 
 
 def rewrite_and_print_output(
@@ -205,7 +225,7 @@ def rewrite_and_print_output(
     # print results relative to current working directory
     def translate(match):
         return replacement.format(
-            cwd_relative(match.group(1)), *list(match.groups()[1:])
+            cwd_relative(match.group(1), args), *list(match.groups()[1:])
         )
 
     for line in output.split("\n"):
@@ -218,14 +238,14 @@ def translate(match):
 
 def print_style_header(file_list, args):
     tools = [tool for tool, _ in tool_order if getattr(args, tool)]
-    tty.msg("Running style checks on spack:", "selected: " + ", ".join(tools))
+    tty.msg("Running style checks on spack", "selected: " + ", ".join(tools))
 
     # translate modified paths to cwd_relative if needed
     paths = [filename.strip() for filename in file_list]
     if not args.root_relative:
-        paths = [cwd_relative(filename) for filename in paths]
+        paths = [cwd_relative(filename, args) for filename in paths]
 
-    tty.msg("Modified files:", *paths)
+    tty.msg("Modified files", *paths)
     sys.stdout.flush()
 
 
@@ -249,12 +269,9 @@ def run_flake8(flake8_cmd, file_list, args):
     # 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):
-
         output = flake8_cmd(
-            # use .flake8 implicitly to work around bug in flake8 upstream
+            # always run with config from running spack prefix
-            # append-config is ignored if `--config` is explicitly listed
+            "--config=%s" % os.path.join(spack.paths.prefix, ".flake8"),
-            # see: https://gitlab.com/pycqa/flake8/-/issues/455
-            # "--config=.flake8",
             *chunk,
             fail_on_error=False,
             output=str
@@ -269,12 +286,18 @@ def run_flake8(flake8_cmd, file_list, args):
 
 @tool("mypy")
 def run_mypy(mypy_cmd, file_list, args):
-    mpy_args = ["--package", "spack", "--package", "llnl", "--show-error-codes"]
+    # always run with config from running spack prefix
+    mypy_args = [
+        "--config-file", os.path.join(spack.paths.prefix, "pyproject.toml"),
+        "--package", "spack",
+        "--package", "llnl",
+        "--show-error-codes",
+    ]
     # not yet, need other updates to enable this
     # if any([is_package(f) for f in file_list]):
-    #     mpy_args.extend(["--package", "packages"])
+    #     mypy_args.extend(["--package", "packages"])
 
-    output = mypy_cmd(*mpy_args, fail_on_error=False, output=str)
+    output = mypy_cmd(*mypy_args, fail_on_error=False, output=str)
     returncode = mypy_cmd.returncode
 
     rewrite_and_print_output(output, args)
@@ -285,13 +308,16 @@ def run_mypy(mypy_cmd, file_list, args):
 
 @tool("isort")
 def run_isort(isort_cmd, file_list, args):
-    check_fix_args = () if args.fix else ("--check", "--diff")
+    # always run with config from running spack prefix
+    isort_args = ("--settings-file", os.path.join(spack.paths.prefix, "pyproject.toml"))
+    if not args.fix:
+        isort_args += ("--check", "--diff")
 
     pat = re.compile("ERROR: (.*) Imports are incorrectly sorted")
     replacement = "ERROR: {0} Imports are incorrectly sorted"
     returncode = 0
     for chunk in grouper(file_list, 100):
-        packed_args = check_fix_args + tuple(chunk)
+        packed_args = isort_args + tuple(chunk)
         output = isort_cmd(*packed_args, fail_on_error=False, output=str, error=str)
         returncode |= isort_cmd.returncode
 
@@ -303,7 +329,12 @@ def run_isort(isort_cmd, file_list, args):
 
 @tool("black")
 def run_black(black_cmd, file_list, args):
-    check_fix_args = () if args.fix else ("--check", "--diff", "--color")
+    # always run with config from running spack prefix
+    black_args = ("--config", os.path.join(spack.paths.prefix, "pyproject.toml"))
+    if not args.fix:
+        black_args += ("--check", "--diff")
+        if color.get_color_when():  # only show color when spack would
+            black_args += ("--color",)
 
     pat = re.compile("would reformat +(.*)")
     replacement = "would reformat {0}"
@@ -312,33 +343,49 @@ def run_black(black_cmd, file_list, args):
     # 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):
-        packed_args = check_fix_args + tuple(chunk)
+        packed_args = black_args + tuple(chunk)
         output = black_cmd(*packed_args, fail_on_error=False, output=str, error=str)
         returncode |= black_cmd.returncode
 
         rewrite_and_print_output(output, args, pat, replacement)
 
     print_tool_result("black", returncode)
+
     return returncode
 
 
 def style(parser, args):
+    # ensure python version is new enough
+    if sys.version_info < (3, 6):
+        tty.die("spack style requires Python 3.6 or later.")
+
     # save initial working directory for relativizing paths later
-    global initial_working_dir
+    args.initial_working_dir = os.getcwd()
-    initial_working_dir = os.getcwd()
+
+    # ensure that the config files we need actually exist in the spack prefix.
+    # assertions b/c users should not ever see these errors -- they're checked in CI.
+    assert os.path.isfile(os.path.join(spack.paths.prefix, "pyproject.toml"))
+    assert os.path.isfile(os.path.join(spack.paths.prefix, ".flake8"))
+
+    # validate spack root if the user provided one
+    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
+        )
 
     file_list = args.files
     if file_list:
 
         def prefix_relative(path):
-            return os.path.relpath(
+            return os.path.relpath(os.path.abspath(os.path.realpath(path)), args.root)
-                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):
+    with working_dir(args.root):
         if not file_list:
             file_list = changed_files(args.base, args.untracked, args.all)
         print_style_header(file_list, args)
@@ -350,18 +397,24 @@ def prefix_relative(path):
                 run_function, required = tools[tool_name]
                 print_tool_header(tool_name)
 
+                try:
                     # Bootstrap tools so we don't need to require install
                     with spack.bootstrap.ensure_bootstrap_configuration():
                         spec = spack.spec.Spec(tool_spec)
+                        cmd = None
                         cmd = spack.bootstrap.get_executable(
                             tool_name, spec=spec, install=True
                         )
                         if not cmd:
                             color.cprint("  @y{%s not in PATH, skipped}" % tool_name)
                             continue
-
                         returncode |= run_function(cmd, file_list, args)
 
+                except Exception as e:
+                    raise spack.error.SpackError(
+                        "Couldn't bootstrap %s:" % tool_name, str(e)
+                    )
+
     if returncode == 0:
         tty.msg(color.colorize("@*{spack style checks were clean}"))
     else:

lib/spack/spack/test/cmd/style.py

@@ -3,6 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+import filecmp
 import os
 import shutil
 import sys
@@ -17,8 +18,20 @@
 from spack.cmd.style import changed_files
 from spack.util.executable import which
 
+#: directory with sample style files
+style_data = os.path.join(spack.paths.test_path, 'data', 'style')
+
+
 style = spack.main.SpackCommand("style")
 
+# spack style requires git to run -- skip the tests if it's not there
+pytestmark = pytest.mark.skipif(not which('git'), reason='requires git')
+
+# The style tools have requirements to use newer Python versions.  We simplify by
+# requiring Python 3.6 or higher to run spack style.
+skip_old_python = pytest.mark.skipif(
+    sys.version_info < (3, 6), reason='requires Python 3.6 or higher')
+
 
 @pytest.fixture(scope="function")
 def flake8_package():
@@ -50,6 +63,9 @@ def flake8_package_with_errors(scope="function"):
         shutil.copy(filename, tmp)
         package = FileFilter(filename)
         package.filter("state = 'unmodified'", "state    =    'modified'", string=True)
+        package.filter(
+            "from spack import *", "from spack import *\nimport os", string=True
+        )
         yield filename
     finally:
         shutil.move(tmp, filename)
@@ -65,6 +81,24 @@ def test_changed_files(flake8_package):
     assert flake8_package in files
 
 
+def test_changed_no_base(tmpdir, capfd):
+    """Ensure that we fail gracefully with no base branch."""
+    tmpdir.join("bin").ensure("spack")
+    git = which("git", required=True)
+    with tmpdir.as_cwd():
+        git("init")
+        git("config", "user.name", "test user")
+        git("config", "user.email", "test@user.com")
+        git("add", ".")
+        git("commit", "-m", "initial commit")
+
+        with pytest.raises(SystemExit):
+            changed_files(base="foobar")
+
+        out, err = capfd.readouterr()
+        assert "This repository does not have a 'foobar' branch." in err
+
+
 def test_changed_files_all_files(flake8_package):
     # it's hard to guarantee "all files", so do some sanity checks.
     files = set([
@@ -92,12 +126,134 @@ def test_changed_files_all_files(flake8_package):
     assert not any(f.startswith(spack.paths.external_path) for f in files)
 
 
-# As of flake8 3.0.0, Python 2.6 and 3.3 are no longer supported
+@pytest.mark.skipif(sys.version_info >= (3, 6), reason="doesn't apply to newer python")
-# http://flake8.pycqa.org/en/latest/release-notes/3.0.0.html
+def test_fail_on_old_python():
-skip_old_python = pytest.mark.skipif(
+    """Ensure that `spack style` runs but fails with older python."""
-    sys.version_info[:2] <= (2, 6) or (3, 0) <= sys.version_info[:2] <= (3, 3),
+    style(fail_on_error=False)
-    reason="flake8 no longer supports Python 2.6 or 3.3 and older",
+    assert style.returncode != 0
-)
+
+
+@skip_old_python
+def test_bad_root(tmpdir):
+    """Ensure that `spack style` doesn't run on non-spack directories."""
+    style("--root", str(tmpdir), fail_on_error=False)
+    assert style.returncode != 0
+
+
+def test_style_is_package(tmpdir):
+    """Ensure the is_package() function works."""
+    assert spack.cmd.style.is_package(
+        "var/spack/repos/builtin/packages/hdf5/package.py"
+    )
+    assert spack.cmd.style.is_package(
+        "var/spack/repos/builtin/packages/zlib/package.py"
+    )
+    assert not spack.cmd.style.is_package("lib/spack/spack/spec.py")
+    assert not spack.cmd.style.is_package("lib/spack/external/pytest.py")
+
+
+@skip_old_python
+def test_bad_bootstrap(monkeypatch):
+    """Ensure we fail gracefully when we can't bootstrap spack style."""
+    monkeypatch.setattr(spack.cmd.style, "tool_order", [
+        ("foobartool", "foobartool"),  # bad package to force concretization failure
+    ])
+    style(fail_on_error=False)
+    assert style.returncode != 0
+
+
+@pytest.fixture
+def external_style_root(flake8_package_with_errors, tmpdir):
+    """Create a mock git repository for running spack style."""
+    git = which("git", required=True)
+
+    # create a sort-of spack-looking directory
+    script = tmpdir / "bin" / "spack"
+    script.ensure()
+    spack_dir = tmpdir / "lib" / "spack" / "spack"
+    spack_dir.ensure("__init__.py")
+    llnl_dir = tmpdir / "lib" / "spack" / "llnl"
+    llnl_dir.ensure("__init__.py")
+
+    # create a base develop branch
+    with tmpdir.as_cwd():
+        git("init")
+        git("config", "user.name", "test user")
+        git("config", "user.email", "test@user.com")
+        git("add", ".")
+        git("commit", "-m", "initial commit")
+        git("branch", "-m", "develop")
+        git("checkout", "-b", "feature")
+
+    # copy the buggy package in
+    py_file = spack_dir / "dummy.py"
+    py_file.ensure()
+    shutil.copy(flake8_package_with_errors, str(py_file))
+
+    # add the buggy file on the feature branch
+    with tmpdir.as_cwd():
+        git("add", str(py_file))
+        git("commit", "-m", "add new file")
+
+    yield tmpdir, py_file
+
+
+@skip_old_python
+@pytest.mark.skipif(not which("black"), reason="black is not installed.")
+@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
+def test_fix_style(external_style_root):
+    """Make sure spack style --fix works."""
+    tmpdir, py_file = external_style_root
+
+    broken_dummy = os.path.join(style_data, "broken.dummy")
+    broken_py = str(tmpdir / "lib" / "spack" / "spack" / "broken.py")
+    fixed_py = os.path.join(style_data, "fixed.py")
+
+    shutil.copy(broken_dummy, broken_py)
+    assert not filecmp.cmp(broken_py, fixed_py)
+
+    output = style(
+        "--root", str(tmpdir),
+        "--no-mypy",    # mypy doesn't fix, so skip it
+        "--no-flake8",  # flake8 doesn't fix, so skip it
+        "--black",
+        "--fix",
+    )
+    print(output)
+    assert filecmp.cmp(broken_py, fixed_py)
+
+
+@skip_old_python
+@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
+@pytest.mark.skipif(not which("isort"), reason="isort is not installed.")
+@pytest.mark.skipif(not which("mypy"), reason="mypy is not installed.")
+@pytest.mark.skipif(not which("black"), reason="black is not installed.")
+def test_external_root(external_style_root):
+    """Ensure we can run in a separate root directory w/o configuration files."""
+    tmpdir, py_file = external_style_root
+
+    # make sure tools are finding issues with external root,
+    # not the real one.
+    output = style(
+        "--root-relative", "--black", "--root", str(tmpdir),
+        fail_on_error=False
+    )
+
+    # make sure it failed
+    assert style.returncode != 0
+
+    # isort error
+    assert "%s Imports are incorrectly sorted" % str(py_file) in output
+
+    # mypy error
+    assert 'lib/spack/spack/dummy.py:10: error: Name "Package" is not defined' in output
+
+    # black error
+    assert "--- lib/spack/spack/dummy.py" in output
+    assert "+++ lib/spack/spack/dummy.py" in output
+
+    # flake8 error
+    assert "lib/spack/spack/dummy.py:7: [F401] 'os' imported but unused" in output
 
 
 @skip_old_python
@@ -138,7 +294,7 @@ def test_style_with_errors(flake8_package_with_errors):
     root_relative = os.path.relpath(flake8_package_with_errors, spack.paths.prefix)
     output = style("--root-relative", flake8_package_with_errors, fail_on_error=False)
     assert root_relative in output
-    assert style.returncode == 1
+    assert style.returncode != 0
     assert "spack style found errors" in output
 
 
@@ -148,5 +304,5 @@ def test_style_with_errors(flake8_package_with_errors):
 def test_style_with_black(flake8_package_with_errors):
     output = style("--black", flake8_package_with_errors, fail_on_error=False)
     assert "black found errors" in output
-    assert style.returncode == 1
+    assert style.returncode != 0
     assert "spack style found errors" in output

lib/spack/spack/test/data/style/broken.dummy

@@ -0,0 +1,12 @@
+# Copyright 2013-2021 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)
+
+import sys
+import os
+
+def this_is_a_function():
+    """This is a docstring."""
+    def this_should_be_offset():
+        sys.stdout.write(os.name)

lib/spack/spack/test/data/style/fixed.py

@@ -0,0 +1,14 @@
+# Copyright 2013-2021 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)
+
+import os
+import sys
+
+
+def this_is_a_function():
+    """This is a docstring."""
+
+    def this_should_be_offset():
+        sys.stdout.write(os.name)

share/spack/spack-completion.bash

@@ -1008,7 +1008,7 @@ _spack_find() {
 _spack_flake8() {
     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"
+        SPACK_COMPREPLY="-h --help -b --base -a --all -r --root-relative -U --no-untracked -f --fix --no-isort --no-flake8 --no-mypy --black --root"
     else
         SPACK_COMPREPLY=""
     fi
@@ -1614,7 +1614,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"
+        SPACK_COMPREPLY="-h --help -b --base -a --all -r --root-relative -U --no-untracked -f --fix --no-isort --no-flake8 --no-mypy --black --root"
     else
         SPACK_COMPREPLY=""
     fi