Merge lp:~townsend/libertine/catch-runtime-errors into lp:libertine

[Christopher Townsend]

Catch runtime errors raised when starting a container so a crash file is not generated and to tell the GUI that there was an error so the user can be presented a proper notification.

[Libertine CI Bot (libertine-ci-bot)]

FAILED: Continuous integration, rev:204
https://jenkins.canonical.com/libertine/job/libertine-ci-verify-mp/185/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/libertine/job/generic-ci-generate-dsc/188
    FAILURE: https://jenkins.canonical.com/libertine/job/libertine-ci-build-and-test-dsc/architecture=amd64,distribution=vivid,label_exp=jenkins-slave-0%20%7C%7C%20jenkins-slave-2/147/console
    FAILURE: https://jenkins.canonical.com/libertine/job/libertine-ci-build-and-test-dsc/architecture=amd64,distribution=xenial,label_exp=jenkins-slave-0%20%7C%7C%20jenkins-slave-2/147/console
    FAILURE: https://jenkins.canonical.com/libertine/job/libertine-ci-build-and-test-dsc/architecture=i386,distribution=vivid,label_exp=jenkins-slave-0%20%7C%7C%20jenkins-slave-2/147/console
    FAILURE: https://jenkins.canonical.com/libertine/job/libertine-ci-build-and-test-dsc/architecture=i386,distribution=xenial,label_exp=jenkins-slave-0%20%7C%7C%20jenkins-slave-2/147/console
    None: https://jenkins.canonical.com/libertine/job/libertine-ci-update-mp/1083/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/libertine/job/libertine-ci-verify-mp/185/rebuild

review: Needs Fixing (continuous-integration)

[lp:~townsend/libertine/catch-runtime-errors]

205. By Christopher Townsend on 2016-04-18

Fix remove-package return values to fix broken test.

[Libertine CI Bot (libertine-ci-bot)]

PASSED: Continuous integration, rev:205
https://jenkins.canonical.com/libertine/job/libertine-ci-verify-mp/186/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/libertine/job/generic-ci-generate-dsc/189
    SUCCESS: https://jenkins.canonical.com/libertine/job/libertine-ci-build-and-test-dsc/architecture=amd64,distribution=vivid,label_exp=jenkins-slave-0%20%7C%7C%20jenkins-slave-2/148
    SUCCESS: https://jenkins.canonical.com/libertine/job/libertine-ci-build-and-test-dsc/architecture=amd64,distribution=xenial,label_exp=jenkins-slave-0%20%7C%7C%20jenkins-slave-2/148
    SUCCESS: https://jenkins.canonical.com/libertine/job/libertine-ci-build-and-test-dsc/architecture=i386,distribution=vivid,label_exp=jenkins-slave-0%20%7C%7C%20jenkins-slave-2/148
    SUCCESS: https://jenkins.canonical.com/libertine/job/libertine-ci-build-and-test-dsc/architecture=i386,distribution=xenial,label_exp=jenkins-slave-0%20%7C%7C%20jenkins-slave-2/148
    None: https://jenkins.canonical.com/libertine/job/libertine-ci-update-mp/1084/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/libertine/job/libertine-ci-verify-mp/186/rebuild

review: Approve (continuous-integration)

[Larry Price (larryprice)]

This feels like a lot of redundant code with a try-catch in every throwable method. Have you considered making a class or helper function that takes in and calls a lambda wrapped in the try-catch? Something like:

def try_container_action(self, func):
    try:
        with ContainerRunning(self.container):
            func()
    except RuntimeError as e:
        return handle_runtime_error(e)

Where "func" is a lambda for the one-liner in each method. You could also pull the contents of handle_runtime_error(e) into that method. Your thoughts?

[Larry Price (larryprice)]

> This feels like a lot of redundant code with a try-catch in every throwable
> method. Have you considered making a class or helper function that takes in
> and calls a lambda wrapped in the try-catch? Something like:
>
> def try_container_action(self, func):
> try:
> with ContainerRunning(self.container):
> func()
> except RuntimeError as e:
> return handle_runtime_error(e)
>
> Where "func" is a lambda for the one-liner in each method. You could also pull
> the contents of handle_runtime_error(e) into that method. Your thoughts?

We talked about this and decided to move on with our lives.

review: Approve

=== modified file 'python/libertine/Libertine.py'
--- python/libertine/Libertine.py	2016-04-13 18:02:56 +0000
+++ python/libertine/Libertine.py	2016-04-18 15:21:37 +0000
@@ -59,6 +59,11 @@
     return '/usr/bin/apt ' + apt_args_for_verbosity_level(verbosity) + ' --option Apt::Cmd::Disable-Script-Warning=true '
 
 
+def handle_runtime_error(error):
+    print("%s" % error)
+    return False
+
+
 class BaseContainer(metaclass=abc.ABCMeta):
     """
     An abstract base container to provide common functionality for all
@@ -311,15 +316,21 @@
         """
         Updates the contents of the container.
         """
-        with ContainerRunning(self.container):
-            self.container.update_packages(verbosity)
+        try:
+            with ContainerRunning(self.container):
+                self.container.update_packages(verbosity)
+        except RuntimeError as e:
+            return handle_runtime_error(e)
 
     def install_package(self, package_name, verbosity=1):
         """
         Installs a package in the container.
         """
-        with ContainerRunning(self.container):
-            return self.container.install_package(package_name, verbosity)
+        try:
+            with ContainerRunning(self.container):
+                return self.container.install_package(package_name, verbosity)
+        except RuntimeError as e:
+            return handle_runtime_error(e)
 
     def remove_package(self, package_name, verbosity=1):
         """
@@ -328,9 +339,13 @@
         :param package_name: The name of the package to be removed.
         :param verbosity: The verbosity level of the progress reporting.
         """
-        with ContainerRunning(self.container):
-            self.container.run_in_container(apt_command_prefix(verbosity) + "purge '" + package_name + "'") == 0
-            self.container.run_in_container(apt_command_prefix(verbosity) + "autoremove --purge") == 0
+        try:
+            with ContainerRunning(self.container):
+                if not self.container.run_in_container(apt_command_prefix(verbosity) + "purge '" + package_name + "'") == 0:
+                    return False
+                return self.container.run_in_container(apt_command_prefix(verbosity) + "autoremove --purge") == 0
+        except RuntimeError as e:
+            return handle_runtime_error(e)
 
     def search_package_cache(self, search_string):
         """
@@ -339,8 +354,11 @@
         :param search_string: the (regex) to use to search the package cache.
             The regex is quoted to sanitize it.
         """
-        with ContainerRunning(self.container):
-            self.container.run_in_container("/usr/bin/apt-cache search '" + search_string + "'")
+        try:
+            with ContainerRunning(self.container):
+                self.container.run_in_container("/usr/bin/apt-cache search '" + search_string + "'")
+        except RuntimeError as e:
+            return handle_runtime_error(e)
 
     def launch_application(self, app_exec_line):
         """
@@ -380,9 +398,15 @@
             example, 'apt-cache policy package-foo'
         :rtype: The output of the given command.
         """
-        with ContainerRunning(self.container):
-            return self.container.run_in_container(exec_line)
+        try:
+            with ContainerRunning(self.container):
+                return self.container.run_in_container(exec_line)
+        except RuntimeError as e:
+            return handle_runtime_error(e)
 
     def configure_command(self, command, *args):
-        with ContainerRunning(self.container):
-            return self.container.configure_command(command, *args)
+        try:
+            with ContainerRunning(self.container):
+                return self.container.configure_command(command, *args)
+        except RuntimeError as e:
+            return handle_runtime_error(e)

=== modified file 'tools/libertine-container-manager'
--- tools/libertine-container-manager	2016-04-14 17:05:21 +0000
+++ tools/libertine-container-manager	2016-04-18 15:21:37 +0000
@@ -421,7 +421,10 @@
     container = LibertineContainer(args.id)
     update_package_install_status(args.id, args.package, "removing")
 
-    container.remove_package(args.package, args.verbosity)
+    if not container.remove_package(args.package, args.verbosity):
+        update_package_install_status(args.id, package, "installed")
+        sys.exit(1)
+
     update_package_install_status(args.id, args.package, "removed")
 
     delete_package(args.id, args.package)
@@ -435,7 +438,8 @@
         args.id = get_default_container_id()
 
     container = LibertineContainer(args.id)
-    container.search_package_cache(args.search_string)
+    if not container.search_package_cache(args.search_string):
+        sys.exit(1)
 
 
 def update(args):
@@ -448,7 +452,10 @@
     container = LibertineContainer(args.id)
 
     update_container_install_status(args.id, "updating")
-    container.update_libertine_container(args.verbosity)
+    if not container.update_libertine_container(args.verbosity):
+        update_container_install_status(args.id, "ready")
+        sys.exit(1)
+
     update_container_install_status(args.id, "ready")
 
 
@@ -500,7 +507,9 @@
             print("i386 multiarch support is already %s" % multiarch)
             sys.exit(1)
 
-        container.configure_command('multiarch', args.multiarch)
+        if not container.configure_command('multiarch', args.multiarch):
+            sys.exit(1)
+
         update_container_multiarch_support(args.id, multiarch)
 
     elif args.add_archive: