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.
This commit is contained in:
markus-ferrell
GitHub
parent
843e1e80f0
commit
c202a045e6
6 changed files with 92 additions and 61 deletions
|
|
@ -10,11 +10,12 @@
|
|||
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")
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@
|
|||
|
||||
import os
|
||||
import sys
|
||||
from pathlib import PurePath
|
||||
|
||||
import pytest
|
||||
|
||||
|
|
@ -16,13 +17,16 @@
|
|||
|
||||
|
||||
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 @@ def test_read_unicode(tmpdir, working_env):
|
|||
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":
|
||||
|
|
|
|||
|
|
@ -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 _source_single_file(file_and_args, environment):
|
|||
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))
|
||||
|
|
|
|||
|
|
@ -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 @@ def __init__(self, name):
|
|||
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 @@ def name(self):
|
|||
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 @@ def path(self):
|
|||
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 @@ def streamify(arg, mode):
|
|||
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 @@ def __str__(self):
|
|||
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 p in paths:
|
||||
if p.name == "bin":
|
||||
with_parents.append(p.parent)
|
||||
return with_parents
|
||||
|
||||
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 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):
|
||||
|
|
|
|||
|
|
@ -109,7 +109,7 @@ def install(self, spec, prefix):
|
|||
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,
|
||||
)
|
||||
|
||||
|
|
|
|||
|
|
@ -168,15 +168,15 @@ def test_env(self):
|
|||
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.
|
||||
|
|
|
|||
Loading…
Reference in a new issue