_loader/loader: Extract check_circular_deps to utils and Cythonizebschubert/more-cython

- This requires access to 'LoadElement' from _loader/loadelement, we
therefore add a definition file for this module and export it.

5 files changed, 102 insertions, 66 deletions

diff --git a/setup.py b/setup.py
index e292a6d77..aa6d16c79 100755
--- a/setup.py
+++ b/setup.py
@@ -369,7 +369,7 @@ def register_cython_module(module_name, dependencies=None):
 
     implementation_file, definition_file = files_from_module(module_name)
 
-    assert os.path.exists(implementation_file)
+    assert os.path.exists(implementation_file), "{} not found".format(implementation_file)
 
     depends = []
     if os.path.exists(definition_file):
@@ -397,7 +397,7 @@ register_cython_module(
     "buildstream._loader.loadelement",
     dependencies=["buildstream._loader.types", "buildstream._yaml"],
 )
-register_cython_module("buildstream._loader.utils")
+register_cython_module("buildstream._loader.utils", dependencies=["buildstream._loader.loadelement"])
 register_cython_module("buildstream._loader.types", dependencies=["buildstream._yaml"])
 register_cython_module("buildstream._yaml")
 register_cython_module("buildstream._variables", dependencies=["buildstream._yaml"])
diff --git a/src/buildstream/_loader/loadelement.pxd b/src/buildstream/_loader/loadelement.pxd
new file mode 100644
index 000000000..e918ad6ed
--- /dev/null
+++ b/src/buildstream/_loader/loadelement.pxd
@@ -0,0 +1,38 @@
+#
+#  Copyright (C) 2019 Bloomberg L.P.
+#
+#  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:
+#        Benjamin Schubert <bschubert@bloomberg.net>
+
+# Documentation for each class and method here can be found in the adjacent
+# implementation file (loadelement.pyx)
+
+
+from .. cimport _yaml
+
+cdef class LoadElement:
+
+    cdef public _yaml.Node node
+    cdef public str name
+    cdef public str full_name
+    cdef public list dependencies
+    cdef public int node_id
+    cdef public bint meta_done
+    cdef public object _loader
+    # FIXME: Pyroaring exporting its symbols would be nice
+    cdef public object _dep_cache
+
+    cdef void _ensure_depends_cache(self)
diff --git a/src/buildstream/_loader/loadelement.pyx b/src/buildstream/_loader/loadelement.pyx
index d28798eee..27070c818 100644
--- a/src/buildstream/_loader/loadelement.pyx
+++ b/src/buildstream/_loader/loadelement.pyx
@@ -67,16 +67,6 @@ cdef class LoadDependency:
 #
 cdef class LoadElement:
 
-    cdef public _yaml.Node node
-    cdef public str name
-    cdef public str full_name
-    cdef public list dependencies
-    cdef public int node_id
-    cdef public bint meta_done
-    cdef public object _loader
-    # FIXME: Pyroaring exporting its symbols would be nice
-    cdef public object _dep_cache
-
     def __init__(self, _yaml.Node node, str filename, object loader):
 
         #
@@ -136,7 +126,7 @@ cdef class LoadElement:
     ###########################################
     #            Private Methods              #
     ###########################################
-    cdef _ensure_depends_cache(self):
+    cdef void _ensure_depends_cache(self):
         cdef LoadDependency dep
         cdef LoadElement elt
 
diff --git a/src/buildstream/_loader/loader.py b/src/buildstream/_loader/loader.py
index 5386eaf00..0767e3687 100644
--- a/src/buildstream/_loader/loader.py
+++ b/src/buildstream/_loader/loader.py
@@ -29,7 +29,7 @@ from .._profile import Topics, PROFILER
 from .._includes import Includes
 
 from .types import Symbol
-from .utils import valid_chars_name
+from .utils import check_circular_deps, valid_chars_name
 from .loadelement import LoadElement, LoadDependency, _extract_depends_from_node
 from .metaelement import MetaElement
 from .metasource import MetaSource
@@ -128,7 +128,7 @@ class Loader():
         )
 
         with PROFILER.profile(Topics.CIRCULAR_CHECK, "_".join(targets)):
-            self._check_circular_deps(dummy_target)
+            check_circular_deps(dummy_target)
 
         ret = []
         #
@@ -327,57 +327,6 @@ class Loader():
         # Nothing more in the queue, return the top level element we loaded.
         return top_element
 
-    # _check_circular_deps():
-    #
-    # Detect circular dependencies on LoadElements with
-    # dependencies already resolved.
-    #
-    # Args:
-    #    element (str): The element to check
-    #
-    # Raises:
-    #    (LoadError): In case there was a circular dependency error
-    #
-    @staticmethod
-    def _check_circular_deps(top_element):
-
-        sequence = [top_element]
-        sequence_indices = [0]
-        check_elements = set(sequence)
-        validated = set()
-
-        while sequence:
-            this_element = sequence[-1]
-            index = sequence_indices[-1]
-            if index < len(this_element.dependencies):
-                element = this_element.dependencies[index].element
-                sequence_indices[-1] = index + 1
-                if element in check_elements:
-                    # Create `chain`, the loop of element dependencies from this
-                    # element back to itself, by trimming everything before this
-                    # element from the sequence under consideration.
-                    chain = [element.full_name for element in sequence[sequence.index(element):]]
-                    chain.append(element.full_name)
-                    raise LoadError(LoadErrorReason.CIRCULAR_DEPENDENCY,
-                                    ("Circular dependency detected at element: {}\n" +
-                                     "Dependency chain: {}")
-                                    .format(element.full_name, " -> ".join(chain)))
-                if element not in validated:
-                    # We've not already validated this element, so let's
-                    # descend into it to check it out
-                    sequence.append(element)
-                    sequence_indices.append(0)
-                    check_elements.add(element)
-                # Otherwise we'll head back around the loop to validate the
-                # next dependency in this entry
-            else:
-                # Done with entry, pop it off, indicate we're no longer
-                # in its chain, and mark it valid
-                sequence.pop()
-                sequence_indices.pop()
-                check_elements.remove(this_element)
-                validated.add(this_element)
-
     # _sort_dependencies():
     #
     # Sort dependencies of each element by their dependencies,
diff --git a/src/buildstream/_loader/utils.pyx b/src/buildstream/_loader/utils.pyx
index 258a34ab7..1546fafa2 100644
--- a/src/buildstream/_loader/utils.pyx
+++ b/src/buildstream/_loader/utils.pyx
@@ -18,6 +18,9 @@
 #        Benjamin Schubert <bschubert@bloomberg.net>
 #
 
+from .._exceptions import LoadError, LoadErrorReason
+from .loadelement cimport LoadElement
+
 
 # Check if given filename containers valid characters.
 #
@@ -50,3 +53,59 @@ def valid_chars_name(str name):
                 return False
 
     return True
+
+
+# _check_circular_deps():
+#
+# Detect circular dependencies on LoadElements with
+# dependencies already resolved.
+#
+# Args:
+#    element (LoadElement): The element to check
+#
+# Raises:
+#    (LoadError): In case there was a circular dependency error
+#
+def check_circular_deps(LoadElement top_element):
+
+    cdef list sequence = [top_element]
+    cdef list sequence_indices = [0]
+    # TODO: if pyroaring exported BitMap objects, we could use this here
+    cdef set check_elements = set(sequence)
+    cdef set validated = set()
+
+    cdef LoadElement this_element, element
+    cdef Py_ssize_t index
+
+    while sequence:
+        this_element = sequence[-1]
+        index = sequence_indices[-1]
+
+        if index < len(this_element.dependencies):
+            element = <LoadElement> this_element.dependencies[index].element
+            sequence_indices[-1] = index + 1
+            if element in check_elements:
+                # Create `chain`, the loop of element dependencies from this
+                # element back to itself, by trimming everything before this
+                # element from the sequence under consideration.
+                chain = [(<LoadElement> elem).full_name for elem in sequence[sequence.index(element):]]
+                chain.append(element.full_name)
+                raise LoadError(LoadErrorReason.CIRCULAR_DEPENDENCY,
+                                ("Circular dependency detected at element: {}\n" +
+                                 "Dependency chain: {}")
+                                .format(element.full_name, " -> ".join(chain)))
+            if element not in validated:
+                # We've not already validated this element, so let's
+                # descend into it to check it out
+                sequence.append(element)
+                sequence_indices.append(0)
+                check_elements.add(element)
+            # Otherwise we'll head back around the loop to validate the
+            # next dependency in this entry
+        else:
+            # Done with entry, pop it off, indicate we're no longer
+            # in its chain, and mark it valid
+            sequence.pop()
+            sequence_indices.pop()
+            check_elements.remove(this_element)
+            validated.add(this_element)