Spack on Windows: fix "spack load --list" and "spack unload" (#35720)

Fix the following on Windows: * `spack load --list` (this printed 0 packages even if packages were loaded) * `spack unload <package>` (this said that the package is not loaded even if it was) Update unit tests for `spack load` to also run on Windows (specifically for ".bat"). This involved refactoring a few tests to parameterize based on whether the unit tests are being run on a Windows system (and to account for batch syntax).
2024-06-27 14:44:36 -04:00
parent b28b26c39a
commit 4c7d18a772
6 changed files with 121 additions and 77 deletions
--- a/lib/spack/spack/cmd/init.py
+++ b/lib/spack/spack/cmd/init.py
@@ -444,7 +444,7 @@ def format_list(specs):
 def filter_loaded_specs(specs):
    """Filter a list of specs returning only those that are
    currently loaded."""
-    hashes = os.environ.get(uenv.spack_loaded_hashes_var, "").split(":")
+    hashes = os.environ.get(uenv.spack_loaded_hashes_var, "").split(os.pathsep)
    return [x for x in specs if x.dag_hash() in hashes]


--- a/lib/spack/spack/cmd/unload.py
+++ b/lib/spack/spack/cmd/unload.py
@@ -71,7 +71,7 @@ def unload(parser, args):
            "Cannot specify specs on command line when unloading all specs with '--all'"
        )

-    hashes = os.environ.get(uenv.spack_loaded_hashes_var, "").split(":")
+    hashes = os.environ.get(uenv.spack_loaded_hashes_var, "").split(os.pathsep)
    if args.specs:
        specs = [
            spack.cmd.disambiguate_spec_from_hashes(spec, hashes)
--- a/lib/spack/spack/test/cmd/find.py
+++ b/lib/spack/spack/test/cmd/find.py
@@ -434,7 +434,7 @@ def test_find_loaded(database, working_env):
    output = find("--loaded", "--group")
    assert output == ""

-    os.environ[uenv.spack_loaded_hashes_var] = ":".join(
+    os.environ[uenv.spack_loaded_hashes_var] = os.pathsep.join(
        [x.dag_hash() for x in spack.store.STORE.db.query()]
    )
    output = find("--loaded")
--- a/lib/spack/spack/test/cmd/load.py
+++ b/lib/spack/spack/test/cmd/load.py
@@ -4,6 +4,7 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 import os
 import re
+import sys

 import pytest

@@ -17,101 +18,125 @@
 install = SpackCommand("install")
 location = SpackCommand("location")

-pytestmark = pytest.mark.not_on_windows("does not run on windows")
-

 def test_manpath_trailing_colon(
    install_mockery, mock_fetch, mock_archive, mock_packages, working_env
 ):
+    (shell, set_command, commandsep) = (
+        ("--bat", 'set "%s=%s"', "\n")
+        if sys.platform == "win32"
+        else ("--sh", "export %s=%s", ";")
+    )
+
    """Test that the commands generated by load add the MANPATH prefix
    inspections. Also test that Spack correctly preserves the default/existing
    manpath search path via a trailing colon"""
    install("mpileaks")

-    sh_out = load("--sh", "mpileaks")
-    lines = sh_out.split("\n")
-    assert any(re.match(r"export MANPATH=.*:;", ln) for ln in lines)
+    sh_out = load(shell, "mpileaks")
+    lines = [line.strip("\n") for line in sh_out.split(commandsep)]
+    assert any(re.match(set_command % ("MANPATH", ".*" + os.pathsep), ln) for ln in lines)
+    os.environ["MANPATH"] = "/tmp/man" + os.pathsep

-    os.environ["MANPATH"] = "/tmp/man:"
-
-    sh_out = load("--sh", "mpileaks")
-    lines = sh_out.split("\n")
-    assert any(re.match(r"export MANPATH=.*:/tmp/man:;", ln) for ln in lines)
+    sh_out = load(shell, "mpileaks")
+    lines = [line.strip("\n") for line in sh_out.split(commandsep)]
+    assert any(
+        re.match(set_command % ("MANPATH", ".*" + os.pathsep + "/tmp/man" + os.pathsep), ln)
+        for ln in lines
+    )


 def test_load_recursive(install_mockery, mock_fetch, mock_archive, mock_packages, working_env):
-    """Test that `spack load` applies prefix inspections of its required runtime deps in
-    topo-order"""
-    install("mpileaks")
-    mpileaks_spec = spack.spec.Spec("mpileaks").concretized()
+    def test_load_shell(shell, set_command):
+        """Test that `spack load` applies prefix inspections of its required runtime deps in
+        topo-order"""
+        install("mpileaks")
+        mpileaks_spec = spack.spec.Spec("mpileaks").concretized()

-    # Ensure our reference variable is cleed.
-    os.environ["CMAKE_PREFIX_PATH"] = "/hello:/world"
+        # Ensure our reference variable is clean.
+        os.environ["CMAKE_PREFIX_PATH"] = "/hello" + os.pathsep + "/world"

-    sh_out = load("--sh", "mpileaks")
-    csh_out = load("--csh", "mpileaks")
+        shell_out = load(shell, "mpileaks")

-    def extract_cmake_prefix_path(output, prefix):
-        return next(cmd for cmd in output.split(";") if cmd.startswith(prefix))[
-            len(prefix) :
-        ].split(":")
+        def extract_value(output, variable):
+            match = re.search(set_command % variable, output, flags=re.MULTILINE)
+            value = match.group(1)
+            return value.split(os.pathsep)

-    # Map a prefix found in CMAKE_PREFIX_PATH back to a package name in mpileaks' DAG.
-    prefix_to_pkg = lambda prefix: next(
-        s.name for s in mpileaks_spec.traverse() if s.prefix == prefix
-    )
+        # Map a prefix found in CMAKE_PREFIX_PATH back to a package name in mpileaks' DAG.
+        prefix_to_pkg = lambda prefix: next(
+            s.name for s in mpileaks_spec.traverse() if s.prefix == prefix
+        )

-    paths_sh = extract_cmake_prefix_path(sh_out, prefix="export CMAKE_PREFIX_PATH=")
-    paths_csh = extract_cmake_prefix_path(csh_out, prefix="setenv CMAKE_PREFIX_PATH ")
+        paths_shell = extract_value(shell_out, "CMAKE_PREFIX_PATH")

-    # Shouldn't be a difference between loading csh / sh, so check they're the same.
-    assert paths_sh == paths_csh
+        # We should've prepended new paths, and keep old ones.
+        assert paths_shell[-2:] == ["/hello", "/world"]

-    # We should've prepended new paths, and keep old ones.
-    assert paths_sh[-2:] == ["/hello", "/world"]
+        # All but the last two paths are added by spack load; lookup what packages they're from.
+        pkgs = [prefix_to_pkg(p) for p in paths_shell[:-2]]

-    # All but the last two paths are added by spack load; lookup what packages they're from.
-    pkgs = [prefix_to_pkg(p) for p in paths_sh[:-2]]
+        # Do we have all the runtime packages?
+        assert set(pkgs) == set(
+            s.name for s in mpileaks_spec.traverse(deptype=("link", "run"), root=True)
+        )

-    # Do we have all the runtime packages?
-    assert set(pkgs) == set(
-        s.name for s in mpileaks_spec.traverse(deptype=("link", "run"), root=True)
-    )
+        # Finally, do we list them in topo order?
+        for i, pkg in enumerate(pkgs):
+            set(s.name for s in mpileaks_spec[pkg].traverse(direction="parents")) in set(pkgs[:i])

-    # Finally, do we list them in topo order?
-    for i, pkg in enumerate(pkgs):
-        set(s.name for s in mpileaks_spec[pkg].traverse(direction="parents")) in set(pkgs[:i])
+        # Lastly, do we keep track that mpileaks was loaded?
+        assert (
+            extract_value(shell_out, uenv.spack_loaded_hashes_var)[0] == mpileaks_spec.dag_hash()
+        )
+        return paths_shell

-    # Lastly, do we keep track that mpileaks was loaded?
-    assert f"export {uenv.spack_loaded_hashes_var}={mpileaks_spec.dag_hash()}" in sh_out
-    assert f"setenv {uenv.spack_loaded_hashes_var} {mpileaks_spec.dag_hash()}" in csh_out
+    if sys.platform == "win32":
+        shell, set_command = ("--bat", r'set "%s=(.*)"')
+        test_load_shell(shell, set_command)
+    else:
+        params = [("--sh", r"export %s=([^;]*)"), ("--csh", r"setenv %s ([^;]*)")]
+        shell, set_command = params[0]
+        paths_sh = test_load_shell(shell, set_command)
+        shell, set_command = params[1]
+        paths_csh = test_load_shell(shell, set_command)
+        assert paths_sh == paths_csh


-def test_load_includes_run_env(install_mockery, mock_fetch, mock_archive, mock_packages):
+@pytest.mark.parametrize(
+    "shell,set_command",
+    (
+        [("--bat", 'set "%s=%s"')]
+        if sys.platform == "win32"
+        else [("--sh", "export %s=%s"), ("--csh", "setenv %s %s")]
+    ),
+)
+def test_load_includes_run_env(
+    shell, set_command, install_mockery, mock_fetch, mock_archive, mock_packages
+):
    """Tests that environment changes from the package's
    `setup_run_environment` method are added to the user environment in
    addition to the prefix inspections"""
    install("mpileaks")

-    sh_out = load("--sh", "mpileaks")
-    csh_out = load("--csh", "mpileaks")
+    shell_out = load(shell, "mpileaks")

-    assert "export FOOBAR=mpileaks" in sh_out
-    assert "setenv FOOBAR mpileaks" in csh_out
+    assert set_command % ("FOOBAR", "mpileaks") in shell_out


 def test_load_first(install_mockery, mock_fetch, mock_archive, mock_packages):
    """Test with and without the --first option"""
+    shell = "--bat" if sys.platform == "win32" else "--sh"
    install("libelf@0.8.12")
    install("libelf@0.8.13")

    # Now there are two versions of libelf, which should cause an error
-    out = load("--sh", "libelf", fail_on_error=False)
+    out = load(shell, "libelf", fail_on_error=False)
    assert "matches multiple packages" in out
    assert "Use a more specific spec" in out

    # Using --first should avoid the error condition
-    load("--sh", "--first", "libelf")
+    load(shell, "--first", "libelf")


 def test_load_fails_no_shell(install_mockery, mock_fetch, mock_archive, mock_packages):
@@ -122,7 +147,24 @@ def test_load_fails_no_shell(install_mockery, mock_fetch, mock_archive, mock_pac
    assert "To set up shell support" in out


-def test_unload(install_mockery, mock_fetch, mock_archive, mock_packages, working_env):
+@pytest.mark.parametrize(
+    "shell,set_command,unset_command",
+    (
+        [("--bat", 'set "%s=%s"', 'set "%s="')]
+        if sys.platform == "win32"
+        else [("--sh", "export %s=%s", "unset %s"), ("--csh", "setenv %s %s", "unsetenv %s")]
+    ),
+)
+def test_unload(
+    shell,
+    set_command,
+    unset_command,
+    install_mockery,
+    mock_fetch,
+    mock_archive,
+    mock_packages,
+    working_env,
+):
    """Tests that any variables set in the user environment are undone by the
    unload command"""
    install("mpileaks")
@@ -130,16 +172,16 @@ def test_unload(install_mockery, mock_fetch, mock_archive, mock_packages, workin

    # Set so unload has something to do
    os.environ["FOOBAR"] = "mpileaks"
-    os.environ[uenv.spack_loaded_hashes_var] = "%s:%s" % (mpileaks_spec.dag_hash(), "garbage")
+    os.environ[uenv.spack_loaded_hashes_var] = ("%s" + os.pathsep + "%s") % (
+        mpileaks_spec.dag_hash(),
+        "garbage",
+    )

-    sh_out = unload("--sh", "mpileaks")
-    csh_out = unload("--csh", "mpileaks")
+    shell_out = unload(shell, "mpileaks")

-    assert "unset FOOBAR" in sh_out
-    assert "unsetenv FOOBAR" in csh_out
+    assert (unset_command % "FOOBAR") in shell_out

-    assert "export %s=garbage" % uenv.spack_loaded_hashes_var in sh_out
-    assert "setenv %s garbage" % uenv.spack_loaded_hashes_var in csh_out
+    assert set_command % (uenv.spack_loaded_hashes_var, "garbage") in shell_out


 def test_unload_fails_no_shell(
--- a/lib/spack/spack/util/environment.py
+++ b/lib/spack/spack/util/environment.py
@@ -679,8 +679,8 @@ def shell_modifications(
            for modifier in actions:
                modifier.execute(new_env)

-        if "MANPATH" in new_env and not new_env["MANPATH"].endswith(":"):
-            new_env["MANPATH"] += ":"
+        if "MANPATH" in new_env and not new_env["MANPATH"].endswith(os.pathsep):
+            new_env["MANPATH"] += os.pathsep

        cmds = ""