Merge lp:~mvo/click/lp1245826-multiple-hookfiles into lp:click/devel

[Michael Vogt]

Add support for multiple click hook files.

Add support for multiple click hook files.

This is based on the work of lp:~mardy/click/lp1245826. Feedback on the approach appreciated, the actual code needs a refactor to get rid of the duplication of the relative_path reading code.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:524
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~mvo/click/lp1245826-multiple-hookfiles/+merge/235474/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/click-devel-ci/69/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-amd64-ci/71/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/69/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-i386-ci/69/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/click-devel-ci/69/rebuild

review: Needs Fixing (continuous-integration)

[lp:~mvo/click/lp1245826-multiple-hookfiles]

525. By Michael Vogt on 2014-09-23

refactor duplicated code into get_relative_paths_for_hookname()

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:525
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~mvo/click/lp1245826-multiple-hookfiles/+merge/235474/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/click-devel-ci/70/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-amd64-ci/72/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/70/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-i386-ci/70/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/click-devel-ci/70/rebuild

review: Needs Fixing (continuous-integration)

[lp:~mvo/click/lp1245826-multiple-hookfiles]

526. By Michael Vogt on 2014-09-30

fix pep8 failure

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:526
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~mvo/click/lp1245826-multiple-hookfiles/+merge/235474/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/click-devel-ci/85/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-amd64-ci/87
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/85
        deb: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/85/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-i386-ci/85

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/click-devel-ci/85/rebuild

review: Needs Fixing (continuous-integration)

Unmerged revisions

526. By Michael Vogt on 2014-09-30

fix pep8 failure

525. By Michael Vogt on 2014-09-23

refactor duplicated code into get_relative_paths_for_hookname()

524. By Michael Vogt on 2014-09-22

add support for relative_path in get_app_hooks

523. By Michael Vogt on 2014-09-22

fix remove and add test

522. By Michael Vogt on 2014-09-22

Add support for multiple hook files

This allows writing hook files like:
"""
 Pattern: %s/${id}_${basename}
"""

and manifest with:
"""
hooks": {
  "app": {
     "service": ["target-1","target-2"]
  }
}
"""

=== modified file 'click/tests/gimock.py'
--- click/tests/gimock.py	2014-06-23 21:14:06 +0000
+++ click/tests/gimock.py	2014-09-30 05:57:11 +0000
@@ -443,7 +443,8 @@
                 env["LD_PRELOAD"] = lib_path
             subp = subprocess.Popen(args, close_fds=False, env=env,
                                     stdout=subprocess.PIPE,
-                                    stderr=subprocess.PIPE)
+                                    stderr=subprocess.PIPE,
+                                    universal_newlines=True)
             os.close(wfd)
             reader = os.fdopen(rfd, "rb")
             stdout, stderr = subp.communicate()

=== modified file 'click/tests/test_hooks.py'
--- click/tests/test_hooks.py	2014-07-11 17:20:51 +0000
+++ click/tests/test_hooks.py	2014-09-30 05:57:11 +0000
@@ -424,6 +424,31 @@
                 os.path.join(self.temp_dir, "test-2", "2.0", "target-2"),
                 os.readlink(path_2))
 
+    def test_install_hook_with_multiple_files(self):
+        with self.run_in_subprocess(
+                "click_get_hooks_dir") as (enter, preloads):
+            enter()
+            self._setup_hooks_dir(
+                preloads, hooks_dir=os.path.join(self.temp_dir, "hooks"))
+            self._make_hook_file(
+                "Pattern: %s/${id}_${basename}.new" % self.temp_dir,
+                hookname="new")
+            self._make_installed_click("test-1", "1.0", json_data={
+                    "hooks": {"test1-app": {"new": ["target-1","target-2"]}}})
+            hook = Click.Hook.open(self.db, "new")
+            hook.install(user_name=None)
+            for hook_file in ("target-1", "target-2"):
+                path = os.path.join(
+                    self.temp_dir, "test-1_test1-app_1.0_%s.new" % hook_file)
+                expected_path = os.path.join(
+                    self.temp_dir, "test-1", "1.0", hook_file)
+                self.assertEqual(expected_path, os.readlink(path))
+            hook.remove(user_name=None)
+            for hook_file in ("target-1", "target-2"):
+                path = os.path.join(
+                    self.temp_dir, "test-1_test1-app_1.0_%s.new" % hook_file)
+                self.assertEqual(os.path.lexists(path), False)
+
     def test_remove(self):
         with self.run_in_subprocess(
                 "click_get_hooks_dir") as (enter, preloads):
@@ -1114,6 +1139,10 @@
                 print("Pattern: %s/yelp/other-${id}.txt" % self.temp_dir,
                       file=f)
                 print("Hook-Name: yelp", file=f)
+            with mkfile(os.path.join(hooks_dir, "service.hook")) as f:
+                print("Pattern: %s/service/${id}_${basename}.txt" % self.temp_dir,
+                      file=f)
+                print("Hook-Name: service", file=f)
             os.mkdir(os.path.join(self.temp_dir, "unity"))
             unity_path = os.path.join(
                 self.temp_dir, "unity", "test_app_1.0.scope")
@@ -1125,18 +1154,33 @@
             yelp_other_path = os.path.join(
                 self.temp_dir, "yelp", "other-test_app_1.0.txt")
             os.symlink("dummy", yelp_other_path)
+            os.mkdir(os.path.join(self.temp_dir, "service"))
+            service1_path = os.path.join(
+                self.temp_dir, "service", "test_app_1.0_service1.txt")
+            os.symlink("dummy", service1_path)
+            service2_path = os.path.join(
+                self.temp_dir, "service", "test_app_1.0_service2.txt")
+            os.symlink("dummy", service2_path)
             package_dir = os.path.join(self.temp_dir, "test")
             with mkfile(os.path.join(
                     package_dir, "1.0", ".click", "info",
                     "test.manifest")) as f:
                 json.dump(
-                    {"hooks": {
-                        "app": {"yelp": "foo.txt", "unity": "foo.scope"}}
+                    {"hooks":
+                      {
+                        "app": {
+                            "yelp": "foo.txt",
+                            "unity": "foo.scope",
+                            "service": ["service1", "service2"],
+                        },
+                      }
                     }, f)
             Click.package_remove_hooks(self.db, "test", "1.0", user_name=None)
             self.assertFalse(os.path.lexists(unity_path))
             self.assertFalse(os.path.lexists(yelp_docs_path))
             self.assertFalse(os.path.lexists(yelp_other_path))
+            self.assertFalse(os.path.lexists(service1_path))
+            self.assertFalse(os.path.lexists(service2_path))
 
 
 class TestPackageHooksValidateFramework(TestClickHookBase):

=== modified file 'lib/click/hooks.vala'
--- lib/click/hooks.vala	2014-09-10 11:57:55 +0000
+++ lib/click/hooks.vala	2014-09-30 05:57:11 +0000
@@ -227,23 +227,24 @@
 			Gee.Comparable<AppHook> {
 	public string app_name { get; construct; }
 	public string hook_name { get; construct; }
+	public string relative_path { get; construct; }
 
 	public
-	AppHook (string app_name, string hook_name)
+	AppHook (string app_name, string hook_name, string? relative_path = null)
 	{
-		Object (app_name: app_name, hook_name: hook_name);
+		Object (app_name: app_name, hook_name: hook_name, relative_path: relative_path);
 	}
 
 	public uint
 	hash ()
 	{
-		return app_name.hash () ^ hook_name.hash ();
+		return app_name.hash () ^ hook_name.hash () ^ relative_path.hash();
 	}
 
 	public bool
 	equal_to (AppHook obj)
 	{
-		return app_name == obj.app_name && hook_name == obj.hook_name;
+		return app_name == obj.app_name && hook_name == obj.hook_name && relative_path == obj.relative_path;
 	}
 
 	public int
@@ -252,7 +253,10 @@
 		var ret = strcmp (app_name, obj.app_name);
 		if (ret != 0)
 			return ret;
-		return strcmp (hook_name, obj.hook_name);
+		ret = strcmp (hook_name, obj.hook_name);
+		if (ret != 0)
+			return ret;
+		return strcmp (relative_path, obj.relative_path);
 	}
 }
 
@@ -588,10 +592,11 @@
 	 * @version: A version string.
 	 * @app_name: An application name.
 	 * @user_name: (allow-none): A user name, or null.
+	 * @base_name: (allow-none): A base name, or null.
 	 */
 	public string
 	get_pattern (string package, string version, string app_name,
-		     string? user_name = null) throws HooksError
+				 string? user_name = null, string? base_name = "") throws HooksError
 	{
 		var builder = new VariantBuilder (new VariantType ("a{sms}"));
 		var app_id = get_app_id (package, version, app_name);
@@ -600,9 +605,9 @@
 		builder.add ("{sms}", "id", app_id);
 		builder.add ("{sms}", "user", user_name);
 		builder.add ("{sms}", "home", user_home);
+		builder.add ("{sms}", "basename", base_name);
 		if (is_single_version) {
-			var short_app_id = get_short_app_id (package,
-							     app_name);
+			var short_app_id = get_short_app_id (package, app_name);
 			builder.add ("{sms}", "short-id", short_app_id);
 		}
 		var ret = pattern_format (pattern, builder.end ());
@@ -799,7 +804,8 @@
 		else
 			path = db.get_path (package, version);
 		var target = Path.build_filename (path, relative_path);
-		var link = get_pattern (package, version, app_name, user_name);
+		var link = get_pattern (package, version, app_name, user_name,
+								Path.get_basename (relative_path));
 		if (is_symlink (link) && FileUtils.read_link (link) == target)
 			return;
 		ensuredir (Path.get_dirname (link));
@@ -862,10 +868,10 @@
 	 */
 	public void
 	remove_package (string package, string version, string app_name,
-			string? user_name = null) throws Error
+				string? user_name = null, string? base_name = null) throws Error
 	{
 		unlink_force (get_pattern
-			(package, version, app_name, user_name));
+				  (package, version, app_name, user_name, base_name));
 		run_commands (user_name);
 	}
 
@@ -953,17 +959,14 @@
 			var manifest_hooks = read_manifest_hooks
 				(db, unpacked.package, unpacked.version);
 			foreach (var app_name in manifest_hooks.get_members ()) {
-				var hooks = manifest_hooks.get_object_member
-					(app_name);
-				if (hooks.has_member (hook_name)) {
-					var relative_path = hooks.get_string_member
-						(hook_name);
-					ret.prepend (new RelevantApp
-						(unpacked.package,
-						 unpacked.version, app_name,
-						 unpacked.user_name,
-						 relative_path));
-				}
+				foreach (var relative_path in get_relative_paths_for_hookname(manifest_hooks, app_name, hook_name))
+					{
+						ret.prepend (new RelevantApp
+									 (unpacked.package,
+									  unpacked.version, app_name,
+									  unpacked.user_name,
+									  relative_path));
+					}
 			}
 		}
 		ret.reverse ();
@@ -998,7 +1001,7 @@
 	{
 		foreach (var app in get_relevant_apps (user_name))
 			remove_package (app.package, app.version, app.app_name,
-					app.user_name);
+							app.user_name, app.relative_path);
 	}
 
 	/**
@@ -1058,6 +1061,28 @@
 	}
 }
 
+private List<string>
+get_relative_paths_for_hookname(Json.Object manifest, string app_name, string hook_name) throws Error
+{
+	var ret = new List<string> ();
+	var hooks = manifest.get_object_member (app_name);
+	if (!hooks.has_member (hook_name))
+		return ret;
+
+	var node = hooks.get_member(hook_name);
+	var value_type = node.get_value_type();
+	// support both single string and list for hook targets in manifest
+	if (value_type.name() == "gchararray")
+		ret.append (node.get_string());
+	else {
+		var array = node.get_array ();
+		foreach(var elm in array.get_elements ()) {
+			ret.append (elm.get_string ());
+		}
+	}
+	return ret;
+}
+
 private Gee.TreeSet<AppHook>
 get_app_hooks (Json.Object manifest)
 {
@@ -1065,7 +1090,8 @@
 	foreach (var app_name in manifest.get_members ()) {
 		var hooks = manifest.get_object_member (app_name);
 		foreach (var hook_name in hooks.get_members ())
-			items.add (new AppHook (app_name, hook_name));
+			foreach (var relative_path in get_relative_paths_for_hookname(manifest, app_name, hook_name))
+				items.add (new AppHook (app_name, hook_name, relative_path));
 	}
 	return items;
 }
@@ -1105,7 +1131,7 @@
 			if (! hook.is_single_version)
 				continue;
 			hook.remove_package (package, old_version,
-					     app_hook.app_name, user_name);
+						 app_hook.app_name, user_name, app_hook.relative_path);
 		}
 	}
 
@@ -1116,14 +1142,14 @@
 		var hook_names = app_hooks.get_members ();
 		hook_names.sort (strcmp);
 		foreach (var hook_name in hook_names) {
-			var relative_path = app_hooks.get_string_member
-				(hook_name);
-			foreach (var hook in Hook.open_all (db, hook_name)) {
+			foreach (var relative_path in get_relative_paths_for_hookname (new_manifest, app_name, hook_name)) {
+ 			  foreach (var hook in Hook.open_all (db, hook_name)) {
 				if (hook.is_user_level != (user_name != null))
 					continue;
 				hook.install_package (package, new_version,
 						      app_name, relative_path,
 						      user_name);
+			  }
 			}
 		}
 	}
@@ -1152,7 +1178,7 @@
 			if (hook.is_user_level != (user_name != null))
 				continue;
 			hook.remove_package (package, old_version,
-					     app_hook.app_name, user_name);
+						 app_hook.app_name, user_name, app_hook.relative_path);
 		}
 	}
 }