graph: various minor fixes

Apply fixes requested and suggested during review.

Author: rezib

lib/rift/Controller.py

@@ -1107,8 +1107,8 @@ def get_packages_to_build(config, staff, modules, args):
 
     def result_position(new_build_requirements):
         """
-        Return the first index in result of packages in provided build
-        requirements list.
+        Return the index of the first package in result that appear as a build
+        requirement in new_build_requirements
         """
         for build_requirement in new_build_requirements:
             for index, package in enumerate(result):

lib/rift/graph.py

@@ -1,5 +1,5 @@
 #
-# Copyright (C) 2024 CEA
+# Copyright (C) 2025 CEA
 #
 # This file is part of Rift project.
 #
@@ -54,6 +54,16 @@ def __init__(self, package):
         self.subpackages = spec.provides
         # Parse buildrequires string in spec file to discard explicit versions
         # enforcement.
+        #
+        # Note this is currently done this way for the sake of simplicity,
+        # despite the value that could be provided by these version constraints.
+        # It could notably be interesting to extract lesser version constraints
+        # when a dependency is updated to a greater version.
+        #
+        # Currently, the automatic rebuilds of recursive reverse dependencies
+        # eventually fail at some point because of invalid versioning in this
+        # case but it could be nice to fail faster by detecting mismatching
+        # versions before the actual builds.
         self.build_requires = [
             value.group(1)
             for value
@@ -99,24 +109,13 @@ def dump(self):
         """Dump graph in its current state with logging message."""
         for node in self.nodes:
             logging.info("→ %s", node.package.name)
+            logging.info("  provides: %s", str(node.subpackages))
             logging.info("  requires: %s", node.build_requires)
-            logging.info("  subpackages: %s", str(node.subpackages))
             logging.info(
-                "  rdeps: %s", str([rdep.package.name for rdep in node.rdeps])
+                "  is required by: %s",
+                str([rdep.package.name for rdep in node.rdeps])
             )
 
-    def solve(self, package):
-        """
-        Return list of recursive build requirements for the provided package.
-        """
-        self.path = []  # Start with empty path
-        for node in self.nodes:
-            if node.package.name == package.name:
-                return self._solve(node, "User request")
-
-        # Package not found in graph, return empty list.
-        return []
-
     def _dep_index(self, new, result):
         """
         The new and results arguments are list of build requirements. The result
@@ -148,7 +147,7 @@ def _dep_index(self, new, result):
     def _solve(self, node, reason, depth=0):
         """
         Return list of recursive build requirements for the provided package
-        dependency node. The reason argument is a string to textually justify
+        dependency node. The "reason" argument is a string to textually justify
         the build requirement of the given node. The depth argument is used to
         track the depth of recursive path in the dependency graph.
         """
@@ -176,18 +175,19 @@ def _solve(self, node, reason, depth=0):
             if rdep.package.depends is not None:
                 reason = f"depends on {node.package.name}"
             else:
-                reason = "build requires on " + ", ".join(
+                reason = "build depends on " + ", ".join(
                     node.required_subpackages(rdep)
                 )
             # If reverse dependency has already been processed in the processing
             # path to the current node, add it to resulting list and stop
             # processing to avoid endless loop.
             if rdep in self.path[0:depth]:
                 logging.debug(
-                    "%s   ⥀ Loop detected on node %s at depth %d",
+                    "%s   ⥀ Loop detected on node %s at depth %d: %s",
                     '  '*depth,
                     rdep.package.name,
-                    depth
+                    depth,
+                    '→'.join(node.package.name for node in self.path + [rdep]),
                 )
                 result.append(BuildRequirement(rdep.package, [reason]))
                 continue
@@ -196,7 +196,7 @@ def _solve(self, node, reason, depth=0):
                 '  '*depth,
                 rdep.package.name
             )
-            # Iterate over all recursively solve build requirements for this
+            # Iterate over all recursively solved build requirements for this
             # reverse dependency.
             build_requirements = self._solve(rdep, reason, depth+1)
             for idx, build_requirement in enumerate(build_requirements):
@@ -218,6 +218,28 @@ def _solve(self, node, reason, depth=0):
                     result.insert(position, build_requirement)
         return result
 
+    def solve(self, package):
+        """
+        Return list of recursive build requirements for the provided package.
+        """
+        self.path = []  # Start with empty path
+        for node in self.nodes:
+            if node.package.name == package.name:
+                return self._solve(node, "User request")
+
+        # Package not found in graph, return empty list.
+        return []
+
+    def _insert(self, package):
+        """Insert package in the graph."""
+        node = PackageDependencyNode(package)
+        for _node in self.nodes:
+            if _node.depends_on(node):
+                node.rdeps.append(_node)
+            if node.depends_on(_node):
+                _node.rdeps.append(node)
+        self.nodes.append(node)
+
     def build(self, packages):
         """Build graph with the provided packages."""
         tic = time.perf_counter()
@@ -227,24 +249,14 @@ def build(self, packages):
             try:
                 package.load()
             except FileNotFoundError as err:
-                logging.warning("Skipping package %s unable to load: %s",
+                logging.warning("Skipping package '%s' unable to load: %s",
                                 package.name, err)
                 continue
             self._insert(package)
         toc = time.perf_counter()
         logging.debug("Graph built in %0.4f seconds", toc - tic)
         logging.debug("Graph size: %d", len(self.nodes))
 
-    def _insert(self, package):
-        """Insert package in the graph."""
-        node = PackageDependencyNode(package)
-        for _node in self.nodes:
-            if _node.depends_on(node):
-                node.rdeps.append(_node)
-            if node.depends_on(_node):
-                _node.rdeps.append(node)
-        self.nodes.append(node)
-
     @classmethod
     def from_project(cls, config, staff, modules):
         """

tests/Controller.py

@@ -1126,6 +1126,45 @@ def test_get_packages_to_build_package_order(self):
             [pkg.name for pkg in pkgs], ['libtwo', 'libone', 'my-software']
         )
 
+    def test_get_packages_to_build_cyclic_deps(self):
+        """ Test get_packages_to_build() returns correctly ordered list of reverse dependencies. """
+        self.make_pkg(
+            name='libone',
+            metadata={
+                'depends': 'libtwo'
+            }
+        )
+        self.make_pkg(
+            name='libtwo',
+            metadata={
+                'depends': 'libthree'
+            }
+        )
+        self.make_pkg(
+            name='libthree',
+            metadata={
+                'depends': 'libone'
+            }
+        )
+        # Enable tracking, disable --skip-deps
+        self.config.set('dependency_tracking', True)
+        args = Mock()
+        args.skip_deps = False
+        args.packages = ['libone']
+        with self.assertLogs(level="DEBUG") as cm:
+            pkgs = get_packages_to_build(
+                self.config, self.staff, self.modules, args
+            )
+        # Package libone must be present after libtwo and my-software must be
+        # present after both libtwo and libone in the order list of build
+        # requirements.
+        self.assertCountEqual(
+            [pkg.name for pkg in pkgs], ['libthree', 'libtwo', 'libone']
+        )
+        self.assertIn(
+            'DEBUG:root:       ⥀ Loop detected on node libone at depth 2: libone→libthree→libtwo→libone',
+            cm.output
+        )
 
 class ControllerArgumentsTest(RiftTestCase):
     """ Arguments parsing tests for Controller module"""

tests/graph.py

@@ -1,5 +1,5 @@
 #
-# Copyright (C) 2020 CEA
+# Copyright (C) 2025 CEA
 #
 import os
 import shutil
@@ -47,8 +47,8 @@ def test_packages_unable_load(self):
         # Check warning message has been emitted
         self.assertEqual(
             cm.output,
-            [ "WARNING:root:Skipping package failed unable to load: [Errno 2] "
-              "No such file or directory: "
+            [ "WARNING:root:Skipping package 'failed' unable to load: [Errno 2]"
+              " No such file or directory: "
               f"'{self.projdir}/packages/failed/info.yaml'" ]
         )
         # Check success package is successfully loaded anyway.
@@ -59,7 +59,6 @@ def test_dump(self):
         """ Test graph dump """
         pkg_name = 'fake'
         self.make_pkg(name=pkg_name)
-        package = Package(pkg_name, self.config, self.staff, self.modules)
         graph = PackagesDependencyGraph.from_project(
             self.config,
             self.staff,
@@ -71,9 +70,9 @@ def test_dump(self):
             cm.output,
             [
                 'INFO:root:→ fake',
+                "INFO:root:  provides: ['fake', 'fake-provide']",
                 "INFO:root:  requires: ['br-package']",
-                "INFO:root:  subpackages: ['fake', 'fake-provide']",
-                'INFO:root:  rdeps: []'
+                'INFO:root:  is required by: []'
             ]
         )
 
@@ -215,7 +214,7 @@ def load_graph():
         self.assertEqual(build_requirements[1].package.name, 'my-software')
         self.assertEqual(
             build_requirements[1].reasons,
-            ["build requires on libone-devel"]
+            ["build depends on libone-devel"]
         )
 
         # Rebuild of libtwo triggers rebuild of libone and my-software because
@@ -231,14 +230,14 @@ def load_graph():
         self.assertEqual(build_requirements[1].package.name, 'libone')
         self.assertEqual(
             build_requirements[1].reasons,
-            ["build requires on libtwo-devel"]
+            ["build depends on libtwo-devel"]
         )
         self.assertEqual(build_requirements[2].package.name, 'my-software')
         self.assertCountEqual(
             build_requirements[2].reasons,
             [
-                "build requires on libone-devel",
-                "build requires on libtwo-devel"
+                "build depends on libone-devel",
+                "build depends on libtwo-devel"
             ]
         )
 
@@ -304,7 +303,7 @@ def load_graph():
         self.assertEqual(build_requirements[1].package.name, 'my-software')
         self.assertEqual(
             build_requirements[1].reasons,
-            ["build requires on libone-provide"]
+            ["build depends on libone-provide"]
         )
 
     def test_loop(self):