element.py: Always expand all variables at element creation

This will allow all users to not have to care about whether the option
is expanded or not, making it easier to use variables everywhere

8 files changed, 59 insertions, 52 deletions

diff --git a/NEWS b/NEWS
index f7e79b13d..b69e3c95f 100644
--- a/NEWS
+++ b/NEWS
@@ -22,6 +22,14 @@ Plugins
   o The `pip` element has been removed. Please use the one from bst-plugins-experimental
 
 
+API
+---
+
+  o `Element.node_subst_vars` and `Element.node_subst_sequence_vars` are now deprecated
+     and will get removed in the next version. All config entries are now resolved so there
+     is no need to use them anymore.
+
+
 ==================
 buildstream 1.93.3
 ==================
diff --git a/src/buildstream/_project.py b/src/buildstream/_project.py
index 45f3b4d34..b8d34c862 100644
--- a/src/buildstream/_project.py
+++ b/src/buildstream/_project.py
@@ -836,7 +836,7 @@ class Project:
         output.options.load(options_node)
         if self.junction:
             # load before user configuration
-            output.options.load_yaml_values(self.junction.options, transform=self.junction.node_subst_vars)
+            output.options.load_yaml_values(self.junction.options, transform=lambda n: n.as_str())
 
         # Collect option values specified in the user configuration
         overrides = self._context.get_overrides(self.name)
diff --git a/src/buildstream/_variables.pyx b/src/buildstream/_variables.pyx
index f9de2a6cc..9dc6cf623 100644
--- a/src/buildstream/_variables.pyx
+++ b/src/buildstream/_variables.pyx
@@ -25,7 +25,7 @@ import sys
 
 from ._exceptions import LoadError
 from .exceptions import LoadErrorReason
-from .node cimport MappingNode
+from .node cimport MappingNode, Node, ScalarNode, SequenceNode
 
 # Variables are allowed to have dashes here
 #
@@ -75,6 +75,27 @@ cdef class Variables:
         self._expstr_map = self._resolve(node)
         self.flat = self._flatten()
 
+    # expand()
+    #
+    # Expand all the variables found in the given Node, recursively.
+    # This does the change in place, modifying the node. If you want to keep
+    # the node untouched, you should use `node.clone()` beforehand
+    #
+    # Args:
+    #   (Node): A node for which to substitute the values
+    #
+    cpdef expand(self, Node node):
+        if isinstance(node, ScalarNode):
+            (<ScalarNode> node).value = self.subst((<ScalarNode> node).value)
+        elif isinstance(node, SequenceNode):
+            for entry in (<SequenceNode> node).value:
+                self.expand(entry)
+        elif isinstance(node, MappingNode):
+            for entry in (<MappingNode> node).value.values():
+                self.expand(entry)
+        else:
+            assert False, "Unknown 'Node' type"
+
     # subst():
     #
     # Substitutes any variables in 'string' and returns the result.
@@ -88,7 +109,7 @@ cdef class Variables:
     # Raises:
     #    LoadError, if the string contains unresolved variable references.
     #
-    def subst(self, str string):
+    cpdef subst(self, str string):
         expstr = _parse_expstr(string)
 
         try:
diff --git a/src/buildstream/buildelement.py b/src/buildstream/buildelement.py
index 32b64a8ed..ce6bb3393 100644
--- a/src/buildstream/buildelement.py
+++ b/src/buildstream/buildelement.py
@@ -172,10 +172,7 @@ class BuildElement(Element):
         node.validate_keys(_command_steps)
 
         for command_name in _legacy_command_steps:
-            if command_name in _command_steps:
-                self.__commands[command_name] = self.__get_commands(node, command_name)
-            else:
-                self.__commands[command_name] = []
+            self.__commands[command_name] = node.get_str_list(command_name, [])
 
     def preflight(self):
         pass
@@ -305,10 +302,6 @@ class BuildElement(Element):
     #############################################################
     #                   Private Local Methods                   #
     #############################################################
-    def __get_commands(self, node, name):
-        raw_commands = node.get_sequence(name, [])
-        return [self.node_subst_vars(command) for command in raw_commands]
-
     def __run_command(self, sandbox, cmd):
         # Note the -e switch to 'sh' means to exit with an error
         # if any untested command fails.
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index d8c0f6f79..67a915e48 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -76,6 +76,7 @@ import os
 import re
 import stat
 import copy
+import warnings
 from collections import OrderedDict
 import contextlib
 from contextlib import contextmanager
@@ -286,7 +287,8 @@ class Element(Plugin):
 
         # Collect the composited environment now that we have variables
         unexpanded_env = self.__extract_environment(project, meta)
-        self.__environment = self.__expand_environment(unexpanded_env)
+        self.__variables.expand(unexpanded_env)
+        self.__environment = unexpanded_env.strip_node_info()
 
         # Collect the environment nocache blacklist list
         nocache = self.__extract_env_nocache(project, meta)
@@ -300,6 +302,8 @@ class Element(Plugin):
         # Collect the composited element configuration and
         # ask the element to configure itself.
         self.__config = self.__extract_config(meta)
+        self.__variables.expand(self.__config)
+
         self._configure(self.__config)
 
         # Extract remote execution URL
@@ -502,6 +506,8 @@ class Element(Plugin):
     def node_subst_vars(self, node: "ScalarNode") -> str:
         """Replace any variables in the string contained in the node and returns it.
 
+        **Warning**: The method is deprecated and will get removed in the next version
+
         Args:
            node: A ScalarNode loaded from YAML
 
@@ -519,15 +525,17 @@ class Element(Plugin):
           # variables in the returned string
           name = self.node_subst_vars(node.get_scalar('name'))
         """
-        try:
-            return self.__variables.subst(node.as_str())
-        except LoadError as e:
-            provenance = node.get_provenance()
-            raise LoadError("{}: {}".format(provenance, e), e.reason, detail=e.detail) from e
+        # FIXME: remove this
+        warnings.warn(
+            "configuration is now automatically expanded, this is a no-op and will be removed.", DeprecationWarning
+        )
+        return node.as_str()
 
     def node_subst_sequence_vars(self, node: "SequenceNode[ScalarNode]") -> List[str]:
         """Substitute any variables in the given sequence
 
+        **Warning**: The method is deprecated and will get removed in the next version
+
         Args:
           node: A SequenceNode loaded from YAML
 
@@ -538,14 +546,11 @@ class Element(Plugin):
           :class:`.LoadError`
 
         """
-        ret = []
-        for value in node:
-            try:
-                ret.append(self.__variables.subst(value.as_str()))
-            except LoadError as e:
-                provenance = value.get_provenance()
-                raise LoadError("{}: {}".format(provenance, e), e.reason, detail=e.detail) from e
-        return ret
+        # FIXME: remove this
+        warnings.warn(
+            "configuration is now automatically expanded, this is a no-op and will be removed.", DeprecationWarning
+        )
+        return node.as_str_list()
 
     def compute_manifest(
         self, *, include: Optional[List[str]] = None, exclude: Optional[List[str]] = None, orphans: bool = True
@@ -760,11 +765,8 @@ class Element(Plugin):
 
         if bstdata is not None:
             with sandbox.batch(SandboxFlags.NONE):
-                commands = bstdata.get_sequence("integration-commands", [])
-                for command in commands:
-                    cmd = self.node_subst_vars(command)
-
-                    sandbox.run(["sh", "-e", "-c", cmd], 0, env=environment, cwd="/", label=cmd)
+                for command in bstdata.get_str_list("integration-commands", []):
+                    sandbox.run(["sh", "-e", "-c", command], 0, env=environment, cwd="/", label=command)
 
     def stage_sources(self, sandbox: "Sandbox", directory: str) -> None:
         """Stage this element's sources to a directory in the sandbox
@@ -2566,17 +2568,6 @@ class Element(Plugin):
 
         return environment
 
-    # This will resolve the final environment to be used when
-    # creating sandboxes for this element
-    #
-    def __expand_environment(self, environment):
-        # Resolve variables in environment value strings
-        final_env = {}
-        for key, value in environment.items():
-            final_env[key] = self.node_subst_vars(value)
-
-        return final_env
-
     @classmethod
     def __extract_env_nocache(cls, project, meta):
         if meta.is_junction:
diff --git a/src/buildstream/plugin.py b/src/buildstream/plugin.py
index dfb54332f..6795043e7 100644
--- a/src/buildstream/plugin.py
+++ b/src/buildstream/plugin.py
@@ -301,11 +301,6 @@ class Plugin:
         should be used to ensure that the user has not specified keys in `node` which are unsupported
         by the plugin.
 
-        .. note::
-
-           For Elements, when variable substitution is desirable, the
-           :func:`Element.node_subst_vars() <buildstream.element.Element.node_subst_vars>`
-           method can be used.
         """
         raise ImplError(
             "{tag} plugin '{kind}' does not implement configure()".format(tag=self.__type_tag, kind=self.get_kind())
diff --git a/src/buildstream/plugins/elements/import.py b/src/buildstream/plugins/elements/import.py
index d9961aa6f..de7ee8af4 100644
--- a/src/buildstream/plugins/elements/import.py
+++ b/src/buildstream/plugins/elements/import.py
@@ -46,8 +46,8 @@ class ImportElement(Element):
     def configure(self, node):
         node.validate_keys(["source", "target"])
 
-        self.source = self.node_subst_vars(node.get_scalar("source"))
-        self.target = self.node_subst_vars(node.get_scalar("target"))
+        self.source = node.get_str("source")
+        self.target = node.get_str("target")
 
     def preflight(self):
         # Assert that we have at least one source to fetch.
diff --git a/src/buildstream/plugins/elements/script.py b/src/buildstream/plugins/elements/script.py
index 9d780ebe2..502212e10 100644
--- a/src/buildstream/plugins/elements/script.py
+++ b/src/buildstream/plugins/elements/script.py
@@ -46,14 +46,13 @@ class ScriptElement(buildstream.ScriptElement):
 
     def configure(self, node):
         for n in node.get_sequence("layout", []):
-            dst = self.node_subst_vars(n.get_scalar("destination"))
-            elm = self.node_subst_vars(n.get_scalar("element", None))
+            dst = n.get_str("destination")
+            elm = n.get_str("element", None)
             self.layout_add(elm, dst)
 
         node.validate_keys(["commands", "root-read-only", "layout"])
 
-        cmds = self.node_subst_sequence_vars(node.get_sequence("commands"))
-        self.add_commands("commands", cmds)
+        self.add_commands("commands", node.get_str_list("commands"))
 
         self.set_work_dir()
         self.set_install_root()