From 603b63bbace2161b3d0e2a9cb107fd8968d7bffe Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Wed, 24 Nov 2021 13:21:26 -0800 Subject: [PATCH] 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 Reviewed-by: James Valleroy --- actions/service | 9 ++- plinth/__main__.py | 6 +- plinth/app.py | 102 +++++++++++++++++++++++++ plinth/module_loader.py | 102 +------------------------ plinth/modules/letsencrypt/__init__.py | 4 +- plinth/signals.py | 2 +- plinth/tests/test_app.py | 82 +++++++++++++++++++- 7 files changed, 198 insertions(+), 109 deletions(-) diff --git a/actions/service b/actions/service index dab13c73e..2da7c4d3b 100755 --- a/actions/service +++ b/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.app import App +from plinth import action_utils +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() - for app in App.list(): + app_module.apps_init() + for app in app_module.App.list(): components = app.get_components_of_type(Daemon) for component in components: services.add(component.unit) diff --git a/plinth/__main__.py b/plinth/__main__.py index e3a4ce8c4..12f898879 100644 --- a/plinth/__main__.py +++ b/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() - module_loader.apps_post_init() + app_module.apps_init() + app_module.apps_post_init() frontpage.add_custom_shortcuts() if arguments.setup is not False: diff --git a/plinth/app.py b/plinth/app.py index 45e6e0b94..a1c8e4247 100644 --- a/plinth/app.py +++ b/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") diff --git a/plinth/module_loader.py b/plinth/module_loader.py index b90dc9388..02a6bdd68 100644 --- a/plinth/module_loader.py +++ b/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.signals import post_module_loading, pre_module_loading +from plinth import cfg +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 diff --git a/plinth/modules/letsencrypt/__init__.py b/plinth/modules/letsencrypt/__init__.py index c7538b4ed..812b27116 100644 --- a/plinth/modules/letsencrypt/__init__.py +++ b/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.""" diff --git a/plinth/signals.py b/plinth/signals.py index c064d4351..e476b2acb 100644 --- a/plinth/signals.py +++ b/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() diff --git a/plinth/tests/test_app.py b/plinth/tests/test_app.py index 759b904f0..a7603622f 100644 --- a/plinth/tests/test_app.py +++ b/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']