Merge lp:~cjwatson/ubuntu-app-launch/libclick-pkgdir into lp:ubuntu-app-launch/14.04

Proposed by Colin Watson on 2014-03-07
Status: Merged
Approved by: Charles Kerr on 2014-03-07
Approved revision: 138
Merged at revision: 140
Proposed branch: lp:~cjwatson/ubuntu-app-launch/libclick-pkgdir
Merge into: lp:ubuntu-app-launch/14.04
Diff against target: 389 lines (+91/-75)
8 files modified
CMakeLists.txt (+5/-2)
click-exec.c (+44/-33)
debian/control (+1/-0)
desktop-hook.c (+27/-34)
tests/CMakeLists.txt (+3/-0)
tests/click (+0/-3)
tests/click-db-dir/test.conf.in (+2/-0)
tests/exec-util-test.cc (+9/-3)
To merge this branch: bzr merge lp:~cjwatson/ubuntu-app-launch/libclick-pkgdir
Reviewer Review Type Date Requested Status
Charles Kerr (community) 2014-03-07 Approve on 2014-03-07
PS Jenkins bot (community) continuous-integration Approve on 2014-03-07
Review via email: mp+209909@code.launchpad.net

Commit message

Use libclick to get the package directory, saving about 0.7 seconds from Click application startup (on mako).

Description of the change

This branch converts upstart-app-launch to using libclick to get the package directory. (It still forks the click binary to get the manifest, because libclick doesn't yet have appropriate functions for that; I'm working on that and should be able to submit a follow-up branch soon.)

To make this work properly, I had to give click-exec environment variables to look at a different Click database directory, and rearrange some test files to be structured a bit more like a real Click database. Ted said on IRC that adding these environment variables should be fine from the point of view of security as all a user can do is "compromise" their own shell.

I've tested this manually on mako after building on porter-armhf.c.c, and run some timings. With upstart-app-launch 0.3+14.04.20140220-0ubuntu3, the median of nine runs of "time upstart-app-launch com.ubuntu.calculator_calculator_0.1.3.224" (followed by "upstart-app-stop com.ubuntu.calculator_calculator_0.1.3.224" to clean up) was 1.772 seconds. With this change, the same measurement returns 1.037 seconds. This is therefore a saving of a bit over 0.7 seconds here, or a little over 40% of upstart-app-launch's run-time.

To post a comment you must log in.
Charles Kerr (charlesk) wrote :

Colin, thanks for this, a 40% improvement is huge news! :-)

In the code itself, there are a couple of trivial issues you might want to fix:

 * in click-exec.c around line 98, need to unref the db before returning if click_user_new_for_user() fails.

 * in click-exec.c around line 103, need to unref the db and the user if click_user_get_path() fails.

138. By Colin Watson on 2014-03-07

Fix some reference leaks.

Colin Watson (cjwatson) wrote :

Ah yes, of course. Pushed a fix, thanks.

Charles Kerr (charlesk) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2014-02-13 22:43:55 +0000
3+++ CMakeLists.txt 2014-03-07 14:19:16 +0000
4@@ -52,6 +52,9 @@
5 pkg_check_modules(ZEITGEIST REQUIRED zeitgeist-2.0)
6 include_directories(${ZEITGEIST_INCLUDE_DIRS})
7
8+pkg_check_modules(CLICK REQUIRED click-0.4)
9+include_directories(${CLICK_INCLUDE_DIRS})
10+
11 pkg_check_modules(LIBUPSTART REQUIRED libupstart)
12 include_directories(${LIBUPSTART_INCLUDE_DIRS})
13
14@@ -92,7 +95,7 @@
15 add_lttng_gen_tp(NAME click-exec-trace)
16 add_executable(click-exec click-exec.c click-exec-trace.c)
17 set_target_properties(click-exec PROPERTIES OUTPUT_NAME "click-exec")
18-target_link_libraries(click-exec helpers ${LTTNG_LIBRARIES})
19+target_link_libraries(click-exec helpers ${LTTNG_LIBRARIES} ${CLICK_LIBRARIES})
20 install(TARGETS click-exec RUNTIME DESTINATION "${pkglibexecdir}")
21
22 ####################
23@@ -101,7 +104,7 @@
24
25 add_executable(desktop-hook desktop-hook.c)
26 set_target_properties(desktop-hook PROPERTIES OUTPUT_NAME "desktop-hook")
27-target_link_libraries(desktop-hook helpers)
28+target_link_libraries(desktop-hook helpers ${CLICK_LIBRARIES})
29 install(TARGETS desktop-hook RUNTIME DESTINATION "${pkglibexecdir}")
30
31 ####################
32
33=== modified file 'click-exec.c'
34--- click-exec.c 2014-02-07 14:56:27 +0000
35+++ click-exec.c 2014-03-07 14:19:16 +0000
36@@ -18,6 +18,7 @@
37 */
38
39 #include <gio/gio.h>
40+#include <click.h>
41 #include "helpers.h"
42 #include "click-exec-trace.h"
43
44@@ -34,7 +35,7 @@
45
46 For information on Click packages and the manifest look at the Click package documentation:
47
48-https://click-package.readthedocs.org/en/latest/
49+https://click.readthedocs.org/en/latest/
50
51 */
52
53@@ -80,45 +81,55 @@
54 }
55
56 /* Check click to find out where the files are */
57- gchar * cmdline = g_strdup_printf("click pkgdir \"%s\"", package);
58-
59- gchar * output = NULL;
60- g_spawn_command_line_sync(cmdline, &output, NULL, NULL, &error);
61- g_free(cmdline);
62+ ClickDB * db = click_db_new();
63+ /* If TEST_CLICK_DB is unset, this reads the system database. */
64+ click_db_read(db, g_getenv("TEST_CLICK_DB"), &error);
65+ if (error != NULL) {
66+ g_warning("Unable to read Click database: %s", error->message);
67+ g_error_free(error);
68+ g_free(package);
69+ return 1;
70+ }
71+ /* If TEST_CLICK_USER is unset, this uses the current user name. */
72+ ClickUser * user = click_user_new_for_user(db, g_getenv("TEST_CLICK_USER"), &error);
73+ if (error != NULL) {
74+ g_warning("Unable to read Click database: %s", error->message);
75+ g_error_free(error);
76+ g_free(package);
77+ g_object_unref(db);
78+ return 1;
79+ }
80+ gchar * pkgdir = click_user_get_path(user, package, &error);
81+ if (error != NULL) {
82+ g_warning("Unable to get the Click package directory for %s: %s", package, error->message);
83+ g_error_free(error);
84+ g_free(package);
85+ g_object_unref(user);
86+ g_object_unref(db);
87+ return 1;
88+ }
89+ g_object_unref(user);
90+ g_object_unref(db);
91
92 tracepoint(upstart_app_launch, click_found_pkgdir);
93
94- /* If we have an extra newline, we can delete it. */
95- gchar * newline = g_strstr_len(output, -1, "\n");
96- if (newline != NULL) {
97- newline[0] = '\0';
98- }
99-
100- if (error != NULL) {
101- g_warning("Unable to get the package directory from click: %s", error->message);
102- g_error_free(error);
103- g_free(output); /* Probably not set, but just in case */
104- g_free(package);
105- return 1;
106- }
107-
108- if (!g_file_test(output, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) {
109- g_warning("Application directory '%s' doesn't exist", output);
110- g_free(output);
111- g_free(package);
112- return 1;
113- }
114-
115- g_debug("Setting 'APP_DIR' to '%s'", output);
116- set_upstart_variable("APP_DIR", output, FALSE);
117-
118- set_confined_envvars(package, output);
119+ if (!g_file_test(pkgdir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) {
120+ g_warning("Application directory '%s' doesn't exist", pkgdir);
121+ g_free(pkgdir);
122+ g_free(package);
123+ return 1;
124+ }
125+
126+ g_debug("Setting 'APP_DIR' to '%s'", pkgdir);
127+ set_upstart_variable("APP_DIR", pkgdir, FALSE);
128+
129+ set_confined_envvars(package, pkgdir);
130
131 tracepoint(upstart_app_launch, click_configured_env);
132
133- gchar * desktopfile = manifest_to_desktop(output, app_id);
134+ gchar * desktopfile = manifest_to_desktop(pkgdir, app_id);
135
136- g_free(output);
137+ g_free(pkgdir);
138 g_free(package);
139
140 if (desktopfile == NULL) {
141
142=== modified file 'debian/control'
143--- debian/control 2014-02-11 03:14:30 +0000
144+++ debian/control 2014-03-07 14:19:16 +0000
145@@ -7,6 +7,7 @@
146 dbus-x11,
147 dbus-test-runner,
148 debhelper (>= 9),
149+ libclick-0.4-dev,
150 libdbus-1-dev,
151 libdbustest1-dev (>= 14.04.0),
152 libgirepository1.0-dev,
153
154=== modified file 'desktop-hook.c'
155--- desktop-hook.c 2013-09-13 16:30:01 +0000
156+++ desktop-hook.c 2014-03-07 14:19:16 +0000
157@@ -24,7 +24,7 @@
158 This is a hook for Click packages. You can find information on Click package hooks in
159 the click documentation:
160
161-https://click-package.readthedocs.org/en/latest/
162+https://click.readthedocs.org/en/latest/
163
164 Probably the biggest thing to understand for how this code works is that you need to
165 understand that this hook is run after one, or many packages are installed. A set of
166@@ -44,6 +44,7 @@
167
168 #include <gio/gio.h>
169 #include <glib/gstdio.h>
170+#include <click.h>
171 #include <string.h>
172 #include <errno.h>
173
174@@ -390,40 +391,32 @@
175 }
176
177 /* Check click to find out where the files are */
178- gchar * cmdline = g_strdup_printf("click pkgdir \"%s\"", package);
179+ ClickUser * user = click_user_new_for_user(NULL, NULL, &error);
180+ if (error != NULL) {
181+ g_warning("Unable to read Click database: %s", error->message);
182+ g_error_free(error);
183+ g_free(package);
184+ return;
185+ }
186+ gchar * pkgdir = click_user_get_path(user, package, &error);
187+ if (error != NULL) {
188+ g_warning("Unable to get the Click package directory for %s: %s", package, error->message);
189+ g_error_free(error);
190+ g_free(package);
191+ return;
192+ }
193+ g_object_unref(user);
194 g_free(package);
195
196- gchar * output = NULL;
197- g_spawn_command_line_sync(cmdline, &output, NULL, NULL, &error);
198- g_free(cmdline);
199-
200- /* If we have an extra newline, we can hide it. */
201- if (output != NULL) {
202- gchar * newline = NULL;
203-
204- newline = g_strstr_len(output, -1, "\n");
205-
206- if (newline != NULL) {
207- newline[0] = '\0';
208- }
209- }
210-
211- if (error != NULL) {
212- g_warning("Unable to get the package directory from click: %s", error->message);
213- g_error_free(error);
214- g_free(output); /* Probably not set, but just in case */
215- return;
216- }
217-
218- if (!g_file_test(output, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) {
219- g_warning("Directory returned by click '%s' couldn't be found", output);
220- g_free(output);
221- return;
222- }
223-
224- gchar * indesktop = manifest_to_desktop(output, state->app_id);
225+ if (!g_file_test(pkgdir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) {
226+ g_warning("Directory returned by click '%s' couldn't be found", pkgdir);
227+ g_free(pkgdir);
228+ return;
229+ }
230+
231+ gchar * indesktop = manifest_to_desktop(pkgdir, state->app_id);
232 if (indesktop == NULL) {
233- g_free(output);
234+ g_free(pkgdir);
235 return;
236 }
237
238@@ -432,11 +425,11 @@
239 gchar * desktoppath = g_build_filename(desktopdir, desktopfile, NULL);
240 g_free(desktopfile);
241
242- copy_desktop_file(indesktop, desktoppath, output, state->app_id);
243+ copy_desktop_file(indesktop, desktoppath, pkgdir, state->app_id);
244
245 g_free(desktoppath);
246 g_free(indesktop);
247- g_free(output);
248+ g_free(pkgdir);
249
250 return;
251 }
252
253=== modified file 'tests/CMakeLists.txt'
254--- tests/CMakeLists.txt 2014-01-22 15:47:41 +0000
255+++ tests/CMakeLists.txt 2014-03-07 14:19:16 +0000
256@@ -1,6 +1,9 @@
257
258 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
259
260+configure_file ("click-db-dir/test.conf.in" "${CMAKE_CURRENT_BINARY_DIR}/click-db-dir/test.conf" @ONLY)
261+set_directory_properties (PROPERTIES ADDITIONAL_MAKE_CLEAN_FILES "${CMAKE_CURRENT_BINARY_DIR}/click-db-dir/test.conf")
262+
263 # Google Test
264
265 include_directories(${GTEST_INCLUDE_DIR})
266
267=== modified file 'tests/click'
268--- tests/click 2013-12-13 20:32:49 +0000
269+++ tests/click 2014-03-07 14:19:16 +0000
270@@ -2,9 +2,6 @@
271
272 if [ ${1} == "info" ] ; then
273 /bin/cat "${XDG_DATA_DIRS}/click-app-dir/.click/info/${2}.manifest"
274-else if [ ${1} == "pkgdir" ] ; then
275- echo "${XDG_DATA_DIRS}/click-app-dir"
276 else
277 exit 1
278 fi
279-fi
280
281=== added directory 'tests/click-db-dir'
282=== added file 'tests/click-db-dir/test.conf.in'
283--- tests/click-db-dir/test.conf.in 1970-01-01 00:00:00 +0000
284+++ tests/click-db-dir/test.conf.in 2014-03-07 14:19:16 +0000
285@@ -0,0 +1,2 @@
286+[Click Database]
287+root = @CMAKE_CURRENT_SOURCE_DIR@/click-root-dir
288
289=== added directory 'tests/click-root-dir'
290=== added directory 'tests/click-root-dir/.click'
291=== added directory 'tests/click-root-dir/.click/users'
292=== added directory 'tests/click-root-dir/.click/users/test-user'
293=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.bad-version'
294=== target is u'../../../com.test.bad-version/1.2.3'
295=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.good'
296=== target is u'../../../com.test.good/1.2.3'
297=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.multiple'
298=== target is u'../../../com.test.multiple/1.2.3'
299=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-app'
300=== target is u'../../../com.test.no-app/1.2.3'
301=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-hooks'
302=== target is u'../../../com.test.no-hooks/1.2.3'
303=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-json'
304=== target is u'../../../com.test.no-json/1.2.3'
305=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-object'
306=== target is u'../../../com.test.no-object/1.2.3'
307=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-version'
308=== target is u'../../../com.test.no-version/1.2.3'
309=== added directory 'tests/click-root-dir/com.test.bad-version'
310=== added symlink 'tests/click-root-dir/com.test.bad-version/1.2.3'
311=== target is u'../../click-app-dir'
312=== added directory 'tests/click-root-dir/com.test.good'
313=== added symlink 'tests/click-root-dir/com.test.good/1.2.3'
314=== target is u'../../click-app-dir'
315=== added directory 'tests/click-root-dir/com.test.multiple'
316=== added symlink 'tests/click-root-dir/com.test.multiple/1.2.3'
317=== target is u'../../click-app-dir'
318=== added directory 'tests/click-root-dir/com.test.no-app'
319=== added symlink 'tests/click-root-dir/com.test.no-app/1.2.3'
320=== target is u'../../click-app-dir'
321=== added directory 'tests/click-root-dir/com.test.no-hooks'
322=== added symlink 'tests/click-root-dir/com.test.no-hooks/1.2.3'
323=== target is u'../../click-app-dir'
324=== added directory 'tests/click-root-dir/com.test.no-json'
325=== added symlink 'tests/click-root-dir/com.test.no-json/1.2.3'
326=== target is u'../../click-app-dir'
327=== added directory 'tests/click-root-dir/com.test.no-object'
328=== added symlink 'tests/click-root-dir/com.test.no-object/1.2.3'
329=== target is u'../../click-app-dir'
330=== added directory 'tests/click-root-dir/com.test.no-version'
331=== added symlink 'tests/click-root-dir/com.test.no-version/1.2.3'
332=== target is u'../../click-app-dir'
333=== modified file 'tests/exec-util-test.cc'
334--- tests/exec-util-test.cc 2013-12-17 20:29:43 +0000
335+++ tests/exec-util-test.cc 2014-03-07 14:19:16 +0000
336@@ -90,6 +90,8 @@
337 DbusTestDbusMockObject * obj = dbus_test_dbus_mock_get_object(mock, "/com/ubuntu/Upstart", "com.ubuntu.Upstart0_6", NULL);
338
339 g_setenv("APP_ID", "com.test.good_application_1.2.3", TRUE);
340+ g_setenv("TEST_CLICK_DB", "click-db-dir", TRUE);
341+ g_setenv("TEST_CLICK_USER", "test-user", TRUE);
342
343 g_spawn_command_line_sync(CLICK_EXEC_TOOL, NULL, NULL, NULL, NULL);
344
345@@ -114,6 +116,8 @@
346 bool got_app_desktop = false;
347 bool got_app_desktop_path = false;
348
349+#define APP_DIR CMAKE_SOURCE_DIR "/click-root-dir/.click/users/test-user/com.test.good"
350+
351 for (i = 0; i < len; i++) {
352 EXPECT_STREQ("SetEnv", calls[i].name);
353
354@@ -139,7 +143,7 @@
355 } else if (g_strcmp0(var, "XDG_RUNTIME_DIR") == 0) {
356 got_runtime_dir = true;
357 } else if (g_strcmp0(var, "XDG_DATA_DIRS") == 0) {
358- EXPECT_TRUE(g_str_has_prefix(value, CMAKE_SOURCE_DIR "/click-app-dir:"));
359+ EXPECT_TRUE(g_str_has_prefix(value, APP_DIR ":"));
360 got_data_dirs = true;
361 } else if (g_strcmp0(var, "TMPDIR") == 0) {
362 EXPECT_TRUE(g_str_has_suffix(value, "com.test.good"));
363@@ -148,7 +152,7 @@
364 EXPECT_TRUE(g_str_has_suffix(value, "com.test.good"));
365 got_shader_dir = true;
366 } else if (g_strcmp0(var, "APP_DIR") == 0) {
367- EXPECT_STREQ(CMAKE_SOURCE_DIR "/click-app-dir", value);
368+ EXPECT_STREQ(APP_DIR, value);
369 got_app_dir = true;
370 } else if (g_strcmp0(var, "APP_EXEC") == 0) {
371 EXPECT_STREQ("foo", value);
372@@ -156,7 +160,7 @@
373 } else if (g_strcmp0(var, "APP_DESKTOP_FILE") == 0) {
374 got_app_desktop = true;
375 } else if (g_strcmp0(var, "APP_DESKTOP_FILE_PATH") == 0) {
376- EXPECT_STREQ(CMAKE_SOURCE_DIR "/click-app-dir/application.desktop", value);
377+ EXPECT_STREQ(APP_DIR "/application.desktop", value);
378 got_app_desktop_path = true;
379 } else {
380 g_warning("Unknown variable! %s", var);
381@@ -166,6 +170,8 @@
382 g_free(var);
383 }
384
385+#undef APP_DIR
386+
387 EXPECT_TRUE(got_app_isolation);
388 EXPECT_TRUE(got_cache_home);
389 EXPECT_TRUE(got_config_home);

Subscribers

People subscribed via source and target branches