Merge lp:~mvo/click/lp1232130-kill-on-remove-2 into lp:click/devel

[Michael Vogt]

When uninstalling a app, stop it if its running.

This branch stops running applications when a click package gets removed. The previous branch did not work because the ubuntu-app-stop command needs access to the session bus to work but click/PK run in a different context. Fortunately the users session bus address is available in /run/user which this branch uses now.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:437
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~mvo/click/lp1232130-kill-on-remove-2/+merge/237601/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/click-devel-ci/92/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-amd64-ci/94
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/92
        deb: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/92/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-i386-ci/92

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/click-devel-ci/92/rebuild

review: Needs Fixing (continuous-integration)

[Barry Warsaw (barry)]

One minor comment, but otherwise LGTM.

review: Approve

[Michael Vogt (mvo)]

On Tue, Feb 17, 2015 at 04:15:44PM -0000, Barry Warsaw wrote:
> Review: Approve
>
> One minor comment, but otherwise LGTM.

Thanks for the review!

[..]
> > +class StopAppTestCase(TestCase):
> > +
> > + def setUp(self):
> > + super(StopAppTestCase, self).setUp()
>
> I forget, does click still have to be Python 2 compatible? If not, then:
>
> super().setUp()
>
> because magic!

Woah, thats cool. I will keep that in mind :) We currently still
support py2.7 unfortunately. Might be worth reconsidering - but not
for this one line :)

Cheers,
 Michael

=== modified file 'click/tests/helpers.py'
--- click/tests/helpers.py	2014-09-29 13:23:52 +0000
+++ click/tests/helpers.py	2014-10-08 14:13:24 +0000
@@ -30,6 +30,7 @@
 
 import contextlib
 from functools import wraps
+import json
 import os
 import re
 import shutil
@@ -59,6 +60,26 @@
     return wrapper
 
 
+def make_installed_click(db, db_dir, package="test-1", version="1.0",
+                         json_data={}, make_current=True, user="@all"):
+    """Create a fake installed click package for the given db/db_dir"""
+    json_data["name"] = package
+    json_data["version"] = version
+    with mkfile_utf8(os.path.join(
+            db_dir, package, version, ".click", "info",
+            "%s.manifest" % package)) as f:
+        json.dump(json_data, f, ensure_ascii=False)
+    if make_current:
+        os.symlink(
+            version, os.path.join(db_dir, package, "current"))
+    if user == "@all":
+        registry = Click.User.for_all_users(db)
+    else:
+        registry = Click.User.for_user(db, user)
+    registry.set_version(package, version)
+    return os.path.join(db_dir, package, version)
+
+
 class TestCase(gimock.GIMockTestCase):
     def setUp(self):
         super(TestCase, self).setUp()
 
=== modified file 'click/tests/test_user.py'
--- click/tests/test_user.py	2014-09-29 11:12:52 +0000
+++ click/tests/test_user.py	2014-10-08 14:13:24 +0000
@@ -34,6 +34,7 @@
 from click.tests.gimock_types import Passwd
 from click.tests.helpers import (
     TestCase,
+    make_installed_click,
     mkfile,
     make_file_with_content,
     touch,
@@ -557,21 +558,34 @@
         self.assertEqual(b_overlay, registry.get_path("b"))
         self.assertTrue(registry.is_removable("b"))
 
-    def test_app_stops_on_remove(self):
+
+class StopAppTestCase(TestCase):
+
+    def setUp(self):
+        super(StopAppTestCase, self).setUp()
+        self.use_temp_dir()
+        self.db = Click.DB()
+        self.db.add(self.temp_dir)
+
+        # setup fake app_stop
         fake_app_stop = os.path.join(self.temp_dir, "bin", "ubuntu-app-stop")
-        fake_app_stop_output = os.path.join(self.temp_dir, "fake-app-stop.out")
+        self.fake_app_stop_output = os.path.join(
+            self.temp_dir, "fake-app-stop.out")
         fake_app_stop_content = dedent("""\
         #!/bin/sh
         echo "$@" >> %s
-        """ % fake_app_stop_output)
+        """ % self.fake_app_stop_output)
         make_file_with_content(fake_app_stop, fake_app_stop_content, 0o755)
         # its ok to modify env here, click.helpers.TestCase will take care
         # of it
         os.environ["PATH"] = "%s:%s" % (
             os.path.dirname(fake_app_stop), os.environ["PATH"])
-        # get a app with manifest etc all
-        _, registry = self._setUpMultiDB()
-        registry.remove("a")
+
+    def test_app_stops_on_remove(self):
+        make_installed_click(self.db, self.temp_dir, "meep", "2.0",
+                             {"hooks": {"a-app": {}}})
+        registry = Click.User.for_user(self.db, "user")
+        registry.remove("meep")
         # ensure that stop was called with the right app
-        with open(fake_app_stop_output) as f:
-            self.assertEqual("a_a-app_1.1", f.read().strip())
+        with open(self.fake_app_stop_output) as f:
+            self.assertEqual("meep_a-app_2.0", f.read().strip())
 
=== modified file 'lib/click/user.vala'
--- lib/click/user.vala	2014-09-29 13:23:12 +0000
+++ lib/click/user.vala	2014-10-08 14:13:24 +0000
@@ -598,8 +598,49 @@
 					       old_version, version, name);
 	}
 
-	private bool
-	stop_running_app (string package, string version)
+	private string
+	get_dbus_session_bus_env_for_current_user()
+	{
+		string euid = "%i".printf((int)(Posix.geteuid ()));
+		var dbus_session_file = Path.build_filename(
+			"/run", "user", euid, "dbus-session");
+		string session_env;
+		try {
+			FileUtils.get_contents(dbus_session_file, out session_env);
+			session_env = session_env.strip();
+		} catch (Error e) {
+			warning("Can not get the dbus session to stop app (%s)", e.message);
+		}
+		return session_env;
+	}
+
+	private bool
+	stop_single_app (string app_id)
+	{
+		// get the users dbus session when we run as root first as this
+		// is where ubuntu-app-stop listens
+		string[] envp = Environ.get();
+		envp += get_dbus_session_bus_env_for_current_user();
+
+		string[] command = {
+			"ubuntu-app-stop", app_id
+		};
+		bool res = false;
+		try {
+			int exit_status;
+			Process.spawn_sync
+			(null, command, envp,
+			 SpawnFlags.SEARCH_PATH,
+			 null, null, null, out exit_status);
+			res = Process.check_exit_status (exit_status);
+		} catch (Error e) {
+			res = false;
+		}
+		return res;
+	}
+
+	private bool
+	stop_running_apps_for_package (string package, string version)
 	{
 		var res = true;
 		if (! find_on_path ("ubuntu-app-stop"))
@@ -619,24 +660,7 @@
 		}
 		var hooks = manifest.get_object_member ("hooks");
 		foreach (unowned string app_name in hooks.get_members ())
-		{
-			// FIXME: move this into a "stop_single_app" helper
-			string[] command = {
-				"ubuntu-app-stop",
-				@"$(package)_$(app_name)_$(version)"
-			};
-			try {
-				int exit_status;
-				Process.spawn_sync
-					(null, command, null,
-					 SpawnFlags.SEARCH_PATH |
-					 SpawnFlags.STDOUT_TO_DEV_NULL,
-					 null, null, null, out exit_status);
-				res &= Process.check_exit_status (exit_status);
-			} catch (Error e) {
-				res &= false;
-			}
-		}
+			res &= stop_single_app (@"$(package)_$(app_name)_$(version)");
 		return res;
 	}
 
@@ -658,6 +682,8 @@
 			old_version = Path.get_basename (target);
 			drop_privileges ();
 			try {
+				// stop before removing the path to the manifest
+				stop_running_apps_for_package (package, old_version);
 				unlink_force (path);
 			} finally {
 				regain_privileges ();
@@ -673,19 +699,14 @@
 			ensure_db ();
 			drop_privileges ();
 			try {
+				// stop before removing the path to the manifest
+				stop_running_apps_for_package (package, old_version);
 				symlink_force (HIDDEN_VERSION, path);
 			} finally {
 				regain_privileges ();
 			}
 		}
 
-		drop_privileges ();
-		try {
-			stop_running_app (package, old_version);
-		} finally {
-			regain_privileges ();
-		}
-
 		if (! is_pseudo_user)
 			package_remove_hooks (db, package, old_version, name);
 	}
 
=== modified file 'pk-plugin/pk-plugin-click.c'
--- pk-plugin/pk-plugin-click.c	2014-09-02 08:44:39 +0000
+++ pk-plugin/pk-plugin-click.c	2014-10-08 14:13:24 +0000
@@ -637,6 +637,11 @@
 	GError *error = NULL;
 	gchar *summary = NULL;
 
+	// PK does not set a PATH, but we need one for removal
+	const gchar *old_path = g_getenv("PATH");
+	if(old_path == NULL)
+		g_setenv("PATH", DEFAULT_PATH, 0);
+
 	username = click_get_username_for_uid
 		(pk_transaction_get_uid (transaction));
 	if (!username) {
@@ -701,6 +706,9 @@
 	ret = TRUE;
 
 out:
+	if(old_path == NULL)
+		g_unsetenv("PATH");
+
 	g_free (summary);
 	if (error)
 		g_error_free (error);