Merge branch 'tristan/loader-dependency-refactor' into 'master'tristan/loader-dependency-refactor

_loader: Use only one Dependency() class

See merge request BuildStream/buildstream!2019

5 files changed, 249 insertions, 234 deletions

diff --git a/setup.py b/setup.py
index 930c9de40..d89d5e6ac 100755
--- a/setup.py
+++ b/setup.py
@@ -304,8 +304,7 @@ BUILD_EXTENSIONS = []
 
 register_cython_module("buildstream.node")
 register_cython_module("buildstream._loader._loader")
-register_cython_module("buildstream._loader.loadelement")
-register_cython_module("buildstream._loader.types", dependencies=["buildstream.node"])
+register_cython_module("buildstream._loader.loadelement", dependencies=["buildstream.node"])
 register_cython_module("buildstream._yaml", dependencies=["buildstream.node"])
 register_cython_module("buildstream._types")
 register_cython_module("buildstream._utils")
diff --git a/src/buildstream/_loader/loadelement.pyx b/src/buildstream/_loader/loadelement.pyx
index de2f96b37..6cd5b46b7 100644
--- a/src/buildstream/_loader/loadelement.pyx
+++ b/src/buildstream/_loader/loadelement.pyx
@@ -1,5 +1,5 @@
 #
-#  Copyright (C) 2016 Codethink Limited
+#  Copyright (C) 2020 Codethink Limited
 #
 #  This program is free software; you can redistribute it and/or
 #  modify it under the terms of the GNU Lesser General Public
@@ -24,8 +24,8 @@ from pyroaring import BitMap, FrozenBitMap  # pylint: disable=no-name-in-module
 from .._exceptions import LoadError
 from ..exceptions import LoadErrorReason
 from ..element import Element
-from ..node cimport MappingNode, ProvenanceInformation
-from .types import Symbol, extract_depends_from_node
+from ..node cimport MappingNode, Node, ProvenanceInformation, ScalarNode, SequenceNode
+from .types import Symbol
 
 
 # Counter to get ids to LoadElements
@@ -39,26 +39,135 @@ cdef int _next_synthetic_counter():
 
 # Dependency():
 #
-# A link from a LoadElement to its dependencies.
+# Early stage data model for dependencies objects, the LoadElement has
+# Dependency objects which in turn refer to other LoadElements in the data
+# model.
 #
-# Keeps a link to one of the current Element's dependencies, together with
-# its dependency attributes.
+# The constructor is incomplete, normally dependencies are loaded
+# via the Dependency.load() API below. The constructor arguments are
+# only used as a convenience to create the dummy Dependency objects
+# at the toplevel of the load sequence in the Loader.
 #
 # Args:
 #    element (LoadElement): a LoadElement on which there is a dependency
 #    dep_type (str): the type of dependency this dependency link is
-#    strict (bint): whether the dependency is strict
+#
 cdef class Dependency:
-    cdef readonly LoadElement element
-    cdef readonly str dep_type
-    cdef readonly bint strict
-    cdef readonly ProvenanceInformation provenance
-
-    def __cinit__(self, LoadElement element, str dep_type, bint strict, ProvenanceInformation provenance):
+    cdef readonly LoadElement element  # The resolved LoadElement
+    cdef readonly str dep_type  # The dependency type (runtime or build or both)
+    cdef readonly str name  # The project local dependency name
+    cdef readonly str junction  # The junction path of the dependency name, if any
+    cdef readonly bint strict  # Whether this is a strict dependency
+    cdef Node _node  # The original node of the dependency
+
+    def __cinit__(self, element=None, dep_type=None):
         self.element = element
         self.dep_type = dep_type
-        self.strict = strict
-        self.provenance = provenance
+        self.name = None
+        self.junction = None
+        self.strict = False
+        self._node = None
+
+    # provenance
+    #
+    # A property to return the ProvenanceInformation for this
+    # dependency.
+    #
+    @property
+    def provenance(self):
+        return self._node.get_provenance()
+
+    # set_element()
+    #
+    # Sets the resolved LoadElement
+    #
+    # When Dependencies are initially loaded, the `element` member
+    # will be None until later on when the Loader loads the LoadElement
+    # objects based on the Dependency `name` and `junction`, the Loader
+    # will then call this to resolve the `element` member.
+    #
+    # Args:
+    #    element (LoadElement): The resolved LoadElement
+    #
+    cpdef set_element(self, element: LoadElement):
+        self.element = element
+
+    # load()
+    #
+    # Load the dependency from a Node
+    #
+    # Args:
+    #    dep (Node): A node to load the dependency from
+    #    default_dep_type (str): The default dependency type
+    #
+    cdef load(self, Node dep, str default_dep_type):
+        cdef str dep_type
+
+        self._node = dep
+        self.element = None
+
+        if type(dep) is ScalarNode:
+            self.name = dep.as_str()
+            self.dep_type = default_dep_type
+            self.junction = None
+            self.strict = False
+
+        elif type(dep) is MappingNode:
+            if default_dep_type:
+                (<MappingNode> dep).validate_keys(['filename', 'junction', 'strict'])
+                dep_type = default_dep_type
+            else:
+                (<MappingNode> dep).validate_keys(['filename', 'type', 'junction', 'strict'])
+
+                # Make type optional, for this we set it to None
+                dep_type = (<MappingNode> dep).get_str(<str> Symbol.TYPE, None)
+                if dep_type is None or dep_type == <str> Symbol.ALL:
+                    dep_type = None
+                elif dep_type not in [Symbol.BUILD, Symbol.RUNTIME]:
+                    provenance = dep.get_scalar(Symbol.TYPE).get_provenance()
+                    raise LoadError("{}: Dependency type '{}' is not 'build', 'runtime' or 'all'"
+                                    .format(provenance, dep_type), LoadErrorReason.INVALID_DATA)
+
+            self.name = (<MappingNode> dep).get_str(<str> Symbol.FILENAME)
+            self.dep_type = dep_type
+            self.junction = (<MappingNode> dep).get_str(<str> Symbol.JUNCTION, None)
+            self.strict = (<MappingNode> dep).get_bool(<str> Symbol.STRICT, False)
+
+            # Here we disallow explicitly setting 'strict' to False.
+            #
+            # This is in order to keep the door open to allowing the project.conf
+            # set the default of dependency 'strict'-ness which might be useful
+            # for projects which use mostly static linking and the like, in which
+            # case we can later interpret explicitly non-strict dependencies
+            # as an override of the project default.
+            #
+            if self.strict == False and Symbol.STRICT in dep:
+                provenance = dep.get_scalar(Symbol.STRICT).get_provenance()
+                raise LoadError("{}: Setting 'strict' to False is unsupported"
+                                .format(provenance), LoadErrorReason.INVALID_DATA)
+
+        else:
+            raise LoadError("{}: Dependency is not specified as a string or a dictionary".format(self.provenance),
+                            LoadErrorReason.INVALID_DATA)
+
+        # Only build dependencies are allowed to be strict
+        #
+        if self.strict and self.dep_type == Symbol.RUNTIME:
+            raise LoadError("{}: Runtime dependency {} specified as `strict`.".format(self.provenance, self.name),
+                            LoadErrorReason.INVALID_DATA,
+                            detail="Only dependencies required at build time may be declared `strict`.")
+
+        # `:` characters are not allowed in filename if a junction was
+        # explicitly specified
+        if self.junction and ':' in self.name:
+            raise LoadError("{}: Dependency {} contains `:` in its name. "
+                            "`:` characters are not allowed in filename when "
+                            "junction attribute is specified.".format(self.provenance, self.name),
+                            LoadErrorReason.INVALID_DATA)
+
+        # Attempt to split name if no junction was specified explicitly
+        if not self.junction and ':' in self.name:
+            self.junction, self.name = self.name.rsplit(':', maxsplit=1)
 
 
 # LoadElement():
@@ -137,7 +246,6 @@ cdef class LoadElement:
             # parse their dependencies we cannot rely on the built-in ElementError.
             deps = extract_depends_from_node(self.node)
             if deps:
-                provenance = self.node
                 raise LoadError(
                     "{}: Dependencies are forbidden for 'link' elements".format(element),
                     LoadErrorReason.LINK_FORBIDDEN_DEPENDENCIES
@@ -269,3 +377,73 @@ def sort_dependencies(LoadElement element, set visited):
                 working_elements.append(dep.element)
 
         element.dependencies.sort(key=cmp_to_key(_dependency_cmp))
+
+
+# _extract_depends_from_node():
+#
+# Helper for extract_depends_from_node to get dependencies of a particular type
+#
+# Adds to an array of Dependency objects from a given dict node 'node',
+# allows both strings and dicts for expressing the dependency.
+#
+# After extracting depends, the symbol is deleted from the node
+#
+# Args:
+#    node (Node): A YAML loaded dictionary
+#    key (str): the key on the Node corresponding to the dependency type
+#    default_dep_type (str): type to give to the dependency
+#    acc (list): a list in which to add the loaded dependencies
+#    rundeps (dict): a dictionary mapping dependency (junction, name) to dependency for runtime deps
+#    builddeps (dict): a dictionary mapping dependency (junction, name) to dependency for build deps
+#
+cdef void _extract_depends_from_node(Node node, str key, str default_dep_type, list acc, dict rundeps, dict builddeps) except *:
+    cdef SequenceNode depends = node.get_sequence(key, [])
+    cdef Node dep_node
+    cdef tuple deptup
+
+    for dep_node in depends:
+        dependency = Dependency()
+        dependency.load(dep_node, default_dep_type)
+        deptup = (dependency.junction, dependency.name)
+        if dependency.dep_type in [Symbol.BUILD, None]:
+            if deptup in builddeps:
+                raise LoadError("{}: Duplicate build dependency found at {}."
+                                .format(dependency.provenance, builddeps[deptup].provenance),
+                                LoadErrorReason.DUPLICATE_DEPENDENCY)
+            else:
+                builddeps[deptup] = dependency
+        if dependency.dep_type in [Symbol.RUNTIME, None]:
+            if deptup in rundeps:
+                raise LoadError("{}: Duplicate runtime dependency found at {}."
+                                .format(dependency.provenance, rundeps[deptup].provenance),
+                                LoadErrorReason.DUPLICATE_DEPENDENCY)
+            else:
+                rundeps[deptup] = dependency
+        acc.append(dependency)
+
+    # Now delete the field, we dont want it anymore
+    node.safe_del(key)
+
+
+# extract_depends_from_node():
+#
+# Creates an array of Dependency objects from a given dict node 'node',
+# allows both strings and dicts for expressing the dependency and
+# throws a comprehensive LoadError in the case that the node is malformed.
+#
+# After extracting depends, the symbol is deleted from the node
+#
+# Args:
+#    node (Node): A YAML loaded dictionary
+#
+# Returns:
+#    (list): a list of Dependency objects
+#
+def extract_depends_from_node(Node node):
+    cdef list acc = []
+    cdef dict rundeps = {}
+    cdef dict builddeps = {}
+    _extract_depends_from_node(node, <str> Symbol.BUILD_DEPENDS, <str> Symbol.BUILD, acc, rundeps, builddeps)
+    _extract_depends_from_node(node, <str> Symbol.RUNTIME_DEPENDS, <str> Symbol.RUNTIME, acc, rundeps, builddeps)
+    _extract_depends_from_node(node, <str> Symbol.DEPENDS, None, acc, rundeps, builddeps)
+    return acc
diff --git a/src/buildstream/_loader/loader.py b/src/buildstream/_loader/loader.py
index ab299f6f1..b0f4a4a07 100644
--- a/src/buildstream/_loader/loader.py
+++ b/src/buildstream/_loader/loader.py
@@ -29,9 +29,9 @@ from .._profile import Topics, PROFILER
 from .._includes import Includes
 
 from ._loader import valid_chars_name
-from .types import Symbol, extract_depends_from_node
+from .types import Symbol
 from . import loadelement
-from .loadelement import Dependency, LoadElement
+from .loadelement import LoadElement, Dependency, extract_depends_from_node
 from .metaelement import MetaElement
 from .metasource import MetaSource
 from ..types import CoreWarnings, _KeyStrength
@@ -145,9 +145,10 @@ class Loader:
         # Set up a dummy element that depends on all top-level targets
         # to resolve potential circular dependencies between them
         dummy_target = LoadElement(Node.from_dict({}), "", self)
+
         # Pylint is not very happy with Cython and can't understand 'dependencies' is a list
         dummy_target.dependencies.extend(  # pylint: disable=no-member
-            Dependency(element, Symbol.RUNTIME, False, None) for element in target_elements
+            Dependency(element, Symbol.RUNTIME) for element in target_elements
         )
 
         with PROFILER.profile(Topics.CIRCULAR_CHECK, "_".join(targets)):
@@ -444,7 +445,7 @@ class Loader:
         dependencies = extract_depends_from_node(top_element.node)
         # The loader queue is a stack of tuples
         # [0] is the LoadElement instance
-        # [1] is a stack of dependencies to load
+        # [1] is a stack of Dependency objects to load
         # [2] is a list of dependency names used to warn when all deps are loaded
         loader_queue = [(top_element, list(reversed(dependencies)), [])]
 
@@ -484,11 +485,11 @@ class Loader:
                     _, filename, loader = self._parse_name(dep_element.link_target, dep_element.link_target_provenance)
                     dep_element = loader._load_file(filename, dep_element.link_target_provenance)
 
-                # All is well, push the dependency onto the LoadElement
-                # Pylint is not very happy with Cython and can't understand 'dependencies' is a list
-                current_element[0].dependencies.append(  # pylint: disable=no-member
-                    Dependency(dep_element, dep.dep_type, dep.strict, dep.provenance)
-                )
+                # We've now resolved the element for this dependency, lets set the resolved
+                # LoadElement on the dependency and append the dependency to the owning
+                # LoadElement dependency list.
+                dep.set_element(dep_element)
+                current_element[0].dependencies.append(dep)
             else:
                 # We do not have any more dependencies to load for this
                 # element on the queue, report any invalid dep names
diff --git a/src/buildstream/_loader/types.py b/src/buildstream/_loader/types.py
new file mode 100644
index 000000000..290d9bf15
--- /dev/null
+++ b/src/buildstream/_loader/types.py
@@ -0,0 +1,44 @@
+#
+#  Copyright (C) 2020 Codethink Limited
+#
+#  This program is free software; you can redistribute it and/or
+#  modify it under the terms of the GNU Lesser General Public
+#  License as published by the Free Software Foundation; either
+#  version 2 of the License, or (at your option) any later version.
+#
+#  This library is distributed in the hope that it will be useful,
+#  but WITHOUT ANY WARRANTY; without even the implied warranty of
+#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the GNU
+#  Lesser General Public License for more details.
+#
+#  You should have received a copy of the GNU Lesser General Public
+#  License along with this library. If not, see <http://www.gnu.org/licenses/>.
+#
+#  Authors:
+#        Tristan Van Berkom <tristan.vanberkom@codethink.co.uk>
+
+
+# Symbol():
+#
+# A simple object to denote the symbols we load with from YAML
+#
+class Symbol:
+    FILENAME = "filename"
+    KIND = "kind"
+    DEPENDS = "depends"
+    BUILD_DEPENDS = "build-depends"
+    RUNTIME_DEPENDS = "runtime-depends"
+    SOURCES = "sources"
+    CONFIG = "config"
+    VARIABLES = "variables"
+    ENVIRONMENT = "environment"
+    ENV_NOCACHE = "environment-nocache"
+    PUBLIC = "public"
+    TYPE = "type"
+    BUILD = "build"
+    RUNTIME = "runtime"
+    ALL = "all"
+    DIRECTORY = "directory"
+    JUNCTION = "junction"
+    SANDBOX = "sandbox"
+    STRICT = "strict"
diff --git a/src/buildstream/_loader/types.pyx b/src/buildstream/_loader/types.pyx
deleted file mode 100644
index 70f262a10..000000000
--- a/src/buildstream/_loader/types.pyx
+++ /dev/null
@@ -1,207 +0,0 @@
-#
-#  Copyright (C) 2018 Codethink Limited
-#
-#  This program is free software; you can redistribute it and/or
-#  modify it under the terms of the GNU Lesser General Public
-#  License as published by the Free Software Foundation; either
-#  version 2 of the License, or (at your option) any later version.
-#
-#  This library is distributed in the hope that it will be useful,
-#  but WITHOUT ANY WARRANTY; without even the implied warranty of
-#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the GNU
-#  Lesser General Public License for more details.
-#
-#  You should have received a copy of the GNU Lesser General Public
-#  License along with this library. If not, see <http://www.gnu.org/licenses/>.
-#
-#  Authors:
-#        Tristan Van Berkom <tristan.vanberkom@codethink.co.uk>
-
-from .._exceptions import LoadError
-from ..exceptions import LoadErrorReason
-from ..node cimport MappingNode, Node, ProvenanceInformation, ScalarNode, SequenceNode
-
-
-# Symbol():
-#
-# A simple object to denote the symbols we load with from YAML
-#
-class Symbol():
-    FILENAME = "filename"
-    KIND = "kind"
-    DEPENDS = "depends"
-    BUILD_DEPENDS = "build-depends"
-    RUNTIME_DEPENDS = "runtime-depends"
-    SOURCES = "sources"
-    CONFIG = "config"
-    VARIABLES = "variables"
-    ENVIRONMENT = "environment"
-    ENV_NOCACHE = "environment-nocache"
-    PUBLIC = "public"
-    TYPE = "type"
-    BUILD = "build"
-    RUNTIME = "runtime"
-    ALL = "all"
-    DIRECTORY = "directory"
-    JUNCTION = "junction"
-    SANDBOX = "sandbox"
-    STRICT = "strict"
-
-
-# Dependency()
-#
-# A simple object describing a dependency
-#
-# Args:
-#    name (str or Node): The element name
-#    dep_type (str): The type of dependency, can be
-#                    Symbol.ALL, Symbol.BUILD, or Symbol.RUNTIME
-#    junction (str): The element name of the junction, or None
-#    provenance (ProvenanceInformation): The YAML node provenance of where this
-#                                        dependency was declared
-#
-cdef class Dependency:
-    cdef public ProvenanceInformation provenance
-    cdef public str name
-    cdef public str dep_type
-    cdef public str junction
-    cdef public bint strict
-
-    def __init__(self,
-                 Node dep,
-                 str default_dep_type=None):
-        cdef str dep_type
-
-        self.provenance = dep.get_provenance()
-
-        if type(dep) is ScalarNode:
-            self.name = dep.as_str()
-            self.dep_type = default_dep_type
-            self.junction = None
-            self.strict = False
-
-        elif type(dep) is MappingNode:
-            if default_dep_type:
-                (<MappingNode> dep).validate_keys(['filename', 'junction', 'strict'])
-                dep_type = default_dep_type
-            else:
-                (<MappingNode> dep).validate_keys(['filename', 'type', 'junction', 'strict'])
-
-                # Make type optional, for this we set it to None
-                dep_type = (<MappingNode> dep).get_str(<str> Symbol.TYPE, None)
-                if dep_type is None or dep_type == <str> Symbol.ALL:
-                    dep_type = None
-                elif dep_type not in [Symbol.BUILD, Symbol.RUNTIME]:
-                    provenance = dep.get_scalar(Symbol.TYPE).get_provenance()
-                    raise LoadError("{}: Dependency type '{}' is not 'build', 'runtime' or 'all'"
-                                    .format(provenance, dep_type), LoadErrorReason.INVALID_DATA)
-
-            self.name = (<MappingNode> dep).get_str(<str> Symbol.FILENAME)
-            self.dep_type = dep_type
-            self.junction = (<MappingNode> dep).get_str(<str> Symbol.JUNCTION, None)
-            self.strict = (<MappingNode> dep).get_bool(<str> Symbol.STRICT, False)
-
-            # Here we disallow explicitly setting 'strict' to False.
-            #
-            # This is in order to keep the door open to allowing the project.conf
-            # set the default of dependency 'strict'-ness which might be useful
-            # for projects which use mostly static linking and the like, in which
-            # case we can later interpret explicitly non-strict dependencies
-            # as an override of the project default.
-            #
-            if self.strict == False and Symbol.STRICT in dep:
-                provenance = dep.get_scalar(Symbol.STRICT).get_provenance()
-                raise LoadError("{}: Setting 'strict' to False is unsupported"
-                                .format(provenance), LoadErrorReason.INVALID_DATA)
-
-        else:
-            raise LoadError("{}: Dependency is not specified as a string or a dictionary".format(self.provenance),
-                            LoadErrorReason.INVALID_DATA)
-
-        # Only build dependencies are allowed to be strict
-        #
-        if self.strict and self.dep_type == Symbol.RUNTIME:
-            raise LoadError("{}: Runtime dependency {} specified as `strict`.".format(self.provenance, self.name),
-                            LoadErrorReason.INVALID_DATA,
-                            detail="Only dependencies required at build time may be declared `strict`.")
-
-        # `:` characters are not allowed in filename if a junction was
-        # explicitly specified
-        if self.junction and ':' in self.name:
-            raise LoadError("{}: Dependency {} contains `:` in its name. "
-                            "`:` characters are not allowed in filename when "
-                            "junction attribute is specified.".format(self.provenance, self.name),
-                            LoadErrorReason.INVALID_DATA)
-
-        # Attempt to split name if no junction was specified explicitly
-        if not self.junction and ':' in self.name:
-            self.junction, self.name = self.name.rsplit(':', maxsplit=1)
-
-
-# _extract_depends_from_node():
-#
-# Helper for extract_depends_from_node to get dependencies of a particular type
-#
-# Adds to an array of Dependency objects from a given dict node 'node',
-# allows both strings and dicts for expressing the dependency.
-#
-# After extracting depends, the symbol is deleted from the node
-#
-# Args:
-#    node (Node): A YAML loaded dictionary
-#    key (str): the key on the Node corresponding to the dependency type
-#    default_dep_type (str): type to give to the dependency
-#    acc (list): a list in which to add the loaded dependencies
-#    rundeps (dict): a dictionary mapping dependency (junction, name) to dependency for runtime deps
-#    builddeps (dict): a dictionary mapping dependency (junction, name) to dependency for build deps
-#
-cdef void _extract_depends_from_node(Node node, str key, str default_dep_type, list acc, dict rundeps, dict builddeps) except *:
-    cdef SequenceNode depends = node.get_sequence(key, [])
-    cdef Node dep_node
-    cdef tuple deptup
-
-    for dep_node in depends:
-        dependency = Dependency(dep_node, default_dep_type=default_dep_type)
-        deptup = (dependency.junction, dependency.name)
-        if dependency.dep_type in [Symbol.BUILD, None]:
-            if deptup in builddeps:
-                raise LoadError("{}: Duplicate build dependency found at {}."
-                                .format(dependency.provenance, builddeps[deptup].provenance),
-                                LoadErrorReason.DUPLICATE_DEPENDENCY)
-            else:
-                builddeps[deptup] = dependency
-        if dependency.dep_type in [Symbol.RUNTIME, None]:
-            if deptup in rundeps:
-                raise LoadError("{}: Duplicate runtime dependency found at {}."
-                                .format(dependency.provenance, rundeps[deptup].provenance),
-                                LoadErrorReason.DUPLICATE_DEPENDENCY)
-            else:
-                rundeps[deptup] = dependency
-        acc.append(dependency)
-
-    # Now delete the field, we dont want it anymore
-    node.safe_del(key)
-
-
-# extract_depends_from_node():
-#
-# Creates an array of Dependency objects from a given dict node 'node',
-# allows both strings and dicts for expressing the dependency and
-# throws a comprehensive LoadError in the case that the node is malformed.
-#
-# After extracting depends, the symbol is deleted from the node
-#
-# Args:
-#    node (Node): A YAML loaded dictionary
-#
-# Returns:
-#    (list): a list of Dependency objects
-#
-def extract_depends_from_node(Node node):
-    cdef list acc = []
-    cdef dict rundeps = {}
-    cdef dict builddeps = {}
-    _extract_depends_from_node(node, <str> Symbol.BUILD_DEPENDS, <str> Symbol.BUILD, acc, rundeps, builddeps)
-    _extract_depends_from_node(node, <str> Symbol.RUNTIME_DEPENDS, <str> Symbol.RUNTIME, acc, rundeps, builddeps)
-    _extract_depends_from_node(node, <str> Symbol.DEPENDS, None, acc, rundeps, builddeps)
-    return acc