diff options
| -rw-r--r-- | .github/workflows/unit_tests.yaml | 7 | ||||
| -rw-r--r-- | lib/spack/spack/cmd/style.py | 137 | ||||
| -rw-r--r-- | lib/spack/spack/test/cmd/style.py | 172 | ||||
| -rw-r--r-- | lib/spack/spack/test/data/style/broken.dummy | 12 | ||||
| -rw-r--r-- | lib/spack/spack/test/data/style/fixed.py | 14 | ||||
| -rwxr-xr-x | share/spack/spack-completion.bash | 4 |
6 files changed, 293 insertions, 53 deletions
diff --git a/.github/workflows/unit_tests.yaml b/.github/workflows/unit_tests.yaml index 9caef56566..32b1baf39a 100644 --- a/.github/workflows/unit_tests.yaml +++ b/.github/workflows/unit_tests.yaml @@ -130,7 +130,7 @@ jobs: sudo apt-get -y update # Needed for unit tests sudo apt-get -y install \ - coreutils cvs gfortran graphviz gnupg2 mercurial ninja-build \ + coreutils cvs gfortran graphviz gnupg2 mercurial ninja-build \ patchelf # Needed for kcov sudo apt-get -y install cmake binutils-dev libcurl4-openssl-dev @@ -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. diff --git a/lib/spack/spack/cmd/style.py b/lib/spack/spack/cmd/style.py index 2841a4af0f..473827366b 100644 --- a/lib/spack/spack/cmd/style.py +++ b/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 -initial_working_dir = None - -#: List of directories to exclude from checks. -exclude_directories = [spack.paths.external_path] +#: List of directories to exclude from checks -- relative to spack root +exclude_directories = [ + os.path.relpath(spack.paths.external_path, spack.paths.prefix), +] #: 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 @@ class tool(object): 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: @@ -189,13 +203,19 @@ def setup_parser(subparser): 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 rewrite_and_print_output( 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 - # append-config is ignored if `--config` is explicitly listed - # see: https://gitlab.com/pycqa/flake8/-/issues/455 - # "--config=.flake8", + # always run with config from running spack prefix + "--config=%s" % os.path.join(spack.paths.prefix, ".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 - initial_working_dir = os.getcwd() + args.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( - os.path.abspath(os.path.realpath(path)), spack.paths.prefix - ) + return os.path.relpath(os.path.abspath(os.path.realpath(path)), args.root) 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,17 +397,23 @@ def style(parser, args): run_function, required = tools[tool_name] print_tool_header(tool_name) - # Bootstrap tools so we don't need to require install - with spack.bootstrap.ensure_bootstrap_configuration(): - spec = spack.spec.Spec(tool_spec) - cmd = spack.bootstrap.get_executable( - tool_name, spec=spec, install=True + 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 not cmd: - color.cprint(" @y{%s not in PATH, skipped}" % tool_name) - continue - - returncode |= run_function(cmd, file_list, args) if returncode == 0: tty.msg(color.colorize("@*{spack style checks were clean}")) diff --git a/lib/spack/spack/test/cmd/style.py b/lib/spack/spack/test/cmd/style.py index ceaae5f694..65a301bc27 100644 --- a/lib/spack/spack/test/cmd/style.py +++ b/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 @@ import spack.repo 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 -# http://flake8.pycqa.org/en/latest/release-notes/3.0.0.html -skip_old_python = 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(sys.version_info >= (3, 6), reason="doesn't apply to newer python") +def test_fail_on_old_python(): + """Ensure that `spack style` runs but fails with older python.""" + style(fail_on_error=False) + 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 diff --git a/lib/spack/spack/test/data/style/broken.dummy b/lib/spack/spack/test/data/style/broken.dummy new file mode 100644 index 0000000000..5044a2f476 --- /dev/null +++ b/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) diff --git a/lib/spack/spack/test/data/style/fixed.py b/lib/spack/spack/test/data/style/fixed.py new file mode 100644 index 0000000000..76eec009c6 --- /dev/null +++ b/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) diff --git a/share/spack/spack-completion.bash b/share/spack/spack-completion.bash index 44cc7945fd..bf2a488a85 100755 --- a/share/spack/spack-completion.bash +++ b/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 |