util/environment.py: require string values in env mods (#49987)

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)
+        tty.debug(f"SetEnv: {self.name}={self.value}", level=3)
-        env[self.name] = str(self.value)
+        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}={self.value}", level=3)
-        tty.debug(f"SetPath: {self.name}={string_path}", level=3)
+        env[self.name] = self.value
-        env[self.name] = string_path
 
 
 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) -> None:
-    def set(self, name: str, value: str, *, force: bool = False, raw: bool = False):
         """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 = " ") -> None:
-    def append_flags(self, name: str, value: str, sep: str = " "):
         """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) -> None:
-    def unset(self, name: str):
         """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 = " ") -> None:
-    def remove_flags(self, name: str, value: str, sep: str = " "):
         """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) -> None:
-    def set_path(self, name: str, elements: List[str], separator: str = os.pathsep):
         """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) -> None:
-    def append_path(self, name: str, path: str, separator: str = os.pathsep):
         """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) -> None:
-    def prepend_path(self, name: str, path: str, separator: str = os.pathsep):
         """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) -> None:
-    def remove_first_path(self, name: str, path: str, separator: str = os.pathsep):
         """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) -> None:
-    def remove_last_path(self, name: str, path: str, separator: str = os.pathsep):
         """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) -> None:
-    def remove_path(self, name: str, path: str, separator: str = os.pathsep):
         """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) -> None:
-    def deprioritize_system_paths(self, name: str, separator: str = os.pathsep):
         """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) -> None:
-    def prune_duplicate_paths(self, name: str, separator: str = os.pathsep):
         """Stores a request to remove duplicates from a path list, otherwise
         preserving the order.
 

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":