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

Proposed by Michael Vogt on 2014-10-08
Status: Merged
Approved by: Michael Vogt on 2015-02-18
Approved revision: 437
Merged at revision: 559
Proposed branch: lp:~mvo/click/lp1232130-kill-on-remove-2
Merge into: lp:click/devel
Diff against target: 232 lines (+99/-35)
4 files modified
click/tests/helpers.py (+21/-0)
click/tests/test_user.py (+22/-8)
lib/click/user.vala (+48/-27)
pk-plugin/pk-plugin-click.c (+8/-0)
To merge this branch: bzr merge lp:~mvo/click/lp1232130-kill-on-remove-2
Reviewer Review Type Date Requested Status
Barry Warsaw 2014-10-08 Approve on 2015-02-17
PS Jenkins bot (community) continuous-integration Needs Fixing on 2014-10-08
Review via email: mp+237601@code.launchpad.net

Commit message

When uninstalling a app, stop it if its running.

Description of the change

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.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :

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) wrote :

One minor comment, but otherwise LGTM.

review: Approve
Michael Vogt (mvo) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'click/tests/helpers.py'
2--- click/tests/helpers.py 2014-09-29 13:23:52 +0000
3+++ click/tests/helpers.py 2014-10-08 14:13:24 +0000
4@@ -30,6 +30,7 @@
5
6 import contextlib
7 from functools import wraps
8+import json
9 import os
10 import re
11 import shutil
12@@ -59,6 +60,26 @@
13 return wrapper
14
15
16+def make_installed_click(db, db_dir, package="test-1", version="1.0",
17+ json_data={}, make_current=True, user="@all"):
18+ """Create a fake installed click package for the given db/db_dir"""
19+ json_data["name"] = package
20+ json_data["version"] = version
21+ with mkfile_utf8(os.path.join(
22+ db_dir, package, version, ".click", "info",
23+ "%s.manifest" % package)) as f:
24+ json.dump(json_data, f, ensure_ascii=False)
25+ if make_current:
26+ os.symlink(
27+ version, os.path.join(db_dir, package, "current"))
28+ if user == "@all":
29+ registry = Click.User.for_all_users(db)
30+ else:
31+ registry = Click.User.for_user(db, user)
32+ registry.set_version(package, version)
33+ return os.path.join(db_dir, package, version)
34+
35+
36 class TestCase(gimock.GIMockTestCase):
37 def setUp(self):
38 super(TestCase, self).setUp()
39
40=== modified file 'click/tests/test_user.py'
41--- click/tests/test_user.py 2014-09-29 11:12:52 +0000
42+++ click/tests/test_user.py 2014-10-08 14:13:24 +0000
43@@ -34,6 +34,7 @@
44 from click.tests.gimock_types import Passwd
45 from click.tests.helpers import (
46 TestCase,
47+ make_installed_click,
48 mkfile,
49 make_file_with_content,
50 touch,
51@@ -557,21 +558,34 @@
52 self.assertEqual(b_overlay, registry.get_path("b"))
53 self.assertTrue(registry.is_removable("b"))
54
55- def test_app_stops_on_remove(self):
56+
57+class StopAppTestCase(TestCase):
58+
59+ def setUp(self):
60+ super(StopAppTestCase, self).setUp()
61+ self.use_temp_dir()
62+ self.db = Click.DB()
63+ self.db.add(self.temp_dir)
64+
65+ # setup fake app_stop
66 fake_app_stop = os.path.join(self.temp_dir, "bin", "ubuntu-app-stop")
67- fake_app_stop_output = os.path.join(self.temp_dir, "fake-app-stop.out")
68+ self.fake_app_stop_output = os.path.join(
69+ self.temp_dir, "fake-app-stop.out")
70 fake_app_stop_content = dedent("""\
71 #!/bin/sh
72 echo "$@" >> %s
73- """ % fake_app_stop_output)
74+ """ % self.fake_app_stop_output)
75 make_file_with_content(fake_app_stop, fake_app_stop_content, 0o755)
76 # its ok to modify env here, click.helpers.TestCase will take care
77 # of it
78 os.environ["PATH"] = "%s:%s" % (
79 os.path.dirname(fake_app_stop), os.environ["PATH"])
80- # get a app with manifest etc all
81- _, registry = self._setUpMultiDB()
82- registry.remove("a")
83+
84+ def test_app_stops_on_remove(self):
85+ make_installed_click(self.db, self.temp_dir, "meep", "2.0",
86+ {"hooks": {"a-app": {}}})
87+ registry = Click.User.for_user(self.db, "user")
88+ registry.remove("meep")
89 # ensure that stop was called with the right app
90- with open(fake_app_stop_output) as f:
91- self.assertEqual("a_a-app_1.1", f.read().strip())
92+ with open(self.fake_app_stop_output) as f:
93+ self.assertEqual("meep_a-app_2.0", f.read().strip())
94
95=== modified file 'lib/click/user.vala'
96--- lib/click/user.vala 2014-09-29 13:23:12 +0000
97+++ lib/click/user.vala 2014-10-08 14:13:24 +0000
98@@ -598,8 +598,49 @@
99 old_version, version, name);
100 }
101
102- private bool
103- stop_running_app (string package, string version)
104+ private string
105+ get_dbus_session_bus_env_for_current_user()
106+ {
107+ string euid = "%i".printf((int)(Posix.geteuid ()));
108+ var dbus_session_file = Path.build_filename(
109+ "/run", "user", euid, "dbus-session");
110+ string session_env;
111+ try {
112+ FileUtils.get_contents(dbus_session_file, out session_env);
113+ session_env = session_env.strip();
114+ } catch (Error e) {
115+ warning("Can not get the dbus session to stop app (%s)", e.message);
116+ }
117+ return session_env;
118+ }
119+
120+ private bool
121+ stop_single_app (string app_id)
122+ {
123+ // get the users dbus session when we run as root first as this
124+ // is where ubuntu-app-stop listens
125+ string[] envp = Environ.get();
126+ envp += get_dbus_session_bus_env_for_current_user();
127+
128+ string[] command = {
129+ "ubuntu-app-stop", app_id
130+ };
131+ bool res = false;
132+ try {
133+ int exit_status;
134+ Process.spawn_sync
135+ (null, command, envp,
136+ SpawnFlags.SEARCH_PATH,
137+ null, null, null, out exit_status);
138+ res = Process.check_exit_status (exit_status);
139+ } catch (Error e) {
140+ res = false;
141+ }
142+ return res;
143+ }
144+
145+ private bool
146+ stop_running_apps_for_package (string package, string version)
147 {
148 var res = true;
149 if (! find_on_path ("ubuntu-app-stop"))
150@@ -619,24 +660,7 @@
151 }
152 var hooks = manifest.get_object_member ("hooks");
153 foreach (unowned string app_name in hooks.get_members ())
154- {
155- // FIXME: move this into a "stop_single_app" helper
156- string[] command = {
157- "ubuntu-app-stop",
158- @"$(package)_$(app_name)_$(version)"
159- };
160- try {
161- int exit_status;
162- Process.spawn_sync
163- (null, command, null,
164- SpawnFlags.SEARCH_PATH |
165- SpawnFlags.STDOUT_TO_DEV_NULL,
166- null, null, null, out exit_status);
167- res &= Process.check_exit_status (exit_status);
168- } catch (Error e) {
169- res &= false;
170- }
171- }
172+ res &= stop_single_app (@"$(package)_$(app_name)_$(version)");
173 return res;
174 }
175
176@@ -658,6 +682,8 @@
177 old_version = Path.get_basename (target);
178 drop_privileges ();
179 try {
180+ // stop before removing the path to the manifest
181+ stop_running_apps_for_package (package, old_version);
182 unlink_force (path);
183 } finally {
184 regain_privileges ();
185@@ -673,19 +699,14 @@
186 ensure_db ();
187 drop_privileges ();
188 try {
189+ // stop before removing the path to the manifest
190+ stop_running_apps_for_package (package, old_version);
191 symlink_force (HIDDEN_VERSION, path);
192 } finally {
193 regain_privileges ();
194 }
195 }
196
197- drop_privileges ();
198- try {
199- stop_running_app (package, old_version);
200- } finally {
201- regain_privileges ();
202- }
203-
204 if (! is_pseudo_user)
205 package_remove_hooks (db, package, old_version, name);
206 }
207
208=== modified file 'pk-plugin/pk-plugin-click.c'
209--- pk-plugin/pk-plugin-click.c 2014-09-02 08:44:39 +0000
210+++ pk-plugin/pk-plugin-click.c 2014-10-08 14:13:24 +0000
211@@ -637,6 +637,11 @@
212 GError *error = NULL;
213 gchar *summary = NULL;
214
215+ // PK does not set a PATH, but we need one for removal
216+ const gchar *old_path = g_getenv("PATH");
217+ if(old_path == NULL)
218+ g_setenv("PATH", DEFAULT_PATH, 0);
219+
220 username = click_get_username_for_uid
221 (pk_transaction_get_uid (transaction));
222 if (!username) {
223@@ -701,6 +706,9 @@
224 ret = TRUE;
225
226 out:
227+ if(old_path == NULL)
228+ g_unsetenv("PATH");
229+
230 g_free (summary);
231 if (error)
232 g_error_free (error);

Subscribers

People subscribed via source and target branches

to all changes: