Merge lp:~ted/ubuntu-app-launch/environment-dbus into lp:ubuntu-app-launch/14.04

[Ted Gould]

Set the Upstart job environment using DBus

Switching from shelling out to using a DBus message to set environment variables. This should hopefully make things a little bit faster as we can reuse the same DBus connection over and over.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:100
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/183/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-amd64-ci/42
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-armhf-ci/42
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-i386-ci/42

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

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

review: Approve

=== modified file 'click-exec.c'
--- click-exec.c	2013-12-12 21:34:28 +0000
+++ click-exec.c	2013-12-20 17:42:31 +0000
@@ -17,7 +17,7 @@
  *     Ted Gould <[e-mail]>
  */
 
-#include <glib.h>
+#include <gio/gio.h>
 #include "helpers.h"
 #include "click-exec-trace.h"
 
@@ -55,6 +55,16 @@
 
 	tracepoint(upstart_app_launch, click_start);
 
+	/* Ensure we keep one connection open to the bus for the entire
+	   script even though different people need it throughout */
+	GError * error = NULL;
+	GDBusConnection * bus = g_bus_get_sync(G_BUS_TYPE_SESSION, NULL, &error);
+	if (error != NULL) {
+		g_error("Unable to get session bus: %s", error->message);
+		g_error_free(error);
+		return 1;
+	}
+
 	handshake_t * handshake = starting_handshake_start(app_id);
 	if (handshake == NULL) {
 		g_warning("Unable to setup starting handshake");
@@ -62,7 +72,6 @@
 
 	tracepoint(upstart_app_launch, click_starting_sent);
 
-	GError * error = NULL;
 	gchar * package = NULL;
 	/* 'Parse' the App ID */
 	if (!app_id_to_triplet(app_id, &package, NULL, NULL)) {
@@ -157,5 +166,8 @@
 
 	tracepoint(upstart_app_launch, click_handshake_complete);
 
+	g_dbus_connection_flush_sync(bus, NULL, NULL);
+	g_object_unref(bus);
+
 	return 0;
 }
 
=== modified file 'desktop-exec.c'
--- desktop-exec.c	2013-12-06 10:46:55 +0000
+++ desktop-exec.c	2013-12-20 17:42:31 +0000
@@ -44,6 +44,16 @@
 	g_setenv("LTTNG_UST_REGISTER_TIMEOUT", "0", FALSE); /* Set to zero if not set */
 	tracepoint(upstart_app_launch, desktop_start);
 
+	/* Ensure we keep one connection open to the bus for the entire
+	   script even though different people need it throughout */
+	GError * error = NULL;
+	GDBusConnection * bus = g_bus_get_sync(G_BUS_TYPE_SESSION, NULL, &error);
+	if (error != NULL) {
+		g_error("Unable to get session bus: %s", error->message);
+		g_error_free(error);
+		return 1;
+	}
+
 	handshake_t * handshake = starting_handshake_start(app_id);
 	if (handshake == NULL) {
 		g_warning("Unable to setup starting handshake");
@@ -97,5 +107,8 @@
 
 	tracepoint(upstart_app_launch, desktop_handshake_complete);
 
+	g_dbus_connection_flush_sync(bus, NULL, NULL);
+	g_object_unref(bus);
+
 	return 0;
 }
 
=== modified file 'helpers.c'
--- helpers.c	2013-12-06 11:38:40 +0000
+++ helpers.c	2013-12-20 17:42:31 +0000
@@ -18,6 +18,7 @@
  */
 
 #include <json-glib/json-glib.h>
+#include <upstart.h>
 #include "helpers.h"
 
 /* Take an app ID and validate it and then break it up
@@ -222,35 +223,45 @@
 void
 set_upstart_variable (const gchar * variable, const gchar * value)
 {
-	GError * error = NULL;
-	gchar * command[4] = {
-		"initctl",
-		"set-env",
-		NULL,
-		NULL
-	};
-
-	g_debug("Setting Upstart variable '%s' to '%s'", variable, value);
-	gchar * variablestr = g_strdup_printf("%s=%s", variable, value);
-	command[2] = variablestr;
-
-	g_spawn_sync(NULL, /* working directory */
-		command,
-		NULL, /* environment */
-		G_SPAWN_SEARCH_PATH,
-		NULL, NULL, /* child setup */
-		NULL, /* stdout */
-		NULL, /* stderr */
-		NULL, /* exit status */
-		&error);
-
-	if (error != NULL) {
-		g_warning("Unable to set variable '%s' to '%s': %s", variable, value, error->message);
-		g_error_free(error);
-	}
-
-	g_free(variablestr);
-	return;
+	/* Check to see if we can get the job environment */
+	const gchar * job_name = g_getenv("UPSTART_JOB");
+	const gchar * instance_name = g_getenv("UPSTART_INSTANCE");
+	g_return_if_fail(job_name != NULL);
+
+	/* Get a bus, let's go! */
+	GDBusConnection * bus = g_bus_get_sync(G_BUS_TYPE_SESSION, NULL, NULL);
+	g_return_if_fail(bus != NULL);
+
+	GVariantBuilder builder; /* Target: (assb) */
+	g_variant_builder_init(&builder, G_VARIANT_TYPE_TUPLE);
+
+	/* Setup the job properties */
+	g_variant_builder_open(&builder, G_VARIANT_TYPE_ARRAY);
+	g_variant_builder_add_value(&builder, g_variant_new_string(job_name));
+	if (instance_name != NULL)
+		g_variant_builder_add_value(&builder, g_variant_new_string(instance_name));
+	g_variant_builder_close(&builder);
+
+	/* The value itself */
+	gchar * envstr = g_strdup_printf("%s=%s", variable, value);
+	g_variant_builder_add_value(&builder, g_variant_new_take_string(envstr));
+
+	/* Do we want to replace?  Yes, we do! */
+	g_variant_builder_add_value(&builder, g_variant_new_boolean(TRUE));
+
+	g_dbus_connection_call(bus,
+		DBUS_SERVICE_UPSTART,
+		DBUS_PATH_UPSTART,
+		DBUS_INTERFACE_UPSTART,
+		"SetEnv",
+		g_variant_builder_end(&builder),
+		NULL, /* reply */
+		G_DBUS_CALL_FLAGS_NONE,
+		-1, /* timeout */
+		NULL, /* cancelable */
+		NULL, NULL); /* callback */
+
+	g_object_unref(bus);
 }
 
 /* Convert a URI into a file */
 
=== modified file 'tests/CMakeLists.txt'
--- tests/CMakeLists.txt	2013-12-06 11:47:53 +0000
+++ tests/CMakeLists.txt	2013-12-20 17:42:31 +0000
@@ -14,7 +14,7 @@
 add_executable (helper-test helper-test.cc)
 add_definitions ( -DCMAKE_SOURCE_DIR="${CMAKE_CURRENT_SOURCE_DIR}" )
 add_definitions ( -DCMAKE_BINARY_DIR="${CMAKE_CURRENT_BINARY_DIR}" )
-target_link_libraries (helper-test helpers gtest ${GTEST_LIBS})
+target_link_libraries (helper-test helpers gtest ${GTEST_LIBS} ${DBUSTEST_LIBRARIES})
 
 add_test (helper-test helper-test)
 
 
=== modified file 'tests/helper-test.cc'
--- tests/helper-test.cc	2013-11-22 16:39:38 +0000
+++ tests/helper-test.cc	2013-12-20 17:42:31 +0000
@@ -19,6 +19,8 @@
 
 #include <gtest/gtest.h>
 #include <glib/gstdio.h>
+#include <libdbustest/dbus-test.h>
+#include <gio/gio.h>
 
 extern "C" {
 #include "../helpers.h"
@@ -31,8 +33,12 @@
 	protected:
 		virtual void SetUp() {
 			g_setenv("XDG_DATA_DIRS", CMAKE_SOURCE_DIR, TRUE);
-			g_setenv("PATH", 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;
 		}
 };
@@ -264,18 +270,34 @@
 
 TEST_F(HelperTest, SetConfinedEnvvars)
 {
-	g_unlink(CMAKE_BINARY_DIR "/initctl-output.txt");
+	DbusTestService * service = dbus_test_service_new(NULL);
+	DbusTestDbusMock * mock = dbus_test_dbus_mock_new("com.ubuntu.Upstart");
+
+	DbusTestDbusMockObject * obj = dbus_test_dbus_mock_get_object(mock, "/com/ubuntu/Upstart", "com.ubuntu.Upstart0_6", NULL);
+
+	dbus_test_dbus_mock_object_add_method(mock, obj,
+		"SetEnv",
+		G_VARIANT_TYPE("(assb)"),
+		NULL,
+		"",
+		NULL);
+
+	dbus_test_service_add_task(service, DBUS_TEST_TASK(mock));
+	dbus_test_service_start_tasks(service);
+
+	GDBusConnection * bus = g_bus_get_sync(G_BUS_TYPE_SESSION, NULL, NULL);
+	g_dbus_connection_set_exit_on_close(bus, FALSE);
+	g_object_add_weak_pointer(G_OBJECT(bus), (gpointer *)&bus);
 
 	/* Not a test other than "don't crash" */
 	set_confined_envvars("foo-app-pkg", "/foo/bar");
 
-	ASSERT_TRUE(g_file_test(CMAKE_BINARY_DIR "/initctl-output.txt", G_FILE_TEST_EXISTS));
-
-	gchar * contents = NULL;
-	ASSERT_TRUE(g_file_get_contents(CMAKE_BINARY_DIR "/initctl-output.txt", &contents, NULL, NULL));
-
-	gchar ** lines = g_strsplit(contents, "\n", 0);
-	g_free(contents);
+	guint len = 0;
+	const DbusTestDbusMockCall * calls = dbus_test_dbus_mock_object_get_method_calls(mock, obj, "SetEnv", &len, NULL);
+
+	ASSERT_EQ(len, 8);
+	ASSERT_NE(calls, nullptr);
+
 	unsigned int i;
 
 	bool got_app_isolation = false;
@@ -287,13 +309,13 @@
 	bool got_temp_dir = false;
 	bool got_shader_dir = false;
 
-	for (i = 0; lines[i] != NULL; i++) {
-		g_debug("Checking: '%s'", lines[i]);
-		if (lines[i][0] == '\0') continue;
-
-		ASSERT_TRUE(g_str_has_prefix(lines[i], "set-env "));
-
-		gchar * var = lines[i] + strlen("set-env ");
+	for (i = 0; i < len; i++) {
+		EXPECT_STREQ("SetEnv", calls[i].name);
+
+		GVariant * envvar = g_variant_get_child_value(calls[i].params, 1);
+		gchar * var = g_variant_dup_string(envvar, NULL);
+		g_variant_unref(envvar);
+
 		gchar * equal = g_strstr_len(var, -1, "=");
 		ASSERT_NE(equal, nullptr);
 
@@ -321,13 +343,13 @@
 			ASSERT_TRUE(g_str_has_suffix(value, "foo-app-pkg"));
 			got_shader_dir = true;
 		} else {
-			g_warning("Unknown variable! %s", lines[i]);
+			g_warning("Unknown variable! %s", var);
 			ASSERT_TRUE(false);
 		}
+
+		g_free(var);
 	}
 
-	g_strfreev(lines);
-
 	ASSERT_TRUE(got_app_isolation);
 	ASSERT_TRUE(got_cache_home);
 	ASSERT_TRUE(got_config_home);
@@ -337,6 +359,10 @@
 	ASSERT_TRUE(got_temp_dir);
 	ASSERT_TRUE(got_shader_dir);
 
+	g_object_unref(bus);
+	g_object_unref(mock);
+	g_object_unref(service);
+
 	return;
 }
 
 
=== removed file 'tests/initctl'
--- tests/initctl	2013-11-12 21:56:12 +0000
+++ tests/initctl	1970-01-01 00:00:00 +0000
@@ -1,3 +0,0 @@
-#!/bin/bash
-
-echo $@ >> ${DATA_WRITE_DIR}/initctl-output.txt