Windows: executable/path handling (#37762)

Windows executable paths can have spaces in them, which was leading to
errors when constructing Executable objects: the parser was intended
to handle cases where users could provide an executable along with one
or more space-delimited arguments.

* Executable now assumes that it is constructed with a string argument
  that represents the path to the executable, but no additional arguments.
* Invocations of Executable.__init__ that depended on this have been
  updated (this includes the core, tests, and one instance of builtin
  repository package).
* The error handling for failed invocations of Executable.__call__ now
  includes a check for whether the executable name/path contains a
  space, to help users debug cases where they (now incorrectly)
  concatenate the path and the arguments.

6 files changed, 93 insertions, 62 deletions

diff --git a/lib/spack/spack/paths.py b/lib/spack/spack/paths.py
index 18656b76c2..4e66adf11a 100644
--- a/lib/spack/spack/paths.py
+++ b/lib/spack/spack/paths.py
@@ -10,11 +10,12 @@ throughout Spack and should bring in a minimal number of external
 dependencies.
 """
 import os
+from pathlib import PurePath
 
 import llnl.util.filesystem
 
 #: This file lives in $prefix/lib/spack/spack/__file__
-prefix = llnl.util.filesystem.ancestor(__file__, 4)
+prefix = str(PurePath(llnl.util.filesystem.ancestor(__file__, 4)))
 
 #: synonym for prefix
 spack_root = prefix
@@ -88,7 +89,7 @@ def _get_user_cache_path():
     return os.path.expanduser(os.getenv("SPACK_USER_CACHE_PATH") or "~%s.spack" % os.sep)
 
 
-user_cache_path = _get_user_cache_path()
+user_cache_path = str(PurePath(_get_user_cache_path()))
 
 #: junit, cdash, etc. reports about builds
 reports_path = os.path.join(user_cache_path, "reports")
diff --git a/lib/spack/spack/test/util/executable.py b/lib/spack/spack/test/util/executable.py
index b4e4b4888e..839cf04bfb 100644
--- a/lib/spack/spack/test/util/executable.py
+++ b/lib/spack/spack/test/util/executable.py
@@ -5,6 +5,7 @@
 
 import os
 import sys
+from pathlib import PurePath
 
 import pytest
 
@@ -16,13 +17,16 @@ from spack.hooks.sbang import filter_shebangs_in_directory
 
 
 def test_read_unicode(tmpdir, working_env):
-    script_name = "print_unicode.py"
-    # read the unicode back in and see whether things work
-    if sys.platform == "win32":
-        script = ex.Executable("%s %s" % (sys.executable, script_name))
-    else:
-        script = ex.Executable("./%s" % script_name)
     with tmpdir.as_cwd():
+        script_name = "print_unicode.py"
+        # read the unicode back in and see whether things work
+        if sys.platform == "win32":
+            script = ex.Executable("%s" % (sys.executable))
+            script_args = [script_name]
+        else:
+            script = ex.Executable("./%s" % script_name)
+            script_args = []
+
         os.environ["LD_LIBRARY_PATH"] = spack.main.spack_ld_library_path
         # make a script that prints some unicode
         with open(script_name, "w") as f:
@@ -38,7 +42,7 @@ print(u'\\xc3')
         fs.set_executable(script_name)
         filter_shebangs_in_directory(".", [script_name])
 
-        assert "\xc3" == script(output=str).strip()
+        assert "\xc3" == script(*script_args, output=str).strip()
 
 
 def test_which_relative_path_with_slash(tmpdir, working_env):
@@ -68,7 +72,7 @@ def test_which_with_slash_ignores_path(tmpdir, working_env):
 
     path = str(tmpdir.join("exe"))
     wrong_path = str(tmpdir.join("bin", "exe"))
-    os.environ["PATH"] = os.path.dirname(wrong_path)
+    os.environ["PATH"] = str(PurePath(wrong_path).parent)
 
     with tmpdir.as_cwd():
         if sys.platform == "win32":
diff --git a/lib/spack/spack/util/environment.py b/lib/spack/spack/util/environment.py
index da6832e4dd..30322c320a 100644
--- a/lib/spack/spack/util/environment.py
+++ b/lib/spack/spack/util/environment.py
@@ -1066,8 +1066,8 @@ def environment_after_sourcing_files(
 
     Keyword Args:
         env (dict): the initial environment (default: current environment)
-        shell (str): the shell to use (default: ``/bin/bash``)
-        shell_options (str): options passed to the shell (default: ``-c``)
+        shell (str): the shell to use (default: ``/bin/bash`` or ``cmd.exe`` (Windows))
+        shell_options (str): options passed to the shell (default: ``-c`` or ``/C`` (Windows))
         source_command (str): the command to run (default: ``source``)
         suppress_output (str): redirect used to suppress output of command
             (default: ``&> /dev/null``)
@@ -1075,15 +1075,23 @@ def environment_after_sourcing_files(
             only when the previous command succeeds (default: ``&&``)
     """
     # Set the shell executable that will be used to source files
-    shell_cmd = kwargs.get("shell", "/bin/bash")
-    shell_options = kwargs.get("shell_options", "-c")
-    source_command = kwargs.get("source_command", "source")
-    suppress_output = kwargs.get("suppress_output", "&> /dev/null")
+    if sys.platform == "win32":
+        shell_cmd = kwargs.get("shell", "cmd.exe")
+        shell_options = kwargs.get("shell_options", "/C")
+        suppress_output = kwargs.get("suppress_output", "")
+        source_command = kwargs.get("source_command", "")
+    else:
+        shell_cmd = kwargs.get("shell", "/bin/bash")
+        shell_options = kwargs.get("shell_options", "-c")
+        suppress_output = kwargs.get("suppress_output", "&> /dev/null")
+        source_command = kwargs.get("source_command", "source")
     concatenate_on_success = kwargs.get("concatenate_on_success", "&&")
 
-    shell = Executable(" ".join([shell_cmd, shell_options]))
+    shell = Executable(shell_cmd)
 
     def _source_single_file(file_and_args, environment):
+        shell_options_list = shell_options.split()
+
         source_file = [source_command]
         source_file.extend(x for x in file_and_args)
         source_file = " ".join(source_file)
@@ -1101,7 +1109,14 @@ def environment_after_sourcing_files(
         source_file_arguments = " ".join(
             [source_file, suppress_output, concatenate_on_success, dump_environment_cmd]
         )
-        output = shell(source_file_arguments, output=str, env=environment, ignore_quotes=True)
+        output = shell(
+            *shell_options_list,
+            source_file_arguments,
+            output=str,
+            env=environment,
+            ignore_quotes=True,
+        )
+
         return json.loads(output)
 
     current_environment = kwargs.get("env", dict(os.environ))
diff --git a/lib/spack/spack/util/executable.py b/lib/spack/spack/util/executable.py
index 788348879a..a3f3d16c77 100644
--- a/lib/spack/spack/util/executable.py
+++ b/lib/spack/spack/util/executable.py
@@ -5,14 +5,13 @@
 
 import os
 import re
-import shlex
 import subprocess
 import sys
+from pathlib import Path, PurePath
 
 import llnl.util.tty as tty
 
 import spack.error
-from spack.util.path import Path, format_os_path, path_to_os_path, system_path_filter
 
 __all__ = ["Executable", "which", "ProcessError"]
 
@@ -21,11 +20,12 @@ class Executable:
     """Class representing a program that can be run on the command line."""
 
     def __init__(self, name):
-        # necesary here for the shlex call to succeed
-        name = format_os_path(name, mode=Path.unix)
-        self.exe = shlex.split(str(name))
-        # filter back to platform dependent path
-        self.exe = path_to_os_path(*self.exe)
+        file_path = str(Path(name))
+        if sys.platform != "win32" and name.startswith("."):
+            # pathlib strips the ./ from relative paths so it must be added back
+            file_path = os.path.join(".", file_path)
+        self.exe = [file_path]
+
         self.default_env = {}
         from spack.util.environment import EnvironmentModifications  # no cycle
 
@@ -35,12 +35,10 @@ class Executable:
         if not self.exe:
             raise ProcessError("Cannot construct executable for '%s'" % name)
 
-    @system_path_filter
     def add_default_arg(self, arg):
         """Add a default argument to the command."""
         self.exe.append(arg)
 
-    @system_path_filter
     def add_default_env(self, key, value):
         """Set an environment variable when the command is run.
 
@@ -70,7 +68,7 @@ class Executable:
         Returns:
             str: The basename of the executable
         """
-        return os.path.basename(self.path)
+        return PurePath(self.path).name
 
     @property
     def path(self):
@@ -79,7 +77,7 @@ class Executable:
         Returns:
             str: The path to the executable
         """
-        return self.exe[0]
+        return str(PurePath(self.exe[0]))
 
     def __call__(self, *args, **kwargs):
         """Run this executable in a subprocess.
@@ -235,7 +233,11 @@ class Executable:
             return result
 
         except OSError as e:
-            raise ProcessError("%s: %s" % (self.exe[0], e.strerror), "Command: " + cmd_line_string)
+            message = "Command: " + cmd_line_string
+            if " " in self.exe[0]:
+                message += "\nDid you mean to add a space to the command?"
+
+            raise ProcessError("%s: %s" % (self.exe[0], e.strerror), message)
 
         except subprocess.CalledProcessError as e:
             if fail_on_error:
@@ -269,39 +271,48 @@ class Executable:
         return " ".join(self.exe)
 
 
-@system_path_filter
 def which_string(*args, **kwargs):
     """Like ``which()``, but return a string instead of an ``Executable``."""
     path = kwargs.get("path", os.environ.get("PATH", ""))
     required = kwargs.get("required", False)
 
+    if isinstance(path, list):
+        paths = [Path(str(x)) for x in path]
+
     if isinstance(path, str):
-        path = path.split(os.pathsep)
+        paths = [Path(x) for x in path.split(os.pathsep)]
 
-    for name in args:
-        win_candidates = []
-        if sys.platform == "win32" and (not name.endswith(".exe") and not name.endswith(".bat")):
-            win_candidates = [name + ext for ext in [".exe", ".bat"]]
-        candidate_names = [name] if not win_candidates else win_candidates
+    def get_candidate_items(search_item):
+        if sys.platform == "win32" and not search_item.suffix:
+            return [search_item.parent / (search_item.name + ext) for ext in [".exe", ".bat"]]
 
+        return [Path(search_item)]
+
+    def add_extra_search_paths(paths):
+        with_parents = []
+        with_parents.extend(paths)
         if sys.platform == "win32":
-            new_path = path[:]
-            for p in path:
-                if os.path.basename(p) == "bin":
-                    new_path.append(os.path.dirname(p))
-            path = new_path
-
-        for candidate_name in candidate_names:
-            if os.path.sep in candidate_name:
-                exe = os.path.abspath(candidate_name)
-                if os.path.isfile(exe) and os.access(exe, os.X_OK):
-                    return exe
-            else:
-                for directory in path:
-                    directory = path_to_os_path(directory).pop()
-                    exe = os.path.join(directory, candidate_name)
-                    if os.path.isfile(exe) and os.access(exe, os.X_OK):
-                        return exe
+            for p in paths:
+                if p.name == "bin":
+                    with_parents.append(p.parent)
+        return with_parents
+
+    for search_item in args:
+        search_paths = []
+        search_paths.extend(paths)
+        if search_item.startswith("."):
+            # we do this because pathlib will strip any leading ./
+            search_paths.insert(0, Path.cwd())
+        search_paths = add_extra_search_paths(search_paths)
+
+        search_item = Path(search_item)
+        candidate_items = get_candidate_items(Path(search_item))
+
+        for candidate_item in candidate_items:
+            for directory in search_paths:
+                exe = directory / candidate_item
+                if exe.is_file() and os.access(str(exe), os.X_OK):
+                    return str(exe)
 
     if required:
         raise CommandNotFoundError("spack requires '%s'. Make sure it is in your path." % args[0])
@@ -326,7 +337,7 @@ def which(*args, **kwargs):
         Executable: The first executable that is found in the path
     """
     exe = which_string(*args, **kwargs)
-    return Executable(shlex.quote(exe)) if exe else None
+    return Executable(exe) if exe else None
 
 
 class ProcessError(spack.error.SpackError):
diff --git a/var/spack/repos/builtin.mock/packages/cmake-client/package.py b/var/spack/repos/builtin.mock/packages/cmake-client/package.py
index 2ce5a98dd1..a0694f2d6d 100644
--- a/var/spack/repos/builtin.mock/packages/cmake-client/package.py
+++ b/var/spack/repos/builtin.mock/packages/cmake-client/package.py
@@ -109,7 +109,7 @@ class CmakeClient(CMakePackage):
         print(cmake)
         print(cmake.exe)
         check(
-            cmake.exe[0].startswith(spec["cmake"].prefix.bin),
+            cmake.path.startswith(spec["cmake"].prefix.bin),
             "Wrong cmake was in environment: %s" % cmake,
         )
 
diff --git a/var/spack/repos/builtin/packages/oommf/package.py b/var/spack/repos/builtin/packages/oommf/package.py
index f7127f073e..a3a4a6aeac 100644
--- a/var/spack/repos/builtin/packages/oommf/package.py
+++ b/var/spack/repos/builtin/packages/oommf/package.py
@@ -168,15 +168,15 @@ class Oommf(Package):
     def configure(self, spec, prefix):
         # change into directory with source code
         with working_dir(self.get_oommf_source_root()):
-            configure = Executable("./oommf.tcl pimake distclean")
-            configure()
-            configure2 = Executable("./oommf.tcl pimake upgrade")
-            configure2()
+            configure = Executable("./oommf.tcl")
+            configure("pimake", "distclean")
+            configure2 = Executable("./oommf.tcl")
+            configure2("pimake", "upgrade")
 
     def build(self, spec, prefix):
         with working_dir(self.get_oommf_source_root()):
-            make = Executable("./oommf.tcl pimake ")
-            make()
+            make = Executable("./oommf.tcl")
+            make("pimake")
 
     def install(self, spec, prefix):
         # keep a copy of all the tcl files and everything oommf created.