From 84dcc654ecf16b50fa86ea6c22beb4b9d63f8483 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Wed, 9 Apr 2025 19:29:47 +0200 Subject: [PATCH] util/environment.py: require string values in env mods (#49987) --- lib/spack/spack/util/environment.py | 113 ++++++++---------- .../repos/builtin/packages/groff/package.py | 2 +- 2 files changed, 50 insertions(+), 65 deletions(-) diff --git a/lib/spack/spack/util/environment.py b/lib/spack/spack/util/environment.py index e52807eda74..6f092115311 100644 --- a/lib/spack/spack/util/environment.py +++ b/lib/spack/spack/util/environment.py @@ -12,13 +12,15 @@ import shlex import subprocess import sys -from functools import wraps +import warnings from typing import Any, Callable, Dict, Iterable, List, MutableMapping, Optional, Tuple, Union from llnl.path import path_to_os_path, system_path_filter from llnl.util import tty from llnl.util.lang import dedupe +import spack.error + if sys.platform == "win32": SYSTEM_PATHS = [ "C:\\", @@ -61,28 +63,6 @@ ModificationList = List[Union["NameModifier", "NameValueModifier"]] -def system_env_normalize(func): - """Decorator wrapping calls to system env modifications, - converting all env variable names to all upper case on Windows, no-op - on other platforms before calling env modification method. - - Windows, due to a DOS holdover, treats all env variable names case - insensitively, however Spack's env modification class does not, - meaning setting `Path` and `PATH` would be distinct env operations - for Spack, but would cause a collision when actually performing the - env modification operations on the env. - Normalize all env names to all caps to prevent this collision from the - Spack side.""" - - @wraps(func) - def case_insensitive_modification(self, name: str, *args, **kwargs): - if sys.platform == "win32": - name = name.upper() - return func(self, name, *args, **kwargs) - - return case_insensitive_modification - - def is_system_path(path: Path) -> bool: """Returns True if the argument is a system path, False otherwise.""" return bool(path) and (os.path.normpath(path) in SYSTEM_DIRS) @@ -251,7 +231,7 @@ class NameModifier: __slots__ = ("name", "separator", "trace") def __init__(self, name: str, *, separator: str = os.pathsep, trace: Optional[Trace] = None): - self.name = name + self.name = name.upper() if sys.platform == "win32" else name self.separator = separator self.trace = trace @@ -271,9 +251,17 @@ class NameValueModifier: __slots__ = ("name", "value", "separator", "trace") def __init__( - self, name: str, value: Any, *, separator: str = os.pathsep, trace: Optional[Trace] = None + self, name: str, value: str, *, separator: str = os.pathsep, trace: Optional[Trace] = None ): - self.name = name + self.name = name.upper() if sys.platform == "win32" else name + if not isinstance(value, str): + warnings.warn( + f"{self.__class__.__name__} {self.name}={value}: using a non-string value " + f"{type(value).__name__} is deprecated and will be an error in Spack v1.0", + spack.error.SpackAPIWarning, + stacklevel=4, + ) + value = str(value) self.value = value self.separator = separator self.trace = trace @@ -309,17 +297,17 @@ def __init__( self.raw = raw def execute(self, env: MutableMapping[str, str]): - tty.debug(f"SetEnv: {self.name}={str(self.value)}", level=3) - env[self.name] = str(self.value) + tty.debug(f"SetEnv: {self.name}={self.value}", level=3) + env[self.name] = self.value class AppendFlagsEnv(NameValueModifier): def execute(self, env: MutableMapping[str, str]): - tty.debug(f"AppendFlagsEnv: {self.name}={str(self.value)}", level=3) + tty.debug(f"AppendFlagsEnv: {self.name}={self.value}", level=3) if self.name in env and env[self.name]: - env[self.name] += self.separator + str(self.value) + env[self.name] += self.separator + self.value else: - env[self.name] = str(self.value) + env[self.name] = self.value class UnsetEnv(NameModifier): @@ -331,7 +319,7 @@ def execute(self, env: MutableMapping[str, str]): class RemoveFlagsEnv(NameValueModifier): def execute(self, env: MutableMapping[str, str]): - tty.debug(f"RemoveFlagsEnv: {self.name}-{str(self.value)}", level=3) + tty.debug(f"RemoveFlagsEnv: {self.name}-{self.value}", level=3) environment_value = env.get(self.name, "") flags = environment_value.split(self.separator) if environment_value else [] flags = [f for f in flags if f != self.value] @@ -339,15 +327,24 @@ def execute(self, env: MutableMapping[str, str]): class SetPath(NameValueModifier): + def __init__( + self, + name: str, + value: List[str], + *, + separator: str = os.pathsep, + trace: Optional[Trace] = None, + ): + super().__init__(name, separator.join(value), separator=separator, trace=trace) + def execute(self, env: MutableMapping[str, str]): - string_path = self.separator.join(str(item) for item in self.value) - tty.debug(f"SetPath: {self.name}={string_path}", level=3) - env[self.name] = string_path + tty.debug(f"SetPath: {self.name}={self.value}", level=3) + env[self.name] = self.value class AppendPath(NameValueModifier): def execute(self, env: MutableMapping[str, str]): - tty.debug(f"AppendPath: {self.name}+{str(self.value)}", level=3) + tty.debug(f"AppendPath: {self.name}+{self.value}", level=3) environment_value = env.get(self.name, "") directories = environment_value.split(self.separator) if environment_value else [] directories.append(path_to_os_path(os.path.normpath(self.value)).pop()) @@ -356,7 +353,7 @@ def execute(self, env: MutableMapping[str, str]): class PrependPath(NameValueModifier): def execute(self, env: MutableMapping[str, str]): - tty.debug(f"PrependPath: {self.name}+{str(self.value)}", level=3) + tty.debug(f"PrependPath: {self.name}+{self.value}", level=3) environment_value = env.get(self.name, "") directories = environment_value.split(self.separator) if environment_value else [] directories = [path_to_os_path(os.path.normpath(self.value)).pop()] + directories @@ -365,7 +362,7 @@ def execute(self, env: MutableMapping[str, str]): class RemoveFirstPath(NameValueModifier): def execute(self, env: MutableMapping[str, str]): - tty.debug(f"RemoveFirstPath: {self.name}-{str(self.value)}", level=3) + tty.debug(f"RemoveFirstPath: {self.name}-{self.value}", level=3) environment_value = env.get(self.name, "") directories = environment_value.split(self.separator) directories = [path_to_os_path(os.path.normpath(x)).pop() for x in directories] @@ -377,7 +374,7 @@ def execute(self, env: MutableMapping[str, str]): class RemoveLastPath(NameValueModifier): def execute(self, env: MutableMapping[str, str]): - tty.debug(f"RemoveLastPath: {self.name}-{str(self.value)}", level=3) + tty.debug(f"RemoveLastPath: {self.name}-{self.value}", level=3) environment_value = env.get(self.name, "") directories = environment_value.split(self.separator)[::-1] directories = [path_to_os_path(os.path.normpath(x)).pop() for x in directories] @@ -389,7 +386,7 @@ def execute(self, env: MutableMapping[str, str]): class RemovePath(NameValueModifier): def execute(self, env: MutableMapping[str, str]): - tty.debug(f"RemovePath: {self.name}-{str(self.value)}", level=3) + tty.debug(f"RemovePath: {self.name}-{self.value}", level=3) environment_value = env.get(self.name, "") directories = environment_value.split(self.separator) directories = [ @@ -473,8 +470,7 @@ def _trace(self) -> Optional[Trace]: return Trace(filename=filename, lineno=lineno, context=current_context) - @system_env_normalize - def set(self, name: str, value: str, *, force: bool = False, raw: bool = False): + def set(self, name: str, value: str, *, force: bool = False, raw: bool = False) -> None: """Stores a request to set an environment variable. Args: @@ -486,8 +482,7 @@ def set(self, name: str, value: str, *, force: bool = False, raw: bool = False): item = SetEnv(name, value, trace=self._trace(), force=force, raw=raw) self.env_modifications.append(item) - @system_env_normalize - def append_flags(self, name: str, value: str, sep: str = " "): + def append_flags(self, name: str, value: str, sep: str = " ") -> None: """Stores a request to append 'flags' to an environment variable. Args: @@ -498,8 +493,7 @@ def append_flags(self, name: str, value: str, sep: str = " "): item = AppendFlagsEnv(name, value, separator=sep, trace=self._trace()) self.env_modifications.append(item) - @system_env_normalize - def unset(self, name: str): + def unset(self, name: str) -> None: """Stores a request to unset an environment variable. Args: @@ -508,8 +502,7 @@ def unset(self, name: str): item = UnsetEnv(name, trace=self._trace()) self.env_modifications.append(item) - @system_env_normalize - def remove_flags(self, name: str, value: str, sep: str = " "): + def remove_flags(self, name: str, value: str, sep: str = " ") -> None: """Stores a request to remove flags from an environment variable Args: @@ -520,8 +513,7 @@ def remove_flags(self, name: str, value: str, sep: str = " "): item = RemoveFlagsEnv(name, value, separator=sep, trace=self._trace()) self.env_modifications.append(item) - @system_env_normalize - def set_path(self, name: str, elements: List[str], separator: str = os.pathsep): + def set_path(self, name: str, elements: List[str], separator: str = os.pathsep) -> None: """Stores a request to set an environment variable to a list of paths, separated by a character defined in input. @@ -533,8 +525,7 @@ def set_path(self, name: str, elements: List[str], separator: str = os.pathsep): item = SetPath(name, elements, separator=separator, trace=self._trace()) self.env_modifications.append(item) - @system_env_normalize - def append_path(self, name: str, path: str, separator: str = os.pathsep): + def append_path(self, name: str, path: str, separator: str = os.pathsep) -> None: """Stores a request to append a path to list of paths. Args: @@ -545,8 +536,7 @@ def append_path(self, name: str, path: str, separator: str = os.pathsep): item = AppendPath(name, path, separator=separator, trace=self._trace()) self.env_modifications.append(item) - @system_env_normalize - def prepend_path(self, name: str, path: str, separator: str = os.pathsep): + def prepend_path(self, name: str, path: str, separator: str = os.pathsep) -> None: """Stores a request to prepend a path to list of paths. Args: @@ -557,8 +547,7 @@ def prepend_path(self, name: str, path: str, separator: str = os.pathsep): item = PrependPath(name, path, separator=separator, trace=self._trace()) self.env_modifications.append(item) - @system_env_normalize - def remove_first_path(self, name: str, path: str, separator: str = os.pathsep): + def remove_first_path(self, name: str, path: str, separator: str = os.pathsep) -> None: """Stores a request to remove first instance of path from a list of paths. Args: @@ -569,8 +558,7 @@ def remove_first_path(self, name: str, path: str, separator: str = os.pathsep): item = RemoveFirstPath(name, path, separator=separator, trace=self._trace()) self.env_modifications.append(item) - @system_env_normalize - def remove_last_path(self, name: str, path: str, separator: str = os.pathsep): + def remove_last_path(self, name: str, path: str, separator: str = os.pathsep) -> None: """Stores a request to remove last instance of path from a list of paths. Args: @@ -581,8 +569,7 @@ def remove_last_path(self, name: str, path: str, separator: str = os.pathsep): item = RemoveLastPath(name, path, separator=separator, trace=self._trace()) self.env_modifications.append(item) - @system_env_normalize - def remove_path(self, name: str, path: str, separator: str = os.pathsep): + def remove_path(self, name: str, path: str, separator: str = os.pathsep) -> None: """Stores a request to remove a path from a list of paths. Args: @@ -593,8 +580,7 @@ def remove_path(self, name: str, path: str, separator: str = os.pathsep): item = RemovePath(name, path, separator=separator, trace=self._trace()) self.env_modifications.append(item) - @system_env_normalize - def deprioritize_system_paths(self, name: str, separator: str = os.pathsep): + def deprioritize_system_paths(self, name: str, separator: str = os.pathsep) -> None: """Stores a request to deprioritize system paths in a path list, otherwise preserving the order. @@ -605,8 +591,7 @@ def deprioritize_system_paths(self, name: str, separator: str = os.pathsep): item = DeprioritizeSystemPaths(name, separator=separator, trace=self._trace()) self.env_modifications.append(item) - @system_env_normalize - def prune_duplicate_paths(self, name: str, separator: str = os.pathsep): + def prune_duplicate_paths(self, name: str, separator: str = os.pathsep) -> None: """Stores a request to remove duplicates from a path list, otherwise preserving the order. diff --git a/var/spack/repos/builtin/packages/groff/package.py b/var/spack/repos/builtin/packages/groff/package.py index 8d9b7a714a8..16972d917c5 100644 --- a/var/spack/repos/builtin/packages/groff/package.py +++ b/var/spack/repos/builtin/packages/groff/package.py @@ -92,7 +92,7 @@ def configure_args(self): def setup_run_environment(self, env): if self.spec.satisfies("+x"): dir = join_path(self.prefix.lib, "X11", "app-defaults") - env.set_path("XFILESEARCHPATH", dir) + env.prepend_path("XFILESEARCHPATH", dir) def flag_handler(self, name, flags): if name == "cxxflags":