_loader: Use only one Dependency() class

This moves the Dependency() implementation from the _loader.types
module into the _loader.loadelement module, replacing the duplicate
Dependency() definition there.

Instead of creating _loader.loadelement.Dependency objects on the
fly during the load process, we simply fill in the new LoadElement
member on the original Dependency objects.

This refactor should make the code easier to work with, since any
originally loaded state from the YAML is preserved in the intermediate
LoadElement data model and does not need to be manually handed over
to a separate object.

Summary of changes:

  * _loader/types.pyx: Removed Dependency() implementation from here,
    and renamed to _loader/types.py

  * _loader/loadelement.py: Replaced the second Dependency object
    implementation with the original one from _loader/types.py,
    adding a new LoadElement member which is resolved during the
    load process via the new Dependency.set_element() API

  * _loader/loader.py: Instead of creating a second kind of Dependency
    object on the fly during the load process, simply resolve the
    existing dependencies with Dependency.set_element()

  * setup.py: Fixed up cython build instructions, now that _loader/types.py
    no longer contains any cython code.

4 files changed, 239 insertions, 231 deletions

diff --git a/src/buildstream/_loader/loadelement.pyx b/src/buildstream/_loader/loadelement.pyx
index de2f96b37..49de8d02e 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,126 @@ 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 readonly ProvenanceInformation provenance  # The provenance 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.provenance = None
+
+    # 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.provenance = dep.get_provenance()
+        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():
@@ -269,3 +369,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