fix: circular dependencies error when there are none (#1505)

* test: reproduce bug with wrong circular dependencies

Add a new test case reproducing the problem reported in #1397

* chore: fix typos in comment and docstring

* fix: activate plugins in deterministic order

* test: update test case with latest changes

* fix: set while at True

* test: re-enable circular testing

* fix: pin version

* docs: add info to CHANGES

Co-authored-by: Sijis Aviles <[email protected]>

Author: browniebroke

CHANGES.rst

@@ -34,6 +34,7 @@ fixes:
 - fix: removed deprecated argument reconnection_interval for irc v20.0 (#1568)
 - docs: Add Gentoo packages (#1567)
 - chore: bump actions/setup-python from 3.1.0 to 3.1.2 (#1564)
+- fix: circular dependencies error when there are none (#1505)
 
 v6.1.8 (2021-06-21)
 -------------------

errbot/plugin_manager.py

@@ -5,7 +5,8 @@
 import sys
 import traceback
 from copy import deepcopy
-from importlib import machinery
+from graphlib import CycleError
+from graphlib import TopologicalSorter as BaseTopologicalSorter
 from pathlib import Path
 from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Type
 
@@ -165,6 +166,12 @@ def check_errbot_version(plugin_info: PluginInfo):
 BL_PLUGINS = "bl_plugins"
 
 
+class TopologicalSorter(BaseTopologicalSorter):
+    def find_cycle(self):
+        """Wraps private method as public one."""
+        return self._find_cycle()
+
+
 class BotPluginManager(StoreMixin):
     def __init__(
         self,
@@ -421,11 +428,12 @@ def activate_non_started_plugins(self) -> None:
         """
         Activates all plugins that are not activated, respecting its dependencies.
 
-        :return: Empty string if no problem occured or a string explaining what went wrong.
+        :return: Empty string if no problem occurred or a string explaining what went wrong.
         """
         log.info("Activate bot plugins...")
         errors = ""
-        for name, plugin in self.plugins.items():
+        for name in self.get_plugins_activation_order():
+            plugin = self.plugins.get(name)
             try:
                 if self.is_plugin_blacklisted(name):
                     errors += (
@@ -451,6 +459,29 @@ def activate_non_started_plugins(self) -> None:
                 errors += f"Error: flow {name} failed to start: {e}.\n"
         return errors
 
+    def get_plugins_activation_order(self) -> List[str]:
+        """
+        Calculate plugin activation order, based on their dependencies.
+
+        :return: list of plugin names, in the best order to start them.
+        """
+        plugins_graph = {
+            name: set(info.dependencies) for name, info in self.plugin_infos.items()
+        }
+        plugins_in_cycle = set()
+        while True:
+            plugins_sorter = TopologicalSorter(plugins_graph)
+            try:
+                # Return plugins which are part of a circular dependency at the end,
+                # the rest of the code expects to have all plugins returned
+                return list(plugins_sorter.static_order()) + list(plugins_in_cycle)
+            except CycleError:
+                # Remove cycle from the graph, and
+                cycle = set(plugins_sorter.find_cycle())
+                plugins_in_cycle.update(cycle)
+                for plugin_name in cycle:
+                    plugins_graph.pop(plugin_name)
+
     def _activate_plugin(self, plugin: BotPlugin, plugin_info: PluginInfo) -> None:
         """
         Activate a specific plugin with no check.

setup.py

@@ -42,6 +42,9 @@
     "deepmerge==1.0.1",
 ]
 
+if py_version < (3, 9):
+    deps.append("graphlib-backport==1.0.3")
+
 src_root = os.curdir
 
 

tests/circular_dependencies_test.py

@@ -6,9 +6,9 @@
 pytest_plugins = ["errbot.backends.test"]
 
 
-# def test_if_all_loaded_circular_dependencies(testbot):
-#     """https://github.com/errbotio/errbot/issues/1397"""
-#     plug_names = testbot.bot.plugin_manager.get_all_active_plugin_names()
-#     assert "PluginA" in plug_names
-#     assert "PluginB" in plug_names
-#     assert "PluginC" in plug_names
+def test_if_all_loaded_circular_dependencies(testbot):
+    """https://github.com/errbotio/errbot/issues/1397"""
+    plug_names = testbot.bot.plugin_manager.get_all_active_plugin_names()
+    assert "PluginA" in plug_names
+    assert "PluginB" in plug_names
+    assert "PluginC" in plug_names

tests/commands_test.py

@@ -169,6 +169,10 @@ def test_broken_plugin(testbot):
         )
         assert "import borken  # fails" in testbot.pop_message()
         assert "as it did not load correctly." in testbot.pop_message()
+        assert (
+            "Error: Broken failed to activate: "
+            "'NoneType' object has no attribute 'is_activated'"
+        ) in testbot.pop_message()
         assert "Plugins reloaded." in testbot.pop_message()
     finally:
         rmtree(tempd)

tests/dependencies_test.py

@@ -58,3 +58,10 @@ def test_indirect_circular_dependency(testbot):
     assert "Circular2" not in plug_names
     assert "Circular3" not in plug_names
     assert "Circular4" not in plug_names
+
+
+def test_chained_dependency(testbot):
+    plug_names = testbot.bot.plugin_manager.get_all_active_plugin_names()
+    assert "Chained1" in plug_names
+    assert "Chained2" in plug_names
+    assert "Chained3" in plug_names

tests/dependent_plugins/chained1.plug

@@ -0,0 +1,3 @@
+[Core]
+Name = Chained1
+Module = chained1

tests/dependent_plugins/chained1.py

@@ -0,0 +1,5 @@
+from errbot import BotPlugin
+
+
+class Chained1(BotPlugin):
+    pass

tests/dependent_plugins/chained2.plug

@@ -0,0 +1,4 @@
+[Core]
+Name = Chained2
+Module = chained2
+DependsOn = Chained1

tests/dependent_plugins/chained2.py

@@ -0,0 +1,5 @@
+from errbot import BotPlugin
+
+
+class Chained2(BotPlugin):
+    pass