Merge branch 'bschubert/yaml-tidying' into 'master'

Some tidying up of our yaml API

See merge request BuildStream/buildstream!1644

16 files changed, 77 insertions, 86 deletions

diff --git a/NEWS b/NEWS
index c28e5e601..49d73c072 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,17 @@
 (unreleased)
 ============
 
+API
+---
+
+  o BREAKING CHANGE: 'Element.node_subst_member' has been removed. Please use
+    'Element.node_subst_vars' instead.
+
+  o BREAKING CHANGE: 'Element.node_subst_list' has been removed. Please use
+    'Element.node_subst_sequence_vars' instead.
+
+  o A new 'Node.strip_node_info()' is available and allows getting the
+    underlying data structure for the given node.
 
 ==================
 buildstream 1.91.0
diff --git a/src/buildstream/_options/optionbool.py b/src/buildstream/_options/optionbool.py
index af67df601..f91cb257d 100644
--- a/src/buildstream/_options/optionbool.py
+++ b/src/buildstream/_options/optionbool.py
@@ -37,7 +37,7 @@ class OptionBool(Option):
 
     def load_value(self, node, *, transform=None):
         if transform:
-            self.set_value(transform(node.get_str(self.name)))
+            self.set_value(transform(node.get_scalar(self.name)))
         else:
             self.value = node.get_bool(self.name)
 
diff --git a/src/buildstream/_options/optionenum.py b/src/buildstream/_options/optionenum.py
index be9799acc..4a0941369 100644
--- a/src/buildstream/_options/optionenum.py
+++ b/src/buildstream/_options/optionenum.py
@@ -56,9 +56,11 @@ class OptionEnum(Option):
 
     def load_value(self, node, *, transform=None):
         value_node = node.get_scalar(self.name)
-        self.value = value_node.as_str()
         if transform:
-            self.value = transform(self.value)
+            self.value = transform(value_node)
+        else:
+            self.value = value_node.as_str()
+
         self.validate(self.value, value_node)
 
     def set_value(self, value):
diff --git a/src/buildstream/_options/optionflags.py b/src/buildstream/_options/optionflags.py
index 0ce995709..e5217a718 100644
--- a/src/buildstream/_options/optionflags.py
+++ b/src/buildstream/_options/optionflags.py
@@ -58,9 +58,10 @@ class OptionFlags(Option):
 
     def load_value(self, node, *, transform=None):
         value_node = node.get_sequence(self.name)
-        self.value = value_node.as_str_list()
         if transform:
-            self.value = [transform(x) for x in self.value]
+            self.value = [transform(x) for x in value_node]
+        else:
+            self.value = value_node.as_str_list()
         self.value = sorted(self.value)
         self.validate(self.value, value_node)
 
diff --git a/src/buildstream/_project.py b/src/buildstream/_project.py
index 7ba93bba4..54a011e0d 100644
--- a/src/buildstream/_project.py
+++ b/src/buildstream/_project.py
@@ -789,7 +789,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._subst_string)
+            output.options.load_yaml_values(self.junction.options, transform=self.junction.node_subst_vars)
 
         # Collect option values specified in the user configuration
         overrides = self._context.get_overrides(self.name)
diff --git a/src/buildstream/_workspaces.py b/src/buildstream/_workspaces.py
index a16c840e5..f9023dc54 100644
--- a/src/buildstream/_workspaces.py
+++ b/src/buildstream/_workspaces.py
@@ -630,7 +630,7 @@ class Workspaces():
     def _load_workspace(self, node):
         running_files = node.get_mapping('running_files', default=None)
         if running_files:
-            running_files = running_files._strip_node_info()
+            running_files = running_files.strip_node_info()
 
         dictionary = {
             'prepared': node.get_bool('prepared', default=False),
diff --git a/src/buildstream/buildelement.py b/src/buildstream/buildelement.py
index b33acfe12..7fe97c168 100644
--- a/src/buildstream/buildelement.py
+++ b/src/buildstream/buildelement.py
@@ -281,9 +281,9 @@ class BuildElement(Element):
     #                   Private Local Methods                   #
     #############################################################
     def __get_commands(self, node, name):
-        raw_commands = node.get_str_list(name, [])
+        raw_commands = node.get_sequence(name, [])
         return [
-            self.substitute_variables(command)
+            self.node_subst_vars(command)
             for command in raw_commands
         ]
 
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index 966f0f7cb..65b87e631 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -98,7 +98,7 @@ from . import _cachekey
 from . import _signals
 from . import _site
 from ._platform import Platform
-from .node import Node, _sentinel as _node_sentinel
+from .node import Node
 from .plugin import Plugin
 from .sandbox import SandboxFlags, SandboxCommandError
 from .sandbox._config import SandboxConfig
@@ -112,7 +112,7 @@ from .storage._casbaseddirectory import CasBasedDirectory
 from .storage.directory import VirtualDirectoryError
 
 if TYPE_CHECKING:
-    from .node import MappingNode
+    from .node import MappingNode, ScalarNode, SequenceNode
     from .types import SourceRef
     from typing import Set, Tuple
 
@@ -536,23 +536,17 @@ class Element(Plugin):
 
         return None
 
-    def substitute_variables(self, value):
-        return self.__variables.subst(value)
-
-    def node_subst_member(self, node: 'MappingNode[str, Any]', member_name: str, default: str = _node_sentinel) -> Any:
-        """Fetch the value of a string node member, substituting any variables
-        in the loaded value with the element contextual variables.
+    def node_subst_vars(self, node: 'ScalarNode') -> str:
+        """Replace any variables in the string contained in the node and returns it.
 
         Args:
-           node: A MappingNode loaded from YAML
-           member_name: The name of the member to fetch
-           default: A value to return when *member_name* is not specified in *node*
+           node: A ScalarNode loaded from YAML
 
         Returns:
-           The value of *member_name* in *node*, otherwise *default*
+           The value with all variables replaced
 
         Raises:
-           :class:`.LoadError`: When *member_name* is not found and no *default* was provided
+           :class:`.LoadError`: When the node doesn't contain a string or a variable was not found.
 
         **Example:**
 
@@ -560,31 +554,29 @@ class Element(Plugin):
 
           # Expect a string 'name' in 'node', substituting any
           # variables in the returned string
-          name = self.node_subst_member(node, 'name')
+          name = self.node_subst_vars(node.get_str('name'))
         """
-        value = node.get_str(member_name, default)
         try:
-            return self.__variables.subst(value)
+            return self.__variables.subst(node.as_str())
         except LoadError as e:
-            provenance = node.get_scalar(member_name).get_provenance()
+            provenance = node.get_provenance()
             raise LoadError('{}: {}'.format(provenance, e), e.reason, detail=e.detail) from e
 
-    def node_subst_list(self, node: 'MappingNode[str, Any]', member_name: str) -> List[Any]:
-        """Fetch a list from a node member, substituting any variables in the list
+    def node_subst_sequence_vars(self, node: 'SequenceNode[ScalarNode]') -> List[str]:
+        """Substitute any variables in the given sequence
 
         Args:
-          node: A MappingNode loaded from YAML
-          member_name: The name of the member to fetch (a list)
+          node: A SequenceNode loaded from YAML
 
         Returns:
-          The list in *member_name*
+          The list with every variable replaced
 
         Raises:
           :class:`.LoadError`
 
         """
         ret = []
-        for value in node.get_sequence(member_name):
+        for value in node:
             try:
                 ret.append(self.__variables.subst(value.as_str()))
             except LoadError as e:
@@ -877,9 +869,9 @@ class Element(Plugin):
 
         if bstdata is not None:
             with sandbox.batch(SandboxFlags.NONE):
-                commands = bstdata.get_str_list('integration-commands', [])
+                commands = bstdata.get_sequence('integration-commands', [])
                 for command in commands:
-                    cmd = self.substitute_variables(command)
+                    cmd = self.node_subst_vars(command)
 
                     sandbox.run(['sh', '-e', '-c', cmd], 0, env=environment, cwd='/',
                                 label=cmd)
@@ -2056,21 +2048,6 @@ class Element(Plugin):
 
             os.chmod(script_path, stat.S_IEXEC | stat.S_IREAD)
 
-    # _subst_string()
-    #
-    # Substitue a string, this is an internal function related
-    # to how junctions are loaded and needs to be more generic
-    # than the public node_subst_member()
-    #
-    # Args:
-    #    value (str): A string value
-    #
-    # Returns:
-    #    (str): The string after substitutions have occurred
-    #
-    def _subst_string(self, value):
-        return self.__variables.subst(value)
-
     # Returns the element whose sources this element is ultimately derived from.
     #
     # This is intended for being used to redirect commands that operate on an
@@ -2193,7 +2170,7 @@ class Element(Plugin):
                 'element-plugin-version': self.BST_ARTIFACT_VERSION,
                 'sandbox': self.__sandbox_config.get_unique_key(),
                 'environment': cache_env,
-                'public': self.__public._strip_node_info()
+                'public': self.__public.strip_node_info()
             }
 
             def __get_source_entry(_source):
@@ -2714,8 +2691,8 @@ class Element(Plugin):
     def __expand_environment(self, environment):
         # Resolve variables in environment value strings
         final_env = {}
-        for key in environment.keys():
-            final_env[key] = self.node_subst_member(environment, key)
+        for key, value in environment.items():
+            final_env[key] = self.node_subst_vars(value)
 
         return final_env
 
diff --git a/src/buildstream/node.pxd b/src/buildstream/node.pxd
index f2244a18f..47f46bbdf 100644
--- a/src/buildstream/node.pxd
+++ b/src/buildstream/node.pxd
@@ -29,10 +29,10 @@ cdef class Node:
     # Public Methods
     cpdef Node clone(self)
     cpdef ProvenanceInformation get_provenance(self)
+    cpdef object strip_node_info(self)
 
     # Private Methods used in BuildStream
     cpdef void _assert_fully_composited(self) except *
-    cpdef object _strip_node_info(self)
 
     # Protected Methods
     cdef void _compose_on(self, str key, MappingNode target, list path) except *
diff --git a/src/buildstream/node.pyx b/src/buildstream/node.pyx
index 89bb18eaf..bf6ce3a32 100644
--- a/src/buildstream/node.pyx
+++ b/src/buildstream/node.pyx
@@ -97,6 +97,14 @@ cdef class Node:
         """
         raise NotImplementedError()
 
+    cpdef object strip_node_info(self):
+        """ Remove all the node information (provenance) and return the underlying data as plain python objects 
+
+        Returns:
+            (list, dict, str, None): the underlying data that was held in the node structure.
+        """
+        raise NotImplementedError()
+
     #############################################################
     #                       Public Methods                      #
     #############################################################
@@ -176,14 +184,6 @@ cdef class Node:
     cpdef void _assert_fully_composited(self) except *:
         raise NotImplementedError()
 
-    # _strip_node_info()
-    #
-    # Remove all the node information (provenance) and return
-    # the underlying data as plain python objects (list, dict, str, None)
-    #
-    cpdef object _strip_node_info(self):
-        raise NotImplementedError()
-
     #############################################################
     #                Abstract Protected Methods                 #
     #############################################################
@@ -414,6 +414,9 @@ cdef class ScalarNode(Node):
     cpdef ScalarNode clone(self):
         return self
 
+    cpdef object strip_node_info(self):
+        return self.value
+
     #############################################################
     #              Private Methods implementations              #
     #############################################################
@@ -421,9 +424,6 @@ cdef class ScalarNode(Node):
     cpdef void _assert_fully_composited(self) except *:
         pass
 
-    cpdef object _strip_node_info(self):
-        return self.value
-
     #############################################################
     #                     Protected Methods                     #
     #############################################################
@@ -870,6 +870,12 @@ cdef class MappingNode(Node):
 
         return MappingNode.__new__(MappingNode, self.file_index, self.line, self.column, copy)
 
+    cpdef object strip_node_info(self):
+        cdef str key
+        cdef Node value
+
+        return {key: value.strip_node_info() for key, value in self.value.items()}
+
     #############################################################
     #            Private Methods used in BuildStream            #
     #############################################################
@@ -949,12 +955,6 @@ cdef class MappingNode(Node):
 
             value._assert_fully_composited()
 
-    cpdef object _strip_node_info(self):
-        cdef str key
-        cdef Node value
-
-        return {key: value._strip_node_info() for key, value in self.value.items()}
-
     #############################################################
     #                     Protected Methods                     #
     #############################################################
@@ -1371,6 +1371,10 @@ cdef class SequenceNode(Node):
 
         return SequenceNode.__new__(SequenceNode, self.file_index, self.line, self.column, copy)
 
+    cpdef object strip_node_info(self):
+        cdef Node value
+        return [value.strip_node_info() for value in self.value]
+
     #############################################################
     #              Private Methods implementations              #
     #############################################################
@@ -1380,10 +1384,6 @@ cdef class SequenceNode(Node):
         for value in self.value:
             value._assert_fully_composited()
 
-    cpdef object _strip_node_info(self):
-        cdef Node value
-        return [value._strip_node_info() for value in self.value]
-
     #############################################################
     #                     Protected Methods                     #
     #############################################################
diff --git a/src/buildstream/plugins/elements/import.py b/src/buildstream/plugins/elements/import.py
index 9568bd08e..404a0f4ee 100644
--- a/src/buildstream/plugins/elements/import.py
+++ b/src/buildstream/plugins/elements/import.py
@@ -49,8 +49,8 @@ class ImportElement(Element):
             'source', 'target'
         ])
 
-        self.source = self.node_subst_member(node, 'source')
-        self.target = self.node_subst_member(node, 'target')
+        self.source = self.node_subst_vars(node.get_scalar('source'))
+        self.target = self.node_subst_vars(node.get_scalar('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 a7b53e422..f3f0a2f7a 100644
--- a/src/buildstream/plugins/elements/script.py
+++ b/src/buildstream/plugins/elements/script.py
@@ -47,15 +47,15 @@ class ScriptElement(buildstream.ScriptElement):
 
     def configure(self, node):
         for n in node.get_sequence('layout', []):
-            dst = self.node_subst_member(n, 'destination')
-            elm = self.node_subst_member(n, 'element', None)
+            dst = self.node_subst_vars(n.get_scalar('destination'))
+            elm = self.node_subst_vars(n.get_scalar('element', None))
             self.layout_add(elm, dst)
 
         node.validate_keys([
             'commands', 'root-read-only', 'layout'
         ])
 
-        cmds = self.node_subst_list(node, "commands")
+        cmds = self.node_subst_sequence_vars(node.get_sequence("commands"))
         self.add_commands("commands", cmds)
 
         self.set_work_dir()
diff --git a/src/buildstream/sandbox/_sandboxremote.py b/src/buildstream/sandbox/_sandboxremote.py
index 678b11c32..77bb34fa9 100644
--- a/src/buildstream/sandbox/_sandboxremote.py
+++ b/src/buildstream/sandbox/_sandboxremote.py
@@ -175,7 +175,7 @@ class SandboxRemote(Sandbox):
                     config[tls_key] = resolve_path(config.get_str(tls_key))
 
         # TODO: we should probably not be stripping node info and rather load files the safe way
-        return RemoteExecutionSpec(*[conf._strip_node_info() for conf in service_configs])
+        return RemoteExecutionSpec(*[conf.strip_node_info() for conf in service_configs])
 
     def run_remote_command(self, channel, action_digest):
         # Sends an execution request to the remote execution server.
diff --git a/src/buildstream/source.py b/src/buildstream/source.py
index 1fb318b52..7fc2e9fc0 100644
--- a/src/buildstream/source.py
+++ b/src/buildstream/source.py
@@ -918,8 +918,8 @@ class Source(Plugin):
         # Step 2 - Set the ref in memory, and determine changed state
         #
         # TODO: we are working on dictionaries here, would be nicer to just work on the nodes themselves
-        clean = node._strip_node_info()
-        to_modify = node._strip_node_info()
+        clean = node.strip_node_info()
+        to_modify = node.strip_node_info()
 
         current_ref = self.get_ref()  # pylint: disable=assignment-from-no-return
 
diff --git a/tests/artifactcache/junctions.py b/tests/artifactcache/junctions.py
index 1fafb11f1..dab69ea8d 100644
--- a/tests/artifactcache/junctions.py
+++ b/tests/artifactcache/junctions.py
@@ -24,7 +24,7 @@ def project_set_artifacts(project, url):
         'url': url,
         'push': True
     }
-    _yaml.roundtrip_dump(project_config._strip_node_info(), file=project_conf_file)
+    _yaml.roundtrip_dump(project_config.strip_node_info(), file=project_conf_file)
 
 
 @pytest.mark.datafiles(DATA_DIR)
diff --git a/tests/frontend/workspace.py b/tests/frontend/workspace.py
index 2006fe7f4..dc9f38540 100644
--- a/tests/frontend/workspace.py
+++ b/tests/frontend/workspace.py
@@ -950,7 +950,7 @@ def test_list_supported_workspace(cli, tmpdir, datafiles, workspace_cfg, expecte
     def parse_dict_as_yaml(node):
         tempfile = os.path.join(str(tmpdir), 'yaml_dump')
         _yaml.roundtrip_dump(node, tempfile)
-        return _yaml.load(tempfile)._strip_node_info()
+        return _yaml.load(tempfile).strip_node_info()
 
     project = str(datafiles)
     os.makedirs(os.path.join(project, '.bst'))
@@ -962,7 +962,7 @@ def test_list_supported_workspace(cli, tmpdir, datafiles, workspace_cfg, expecte
     result = cli.run(project=project, args=['workspace', 'list'])
     result.assert_success()
 
-    loaded_config = _yaml.load(workspace_config_path)._strip_node_info()
+    loaded_config = _yaml.load(workspace_config_path).strip_node_info()
 
     # Check that workspace config remains the same if no modifications
     # to workspaces were made
@@ -997,7 +997,7 @@ def test_list_supported_workspace(cli, tmpdir, datafiles, workspace_cfg, expecte
     result.assert_success()
 
     # Check that workspace config is converted correctly if necessary
-    loaded_config = _yaml.load(workspace_config_path)._strip_node_info()
+    loaded_config = _yaml.load(workspace_config_path).strip_node_info()
     assert loaded_config == parse_dict_as_yaml(expected)