[xbuild] Little refactor associated to the graph

conans/client/graph/graph.py

@@ -1,3 +1,5 @@
+from collections import OrderedDict
+
 from conans.model.ref import PackageReference
 
 RECIPE_DOWNLOADED = "Downloaded"
@@ -20,7 +22,45 @@
 BINARY_EDITABLE = "Editable"
 
 
+class _NodeOrderedDict(object):
+    def __init__(self, key=lambda n: n.name):
+        self._nodes = OrderedDict()
+        self._key = key
+
+    def add(self, node):
+        key = self._key(node)
+        inserted = key not in self._nodes
+        self._nodes[key] = node
+        return inserted
+
+    def get(self, key):
+        return self._nodes.get(key)
+
+    def pop(self, key):
+        return self._nodes.pop(key)
+
+    def sort(self, key_fn):
+        """ It will sort the nodes according to the value returned from key_fn """
+        sorted_d = sorted(self._nodes.items(), key=lambda x: key_fn(x[1]))
+        self._nodes = OrderedDict(sorted_d)
+
+    def copy(self):
+        ret = _NodeOrderedDict()
+        ret._nodes = self._nodes.copy()
+        return ret
+
+    def prepend(self, other):
+        items = other._nodes.copy()
+        items.update(self._nodes)
+        self._nodes = items
+
+    def __iter__(self):
+        for _, item in self._nodes.items():
+            yield item
+
+
 class Node(object):
+
     def __init__(self, ref, conanfile, recipe=None):
         self.ref = ref
         self._package_id = None
@@ -37,18 +77,25 @@ def __init__(self, ref, conanfile, recipe=None):
         self.revision_pinned = False  # The revision has been specified by the user
 
         # The dependencies that can conflict to downstream consumers
-        self.public_deps = None  # {ref.name: Node}
+        self._public_deps = _NodeOrderedDict()  # {ref.name: Node}
         # all the public deps only in the closure of this node
         # The dependencies that will be part of deps_cpp_info, can't conflict
-        self.public_closure = None  # {ref.name: Node}
+        self._public_closure = _NodeOrderedDict()  # {ref.name: Node}
         self.inverse_closure = set()  # set of nodes that have this one in their public
-        self.ancestors = None  # set{ref.name}
 
-    def update_ancestors(self, ancestors):
-        # When a diamond is closed, it is necessary to update all upstream ancestors, recursively
-        self.ancestors.update(ancestors)
-        for n in self.neighbors():
-            n.update_ancestors(ancestors)
+        # Node itself is its public closure and deps
+        self._public_deps.add(self)
+        self._public_closure.add(self)
+
+    @property
+    def public_deps(self):
+        # Hide it as an attribute so it cannot be set
+        return self._public_deps
+
+    @property
+    def public_closure(self):
+        # Hide it as an attribute so it cannot be set
+        return self._public_closure
 
     @property
     def package_id(self):
@@ -83,8 +130,11 @@ def add_edge(self, edge):
         if edge.src == self:
             if edge not in self.dependencies:
                 self.dependencies.append(edge)
+                self.public_closure.add(edge.dst)
+                self.public_deps.add(edge.dst)
         else:
             self.dependants.add(edge)
+            self.inverse_closure.add(edge.src)
 
     def neighbors(self):
         return [edge.dst for edge in self.dependencies]

conans/client/graph/graph_binaries.py

@@ -205,7 +205,7 @@ def _handle_private(self, node):
                 if not neigh.private:
                     continue
                 # Current closure contains own node to be skipped
-                for n in neigh.public_closure.values():
+                for n in neigh.public_closure:
                     if n.private:
                         n.binary = BINARY_SKIP
                         self._handle_private(n)

conans/client/graph/graph_builder.py

@@ -27,10 +27,6 @@ def load_graph(self, root_node, check_updates, update, remotes, processed_profil
         check_updates = check_updates or update
         dep_graph = DepsGraph()
         # compute the conanfile entry point for this dependency graph
-        name = root_node.name
-        root_node.public_closure = OrderedDict([(name, root_node)])
-        root_node.public_deps = {name: root_node}
-        root_node.ancestors = set()
         dep_graph.add_node(root_node)
 
         # enter recursive computation
@@ -52,22 +48,18 @@ def extend_build_requires(self, graph, node, build_requires_refs, check_updates,
         new_options = node.conanfile.build_requires_options._reqs_options
         new_reqs = Requirements()
 
-        conanfile = node.conanfile
-        scope = conanfile.display_name
+        scope = node.conanfile.display_name
         requires = [Requirement(ref) for ref in build_requires_refs]
         self._resolve_ranges(graph, requires, scope, update, remotes)
 
         for require in requires:
-            name = require.ref.name
             require.build_require = True
-            self._handle_require(name, node, require, graph, check_updates, update,
+            self._handle_require(node, require, graph, check_updates, update,
                                  remotes, processed_profile, new_reqs, new_options)
 
         new_nodes = set([n for n in graph.nodes if n.package_id is None])
         # This is to make sure that build_requires have precedence over the normal requires
-        ordered_closure = list(node.public_closure.items())
-        ordered_closure.sort(key=lambda x: x[1] not in new_nodes)
-        node.public_closure = OrderedDict(ordered_closure)
+        node.public_closure.sort(key_fn=lambda x: x not in new_nodes)
 
         subgraph = DepsGraph()
         subgraph.aliased = graph.aliased
@@ -122,16 +114,17 @@ def _load_deps(self, dep_graph, node, down_reqs, down_ref, down_options,
         self._resolve_deps(dep_graph, node, update, remotes)
 
         # Expand each one of the current requirements
-        for name, require in node.conanfile.requires.items():
+        for require in node.conanfile.requires.values():
             if require.override:
                 continue
-            self._handle_require(name, node, require, dep_graph, check_updates, update,
+            self._handle_require(node, require, dep_graph, check_updates, update,
                                  remotes, processed_profile, new_reqs, new_options)
 
-    def _handle_require(self, name, node, require, dep_graph, check_updates, update,
+    def _handle_require(self, node, require, dep_graph, check_updates, update,
                         remotes, processed_profile, new_reqs, new_options):
 
-        if name in node.ancestors or name == node.name:
+        name = require.ref.name
+        if name in [it.name for it in node.inverse_closure] or name == node.name:
             raise ConanException("Loop detected: '%s' requires '%s' which is an ancestor too"
                                  % (node.ref, require.ref))
 
@@ -140,34 +133,25 @@ def _handle_require(self, name, node, require, dep_graph, check_updates, update,
         if not previous or ((require.build_require or require.private) and not previous_closure):
             # new node, must be added and expanded
             # node -> new_node
-            new_node = self._create_new_node(node, dep_graph, require, name,
-                                             check_updates, update, remotes,
-                                             processed_profile)
-
-            # The closure of a new node starts with just itself
-            new_node.public_closure = OrderedDict([(new_node.ref.name, new_node)])
-            node.public_closure[name] = new_node
-            new_node.inverse_closure.add(node)
-            node.public_deps[new_node.name] = new_node
+            new_node = self._create_new_node(node, dep_graph, require, check_updates, update,
+                                             remotes, processed_profile)
 
             # If the parent node is a build-require, this new node will be a build-require
             # If the requirement is a build-require, this node will also be a build-require
-            new_node.build_require = node.build_require or require.build_require
+            new_node.build_require = node.build_require or require.build_require  # TODO: needed?
             # New nodes will inherit the private property of its ancestor
-            new_node.private = node.private or require.private
+            new_node.private = node.private or require.private  # TODO: needed?
             if require.private or require.build_require:
                 # If the requirement is private (or build_require), a new public scope is defined
-                new_node.public_deps = node.public_closure.copy()
-                new_node.public_deps[name] = new_node
+                new_node.public_deps.prepend(node.public_closure)  # TODO: ??
             else:
-                new_node.public_deps = node.public_deps.copy()
-                new_node.public_deps[name] = new_node
+                new_node.public_deps.prepend(node.public_deps)  # TODO: ??
 
                 # Update the closure of each dependent
                 for dep_node in node.inverse_closure:
-                    dep_node.public_closure[new_node.name] = new_node
+                    dep_node.public_closure.add(new_node)
                     new_node.inverse_closure.add(dep_node)
-                    dep_node.public_deps[new_node.name] = new_node
+                    dep_node.public_deps.add(new_node)
 
             # RECURSION!
             self._load_deps(dep_graph, new_node, new_reqs, node.ref,
@@ -191,23 +175,19 @@ def _handle_require(self, name, node, require, dep_graph, check_updates, update,
                                      % (node.ref, require.ref, previous.ref))
 
             # Add current ancestors to the previous node
-            previous.update_ancestors(node.ancestors.union([node.name]))
             if previous.private and not require.private:
                 previous.make_public()
 
-            node.public_closure[name] = previous
-            previous.inverse_closure.add(node)
-            node.public_deps[name] = previous
             dep_graph.add_edge(node, previous, require.private, require.build_require)
             # Update the closure of each dependent
-            for name, n in previous.public_closure.items():
+            for n in previous.public_closure:
                 if n.build_require or n.private:
                     continue
-                node.public_closure[name] = n
+                node.public_closure.add(n)
                 n.inverse_closure.add(node)
                 for dep_node in node.inverse_closure:
-                    dep_node.public_closure[name] = n
-                    dep_node.public_deps[name] = n
+                    dep_node.public_closure.add(n)
+                    dep_node.public_deps.add(n)
                     n.inverse_closure.add(dep_node)
 
             # RECURSION!
@@ -303,7 +283,7 @@ def _config_node(self, graph, node, down_reqs, down_ref, down_options):
 
         return new_down_reqs, new_options
 
-    def _create_new_node(self, current_node, dep_graph, requirement, name_req,
+    def _create_new_node(self, current_node, dep_graph, requirement,
                          check_updates, update, remotes, processed_profile, alias_ref=None):
         """ creates and adds a new node to the dependency graph
         """
@@ -321,26 +301,25 @@ def _create_new_node(self, current_node, dep_graph, requirement, name_req,
 
         dep_conanfile = self._loader.load_conanfile(conanfile_path, processed_profile,
                                                     ref=requirement.ref)
-        if recipe_status == RECIPE_EDITABLE:
-            dep_conanfile.in_local_cache = False
-            dep_conanfile.develop = True
 
         if getattr(dep_conanfile, "alias", None):
             alias_ref = alias_ref or new_ref.copy_clear_rev()
             requirement.ref = ConanFileReference.loads(dep_conanfile.alias)
             dep_graph.aliased[alias_ref] = requirement.ref
             return self._create_new_node(current_node, dep_graph, requirement,
-                                         name_req, check_updates, update,
+                                         check_updates, update,
                                          remotes, processed_profile,
                                          alias_ref=alias_ref)
 
+        if recipe_status == RECIPE_EDITABLE:
+            dep_conanfile.in_local_cache = False
+            dep_conanfile.develop = True
+
         logger.debug("GRAPH: new_node: %s" % str(new_ref))
         new_node = Node(new_ref, dep_conanfile)
         new_node.revision_pinned = requirement.ref.revision is not None
         new_node.recipe = recipe_status
         new_node.remote = remote
-        new_node.ancestors = current_node.ancestors.copy()
-        new_node.ancestors.add(current_node.name)
         dep_graph.add_node(new_node)
         dep_graph.add_edge(current_node, new_node, requirement.private, requirement.build_require)
         return new_node

conans/client/graph/graph_manager.py

@@ -244,12 +244,8 @@ def _load_graph(self, root_node, check_updates, update, build_mode, remotes,
         # Sort of closures, for linking order
         inverse_levels = {n: i for i, level in enumerate(graph.inverse_levels()) for n in level}
         for node in graph.nodes:
-            closure = node.public_closure
-            closure.pop(node.name)
-            node_order = list(closure.values())
-            # List sort is stable, will keep the original order of the closure, but prioritize levels
-            node_order.sort(key=lambda n: inverse_levels[n])
-            node.public_closure = node_order
+            node.public_closure.pop(node.name)
+            node.public_closure.sort(key_fn=lambda n: inverse_levels[n])
 
         return graph
 

conans/test/functional/graph/graph_manager_base.py

@@ -93,7 +93,7 @@ def _check_node(self, node, ref, deps, build_deps, dependents, closure):
             self.assertEqual(conanfile.requires[dep.name].ref,
                              dep.ref)
 
-        self.assertEqual(closure, node.public_closure)
+        self.assertListEqual(closure, list(node.public_closure))
         libs = []
         envs = []
         for n in closure:

conans/test/functional/graph/graph_manager_test.py

@@ -36,14 +36,13 @@ def test_transitive(self):
         self.assertEqual(libb.conanfile.name, "libb")
         self.assertEqual(len(libb.dependencies), 0)
         self.assertEqual(len(libb.dependants), 1)
-        self.assertEqual(libb.inverse_neighbors(), [app])
-        self.assertEqual(libb.ancestors, set([app.ref.name]))
+        self.assertListEqual(libb.inverse_neighbors(), [app])
         self.assertEqual(libb.recipe, RECIPE_INCACHE)
 
-        self.assertEqual(app.public_closure, [libb])
-        self.assertEqual(libb.public_closure, [])
-        self.assertEqual(app.public_deps, {"app": app, "libb": libb})
-        self.assertEqual(libb.public_deps, app.public_deps)
+        self.assertListEqual(list(app.public_closure), [libb])
+        self.assertListEqual(list(libb.public_closure), [])
+        self.assertListEqual(list(app.public_deps), [app, libb])
+        self.assertListEqual(list(libb.public_deps), list(app.public_deps))
 
     def test_transitive_two_levels(self):
         # app -> libb0.1 -> liba0.1