fix env_vars: preprend_path and append_path were doing the opposite (eth-cscs#285)

This fixes envvars prepend/append behavior. Indeed, it was doing the
exact opposite, i.e. prepending vars specified under `append_path` and
appending vars under `prepend_path`.

The actual fix is just the first commit
5a1d620, but I also took the chance to
refactor the code aiming at better isolation for testing purposes.

Author: albestro

stackinator/etc/envvars.py

@@ -3,6 +3,7 @@
 import argparse
 import json
 import os
+import re
 from enum import Enum
 from typing import List, Optional
 
@@ -206,6 +207,11 @@ def is_list_var(name: str) -> bool:
     return name in list_variables
 
 
+class VarExpansionError(Exception):
+    def __init__(self, variable_name) -> None:
+        super().__init__(f'"{variable_name}" variable cannot be expanded')
+
+
 class EnvVarSet:
     """
     A set of environment variable updates.
@@ -219,6 +225,57 @@ def __init__(self):
         # toggles whether post export commands will be generated
         self._generate_post = True
 
+    @classmethod
+    def from_envvars(cls, input: dict, substitutions: dict):
+        def expand_vars(s):
+            def var_expansion(m: re.Match):
+                try:
+                    return substitutions[m.group(1)]
+                except KeyError:
+                    raise VarExpansionError(m.group(0))
+
+            return re.sub(r"\$@(\w+)@", var_expansion, s)
+
+        # TODO: one day this code will be revisited because we need to append_path
+        # or prepend_path to a variable that isn't in envvars.is_list_var
+        # On that day, extend the environments.yaml views:uenv:env_vars field
+        # to also accept a list of env var names to add to the blessed list of prefix paths
+
+        env = EnvVarSet()
+
+        for v in input.get("set", []):
+            ((name, value),) = v.items()
+            if value is not None:
+                value = expand_vars(value)
+
+            # insist that the only 'set' operation on prefix variables is to unset/reset them
+            # this requires that users use append and prepend to build up the variables
+            if is_list_var(name) and value is not None:
+                raise RuntimeError(f"{name} is a prefix variable")
+            else:
+                if is_list_var(name):
+                    env.set_list(name, [], EnvVarOp.SET)
+                else:
+                    env.set_scalar(name, value)
+
+        for v in input.get("prepend_path", []):
+            ((name, value),) = v.items()
+            if value is not None:
+                value = expand_vars(value)
+            if not is_list_var(name):
+                raise RuntimeError(f"{name} is not a known prefix path variable")
+            env.set_list(name, [value], EnvVarOp.PREPEND)
+
+        for v in input.get("append_path", []):
+            ((name, value),) = v.items()
+            if value is not None:
+                value = expand_vars(value)
+            if not is_list_var(name):
+                raise RuntimeError(f"{name} is not a known prefix path variable")
+            env.set_list(name, [value], EnvVarOp.APPEND)
+
+        return env
+
     @property
     def lists(self):
         return self._lists

stackinator/recipe.py

@@ -6,7 +6,7 @@
 import yaml
 
 from . import cache, root_logger, schema, spack_util
-from .etc import envvars
+from .etc.envvars import EnvVarSet
 
 
 class Recipe:
@@ -303,60 +303,18 @@ def environment_view_meta(self):
         view_meta = {}
         for _, env in self.environments.items():
             for view in env["views"]:
-                # recipe authors can substitute the name of the view, the mount
-                # and view path into environment variables using '$@key@' where
-                # key is one of view_name, mount and view_path.
-                substitutions = {
-                    "view_name": str(view["name"]),
-                    "mount": str(self.mount),
-                    "view_path": str(view["config"]["root"]),
-                }
-
-                def fill(s):
-                    return re.sub(
-                        r"\$@(\w+)@",
-                        lambda m: substitutions.get(m.group(1), m.group(0)),
-                        s,
-                    )
-
-                ev_inputs = view["extra"]["env_vars"]
-                env = envvars.EnvVarSet()
-
-                # TODO: one day this code will be revisited because we need to append_path
-                # or prepend_path to a variable that isn't in envvars.is_list_var
-                # On that day, extend the environments.yaml views:uenv:env_vars field
-                # to also accept a list of env var names to add to the blessed list of prefix paths
-
-                for v in ev_inputs["set"]:
-                    ((name, value),) = v.items()
-                    if value is not None:
-                        value = fill(value)
-
-                    # insist that the only 'set' operation on prefix variables is to unset/reset them
-                    # this requires that users use append and prepend to build up the variables
-                    if envvars.is_list_var(name) and value is not None:
-                        raise RuntimeError(f"{name} in the {view['name']} view is a prefix variable.")
-                    else:
-                        if envvars.is_list_var(name):
-                            env.set_list(name, [], envvars.EnvVarOp.SET)
-                        else:
-                            env.set_scalar(name, value)
-                for v in ev_inputs["prepend_path"]:
-                    ((name, value),) = v.items()
-                    if value is not None:
-                        value = fill(value)
-                    if not envvars.is_list_var(name):
-                        raise RuntimeError(f"{name} in the {view['name']} view is not a known prefix path variable")
-
-                    env.set_list(name, [value], envvars.EnvVarOp.APPEND)
-                for v in ev_inputs["append_path"]:
-                    ((name, value),) = v.items()
-                    if value is not None:
-                        value = fill(value)
-                    if not envvars.is_list_var(name):
-                        raise RuntimeError(f"{name} in the {view['name']} view is not a known prefix path variable")
-
-                    env.set_list(name, [value], envvars.EnvVarOp.PREPEND)
+                try:
+                    # recipe authors can substitute the name of the view, the mount
+                    # and view path into environment variables using '$@key@' where
+                    # key is one of view_name, mount and view_path.
+                    substitutions = {
+                        "view_name": str(view["name"]),
+                        "mount": str(self.mount),
+                        "view_path": str(view["config"]["root"]),
+                    }
+                    env = EnvVarSet.from_envvars(view["extra"]["env_vars"], substitutions)
+                except Exception as err:
+                    raise RuntimeError(f'In view "{view["name"]}": {err}')
 
                 view_meta[view["name"]] = {
                     "root": view["config"]["root"],