diff --git a/plinth/app.py b/plinth/app.py index 727db85db..a3d1d7781 100644 --- a/plinth/app.py +++ b/plinth/app.py @@ -7,7 +7,6 @@ import collections import enum import inspect import logging -import sys from plinth import cfg from plinth.signals import post_app_loading @@ -542,10 +541,6 @@ def _insert_apps(app_id, app, remaining_apps, ordered_apps): 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 = [ diff --git a/plinth/middleware.py b/plinth/middleware.py index b68db7a89..0dec396ba 100644 --- a/plinth/middleware.py +++ b/plinth/middleware.py @@ -4,7 +4,6 @@ Common Django middleware. """ import logging -import sys from django import urls from django.conf import settings @@ -13,41 +12,26 @@ from django.contrib.auth.decorators import login_required from django.core.exceptions import PermissionDenied from django.shortcuts import render from django.utils.deprecation import MiddlewareMixin -from django.utils.translation import gettext_lazy as _ from stronghold.utils import is_view_func_public from plinth import app as app_module from plinth import setup -from plinth.package import PackageException from plinth.utils import is_user_admin +from . import operation as operation_module from . import views logger = logging.getLogger(__name__) -def _collect_setup_result(request, app): - """Show success/fail message from previous install operation.""" - setup_helper = sys.modules[app.__module__].setup_helper - if not setup_helper.is_finished: - return - - exception = setup_helper.collect_result() - if not exception: - if not app.info.is_essential: - messages.success(request, _('Application installed.')) - else: - if isinstance(exception, PackageException): - error_string = getattr(exception, 'error_string', str(exception)) - error_details = getattr(exception, 'error_details', '') - message = _('Error installing application: {string} ' - '{details}').format(string=error_string, - details=error_details) +def _collect_operations_results(request, app): + """Show success/fail messages from previous operations.""" + operations = operation_module.manager.collect_results(app.app_id) + for operation in operations: + if operation.exception: + messages.error(request, operation.message) else: - message = _('Error installing application: {error}') \ - .format(error=exception) - - messages.error(request, message) + messages.success(request, operation.message) class SetupMiddleware(MiddlewareMixin): @@ -77,9 +61,9 @@ class SetupMiddleware(MiddlewareMixin): app = app_module.App.get(app_id) is_admin = is_user_admin(request) - # Collect and show setup operation result to admins + # Collect and show operations' results to admins if is_admin: - _collect_setup_result(request, app) + _collect_operations_results(request, app) # Check if application is up-to-date if app.get_setup_state() == \ diff --git a/plinth/modules/backups/tests/test_api.py b/plinth/modules/backups/tests/test_api.py index 395e2f271..8ea170b7a 100644 --- a/plinth/modules/backups/tests/test_api.py +++ b/plinth/modules/backups/tests/test_api.py @@ -15,8 +15,6 @@ from ..components import BackupRestore # pylint: disable=protected-access -setup_helper = MagicMock() - def _get_test_manifest(name): return { diff --git a/plinth/package.py b/plinth/package.py index 0fea8449d..a7dbcb734 100644 --- a/plinth/package.py +++ b/plinth/package.py @@ -8,7 +8,6 @@ import json import logging import pathlib import subprocess -import sys import threading from typing import Optional, Union @@ -20,6 +19,9 @@ from plinth import actions, app from plinth.errors import ActionError, MissingPackageError from plinth.utils import format_lazy +from . import operation as operation_module +from .errors import PackageNotInstalledError + logger = logging.getLogger(__name__) @@ -170,12 +172,8 @@ class Packages(app.FollowerComponent): def setup(self, old_version): """Install the packages.""" - # TODO: Drop the need for setup helper. - module_name = self.app.__module__ - module = sys.modules[module_name] - helper = module.setup_helper - helper.install(self.get_actual_packages(), - skip_recommends=self.skip_recommends) + install(self.get_actual_packages(), + skip_recommends=self.skip_recommends) def diagnose(self): """Run diagnostics and return results.""" @@ -387,6 +385,34 @@ class Transaction: self.percentage = int(float(parts[2])) +def install(package_names, skip_recommends=False, force_configuration=None, + reinstall=False, force_missing_configuration=False): + """Install a set of packages marking progress.""" + try: + operation = operation_module.Operation.get_operation() + except AttributeError: + raise RuntimeError( + 'install() must be called from within an operation.') + + if not operation.thread_data.get('allow_install', True): + # Raise error if packages are not already installed. + cache = apt.Cache() + for package_name in package_names: + if not cache[package_name].is_installed: + raise PackageNotInstalledError(package_name) + + return + + logger.info('Running install for app - %s, packages - %s', + operation.app_id, package_names) + + from . import package + transaction = package.Transaction(operation.app_id, package_names) + operation.thread_data['transaction'] = transaction + transaction.install(skip_recommends, force_configuration, reinstall, + force_missing_configuration) + + def is_package_manager_busy(): """Return whether a package manager is running.""" try: diff --git a/plinth/setup.py b/plinth/setup.py index fd5dc4d72..718127465 100644 --- a/plinth/setup.py +++ b/plinth/setup.py @@ -4,20 +4,20 @@ Utilities for performing application setup operations. """ import logging -import sys import threading import time from collections import defaultdict import apt +from django.utils.translation import gettext_noop import plinth from plinth import app as app_module -from plinth.package import Packages +from plinth.package import PackageException, Packages from plinth.signals import post_setup +from . import operation as operation_module from . import package -from .errors import PackageNotInstalledError logger = logging.getLogger(__name__) @@ -28,106 +28,72 @@ _is_shutting_down = False _force_upgrader = None -class Helper: - """Helper routines for modules to show progress.""" +def run_setup_on_app(app_id, allow_install=True): + """Execute the setup process in a thread.""" + # Setup for the module is already running + if operation_module.manager.filter(app_id): + return - def __init__(self, module_name, module): - """Initialize the object.""" - self.module_name = module_name - self.module = module - self.current_operation = None - self.is_finished = None - self.exception = None - self.allow_install = True + # App is already up-to-date + app = app_module.App.get(app_id) + current_version = app.get_setup_version() + if current_version >= app.info.version: + return - def run_in_thread(self): - """Execute the setup process in a thread.""" - thread = threading.Thread(target=self._run) - thread.start() + if not current_version: + name = gettext_noop('Installing app') + else: + name = gettext_noop('Updating app') - def _run(self): - """Collect exceptions when running in a thread.""" - try: - self.run() - except Exception as exception: - self.exception = exception + logger.debug('Creating operation to setup app: %s', app_id) + show_notification = show_message = (current_version + or not app.info.is_essential) + return operation_module.manager.new( + app_id, name, _run_setup_on_app, [app, current_version], + show_message=show_message, show_notification=show_notification, + thread_data={'allow_install': allow_install}) - def collect_result(self): - """Return the exception if any.""" - exception = self.exception - self.exception = None - self.is_finished = None - return exception - def run(self, allow_install=True): - """Execute the setup process.""" - # Setup for the module is already running - if self.current_operation: - return - - app = self.module.app +def _run_setup_on_app(app, current_version): + """Execute the setup process.""" + logger.info('Setup run: %s', app.app_id) + exception_to_update = None + message = None + try: current_version = app.get_setup_version() - if current_version >= app.info.version: - return - - self.allow_install = allow_install - self.exception = None - self.current_operation = None - self.is_finished = False - try: - if hasattr(self.module, 'setup'): - logger.info('Running module setup - %s', self.module_name) - self.module.setup(self, old_version=current_version) - else: - logger.info('Module does not require setup - %s', - self.module_name) - except Exception as exception: - logger.exception('Error running setup - %s', exception) - raise exception + app.setup(old_version=current_version) + app.set_setup_version(app.info.version) + post_setup.send_robust(sender=app.__class__, module_name=app.app_id) + except PackageException as exception: + exception_to_update = exception + error_string = getattr(exception, 'error_string', str(exception)) + error_details = getattr(exception, 'error_details', '') + if not current_version: + message = gettext_noop('Error installing app: {string} ' + '{details}').format(string=error_string, + details=error_details) else: - app.set_setup_version(app.info.version) - post_setup.send_robust(sender=self.__class__, - module_name=self.module_name) - finally: - self.is_finished = True - self.current_operation = None + message = gettext_noop('Error updating app: {string} ' + '{details}').format(string=error_string, + details=error_details) + except Exception as exception: + exception_to_update = exception + if not current_version: + message = gettext_noop('Error installing app: {error}').format( + error=exception) + else: + message = gettext_noop('Error updating app: {error}').format( + error=exception) + else: + if not current_version: + message = gettext_noop('App installed.') + else: + message = gettext_noop('App updated') - def install(self, package_names, skip_recommends=False, - force_configuration=None, reinstall=False, - force_missing_configuration=False): - """Install a set of packages marking progress.""" - if self.allow_install is False: - # Raise error if packages are not already installed. - cache = apt.Cache() - for package_name in package_names: - if not cache[package_name].is_installed: - raise PackageNotInstalledError(package_name) - - return - - logger.info('Running install for module - %s, packages - %s', - self.module_name, package_names) - - transaction = package.Transaction(self.module_name, package_names) - self.current_operation = { - 'step': 'install', - 'transaction': transaction, - } - transaction.install(skip_recommends, force_configuration, reinstall, - force_missing_configuration) - - def call(self, step, method, *args, **kwargs): - """Call an arbitrary method during setup and note down its stage.""" - logger.info('Running step for module - %s, step - %s', - self.module_name, step) - self.current_operation = {'step': step} - return method(*args, **kwargs) - - -def init(module_name, module): - """Create a setup helper for a module for later use.""" - if not hasattr(module, 'setup_helper'): - module.setup_helper = Helper(module_name, module) + logger.info('Setup completed: %s: %s %s', app.app_id, message, + exception_to_update) + operation = operation_module.Operation.get_operation() + operation.on_update(message, exception_to_update) def stop(): @@ -151,8 +117,9 @@ def setup_apps(app_ids=None, essential=False, allow_install=True): if app_ids and app.app_id not in app_ids: continue - module = sys.modules[app.__module__] - module.setup_helper.run(allow_install=allow_install) + operation = run_setup_on_app(app.app_id, allow_install=allow_install) + if operation: + operation.join() def list_dependencies(app_ids=None, essential=False): @@ -173,10 +140,10 @@ def list_dependencies(app_ids=None, essential=False): def run_setup_in_background(): """Run setup in a background thread.""" _set_is_first_setup() - threading.Thread(target=_run_setup).start() + threading.Thread(target=_run_setup_on_startup).start() -def _run_setup(): +def _run_setup_on_startup(): """Run setup with retry till it succeeds.""" sleep_time = 10 while True: @@ -203,7 +170,6 @@ def _run_first_setup(): """Run setup on essential apps on first setup.""" global is_first_setup_running is_first_setup_running = True - # TODO When it errors out, show error in the UI run_setup_on_apps(None, allow_install=False) is_first_setup_running = False diff --git a/plinth/templates/setup.html b/plinth/templates/setup.html index 37cb5ea22..6895ddfa6 100644 --- a/plinth/templates/setup.html +++ b/plinth/templates/setup.html @@ -17,7 +17,7 @@ {% trans "Application installed." %} - {% elif not setup_current_operation %} + {% elif not operations %}

{% if setup_state.value == 'needs-setup' %} @@ -77,38 +77,12 @@ {% else %} - {% if setup_current_operation.step == 'pre' %} -

+ {% for operation in operations %} +
- {% trans "Performing pre-install operation" %} + {{ operation.translated_message }}
- {% elif setup_current_operation.step == 'post' %} -
- - {% trans "Performing post-install operation" %} -
- {% elif setup_current_operation.step == 'install' %} - {% with transaction=setup_current_operation.transaction %} -
- - {% blocktrans trimmed with package_names=transaction.package_names|join:", " status=transaction.status_string %} - Installing {{ package_names }}: {{ status }} - {% endblocktrans %} -
-
-
- - {% blocktrans trimmed with percentage=transaction.percentage %} - {{ percentage }}% complete - {% endblocktrans %} - -
-
- {% endwith %} - {% endif %} + {% endfor %} {% endif %} diff --git a/plinth/tests/functional/__init__.py b/plinth/tests/functional/__init__.py index c9074ea40..c36e6753b 100644 --- a/plinth/tests/functional/__init__.py +++ b/plinth/tests/functional/__init__.py @@ -342,7 +342,7 @@ def install(browser, app_name): install_button_css = '.form-install input[type=submit]' while True: script = 'return (document.readyState == "complete") && ' \ - '(!Boolean(document.querySelector(".installing")));' + '(!Boolean(document.querySelector(".app-operation")));' if not browser.execute_script(script): time.sleep(0.1) elif browser.is_element_present_by_css('.neterror'): @@ -352,15 +352,10 @@ def install(browser, app_name): elif browser.is_element_present_by_css(install_button_css): install_button = browser.find_by_css(install_button_css).first if install_button['disabled']: - if not browser.find_by_name('refresh-packages'): - # Package manager is busy, wait and refresh page - time.sleep(1) - browser.visit(browser.url) - else: - # This app is not available in this distribution - warnings.warn( - f'App {app_name} is not available in distribution') - pytest.skip('App not available in distribution') + # This app is not available in this distribution + warnings.warn( + f'App {app_name} is not available in distribution') + pytest.skip('App not available in distribution') else: install_button.click() else: diff --git a/plinth/tests/test_app.py b/plinth/tests/test_app.py index 2369f0eb7..91ca7fee8 100644 --- a/plinth/tests/test_app.py +++ b/plinth/tests/test_app.py @@ -4,7 +4,6 @@ Test module for App, base class for all applications. """ import collections -import sys from unittest.mock import Mock, call, patch import pytest diff --git a/plinth/tests/test_middleware.py b/plinth/tests/test_middleware.py index 1f1463e6d..e335c2583 100644 --- a/plinth/tests/test_middleware.py +++ b/plinth/tests/test_middleware.py @@ -3,7 +3,7 @@ Test module for custom middleware. """ -from unittest.mock import MagicMock, Mock, patch +from unittest.mock import MagicMock, Mock, call, patch import pytest from django.contrib.auth.models import AnonymousUser, Group, User @@ -136,20 +136,24 @@ class TestSetupMiddleware: view.assert_called_once_with(request, app_id='mockapp') @staticmethod - @patch('plinth.tests.test_middleware.setup_helper') + @patch('plinth.operation.manager') + @patch('django.contrib.messages.error') @patch('django.contrib.messages.success') @patch('django.urls.resolve') @patch('django.urls.reverse', return_value='users:login') @pytest.mark.django_db def test_install_result_collection(reverse, resolve, messages_success, - setup_helper_, app, middleware, kwargs): + messages_error, operation_manager, app, + middleware, kwargs): """Test that module installation result is collected properly.""" resolve.return_value.namespaces = ['mockapp'] - setup_helper_.is_finished = True - setup_helper_.collect_result.return_value = None + operation_manager.collect_results.return_value = [ + Mock(message='message1', exception=None), + Mock(message='message2', exception='x-exception') + ] app.get_setup_state = lambda: app_module.App.SetupState.UP_TO_DATE - # Admin user can't collect result + # Admin user can collect result request = RequestFactory().get('/plinth/mockapp') user = User(username='adminuser') user.save() @@ -162,12 +166,14 @@ class TestSetupMiddleware: response = middleware.process_view(request, **kwargs) assert response is None - assert messages_success.called - setup_helper_.collect_result.assert_called_once_with() + messages_success.assert_has_calls([call(request, 'message1')]) + messages_error.assert_has_calls([call(request, 'message2')]) + operation_manager.collect_results.assert_has_calls([call('mockapp')]) # Non-admin user can't collect result messages_success.reset_mock() - setup_helper_.collect_result.reset_mock() + messages_error.reset_mock() + operation_manager.collect_results.reset_mock() request = RequestFactory().get('/plinth/mockapp') user = User(username='johndoe') user.save() @@ -177,7 +183,8 @@ class TestSetupMiddleware: assert response is None assert not messages_success.called - setup_helper_.collect_result.assert_not_called() + assert not messages_error.called + operation_manager.collect_results.assert_not_called() class TestAdminMiddleware: diff --git a/plinth/tests/test_package.py b/plinth/tests/test_package.py index 96f364096..2f171a57b 100644 --- a/plinth/tests/test_package.py +++ b/plinth/tests/test_package.py @@ -12,8 +12,6 @@ from plinth.app import App from plinth.errors import ActionError, MissingPackageError from plinth.package import Package, Packages, packages_installed, remove -setup_helper = Mock() - class TestPackageExpressions(unittest.TestCase): @@ -86,7 +84,8 @@ def test_packages_get_actual_packages(): assert component.get_actual_packages() == [] -def test_packages_setup(): +@patch('plinth.package.install') +def test_packages_setup(install): """Test setting up packages component.""" class TestApp(App): @@ -96,28 +95,23 @@ def test_packages_setup(): component = Packages('test-component', ['python3', 'bash']) app = TestApp() app.add(component) - setup_helper.reset_mock() app.setup(old_version=3) - setup_helper.install.assert_has_calls( + install.assert_has_calls( [call(['python3', 'bash'], skip_recommends=False)]) component = Packages('test-component', ['bash', 'perl'], skip_recommends=True) app = TestApp() app.add(component) - setup_helper.reset_mock() app.setup(old_version=3) - setup_helper.install.assert_has_calls( - [call(['bash', 'perl'], skip_recommends=True)]) + install.assert_has_calls([call(['bash', 'perl'], skip_recommends=True)]) component = Packages('test-component', [Package('python3') | Package('unknown-package')]) app = TestApp() app.add(component) - setup_helper.reset_mock() app.setup(old_version=3) - setup_helper.install.assert_has_calls( - [call(['python3'], skip_recommends=False)]) + install.assert_has_calls([call(['python3'], skip_recommends=False)]) @patch('apt.Cache') diff --git a/plinth/views.py b/plinth/views.py index 737df09cc..3f85a804d 100644 --- a/plinth/views.py +++ b/plinth/views.py @@ -3,7 +3,6 @@ Main FreedomBox views. """ -import sys import time import urllib.parse @@ -18,14 +17,13 @@ from django.views.generic.edit import FormView from stronghold.decorators import public from plinth import app as app_module -from plinth import package from plinth.daemon import app_is_running from plinth.modules.config import get_advanced_mode from plinth.modules.firewall.components import get_port_forwarding_info from plinth.package import Packages from plinth.translation import get_language_from_request, set_language -from . import forms, frontpage +from . import forms, frontpage, operation, package, setup REDIRECT_FIELD_NAME = 'next' @@ -285,11 +283,10 @@ class SetupView(TemplateView): # Reuse the value of setup_state throughout the view for consistency. context['setup_state'] = app.get_setup_state() - setup_helper = sys.modules[app.__module__].setup_helper - context['setup_current_operation'] = setup_helper.current_operation + context['operations'] = operation.manager.filter(app.app_id) # Perform expensive operation only if needed. - if not context['setup_current_operation']: + if not context['operations']: context[ 'package_manager_is_busy'] = package.is_package_manager_busy() context[ @@ -299,7 +296,7 @@ class SetupView(TemplateView): context['refresh_page_sec'] = None if context['setup_state'] == app_module.App.SetupState.UP_TO_DATE: context['refresh_page_sec'] = 0 - elif context['setup_current_operation']: + elif context['operations']: context['refresh_page_sec'] = 3 return context @@ -310,9 +307,7 @@ class SetupView(TemplateView): # Handle installing/upgrading applications. # Start the application setup, and refresh the page every few # seconds to keep displaying the status. - app = app_module.App.get(self.kwargs['app_id']) - setup_helper = sys.modules[app.__module__].setup_helper - setup_helper.run_in_thread() + setup.run_setup_on_app(self.kwargs['app_id']) # Give a moment for the setup process to start and show # meaningful status.