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

Proposed by Colin Watson on 2014-03-12
Status: Merged
Approved by: Ted Gould on 2014-03-12
Approved revision: 139
Merged at revision: 141
Proposed branch: lp:~cjwatson/ubuntu-app-launch/libclick-manifest
Merge into: lp:ubuntu-app-launch/14.04
Prerequisite: lp:~cjwatson/ubuntu-app-launch/libclick-pkgdir
Diff against target: 417 lines (+83/-117)
9 files modified
CMakeLists.txt (+2/-2)
debian/control (+0/-2)
helpers.c (+33/-35)
libupstart-app-launch/CMakeLists.txt (+1/-0)
libupstart-app-launch/upstart-app-launch.c (+41/-58)
tests/click (+0/-7)
tests/exec-util-test.cc (+0/-4)
tests/helper-test.cc (+3/-4)
tests/libual-test.cc (+3/-5)
To merge this branch: bzr merge lp:~cjwatson/ubuntu-app-launch/libclick-manifest
Reviewer Review Type Date Requested Status
Ted Gould (community) 2014-03-12 Approve on 2014-03-12
PS Jenkins bot (community) continuous-integration Approve on 2014-03-12
Review via email: mp+210520@code.launchpad.net

Commit message

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

Description of the change

This is the other half of the work started in https://code.launchpad.net/~cjwatson/upstart-app-launch/libclick-pkgdir/+merge/209909, now that click 0.4.18 has landed. It converts the remaining parts of upstart-app-launch so that it no longer needs to go through the click binary at all, and does everything click-related using libclick, thereby avoiding the need to start a Python interpreter.

I've tested this manually on mako after building on porter-armhf.c.c, and run some timings. Including previous results, 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) behaves as follows:

  Before any libclick work: 1.772 seconds
  With libclick-pkgdir: 1.037 seconds
  With libclick-pkgdir and libclick-manifest: 0.355 seconds

The combined result of my two branches is therefore a total saving of about 1.4 seconds, or about 80% of upstart-app-launch's run-time.

To post a comment you must log in.
Ted Gould (ted) wrote :

Looks great! Excited about the slightly faster app startup ;-)

review: Approve
Charles Kerr (charlesk) wrote :

This looks great. Thanks Colin!

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-03-12 00:34:52 +0000
3+++ CMakeLists.txt 2014-03-12 00:34:52 +0000
4@@ -52,7 +52,7 @@
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+pkg_check_modules(CLICK REQUIRED click-0.4>=0.4.18)
10 include_directories(${CLICK_INCLUDE_DIRS})
11
12 pkg_check_modules(LIBUPSTART REQUIRED libupstart)
13@@ -76,7 +76,7 @@
14 ####################
15
16 add_library(helpers STATIC helpers.c helpers-keyfile.c)
17-target_link_libraries(helpers ${GIO2_LIBRARIES} ${JSONGLIB_LIBRARIES})
18+target_link_libraries(helpers ${GIO2_LIBRARIES} ${JSONGLIB_LIBRARIES} ${CLICK_LIBRARIES})
19
20 ####################
21 # desktop-exec
22
23=== modified file 'debian/control'
24--- debian/control 2014-03-12 00:34:52 +0000
25+++ debian/control 2014-03-12 00:34:52 +0000
26@@ -33,7 +33,6 @@
27 Architecture: any
28 Depends: ${shlibs:Depends},
29 ${misc:Depends},
30- click (>= 0.4.9),
31 click-apparmor,
32 upstart (>= 1.11),
33 zeitgeist,
34@@ -57,7 +56,6 @@
35 Architecture: any
36 Depends: ${misc:Depends},
37 ${shlibs:Depends},
38- click (>= 0.4.9),
39 Pre-Depends: ${misc:Pre-Depends},
40 Multi-Arch: same
41 Description: library for sending requests to the upstart app launch
42
43=== modified file 'helpers.c'
44--- helpers.c 2014-02-07 14:56:27 +0000
45+++ helpers.c 2014-03-12 00:34:52 +0000
46@@ -18,6 +18,7 @@
47 */
48
49 #include <json-glib/json-glib.h>
50+#include <click.h>
51 #include <upstart.h>
52 #include "helpers.h"
53
54@@ -62,62 +63,54 @@
55 manifest_to_desktop (const gchar * app_dir, const gchar * app_id)
56 {
57 gchar * package = NULL;
58- gchar * output = NULL;
59 gchar * application = NULL;
60 gchar * version = NULL;
61- JsonParser * parser = NULL;
62- GError * error = NULL;
63+ ClickDB * db = NULL;
64+ ClickUser * user = NULL;
65+ JsonObject * manifest = NULL;
66 gchar * desktoppath = NULL;
67+ GError * error = NULL;
68
69 if (!app_id_to_triplet(app_id, &package, &application, &version)) {
70 g_warning("Unable to parse triplet: %s", app_id);
71 return NULL;
72 }
73
74- gchar * cmdline = g_strdup_printf("click info \"%s\"", package);
75- g_spawn_command_line_sync(cmdline, &output, NULL, NULL, &error);
76- g_free(cmdline);
77-
78+ db = click_db_new();
79+ /* If TEST_CLICK_DB is unset, this reads the system database. */
80+ click_db_read(db, g_getenv("TEST_CLICK_DB"), &error);
81+ if (error != NULL) {
82+ g_warning("Unable to read Click database: %s", error->message);
83+ goto manifest_out;
84+ }
85+ /* If TEST_CLICK_USER is unset, this uses the current user name. */
86+ user = click_user_new_for_user(db, g_getenv("TEST_CLICK_USER"), &error);
87+ if (error != NULL) {
88+ g_warning("Unable to read Click database: %s", error->message);
89+ goto manifest_out;
90+ }
91+ manifest = click_user_get_manifest(user, package, &error);
92 if (error != NULL) {
93 g_warning("Unable to get manifest for '%s': %s", package, error->message);
94- g_error_free(error);
95- goto manifest_out;
96- }
97-
98- parser = json_parser_new();
99-
100- json_parser_load_from_data(parser, output, -1, &error);
101- g_free(output);
102-
103- if (error != NULL) {
104- g_warning("Unable to load manifest data '%s': %s", package, error->message);
105- g_error_free(error);
106- goto manifest_out;
107- }
108-
109- JsonNode * root = json_parser_get_root(parser);
110- if (json_node_get_node_type(root) != JSON_NODE_OBJECT) {
111- g_warning("Manifest '%s' doesn't start with an object", package);
112- goto manifest_out;
113- }
114-
115- JsonObject * rootobj = json_node_get_object(root);
116- if (!json_object_has_member(rootobj, "version")) {
117+ goto manifest_out;
118+ }
119+
120+ if (!json_object_has_member(manifest, "version")) {
121 g_warning("Manifest '%s' doesn't have a version", package);
122 goto manifest_out;
123 }
124
125- if (g_strcmp0(json_object_get_string_member(rootobj, "version"), version) != 0) {
126- g_warning("Manifest '%s' version '%s' doesn't match AppID version '%s'", package, json_object_get_string_member(rootobj, "version"), version);
127+ if (g_strcmp0(json_object_get_string_member(manifest, "version"), version) != 0) {
128+ g_warning("Manifest '%s' version '%s' doesn't match AppID version '%s'", package, json_object_get_string_member(manifest, "version"), version);
129 goto manifest_out;
130 }
131
132- if (!json_object_has_member(rootobj, "hooks")) {
133+ if (!json_object_has_member(manifest, "hooks")) {
134 g_warning("Manifest '%s' doesn't have an hooks section", package);
135 goto manifest_out;
136 }
137
138- JsonObject * appsobj = json_object_get_object_member(rootobj, "hooks");
139+ JsonObject * appsobj = json_object_get_object_member(manifest, "hooks");
140 if (appsobj == NULL) {
141 g_warning("Manifest '%s' has an hooks section that is not a JSON object", package);
142 goto manifest_out;
143@@ -151,7 +144,12 @@
144 }
145
146 manifest_out:
147- g_clear_object(&parser);
148+ if (error != NULL)
149+ g_error_free(error);
150+ if (manifest != NULL)
151+ json_object_unref(manifest);
152+ g_clear_object(&user);
153+ g_clear_object(&db);
154 g_free(package);
155 g_free(application);
156 g_free(version);
157
158=== modified file 'libupstart-app-launch/CMakeLists.txt'
159--- libupstart-app-launch/CMakeLists.txt 2014-02-11 03:21:25 +0000
160+++ libupstart-app-launch/CMakeLists.txt 2014-03-12 00:34:52 +0000
161@@ -44,6 +44,7 @@
162 ${GIO2_LIBRARIES}
163 ${LTTNG_LIBRARIES}
164 ${JSONGLIB_LIBRARIES}
165+ ${CLICK_LIBRARIES}
166 -Wl,--no-undefined
167 )
168
169
170=== modified file 'libupstart-app-launch/upstart-app-launch.c'
171--- libupstart-app-launch/upstart-app-launch.c 2014-02-19 21:43:04 +0000
172+++ libupstart-app-launch/upstart-app-launch.c 2014-03-12 00:34:52 +0000
173@@ -19,6 +19,7 @@
174
175 #include "upstart-app-launch.h"
176 #include <json-glib/json-glib.h>
177+#include <click.h>
178 #include <upstart.h>
179 #include <gio/gio.h>
180 #include <string.h>
181@@ -1092,60 +1093,46 @@
182 return TRUE;
183 }
184
185-/* Try and get a manifest file and do a couple sanity checks on it */
186-static JsonParser *
187-get_manifest_file (const gchar * pkg)
188+/* Try and get a manifest and do a couple sanity checks on it */
189+static JsonObject *
190+get_manifest (const gchar * pkg)
191 {
192 /* Get the directory from click */
193 GError * error = NULL;
194- const gchar * click_exec = NULL;
195-
196- if (g_getenv("UAL_CLICK_EXEC") != NULL) {
197- click_exec = g_getenv("UAL_CLICK_EXEC");
198- } else {
199- click_exec = "click";
200- }
201-
202- gchar * cmdline = g_strdup_printf("%s info \"%s\"",
203- click_exec, pkg);
204-
205- gchar * output = NULL;
206- g_spawn_command_line_sync(cmdline, &output, NULL, NULL, &error);
207- g_free(cmdline);
208-
209+
210+ ClickDB * db = click_db_new();
211+ /* If TEST_CLICK_DB is unset, this reads the system database. */
212+ click_db_read(db, g_getenv("TEST_CLICK_DB"), &error);
213+ if (error != NULL) {
214+ g_warning("Unable to read Click database: %s", error->message);
215+ g_error_free(error);
216+ return NULL;
217+ }
218+ /* If TEST_CLICK_USER is unset, this uses the current user name. */
219+ ClickUser * user = click_user_new_for_user(db, g_getenv("TEST_CLICK_USER"), &error);
220+ if (error != NULL) {
221+ g_warning("Unable to read Click database: %s", error->message);
222+ g_error_free(error);
223+ g_object_unref(db);
224+ return NULL;
225+ }
226+ g_object_unref(db);
227+ JsonObject * manifest = click_user_get_manifest(user, pkg, &error);
228 if (error != NULL) {
229 g_warning("Unable to get manifest for '%s' package: %s", pkg, error->message);
230 g_error_free(error);
231- g_free(output);
232- return NULL;
233- }
234-
235- /* Let's look at that manifest file */
236- JsonParser * parser = json_parser_new();
237- json_parser_load_from_data(parser, output, -1, &error);
238- g_free(output);
239-
240- if (error != NULL) {
241- g_warning("Unable to load manifest for '%s': %s", pkg, error->message);
242- g_error_free(error);
243- g_object_unref(parser);
244- return NULL;
245- }
246- JsonNode * root = json_parser_get_root(parser);
247- if (json_node_get_node_type(root) != JSON_NODE_OBJECT) {
248- g_warning("Manifest file for package '%s' does not have an object as its root node", pkg);
249- g_object_unref(parser);
250- return NULL;
251- }
252-
253- JsonObject * rootobj = json_node_get_object(root);
254- if (!json_object_has_member(rootobj, "version")) {
255+ g_object_unref(user);
256+ return NULL;
257+ }
258+ g_object_unref(user);
259+
260+ if (!json_object_has_member(manifest, "version")) {
261 g_warning("Manifest file for package '%s' does not have a version", pkg);
262- g_object_unref(parser);
263+ json_object_unref(manifest);
264 return NULL;
265 }
266
267- return parser;
268+ return manifest;
269 }
270
271 /* Types of search we can do for an app name */
272@@ -1158,7 +1145,7 @@
273
274 /* Figure out the app name if it's one of the keywords */
275 static const gchar *
276-manifest_app_name (JsonParser ** manifest, const gchar * pkg, const gchar * original_app)
277+manifest_app_name (JsonObject ** manifest, const gchar * pkg, const gchar * original_app)
278 {
279 app_name_t app_type = APP_NAME_FIRST;
280
281@@ -1175,12 +1162,10 @@
282 }
283
284 if (*manifest == NULL) {
285- *manifest = get_manifest_file(pkg);
286+ *manifest = get_manifest(pkg);
287 }
288
289- JsonNode * root_node = json_parser_get_root(*manifest);
290- JsonObject * root_obj = json_node_get_object(root_node);
291- JsonObject * hooks = json_object_get_object_member(root_obj, "hooks");
292+ JsonObject * hooks = json_object_get_object_member(*manifest, "hooks");
293
294 if (hooks == NULL) {
295 return NULL;
296@@ -1216,20 +1201,17 @@
297
298 /* Figure out the app version using the manifest */
299 static const gchar *
300-manifest_version (JsonParser ** manifest, const gchar * pkg, const gchar * original_ver)
301+manifest_version (JsonObject ** manifest, const gchar * pkg, const gchar * original_ver)
302 {
303 if (original_ver != NULL && g_strcmp0(original_ver, "current-user-version") != 0) {
304 return original_ver;
305 } else {
306 if (*manifest == NULL) {
307- *manifest = get_manifest_file(pkg);
308+ *manifest = get_manifest(pkg);
309 }
310 g_return_val_if_fail(*manifest != NULL, NULL);
311
312- JsonNode * node = json_parser_get_root(*manifest);
313- JsonObject * obj = json_node_get_object(node);
314-
315- return g_strdup(json_object_get_string_member(obj, "version"));
316+ return g_strdup(json_object_get_string_member(*manifest, "version"));
317 }
318
319 return NULL;
320@@ -1242,7 +1224,7 @@
321
322 const gchar * version = NULL;
323 const gchar * application = NULL;
324- JsonParser * manifest = NULL;
325+ JsonObject * manifest = NULL;
326
327 version = manifest_version(&manifest, pkg, ver);
328 g_return_val_if_fail(version != NULL, NULL);
329@@ -1252,8 +1234,9 @@
330
331 gchar * retval = g_strdup_printf("%s_%s_%s", pkg, application, version);
332
333- /* The parser may hold allocation for some of our strings used above */
334- g_clear_object(&manifest);
335+ /* The object may hold allocation for some of our strings used above */
336+ if (manifest)
337+ json_object_unref(manifest);
338
339 return retval;
340 }
341
342=== removed file 'tests/click'
343--- tests/click 2014-03-12 00:34:52 +0000
344+++ tests/click 1970-01-01 00:00:00 +0000
345@@ -1,7 +0,0 @@
346-#!/bin/bash
347-
348-if [ ${1} == "info" ] ; then
349- /bin/cat "${XDG_DATA_DIRS}/click-app-dir/.click/info/${2}.manifest"
350-else
351- exit 1
352-fi
353
354=== modified file 'tests/exec-util-test.cc'
355--- tests/exec-util-test.cc 2014-03-12 00:34:52 +0000
356+++ tests/exec-util-test.cc 2014-03-12 00:34:52 +0000
357@@ -37,10 +37,6 @@
358 virtual void SetUp() {
359 g_setenv("UPSTART_JOB", "made-up-job", TRUE);
360 g_setenv("XDG_DATA_DIRS", CMAKE_SOURCE_DIR, TRUE);
361- const gchar * oldpath = g_getenv("PATH");
362- gchar * newpath = g_strjoin(":", CMAKE_SOURCE_DIR, oldpath, NULL);
363- g_setenv("PATH", newpath, TRUE);
364- g_free(newpath);
365
366 service = dbus_test_service_new(NULL);
367
368
369=== modified file 'tests/helper-test.cc'
370--- tests/helper-test.cc 2013-12-12 22:14:42 +0000
371+++ tests/helper-test.cc 2014-03-12 00:34:52 +0000
372@@ -33,10 +33,6 @@
373 protected:
374 virtual void SetUp() {
375 g_setenv("XDG_DATA_DIRS", CMAKE_SOURCE_DIR, TRUE);
376- const gchar * oldpath = g_getenv("PATH");
377- gchar * newpath = g_strjoin(":", CMAKE_SOURCE_DIR, oldpath, NULL);
378- g_setenv("PATH", newpath, TRUE);
379- g_free(newpath);
380 g_setenv("DATA_WRITE_DIR", CMAKE_BINARY_DIR, TRUE);
381 g_setenv("UPSTART_JOB", "made-up-job", TRUE);
382 return;
383@@ -422,6 +418,9 @@
384 {
385 gchar * desktop = NULL;
386
387+ g_setenv("TEST_CLICK_DB", "click-db-dir", TRUE);
388+ g_setenv("TEST_CLICK_USER", "test-user", TRUE);
389+
390 desktop = manifest_to_desktop(CMAKE_SOURCE_DIR "/click-app-dir/", "com.test.good_application_1.2.3");
391 ASSERT_STREQ(desktop, CMAKE_SOURCE_DIR "/click-app-dir/application.desktop");
392 g_free(desktop);
393
394=== modified file 'tests/libual-test.cc'
395--- tests/libual-test.cc 2014-02-19 21:43:04 +0000
396+++ tests/libual-test.cc 2014-03-12 00:34:52 +0000
397@@ -77,11 +77,6 @@
398
399 service = dbus_test_service_new(NULL);
400
401- const gchar * oldpath = g_getenv("PATH");
402- gchar * newpath = g_strjoin(":", CMAKE_SOURCE_DIR, oldpath, NULL);
403- g_setenv("PATH", newpath, TRUE);
404- g_free(newpath);
405-
406 debugConnection();
407
408 mock = dbus_test_dbus_mock_new("com.ubuntu.Upstart");
409@@ -416,6 +411,9 @@
410
411 TEST_F(LibUAL, ApplicationId)
412 {
413+ g_setenv("TEST_CLICK_DB", "click-db-dir", TRUE);
414+ g_setenv("TEST_CLICK_USER", "test-user", TRUE);
415+
416 /* Test with current-user-version, should return the version in the manifest */
417 EXPECT_STREQ("com.test.good_application_1.2.3", upstart_app_launch_triplet_to_app_id("com.test.good", "application", "current-user-version"));
418

Subscribers

People subscribed via source and target branches