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

[Colin Watson]

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

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.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:137
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/247/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-amd64-ci/106
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-armhf-ci/106
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-i386-ci/107

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

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

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.

[lp:~cjwatson/ubuntu-app-launch/libclick-pkgdir]

138. By Colin Watson on 2014-03-07

Fix some reference leaks.

[Colin Watson (cjwatson)]

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

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:138
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/249/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-amd64-ci/108
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-armhf-ci/108
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-i386-ci/109

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

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

review: Approve

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2014-02-13 22:43:55 +0000
+++ CMakeLists.txt	2014-03-07 14:19:16 +0000
@@ -52,6 +52,9 @@
 pkg_check_modules(ZEITGEIST REQUIRED zeitgeist-2.0)
 include_directories(${ZEITGEIST_INCLUDE_DIRS})
 
+pkg_check_modules(CLICK REQUIRED click-0.4)
+include_directories(${CLICK_INCLUDE_DIRS})
+
 pkg_check_modules(LIBUPSTART REQUIRED libupstart)
 include_directories(${LIBUPSTART_INCLUDE_DIRS})
 
@@ -92,7 +95,7 @@
 add_lttng_gen_tp(NAME click-exec-trace)
 add_executable(click-exec click-exec.c click-exec-trace.c)
 set_target_properties(click-exec PROPERTIES OUTPUT_NAME "click-exec")
-target_link_libraries(click-exec helpers ${LTTNG_LIBRARIES})
+target_link_libraries(click-exec helpers ${LTTNG_LIBRARIES} ${CLICK_LIBRARIES})
 install(TARGETS click-exec RUNTIME DESTINATION "${pkglibexecdir}")
 
 ####################
@@ -101,7 +104,7 @@
 
 add_executable(desktop-hook desktop-hook.c)
 set_target_properties(desktop-hook PROPERTIES OUTPUT_NAME "desktop-hook")
-target_link_libraries(desktop-hook helpers)
+target_link_libraries(desktop-hook helpers ${CLICK_LIBRARIES})
 install(TARGETS desktop-hook RUNTIME DESTINATION "${pkglibexecdir}")
 
 ####################

=== modified file 'click-exec.c'
--- click-exec.c	2014-02-07 14:56:27 +0000
+++ click-exec.c	2014-03-07 14:19:16 +0000
@@ -18,6 +18,7 @@
  */
 
 #include <gio/gio.h>
+#include <click.h>
 #include "helpers.h"
 #include "click-exec-trace.h"
 
@@ -34,7 +35,7 @@
 
 For information on Click packages and the manifest look at the Click package documentation:
 
-https://click-package.readthedocs.org/en/latest/
+https://click.readthedocs.org/en/latest/
 
 */
 
@@ -80,45 +81,55 @@
 	}
 
 	/* Check click to find out where the files are */
-	gchar * cmdline = g_strdup_printf("click pkgdir \"%s\"", package);
-
-	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);
+		g_free(package);
+		return 1;
+	}
+	/* 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_free(package);
+		g_object_unref(db);
+		return 1;
+	}
+	gchar * pkgdir = click_user_get_path(user, package, &error);
+	if (error != NULL) {
+		g_warning("Unable to get the Click package directory for %s: %s", package, error->message);
+		g_error_free(error);
+		g_free(package);
+		g_object_unref(user);
+		g_object_unref(db);
+		return 1;
+	}
+	g_object_unref(user);
+	g_object_unref(db);
 
 	tracepoint(upstart_app_launch, click_found_pkgdir);
 
-	/* If we have an extra newline, we can delete it. */
-	gchar * newline = g_strstr_len(output, -1, "\n");
-	if (newline != NULL) {
-		newline[0] = '\0';
-	}
-
-	if (error != NULL) {
-		g_warning("Unable to get the package directory from click: %s", error->message);
-		g_error_free(error);
-		g_free(output); /* Probably not set, but just in case */
-		g_free(package);
-		return 1;
-	}
-
-	if (!g_file_test(output, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) {
-		g_warning("Application directory '%s' doesn't exist", output);
-		g_free(output);
-		g_free(package);
-		return 1;
-	}
-
-	g_debug("Setting 'APP_DIR' to '%s'", output);
-	set_upstart_variable("APP_DIR", output, FALSE);
-
-	set_confined_envvars(package, output);
+	if (!g_file_test(pkgdir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) {
+		g_warning("Application directory '%s' doesn't exist", pkgdir);
+		g_free(pkgdir);
+		g_free(package);
+		return 1;
+	}
+
+	g_debug("Setting 'APP_DIR' to '%s'", pkgdir);
+	set_upstart_variable("APP_DIR", pkgdir, FALSE);
+
+	set_confined_envvars(package, pkgdir);
 
 	tracepoint(upstart_app_launch, click_configured_env);
 
-	gchar * desktopfile = manifest_to_desktop(output, app_id);
+	gchar * desktopfile = manifest_to_desktop(pkgdir, app_id);
 
-	g_free(output);
+	g_free(pkgdir);
 	g_free(package);
 
 	if (desktopfile == NULL) {

=== modified file 'debian/control'
--- debian/control	2014-02-11 03:14:30 +0000
+++ debian/control	2014-03-07 14:19:16 +0000
@@ -7,6 +7,7 @@
                dbus-x11,
                dbus-test-runner,
                debhelper (>= 9),
+               libclick-0.4-dev,
                libdbus-1-dev,
                libdbustest1-dev (>= 14.04.0),
                libgirepository1.0-dev,

=== modified file 'desktop-hook.c'
--- desktop-hook.c	2013-09-13 16:30:01 +0000
+++ desktop-hook.c	2014-03-07 14:19:16 +0000
@@ -24,7 +24,7 @@
 This is a hook for Click packages.  You can find information on Click package hooks in
 the click documentation:
 
-https://click-package.readthedocs.org/en/latest/
+https://click.readthedocs.org/en/latest/
 
 Probably the biggest thing to understand for how this code works is that you need to
 understand that this hook is run after one, or many packages are installed.  A set of
@@ -44,6 +44,7 @@
 
 #include <gio/gio.h>
 #include <glib/gstdio.h>
+#include <click.h>
 #include <string.h>
 #include <errno.h>
 
@@ -390,40 +391,32 @@
 	}
 
 	/* Check click to find out where the files are */
-	gchar * cmdline = g_strdup_printf("click pkgdir \"%s\"", package);
+	ClickUser * user = click_user_new_for_user(NULL, NULL, &error);
+	if (error != NULL) {
+		g_warning("Unable to read Click database: %s", error->message);
+		g_error_free(error);
+		g_free(package);
+		return;
+	}
+	gchar * pkgdir = click_user_get_path(user, package, &error);
+	if (error != NULL) {
+		g_warning("Unable to get the Click package directory for %s: %s", package, error->message);
+		g_error_free(error);
+		g_free(package);
+		return;
+	}
+	g_object_unref(user);
 	g_free(package);
 
-	gchar * output = NULL;
-	g_spawn_command_line_sync(cmdline, &output, NULL, NULL, &error);
-	g_free(cmdline);
-
-	/* If we have an extra newline, we can hide it. */
-	if (output != NULL) {
-		gchar * newline = NULL;
-
-		newline = g_strstr_len(output, -1, "\n");
-
-		if (newline != NULL) {
-			newline[0] = '\0';
-		}
-	}
-
-	if (error != NULL) {
-		g_warning("Unable to get the package directory from click: %s", error->message);
-		g_error_free(error);
-		g_free(output); /* Probably not set, but just in case */
-		return;
-	}
-
-	if (!g_file_test(output, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) {
-		g_warning("Directory returned by click '%s' couldn't be found", output);
-		g_free(output);
-		return;
-	}
-
-	gchar * indesktop = manifest_to_desktop(output, state->app_id);
+	if (!g_file_test(pkgdir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) {
+		g_warning("Directory returned by click '%s' couldn't be found", pkgdir);
+		g_free(pkgdir);
+		return;
+	}
+
+	gchar * indesktop = manifest_to_desktop(pkgdir, state->app_id);
 	if (indesktop == NULL) {
-		g_free(output);
+		g_free(pkgdir);
 		return;
 	}
 
@@ -432,11 +425,11 @@
 	gchar * desktoppath = g_build_filename(desktopdir, desktopfile, NULL);
 	g_free(desktopfile);
 
-	copy_desktop_file(indesktop, desktoppath, output, state->app_id);
+	copy_desktop_file(indesktop, desktoppath, pkgdir, state->app_id);
 
 	g_free(desktoppath);
 	g_free(indesktop);
-	g_free(output);
+	g_free(pkgdir);
 
 	return;
 }

=== modified file 'tests/CMakeLists.txt'
--- tests/CMakeLists.txt	2014-01-22 15:47:41 +0000
+++ tests/CMakeLists.txt	2014-03-07 14:19:16 +0000
@@ -1,6 +1,9 @@
 
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
 
+configure_file ("click-db-dir/test.conf.in" "${CMAKE_CURRENT_BINARY_DIR}/click-db-dir/test.conf" @ONLY)
+set_directory_properties (PROPERTIES ADDITIONAL_MAKE_CLEAN_FILES "${CMAKE_CURRENT_BINARY_DIR}/click-db-dir/test.conf")
+
 # Google Test
 
 include_directories(${GTEST_INCLUDE_DIR})

=== modified file 'tests/click'
--- tests/click	2013-12-13 20:32:49 +0000
+++ tests/click	2014-03-07 14:19:16 +0000
@@ -2,9 +2,6 @@
 
 if [ ${1} == "info" ] ; then
 	/bin/cat "${XDG_DATA_DIRS}/click-app-dir/.click/info/${2}.manifest"
-else if [ ${1} == "pkgdir" ] ; then
-	echo "${XDG_DATA_DIRS}/click-app-dir"
 else
 	exit 1
 fi
-fi

=== added directory 'tests/click-db-dir'
=== added file 'tests/click-db-dir/test.conf.in'
--- tests/click-db-dir/test.conf.in	1970-01-01 00:00:00 +0000
+++ tests/click-db-dir/test.conf.in	2014-03-07 14:19:16 +0000
@@ -0,0 +1,2 @@
+[Click Database]
+root = @CMAKE_CURRENT_SOURCE_DIR@/click-root-dir

=== added directory 'tests/click-root-dir'
=== added directory 'tests/click-root-dir/.click'
=== added directory 'tests/click-root-dir/.click/users'
=== added directory 'tests/click-root-dir/.click/users/test-user'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.bad-version'
=== target is u'../../../com.test.bad-version/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.good'
=== target is u'../../../com.test.good/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.multiple'
=== target is u'../../../com.test.multiple/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-app'
=== target is u'../../../com.test.no-app/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-hooks'
=== target is u'../../../com.test.no-hooks/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-json'
=== target is u'../../../com.test.no-json/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-object'
=== target is u'../../../com.test.no-object/1.2.3'
=== added symlink 'tests/click-root-dir/.click/users/test-user/com.test.no-version'
=== target is u'../../../com.test.no-version/1.2.3'
=== added directory 'tests/click-root-dir/com.test.bad-version'
=== added symlink 'tests/click-root-dir/com.test.bad-version/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.good'
=== added symlink 'tests/click-root-dir/com.test.good/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.multiple'
=== added symlink 'tests/click-root-dir/com.test.multiple/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.no-app'
=== added symlink 'tests/click-root-dir/com.test.no-app/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.no-hooks'
=== added symlink 'tests/click-root-dir/com.test.no-hooks/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.no-json'
=== added symlink 'tests/click-root-dir/com.test.no-json/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.no-object'
=== added symlink 'tests/click-root-dir/com.test.no-object/1.2.3'
=== target is u'../../click-app-dir'
=== added directory 'tests/click-root-dir/com.test.no-version'
=== added symlink 'tests/click-root-dir/com.test.no-version/1.2.3'
=== target is u'../../click-app-dir'
=== modified file 'tests/exec-util-test.cc'
--- tests/exec-util-test.cc	2013-12-17 20:29:43 +0000
+++ tests/exec-util-test.cc	2014-03-07 14:19:16 +0000
@@ -90,6 +90,8 @@
 	DbusTestDbusMockObject * obj = dbus_test_dbus_mock_get_object(mock, "/com/ubuntu/Upstart", "com.ubuntu.Upstart0_6", NULL);
 
 	g_setenv("APP_ID", "com.test.good_application_1.2.3", TRUE);
+	g_setenv("TEST_CLICK_DB", "click-db-dir", TRUE);
+	g_setenv("TEST_CLICK_USER", "test-user", TRUE);
 
 	g_spawn_command_line_sync(CLICK_EXEC_TOOL, NULL, NULL, NULL, NULL);
 
@@ -114,6 +116,8 @@
 	bool got_app_desktop = false;
 	bool got_app_desktop_path = false;
 
+#define APP_DIR CMAKE_SOURCE_DIR "/click-root-dir/.click/users/test-user/com.test.good"
+
 	for (i = 0; i < len; i++) {
 		EXPECT_STREQ("SetEnv", calls[i].name);
 
@@ -139,7 +143,7 @@
 		} else if (g_strcmp0(var, "XDG_RUNTIME_DIR") == 0) {
 			got_runtime_dir = true;
 		} else if (g_strcmp0(var, "XDG_DATA_DIRS") == 0) {
-			EXPECT_TRUE(g_str_has_prefix(value, CMAKE_SOURCE_DIR "/click-app-dir:"));
+			EXPECT_TRUE(g_str_has_prefix(value, APP_DIR ":"));
 			got_data_dirs = true;
 		} else if (g_strcmp0(var, "TMPDIR") == 0) {
 			EXPECT_TRUE(g_str_has_suffix(value, "com.test.good"));
@@ -148,7 +152,7 @@
 			EXPECT_TRUE(g_str_has_suffix(value, "com.test.good"));
 			got_shader_dir = true;
 		} else if (g_strcmp0(var, "APP_DIR") == 0) {
-			EXPECT_STREQ(CMAKE_SOURCE_DIR "/click-app-dir", value);
+			EXPECT_STREQ(APP_DIR, value);
 			got_app_dir = true;
 		} else if (g_strcmp0(var, "APP_EXEC") == 0) {
 			EXPECT_STREQ("foo", value);
@@ -156,7 +160,7 @@
 		} else if (g_strcmp0(var, "APP_DESKTOP_FILE") == 0) {
 			got_app_desktop = true;
 		} else if (g_strcmp0(var, "APP_DESKTOP_FILE_PATH") == 0) {
-			EXPECT_STREQ(CMAKE_SOURCE_DIR "/click-app-dir/application.desktop", value);
+			EXPECT_STREQ(APP_DIR "/application.desktop", value);
 			got_app_desktop_path = true;
 		} else {
 			g_warning("Unknown variable! %s", var);
@@ -166,6 +170,8 @@
 		g_free(var);
 	}
 
+#undef APP_DIR
+
 	EXPECT_TRUE(got_app_isolation);
 	EXPECT_TRUE(got_cache_home);
 	EXPECT_TRUE(got_config_home);