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).

6 files changed, 134 insertions, 90 deletions

diff --git a/bin/spack.bat b/bin/spack.bat
index 8328013599..a5a5384e9f 100644
--- a/bin/spack.bat
+++ b/bin/spack.bat
@@ -188,25 +188,27 @@ if NOT "%_sp_args%"=="%_sp_args:--help=%" (
 goto :end_switch
 
 :case_load
-:: If args contain --sh, --csh, or -h/--help: just execute.
-if defined _sp_args (
-    if NOT "%_sp_args%"=="%_sp_args:--help=%" (
-        goto :default_case
-    ) else if NOT "%_sp_args%"=="%_sp_args:-h=%" (
-        goto :default_case
-    ) else if NOT "%_sp_args%"=="%_sp_args:--bat=%" (
-        goto :default_case
-    )
+if NOT defined _sp_args (
+   exit /B 0
+)
+
+:: If args contain --bat, or -h/--help: just execute.
+if NOT "%_sp_args%"=="%_sp_args:--help=%" (
+    goto :default_case
+) else if NOT "%_sp_args%"=="%_sp_args:-h=%" (
+    goto :default_case
+) else if NOT "%_sp_args%"=="%_sp_args:--bat=%" (
+    goto :default_case
+) else if NOT "%_sp_args%"=="%_sp_args:--list=%" (
+    goto :default_case
 )
 
 for /f "tokens=* USEBACKQ" %%I in (
-    `python "%spack%" %_sp_flags% %_sp_subcommand% --bat %_sp_args%`) do %%I
+    `python "%spack%" %_sp_flags% %_sp_subcommand% --bat %_sp_args%`
+    ) do %%I
 
 goto :end_switch
 
-:case_unload
-goto :case_load
-
 :default_case
 python "%spack%" %_sp_flags% %_sp_subcommand% %_sp_args%
 goto :end_switch
diff --git a/lib/spack/spack/cmd/__init__.py b/lib/spack/spack/cmd/__init__.py
index bd6a3eb768..023b7ae73e 100644
--- a/lib/spack/spack/cmd/__init__.py
+++ b/lib/spack/spack/cmd/__init__.py
@@ -444,7 +444,7 @@ def display_specs(specs, args=None, **kwargs):
 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]
 
 
diff --git a/lib/spack/spack/cmd/unload.py b/lib/spack/spack/cmd/unload.py
index 3bafe5e8c6..65daabcd46 100644
--- 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)
diff --git a/lib/spack/spack/test/cmd/find.py b/lib/spack/spack/test/cmd/find.py
index 37a0a7ff14..95d5734451 100644
--- 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")
diff --git a/lib/spack/spack/test/cmd/load.py b/lib/spack/spack/test/cmd/load.py
index e2abe72e0e..73a062ebd4 100644
--- 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 @@ unload = SpackCommand("unload")
 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)
-
-    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)
-
-
-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()
-
-    # Ensure our reference variable is cleed.
-    os.environ["CMAKE_PREFIX_PATH"] = "/hello:/world"
-
-    sh_out = load("--sh", "mpileaks")
-    csh_out = load("--csh", "mpileaks")
-
-    def extract_cmake_prefix_path(output, prefix):
-        return next(cmd for cmd in output.split(";") if cmd.startswith(prefix))[
-            len(prefix) :
-        ].split(":")
+    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
 
-    # 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
+    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
     )
 
-    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 ")
 
-    # 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_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_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)
-    )
-
-    # 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 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
-
-
-def test_load_includes_run_env(install_mockery, mock_fetch, mock_archive, mock_packages):
+def test_load_recursive(install_mockery, mock_fetch, mock_archive, mock_packages, working_env):
+    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 clean.
+        os.environ["CMAKE_PREFIX_PATH"] = "/hello" + os.pathsep + "/world"
+
+        shell_out = load(shell, "mpileaks")
+
+        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
+        )
+
+        paths_shell = extract_value(shell_out, "CMAKE_PREFIX_PATH")
+
+        # We should've prepended new paths, and keep old ones.
+        assert paths_shell[-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]]
+
+        # 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])
+
+        # 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
+
+    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
+
+
+@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(
diff --git a/lib/spack/spack/util/environment.py b/lib/spack/spack/util/environment.py
index 1d53d33efc..40b2507905 100644
--- a/lib/spack/spack/util/environment.py
+++ b/lib/spack/spack/util/environment.py
@@ -679,8 +679,8 @@ class EnvironmentModifications:
             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 = ""