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

[Colin Watson]

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

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.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:139
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/251/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-amd64-ci/110
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-armhf-ci/110
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-i386-ci/111

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/upstart-app-launch-ci/251/rebuild

review: Approve (continuous-integration)

[Ted Gould (ted)]

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

review: Approve

[Charles Kerr (charlesk)]

This looks great. Thanks Colin!

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2014-03-12 00:34:52 +0000
+++ CMakeLists.txt	2014-03-12 00:34:52 +0000
@@ -52,7 +52,7 @@
 pkg_check_modules(ZEITGEIST REQUIRED zeitgeist-2.0)
 include_directories(${ZEITGEIST_INCLUDE_DIRS})
 
-pkg_check_modules(CLICK REQUIRED click-0.4)
+pkg_check_modules(CLICK REQUIRED click-0.4>=0.4.18)
 include_directories(${CLICK_INCLUDE_DIRS})
 
 pkg_check_modules(LIBUPSTART REQUIRED libupstart)
@@ -76,7 +76,7 @@
 ####################
 
 add_library(helpers STATIC helpers.c helpers-keyfile.c)
-target_link_libraries(helpers ${GIO2_LIBRARIES} ${JSONGLIB_LIBRARIES})
+target_link_libraries(helpers ${GIO2_LIBRARIES} ${JSONGLIB_LIBRARIES} ${CLICK_LIBRARIES})
 
 ####################
 # desktop-exec

=== modified file 'debian/control'
--- debian/control	2014-03-12 00:34:52 +0000
+++ debian/control	2014-03-12 00:34:52 +0000
@@ -33,7 +33,6 @@
 Architecture: any
 Depends: ${shlibs:Depends},
          ${misc:Depends},
-         click (>= 0.4.9),
          click-apparmor,
          upstart (>= 1.11),
          zeitgeist,
@@ -57,7 +56,6 @@
 Architecture: any
 Depends: ${misc:Depends},
          ${shlibs:Depends},
-         click (>= 0.4.9),
 Pre-Depends: ${misc:Pre-Depends},
 Multi-Arch: same
 Description: library for sending requests to the upstart app launch

=== modified file 'helpers.c'
--- helpers.c	2014-02-07 14:56:27 +0000
+++ helpers.c	2014-03-12 00:34:52 +0000
@@ -18,6 +18,7 @@
  */
 
 #include <json-glib/json-glib.h>
+#include <click.h>
 #include <upstart.h>
 #include "helpers.h"
 
@@ -62,62 +63,54 @@
 manifest_to_desktop (const gchar * app_dir, const gchar * app_id)
 {
 	gchar * package = NULL;
-	gchar * output = NULL;
 	gchar * application = NULL;
 	gchar * version = NULL;
-	JsonParser * parser = NULL;
-	GError * error = NULL;
+	ClickDB * db = NULL;
+	ClickUser * user = NULL;
+	JsonObject * manifest = NULL;
 	gchar * desktoppath = NULL;
+	GError * error = NULL;
 
 	if (!app_id_to_triplet(app_id, &package, &application, &version)) {
 		g_warning("Unable to parse triplet: %s", app_id);
 		return NULL;
 	}
 
-	gchar * cmdline = g_strdup_printf("click info \"%s\"", package);
-	g_spawn_command_line_sync(cmdline, &output, NULL, NULL, &error);
-	g_free(cmdline);
-
+	db = click_db_new();
+	/* If TEST_CLICK_DB is unset, this reads the system database. */
+	click_db_read(db, g_getenv("TEST_CLICK_DB"), &error);
+	if (error != NULL) {
+		g_warning("Unable to read Click database: %s", error->message);
+		goto manifest_out;
+	}
+	/* If TEST_CLICK_USER is unset, this uses the current user name. */
+	user = click_user_new_for_user(db, g_getenv("TEST_CLICK_USER"), &error);
+	if (error != NULL) {
+		g_warning("Unable to read Click database: %s", error->message);
+		goto manifest_out;
+	}
+	manifest = click_user_get_manifest(user, package, &error);
 	if (error != NULL) {
 		g_warning("Unable to get manifest for '%s': %s", package, error->message);
-		g_error_free(error);
-		goto manifest_out;
-	}
-
-	parser = json_parser_new();
-
-	json_parser_load_from_data(parser, output, -1, &error);
-	g_free(output);
-
-	if (error != NULL) {
-		g_warning("Unable to load manifest data '%s': %s", package, error->message);
-		g_error_free(error);
-		goto manifest_out;
-	}
-
-	JsonNode * root = json_parser_get_root(parser);
-	if (json_node_get_node_type(root) != JSON_NODE_OBJECT) {
-		g_warning("Manifest '%s' doesn't start with an object", package);
-		goto manifest_out;
-	}
-
-	JsonObject * rootobj = json_node_get_object(root);
-	if (!json_object_has_member(rootobj, "version")) {
+		goto manifest_out;
+	}
+
+	if (!json_object_has_member(manifest, "version")) {
 		g_warning("Manifest '%s' doesn't have a version", package);
 		goto manifest_out;
 	}
 
-	if (g_strcmp0(json_object_get_string_member(rootobj, "version"), version) != 0) {
-		g_warning("Manifest '%s' version '%s' doesn't match AppID version '%s'", package, json_object_get_string_member(rootobj, "version"), version);
+	if (g_strcmp0(json_object_get_string_member(manifest, "version"), version) != 0) {
+		g_warning("Manifest '%s' version '%s' doesn't match AppID version '%s'", package, json_object_get_string_member(manifest, "version"), version);
 		goto manifest_out;
 	}
 
-	if (!json_object_has_member(rootobj, "hooks")) {
+	if (!json_object_has_member(manifest, "hooks")) {
 		g_warning("Manifest '%s' doesn't have an hooks section", package);
 		goto manifest_out;
 	}
 
-	JsonObject * appsobj = json_object_get_object_member(rootobj, "hooks");
+	JsonObject * appsobj = json_object_get_object_member(manifest, "hooks");
 	if (appsobj == NULL) {
 		g_warning("Manifest '%s' has an hooks section that is not a JSON object", package);
 		goto manifest_out;
@@ -151,7 +144,12 @@
 	}
 
 manifest_out:
-	g_clear_object(&parser);
+	if (error != NULL)
+		g_error_free(error);
+	if (manifest != NULL)
+		json_object_unref(manifest);
+	g_clear_object(&user);
+	g_clear_object(&db);
 	g_free(package);
 	g_free(application);
 	g_free(version);

=== modified file 'libupstart-app-launch/CMakeLists.txt'
--- libupstart-app-launch/CMakeLists.txt	2014-02-11 03:21:25 +0000
+++ libupstart-app-launch/CMakeLists.txt	2014-03-12 00:34:52 +0000
@@ -44,6 +44,7 @@
 	${GIO2_LIBRARIES}
 	${LTTNG_LIBRARIES}
 	${JSONGLIB_LIBRARIES}
+	${CLICK_LIBRARIES}
 	-Wl,--no-undefined
 )
 

=== modified file 'libupstart-app-launch/upstart-app-launch.c'
--- libupstart-app-launch/upstart-app-launch.c	2014-02-19 21:43:04 +0000
+++ libupstart-app-launch/upstart-app-launch.c	2014-03-12 00:34:52 +0000
@@ -19,6 +19,7 @@
 
 #include "upstart-app-launch.h"
 #include <json-glib/json-glib.h>
+#include <click.h>
 #include <upstart.h>
 #include <gio/gio.h>
 #include <string.h>
@@ -1092,60 +1093,46 @@
 	return TRUE;
 }
 
-/* Try and get a manifest file and do a couple sanity checks on it */
-static JsonParser *
-get_manifest_file (const gchar * pkg)
+/* Try and get a manifest and do a couple sanity checks on it */
+static JsonObject *
+get_manifest (const gchar * pkg)
 {
 	/* Get the directory from click */
 	GError * error = NULL;
-	const gchar * click_exec = NULL;
-
-	if (g_getenv("UAL_CLICK_EXEC") != NULL) {
-		click_exec = g_getenv("UAL_CLICK_EXEC");
-	} else {
-		click_exec = "click";
-	}
-
-	gchar * cmdline = g_strdup_printf("%s info \"%s\"",
-		click_exec, pkg);
-
-	gchar * output = NULL;
-	g_spawn_command_line_sync(cmdline, &output, NULL, NULL, &error);
-	g_free(cmdline);
-
+
+	ClickDB * db = click_db_new();
+	/* If TEST_CLICK_DB is unset, this reads the system database. */
+	click_db_read(db, g_getenv("TEST_CLICK_DB"), &error);
+	if (error != NULL) {
+		g_warning("Unable to read Click database: %s", error->message);
+		g_error_free(error);
+		return NULL;
+	}
+	/* If TEST_CLICK_USER is unset, this uses the current user name. */
+	ClickUser * user = click_user_new_for_user(db, g_getenv("TEST_CLICK_USER"), &error);
+	if (error != NULL) {
+		g_warning("Unable to read Click database: %s", error->message);
+		g_error_free(error);
+		g_object_unref(db);
+		return NULL;
+	}
+	g_object_unref(db);
+	JsonObject * manifest = click_user_get_manifest(user, pkg, &error);
 	if (error != NULL) {
 		g_warning("Unable to get manifest for '%s' package: %s", pkg, error->message);
 		g_error_free(error);
-		g_free(output);
-		return NULL;
-	}
-
-	/* Let's look at that manifest file */
-	JsonParser * parser = json_parser_new();
-	json_parser_load_from_data(parser, output, -1, &error);
-	g_free(output);
-
-	if (error != NULL) {
-		g_warning("Unable to load manifest for '%s': %s", pkg, error->message);
-		g_error_free(error);
-		g_object_unref(parser);
-		return NULL;
-	}
-	JsonNode * root = json_parser_get_root(parser);
-	if (json_node_get_node_type(root) != JSON_NODE_OBJECT) {
-		g_warning("Manifest file for package '%s' does not have an object as its root node", pkg);
-		g_object_unref(parser);
-		return NULL;
-	}
-
-	JsonObject * rootobj = json_node_get_object(root);
-	if (!json_object_has_member(rootobj, "version")) {
+		g_object_unref(user);
+		return NULL;
+	}
+	g_object_unref(user);
+
+	if (!json_object_has_member(manifest, "version")) {
 		g_warning("Manifest file for package '%s' does not have a version", pkg);
-		g_object_unref(parser);
+		json_object_unref(manifest);
 		return NULL;
 	}
 
-	return parser;
+	return manifest;
 }
 
 /* Types of search we can do for an app name */
@@ -1158,7 +1145,7 @@
 
 /* Figure out the app name if it's one of the keywords */
 static const gchar *
-manifest_app_name (JsonParser ** manifest, const gchar * pkg, const gchar * original_app)
+manifest_app_name (JsonObject ** manifest, const gchar * pkg, const gchar * original_app)
 {
 	app_name_t app_type = APP_NAME_FIRST;
 
@@ -1175,12 +1162,10 @@
 	}
 
 	if (*manifest == NULL) {
-		*manifest = get_manifest_file(pkg);
+		*manifest = get_manifest(pkg);
 	}
 
-	JsonNode * root_node = json_parser_get_root(*manifest);
-	JsonObject * root_obj = json_node_get_object(root_node);
-	JsonObject * hooks = json_object_get_object_member(root_obj, "hooks");
+	JsonObject * hooks = json_object_get_object_member(*manifest, "hooks");
 
 	if (hooks == NULL) {
 		return NULL;
@@ -1216,20 +1201,17 @@
 
 /* Figure out the app version using the manifest */
 static const gchar *
-manifest_version (JsonParser ** manifest, const gchar * pkg, const gchar * original_ver)
+manifest_version (JsonObject ** manifest, const gchar * pkg, const gchar * original_ver)
 {
 	if (original_ver != NULL && g_strcmp0(original_ver, "current-user-version") != 0) {
 		return original_ver;
 	} else  {
 		if (*manifest == NULL) {
-			*manifest = get_manifest_file(pkg);
+			*manifest = get_manifest(pkg);
 		}
 		g_return_val_if_fail(*manifest != NULL, NULL);
 
-		JsonNode * node = json_parser_get_root(*manifest);
-		JsonObject * obj = json_node_get_object(node);
-
-		return g_strdup(json_object_get_string_member(obj, "version"));
+		return g_strdup(json_object_get_string_member(*manifest, "version"));
 	}
 
 	return NULL;
@@ -1242,7 +1224,7 @@
 
 	const gchar * version = NULL;
 	const gchar * application = NULL;
-	JsonParser * manifest = NULL;
+	JsonObject * manifest = NULL;
 
 	version = manifest_version(&manifest, pkg, ver);
 	g_return_val_if_fail(version != NULL, NULL);
@@ -1252,8 +1234,9 @@
 
 	gchar * retval = g_strdup_printf("%s_%s_%s", pkg, application, version);
 
-	/* The parser may hold allocation for some of our strings used above */
-	g_clear_object(&manifest);
+	/* The object may hold allocation for some of our strings used above */
+	if (manifest)
+		json_object_unref(manifest);
 
 	return retval;
 }

=== removed file 'tests/click'
--- tests/click	2014-03-12 00:34:52 +0000
+++ tests/click	1970-01-01 00:00:00 +0000
@@ -1,7 +0,0 @@
-#!/bin/bash
-
-if [ ${1} == "info" ] ; then
-	/bin/cat "${XDG_DATA_DIRS}/click-app-dir/.click/info/${2}.manifest"
-else
-	exit 1
-fi

=== modified file 'tests/exec-util-test.cc'
--- tests/exec-util-test.cc	2014-03-12 00:34:52 +0000
+++ tests/exec-util-test.cc	2014-03-12 00:34:52 +0000
@@ -37,10 +37,6 @@
 		virtual void SetUp() {
 			g_setenv("UPSTART_JOB", "made-up-job", TRUE);
 			g_setenv("XDG_DATA_DIRS", CMAKE_SOURCE_DIR, TRUE);
-			const gchar * oldpath = g_getenv("PATH");
-			gchar * newpath = g_strjoin(":", CMAKE_SOURCE_DIR, oldpath, NULL);
-			g_setenv("PATH", newpath, TRUE);
-			g_free(newpath);
 
 			service = dbus_test_service_new(NULL);
 

=== modified file 'tests/helper-test.cc'
--- tests/helper-test.cc	2013-12-12 22:14:42 +0000
+++ tests/helper-test.cc	2014-03-12 00:34:52 +0000
@@ -33,10 +33,6 @@
 	protected:
 		virtual void SetUp() {
 			g_setenv("XDG_DATA_DIRS", CMAKE_SOURCE_DIR, TRUE);
-			const gchar * oldpath = g_getenv("PATH");
-			gchar * newpath = g_strjoin(":", CMAKE_SOURCE_DIR, oldpath, NULL);
-			g_setenv("PATH", newpath, TRUE);
-			g_free(newpath);
 			g_setenv("DATA_WRITE_DIR", CMAKE_BINARY_DIR, TRUE);
 			g_setenv("UPSTART_JOB", "made-up-job", TRUE);
 			return;
@@ -422,6 +418,9 @@
 {
 	gchar * desktop = NULL;
 
+	g_setenv("TEST_CLICK_DB", "click-db-dir", TRUE);
+	g_setenv("TEST_CLICK_USER", "test-user", TRUE);
+
 	desktop = manifest_to_desktop(CMAKE_SOURCE_DIR "/click-app-dir/", "com.test.good_application_1.2.3");
 	ASSERT_STREQ(desktop, CMAKE_SOURCE_DIR "/click-app-dir/application.desktop");
 	g_free(desktop);

=== modified file 'tests/libual-test.cc'
--- tests/libual-test.cc	2014-02-19 21:43:04 +0000
+++ tests/libual-test.cc	2014-03-12 00:34:52 +0000
@@ -77,11 +77,6 @@
 
 			service = dbus_test_service_new(NULL);
 
-			const gchar * oldpath = g_getenv("PATH");
-			gchar * newpath = g_strjoin(":", CMAKE_SOURCE_DIR, oldpath, NULL);
-			g_setenv("PATH", newpath, TRUE);
-			g_free(newpath);
-
 			debugConnection();
 
 			mock = dbus_test_dbus_mock_new("com.ubuntu.Upstart");
@@ -416,6 +411,9 @@
 
 TEST_F(LibUAL, ApplicationId)
 {
+	g_setenv("TEST_CLICK_DB", "click-db-dir", TRUE);
+	g_setenv("TEST_CLICK_USER", "test-user", TRUE);
+
 	/* Test with current-user-version, should return the version in the manifest */
 	EXPECT_STREQ("com.test.good_application_1.2.3", upstart_app_launch_triplet_to_app_id("com.test.good", "application", "current-user-version"));