setup: Drop setup_helper and use the new Operation API

- Task of managing an operation's progress is now performed by the new Operation
class. Drop them from setup helper.

- Task of providing install() method is now moved to package module. Instead of
storing operation specific data in setup_helper like objects, store them in
thread specific storage that can retrieved anywhere during the operation without
holding references.

- Progress of an operation show as a progress bar is currently missing. This
will be regression until fixed later.

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

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 = [

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):
+def _collect_operations_results(request, app):
-    """Show success/fail message from previous install operation."""
+    """Show success/fail messages from previous operations."""
-    setup_helper = sys.modules[app.__module__].setup_helper
+    operations = operation_module.manager.collect_results(app.app_id)
-    if not setup_helper.is_finished:
+    for operation in operations:
-        return
+        if operation.exception:
-
+            messages.error(request, operation.message)
-    exception = setup_helper.collect_result()
-    if not exception:
-        if not app.info.is_essential:
-            messages.success(request, _('Application installed.'))
         else:
-        if isinstance(exception, PackageException):
+            messages.success(request, operation.message)
-            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)
-        else:
-            message = _('Error installing application: {error}') \
-                .format(error=exception)
-
-        messages.error(request, 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() == \

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 {

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,11 +172,7 @@ class Packages(app.FollowerComponent):
 
     def setup(self, old_version):
         """Install the packages."""
-        # TODO: Drop the need for setup helper.
+        install(self.get_actual_packages(),
-        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)
 
     def diagnose(self):
@@ -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:

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:
+def run_setup_on_app(app_id, allow_install=True):
-    """Helper routines for modules to show progress."""
-
-    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
-
-    def run_in_thread(self):
     """Execute the setup process in a thread."""
-        thread = threading.Thread(target=self._run)
-        thread.start()
-
-    def _run(self):
-        """Collect exceptions when running in a thread."""
-        try:
-            self.run()
-        except Exception as exception:
-            self.exception = exception
-
-    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:
+    if operation_module.manager.filter(app_id):
         return
 
-        app = self.module.app
+    # 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
 
-        self.allow_install = allow_install
+    if not current_version:
-        self.exception = None
+        name = gettext_noop('Installing app')
-        self.current_operation = None
+    else:
-        self.is_finished = False
+        name = gettext_noop('Updating app')
+
+    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 _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:
-            if hasattr(self.module, 'setup'):
+        current_version = app.get_setup_version()
-                logger.info('Running module setup - %s', self.module_name)
+        app.setup(old_version=current_version)
-                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
-        else:
         app.set_setup_version(app.info.version)
-            post_setup.send_robust(sender=self.__class__,
+        post_setup.send_robust(sender=app.__class__, module_name=app.app_id)
-                                   module_name=self.module_name)
+    except PackageException as exception:
-        finally:
+        exception_to_update = exception
-            self.is_finished = True
+        error_string = getattr(exception, 'error_string', str(exception))
-            self.current_operation = None
+        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:
+            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,
+    logger.info('Setup completed: %s: %s %s', app.app_id, message,
-                force_configuration=None, reinstall=False,
+                exception_to_update)
-                force_missing_configuration=False):
+    operation = operation_module.Operation.get_operation()
-        """Install a set of packages marking progress."""
+    operation.on_update(message, exception_to_update)
-        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)
 
 
 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__]
+        operation = run_setup_on_app(app.app_id, allow_install=allow_install)
-        module.setup_helper.run(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
 

plinth/templates/setup.html

@@ -17,7 +17,7 @@
 
      {% trans "Application installed." %}
 
-  {% elif not setup_current_operation %}
+  {% elif not operations %}
 
     <p>
       {% if setup_state.value == 'needs-setup' %}
@@ -77,38 +77,12 @@
 
   {% else %}
 
-    {% if setup_current_operation.step == 'pre' %}
+    {% for operation in operations %}
-      <div class="installing install-state-pre">
+      <div class="app-operation">
         <span class="fa fa-refresh fa-spin processing"></span>
-        {% trans "Performing pre-install operation" %}
+        {{ operation.translated_message }}
       </div>
-    {% elif setup_current_operation.step == 'post' %}
+    {% endfor %}
-      <div class="installing install-state-post">
-        <span class="fa fa-refresh fa-spin processing"></span>
-        {% trans "Performing post-install operation" %}
-      </div>
-    {% elif setup_current_operation.step == 'install' %}
-      {% with transaction=setup_current_operation.transaction %}
-        <div class="installing install-state-installing">
-          <span class="fa fa-refresh fa-spin processing"></span>
-          {% blocktrans trimmed with package_names=transaction.package_names|join:", " status=transaction.status_string %}
-            Installing {{ package_names }}: {{ status }}
-          {% endblocktrans %}
-        </div>
-        <div class="progress">
-          <div class="progress-bar progress-bar-striped active
-                      w-{{ transaction.percentage }}"
-               role="progressbar" aria-valuemin="0" aria-valuemax="100"
-               aria-valuenow="{{ transaction.percentage }}">
-            <span class="sr-only">
-              {% blocktrans trimmed with percentage=transaction.percentage %}
-                {{ percentage }}% complete
-              {% endblocktrans %}
-            </span>
-          </div>
-        </div>
-      {% endwith %}
-    {% endif %}
 
   {% endif %}
 

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,11 +352,6 @@ 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')

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

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
+        operation_manager.collect_results.return_value = [
-        setup_helper_.collect_result.return_value = None
+            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
+        messages_success.assert_has_calls([call(request, 'message1')])
-        setup_helper_.collect_result.assert_called_once_with()
+        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:

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(
+    install.assert_has_calls([call(['bash', 'perl'], skip_recommends=True)])
-        [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(
+    install.assert_has_calls([call(['python3'], skip_recommends=False)])
-        [call(['python3'], skip_recommends=False)])
 
 
 @patch('apt.Cache')

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['operations'] = operation.manager.filter(app.app_id)
-        context['setup_current_operation'] = setup_helper.current_operation
 
         # 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.run_setup_on_app(self.kwargs['app_id'])
-                setup_helper = sys.modules[app.__module__].setup_helper
-                setup_helper.run_in_thread()
 
                 # Give a moment for the setup process to start and show
                 # meaningful status.