module_loader, app: Move app init to app module

- Don't try to get the depends from module level and sort modules based on that.

- Instead after all App instances are created, sort the apps based on
app.info.depends and app.info.is_essential.

- Print message that apps have been initialized instead of printing before they
are initialized. The correct order of apps is only known after they have been
initialized and sorted.

- Avoid circular import on module_loader and setup.

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: James Valleroy <jvalleroy@mailbox.org>

actions/service

@@ -7,8 +7,9 @@ Wrapper to list and handle system services
 import argparse
 import os
 
-from plinth import action_utils, cfg, module_loader
+from plinth import action_utils
-from plinth.app import App
+from plinth import app as app_module
+from plinth import cfg, module_loader
 from plinth.daemon import Daemon, RelatedDaemon
 
 cfg.read()
@@ -90,8 +91,8 @@ def _get_managed_services():
     """Get a set of all services managed by FreedomBox."""
     services = set()
     module_loader.load_modules()
-    module_loader.apps_init()
+    app_module.apps_init()
-    for app in App.list():
+    for app in app_module.App.list():
         components = app.get_components_of_type(Daemon)
         for component in components:
             services.add(component.unit)

plinth/__main__.py

@@ -113,7 +113,7 @@ def main():
     if arguments.list_dependencies is not False:
         log.default_level = 'ERROR'
         module_loader.load_modules()
-        module_loader.apps_init()
+        app_module.apps_init()
         list_dependencies(arguments.list_dependencies)
 
     if arguments.list_apps is not False:
@@ -137,8 +137,8 @@ def main():
     menu.init()
 
     module_loader.load_modules()
-    module_loader.apps_init()
+    app_module.apps_init()
-    module_loader.apps_post_init()
+    app_module.apps_post_init()
     frontpage.add_custom_shortcuts()
 
     if arguments.setup is not False:

plinth/app.py

@@ -5,10 +5,17 @@ Base class for all Freedombox applications.
 
 import collections
 import enum
+import inspect
+import logging
 import sys
 
+from plinth import cfg
+from plinth.signals import post_app_loading
+
 from . import clients as clients_module
 
+logger = logging.getLogger(__name__)
+
 
 class App:
     """Implement common functionality for an app.
@@ -454,3 +461,98 @@ class Info(FollowerComponent):
         self.donation_url = donation_url
         if clients:
             clients_module.validate(clients)
+
+
+def apps_init():
+    """Create apps by constructing them with components."""
+    from . import module_loader  # noqa  # Avoid circular import
+    for module_name, module in module_loader.loaded_modules.items():
+        _initialize_module(module_name, module)
+
+    _sort_apps()
+
+    logger.info('Initialized apps - %s', ', '.join(
+        (app.app_id for app in App.list())))
+
+
+def _sort_apps():
+    """Sort apps list according to their essential/dependency order."""
+    apps = App._all_apps
+    ordered_modules = []
+    remaining_apps = dict(apps)  # Make a copy
+    # Place all essential modules ahead of others in module load order
+    sorted_apps = sorted(
+        apps, key=lambda app_id: not App.get(app_id).info.is_essential)
+    for app_id in sorted_apps:
+        if app_id not in remaining_apps:
+            continue
+
+        app = remaining_apps.pop(app_id)
+        try:
+            _insert_apps(app_id, app, remaining_apps, ordered_modules)
+        except KeyError:
+            logger.error('Unsatified dependency for app - %s', app_id)
+
+    new_all_apps = collections.OrderedDict()
+    for app_id in ordered_modules:
+        new_all_apps[app_id] = apps[app_id]
+
+    App._all_apps = new_all_apps
+
+
+def _insert_apps(app_id, app, remaining_apps, ordered_apps):
+    """Insert apps into a list based on dependency order."""
+    if app_id in ordered_apps:
+        return
+
+    for dependency in app.info.depends:
+        if dependency in ordered_apps:
+            continue
+
+        try:
+            app = remaining_apps.pop(dependency)
+        except KeyError:
+            logger.error('Not found or circular dependency - %s, %s', app_id,
+                         dependency)
+            raise
+
+        _insert_apps(dependency, app, remaining_apps, ordered_apps)
+
+    ordered_apps.append(app_id)
+
+
+def _initialize_module(module_name, module):
+    """Perform initialization on all apps in a module."""
+    # Perform setup related initialization on the module
+    from . import setup  # noqa  # Avoid circular import
+    setup.init(module_name, module)
+
+    try:
+        module_classes = inspect.getmembers(module, inspect.isclass)
+        app_classes = [
+            cls for _, cls in module_classes if issubclass(cls, App)
+        ]
+        for app_class in app_classes:
+            module.app = app_class()
+    except Exception as exception:
+        logger.exception('Exception while running init for %s: %s', module,
+                         exception)
+        if cfg.develop:
+            raise
+
+
+def apps_post_init():
+    """Run post initialization on each app."""
+    for app in App.list():
+        try:
+            app.post_init()
+            if not app.needs_setup() and app.is_enabled():
+                app.set_enabled(True)
+        except Exception as exception:
+            logger.exception('Exception while running post init for %s: %s',
+                             app.app_id, exception)
+            if cfg.develop:
+                raise
+
+    logger.debug('App initialization completed.')
+    post_app_loading.send_robust(sender="app")

plinth/module_loader.py

@@ -5,15 +5,14 @@ Discover, load and manage FreedomBox applications.
 
 import collections
 import importlib
-import inspect
 import logging
 import pathlib
 import re
 
 import django
 
-from plinth import app, cfg, setup
+from plinth import cfg
-from plinth.signals import post_module_loading, pre_module_loading
+from plinth.signals import pre_module_loading
 
 logger = logging.getLogger(__name__)
 
@@ -34,63 +33,17 @@ def load_modules():
     import them from modules directory.
     """
     pre_module_loading.send_robust(sender="module_loader")
-    modules = {}
     for module_import_path in get_modules_to_load():
         module_name = module_import_path.split('.')[-1]
         try:
-            modules[module_name] = importlib.import_module(module_import_path)
+            loaded_modules[module_name] = importlib.import_module(
+                module_import_path)
         except Exception as exception:
             logger.exception('Could not import %s: %s', module_import_path,
                              exception)
             if cfg.develop:
                 raise
 
-    ordered_modules = []
-    remaining_modules = dict(modules)  # Make a copy
-    # Place all essential modules ahead of others in module load order
-    sorted_modules = sorted(
-        modules, key=lambda module: not modules[module].app.info.is_essential)
-    for module_name in sorted_modules:
-        if module_name not in remaining_modules:
-            continue
-
-        module = remaining_modules.pop(module_name)
-        try:
-            _insert_modules(module_name, module, remaining_modules,
-                            ordered_modules)
-        except KeyError:
-            logger.error('Unsatified dependency for module - %s', module_name)
-
-    for module_name in ordered_modules:
-        loaded_modules[module_name] = modules[module_name]
-
-
-def _insert_modules(module_name, module, remaining_modules, ordered_modules):
-    """Insert modules into a list based on dependency order"""
-    if module_name in ordered_modules:
-        return
-
-    dependencies = []
-    try:
-        dependencies = module.depends
-    except AttributeError:
-        pass
-
-    for dependency in dependencies:
-        if dependency in ordered_modules:
-            continue
-
-        try:
-            module = remaining_modules.pop(dependency)
-        except KeyError:
-            logger.error('Not found or circular dependency - %s, %s',
-                         module_name, dependency)
-            raise
-
-        _insert_modules(dependency, module, remaining_modules, ordered_modules)
-
-    ordered_modules.append(module_name)
-
 
 def _include_module_urls(module_import_path, module_name):
     """Include the module's URLs in global project URLs list"""
@@ -107,53 +60,6 @@ def _include_module_urls(module_import_path, module_name):
             raise
 
 
-def apps_init():
-    """Create apps by constructing them with components."""
-    logger.info('Initializing apps - %s', ', '.join(loaded_modules))
-    for module_name, module in loaded_modules.items():
-        _initialize_module(module_name, module)
-
-
-def _initialize_module(module_name, module):
-    """Perform module initialization"""
-
-    # Perform setup related initialization on the module
-    setup.init(module_name, module)
-
-    try:
-        module_classes = inspect.getmembers(module, inspect.isclass)
-        app_class = [
-            cls for cls in module_classes if issubclass(cls[1], app.App)
-        ]
-        if module_classes and app_class:
-            module.app = app_class[0][1]()
-    except Exception as exception:
-        logger.exception('Exception while running init for %s: %s', module,
-                         exception)
-        if cfg.develop:
-            raise
-
-
-def apps_post_init():
-    """Run post initialization on each app."""
-    for module in loaded_modules.values():
-        if not hasattr(module, 'app') or not module.app:
-            continue
-
-        try:
-            module.app.post_init()
-            if not module.app.needs_setup() and module.app.is_enabled():
-                module.app.set_enabled(True)
-        except Exception as exception:
-            logger.exception('Exception while running post init for %s: %s',
-                             module, exception)
-            if cfg.develop:
-                raise
-
-    logger.debug('App initialization completed.')
-    post_module_loading.send_robust(sender="module_loader")
-
-
 def get_modules_to_load():
     """Get the list of modules to be loaded"""
     global _modules_to_load

plinth/modules/letsencrypt/__init__.py

@@ -18,7 +18,7 @@ from plinth.modules.apache.components import diagnose_url
 from plinth.modules.backups.components import BackupRestore
 from plinth.modules.names.components import DomainType
 from plinth.package import Packages
-from plinth.signals import domain_added, domain_removed, post_module_loading
+from plinth.signals import domain_added, domain_removed, post_app_loading
 from plinth.utils import format_lazy
 
 from . import components, manifest
@@ -85,7 +85,7 @@ class LetsEncryptApp(app_module.App):
         domain_added.connect(on_domain_added)
         domain_removed.connect(on_domain_removed)
 
-        post_module_loading.connect(_certificate_handle_modified)
+        post_app_loading.connect(_certificate_handle_modified)
 
     def diagnose(self):
         """Run diagnostics and return the results."""

plinth/signals.py

@@ -9,7 +9,7 @@ from django.dispatch import Signal
 pre_module_loading = Signal()
 
 # Arguments: -
-post_module_loading = Signal()
+post_app_loading = Signal()
 
 # Arguments: module_name
 post_setup = Signal()

plinth/tests/test_app.py

@@ -9,7 +9,8 @@ from unittest.mock import Mock, call, patch
 
 import pytest
 
-from plinth.app import App, Component, FollowerComponent, Info, LeaderComponent
+from plinth.app import (App, Component, FollowerComponent, Info,
+                        LeaderComponent, apps_init)
 
 # pylint: disable=protected-access
 
@@ -424,3 +425,82 @@ def test_info_clients_validation():
         }]
     }]
     Info('test-app', 3, clients=clients)
+
+
+class ModuleTest1:
+    """A test module with an app."""
+
+    class App1(App):
+        """A non-essential app that depends on another."""
+        app_id = 'app1'
+
+        def __init__(self):
+            super().__init__()
+            self.add(Info('app1', version=1, depends=['app3']))
+
+
+class ModuleTest2:
+    """A test module with multiple apps."""
+
+    class App2(App):
+        """An essential app."""
+        app_id = 'app2'
+
+        def __init__(self):
+            super().__init__()
+            self.add(Info('app2', version=1, is_essential=True))
+
+    class App3(App):
+        """An non-essential app that is depended on by another."""
+        app_id = 'app3'
+
+        def __init__(self):
+            super().__init__()
+            self.add(Info('app3', version=1))
+
+
+@patch('plinth.module_loader.loaded_modules')
+def test_apps_init(loaded_modules):
+    """Test that initializing all apps works."""
+    loaded_modules.items.return_value = [('test1', ModuleTest1()),
+                                         ('test2', ModuleTest2())]
+
+    apps_init()
+    assert list(App._all_apps.keys()) == ['app2', 'app3', 'app1']
+
+
+class ModuleCircularTest:
+    """A test module with apps depending on each other."""
+
+    class App1(App):
+        """An app depending on app2."""
+        app_id = 'app1'
+
+        def __init__(self):
+            super().__init__()
+            self.add(Info('app1', version=1, depends=['app2']))
+
+    class App2(App):
+        """An app depending on app1."""
+        app_id = 'app2'
+
+        def __init__(self):
+            super().__init__()
+            self.add(Info('app2', version=1, depends=['app1']))
+
+    class App3(App):
+        """An app without dependencies."""
+        app_id = 'app3'
+
+        def __init__(self):
+            super().__init__()
+            self.add(Info('app3', version=1))
+
+
+@patch('plinth.module_loader.loaded_modules')
+def test_apps_init_circular_depends(loaded_modules):
+    """Test initializing apps with circular dependencies."""
+    loaded_modules.items.return_value = [('test', ModuleCircularTest())]
+
+    apps_init()
+    assert list(App._all_apps.keys()) == ['app3']