Merge lp:~mvo/click/hide-apps-on-missing-framework into lp:click

[Michael Vogt]

This branch fixes bug #1271944 - when the hooks are run it will check if the framework requirements are meet. If that is not the case, the hook symlink is removed and the hook command run.

[Colin Watson (cjwatson)]

This looks good apart from the FIXME you identify - I agree with that comment and think this needs to be moved slightly.

review: Needs Fixing

[lp:~mvo/click/hide-apps-on-missing-framework]

429. By Michael Vogt on 2014-05-13

lib/click/hooks.vala: move validate_framework_for_package() check inside get_relevant_apps

[Michael Vogt (mvo)]

Thanks for the review! I moved the check for validate_framework_for_package() into the get_relevant_apps() now.

review: Needs Resubmitting

Unmerged revisions

=== modified file 'click/tests/helpers.py'
--- click/tests/helpers.py	2014-05-08 18:18:39 +0000
+++ click/tests/helpers.py	2014-05-13 17:58:01 +0000
@@ -108,6 +108,14 @@
         self.assertRaisesGError(
             "click_user_error-quark", code, callableObj, *args, **kwargs)
 
+    def _setup_frameworks(self, preloads, frameworks_dir=None, frameworks=[]):
+        frameworks_dir = self._create_mock_framework_dir(frameworks_dir)
+        shutil.rmtree(frameworks_dir, ignore_errors=True)
+        for framework in frameworks:
+            self._create_mock_framework_file(framework)
+        preloads["click_get_frameworks_dir"].side_effect = (
+            lambda: self.make_string(frameworks_dir))
+
     def _create_mock_framework_dir(self, frameworks_dir=None):
         if frameworks_dir is None:
             frameworks_dir = os.path.join(self.temp_dir, "frameworks")

=== modified file 'click/tests/test_hooks.py'
--- click/tests/test_hooks.py	2014-04-03 08:52:02 +0000
+++ click/tests/test_hooks.py	2014-05-13 17:58:01 +0000
@@ -107,6 +107,7 @@
             hooks_dir = self.temp_dir
         preloads["click_get_hooks_dir"].side_effect = (
             lambda: self.make_string(hooks_dir))
+        self.hooks_dir = hooks_dir
 
     def g_spawn_sync_side_effect(self, status_map, working_directory, argv,
                                  envp, flags, child_setup, user_data,
@@ -1113,3 +1114,95 @@
             self.assertFalse(os.path.lexists(unity_path))
             self.assertFalse(os.path.lexists(yelp_docs_path))
             self.assertFalse(os.path.lexists(yelp_other_path))
+
+
+class TestPackageHooksValidateFramework(TestClickHookBase):
+
+    def _make_hook_file(self, hookname="test"):
+        hook_file = os.path.join(self.hooks_dir, "%s.hook" % hookname)
+        with mkfile(hook_file) as f:
+            print("User-Level: yes", file=f)
+            print("Pattern: %s/${id}.test" % self.temp_dir, file=f)
+
+    def _make_manifest(self, package="test-1",version="1.0",
+                       framework="ubuntu-sdk-13.10",
+                       hooks=None):
+        if hooks is None:
+            hooks = {"test1-app": {"test": "target-1"}}
+        with mkfile_utf8(os.path.join(
+                self.temp_dir, package, version, ".click", "info",
+                "%s.manifest" % package)) as f:
+            json.dump({
+                "framework": framework,
+                "hooks": hooks,
+            }, f)
+        os.symlink("1.0", os.path.join(self.temp_dir, package, "current"))
+        user_db = Click.User.for_user(self.db, "test-user")
+        user_db.set_version(package, version)
+
+    def _setup_test_env(self, preloads):
+        preloads["click_get_user_home"].return_value = "/home/test-user"
+        self._setup_hooks_dir(
+            preloads, os.path.join(self.temp_dir, "hooks"))
+        self._make_hook_file()
+        self.hook_symlink_path = os.path.join(
+            self.temp_dir, "test-1_test1-app_1.0.test")
+
+    def test_links_are_kept_on_validate_framework(self):
+        with self.run_in_subprocess(
+                "click_get_hooks_dir", "click_get_user_home",
+                "click_get_frameworks_dir",
+                ) as (enter, preloads):
+            enter()
+            self._setup_frameworks(
+                preloads, frameworks=["ubuntu-sdk-13.10"])
+            self._setup_test_env(preloads)
+            self._make_manifest(framework="ubuntu-sdk-13.10")
+            self.assertTrue(os.path.lexists(self.hook_symlink_path))
+            # run the hooks
+            Click.run_user_hooks(self.db, user_name="test-user")
+            self.assertTrue(os.path.lexists(self.hook_symlink_path))
+
+    def test_links_are_kept_multiple_frameworks(self):
+        with self.run_in_subprocess(
+                "click_get_hooks_dir", "click_get_user_home",
+                "click_get_frameworks_dir",
+                ) as (enter, preloads):
+            enter()
+            self._setup_frameworks(
+                preloads, frameworks=["ubuntu-sdk-14.04", "ubuntu-sdk-13.10"])
+            self._setup_test_env(preloads)
+            self._make_manifest(framework="ubuntu-sdk-13.10")
+            self.assertTrue(os.path.lexists(self.hook_symlink_path))
+            # run the hooks
+            Click.run_user_hooks(self.db, user_name="test-user")
+            self.assertTrue(os.path.lexists(self.hook_symlink_path))
+
+    def test_links_are_removed_on_missng_framework(self):
+        with self.run_in_subprocess(
+                "click_get_hooks_dir", "click_get_user_home",
+                "click_get_frameworks_dir",
+                ) as (enter, preloads):
+            enter()
+            self._setup_frameworks(preloads, frameworks=["missing"])
+            self._setup_test_env(preloads)
+            self._make_manifest(framework="ubuntu-sdk-13.10")
+            self.assertTrue(os.path.lexists(self.hook_symlink_path))
+            # run the hooks
+            Click.run_user_hooks(self.db, user_name="test-user")
+            self.assertFalse(os.path.lexists(self.hook_symlink_path))
+
+    def test_links_are_removed_on_missng_multiple_framework(self):
+        with self.run_in_subprocess(
+                "click_get_hooks_dir", "click_get_user_home",
+                "click_get_frameworks_dir",
+                ) as (enter, preloads):
+            enter()
+            self._setup_frameworks(preloads, frameworks=["ubuntu-sdk-13.10"])
+            self._setup_test_env(preloads)
+            self._make_manifest(
+                framework="ubuntu-sdk-13.10,ubuntu-sdk-13.10-html")
+            self.assertTrue(os.path.lexists(self.hook_symlink_path))
+            # run the hooks
+            Click.run_user_hooks(self.db, user_name="test-user")
+            self.assertFalse(os.path.lexists(self.hook_symlink_path))

=== modified file 'click/tests/test_install.py'
--- click/tests/test_install.py	2014-05-05 13:23:35 +0000
+++ click/tests/test_install.py	2014-05-13 17:58:01 +0000
@@ -103,14 +103,6 @@
             self.temp_dir, control_dir, data_dir, package_path)
         return package_path
 
-    def _setup_frameworks(self, preloads, frameworks_dir=None, frameworks=[]):
-        frameworks_dir = self._create_mock_framework_dir(frameworks_dir)
-        shutil.rmtree(frameworks_dir, ignore_errors=True)
-        for framework in frameworks:
-            self._create_mock_framework_file(framework)
-        preloads["click_get_frameworks_dir"].side_effect = (
-            lambda: self.make_string(frameworks_dir))
-
     def test_audit_no_click_version(self):
         path = self.make_fake_package()
         self.assertRaisesRegex(

=== modified file 'doc/index.rst'
--- doc/index.rst	2014-05-08 12:49:11 +0000
+++ doc/index.rst	2014-05-13 17:58:01 +0000
@@ -38,7 +38,9 @@
 Then run::
 
     $ ./autogen.sh
-    $ ./configure --with-systemdsystemunitdir=/lib/systemd/system \
+    $ ./configure --prefix=/usr \
+        --sysconfdir=/etc \
+        --with-systemdsystemunitdir=/lib/systemd/system \
 	--with-systemduserunitdir=/usr/lib/systemd/user
     $ make
 

=== modified file 'lib/click/hooks.vala'
--- lib/click/hooks.vala	2014-04-08 09:31:40 +0000
+++ lib/click/hooks.vala	2014-05-13 17:58:01 +0000
@@ -56,8 +56,48 @@
 	INCOMPLETE
 }
 
+
+/* vala implementation of click.framework.validate_framework()
+ * 
+ * Note that the required_frameworks string has the form
+ *      framework1, framework2, ...
+ * See doc/file-format.rst for details.
+ */
+private bool
+validate_framework (string required_frameworks)
+{
+	// valid framework names, cf. debian policy §5.6.1
+	var valid_framework_re = new Regex("^[a-z][a-z0-9.+-]+");
+	var base_version = "";
+	foreach (var framework_name in required_frameworks.split(","))
+	{
+		framework_name = framework_name.strip();
+		if(!valid_framework_re.match(framework_name))
+			return false;
+		if(!Framework.has_framework(framework_name))
+			return false;
+		// now check the base-version
+		var framework = Framework.open(framework_name);
+		if (base_version == "")
+			base_version = framework.get_base_version();
+		if (base_version != framework.get_base_version())
+			return false;
+	}
+	return true;
+}
+
+private bool
+validate_framework_for_package (DB db, string package, string? version)
+{
+	var manifest = read_manifest(db, package, version);
+	if (!manifest.has_member("framework"))
+		return true;
+	var required_frameworks = manifest.get_string_member("framework");
+	return validate_framework (required_frameworks);
+}
+
 private Json.Object
-read_manifest_hooks (DB db, string package, string? version)
+read_manifest (DB db, string package, string? version)
 	throws DatabaseError
 {
 	if (version == null)
@@ -69,15 +109,23 @@
 			 @"$package.manifest");
 		parser.load_from_file (manifest_path);
 		var manifest = parser.get_root ().get_object ();
-		if (! manifest.has_member ("hooks"))
-			return new Json.Object ();
-		var hooks = manifest.get_object_member ("hooks");
-		return hooks.ref ();
+		return manifest.ref ();
 	} catch (Error e) {
 		return new Json.Object ();
 	}
 }
 
+private Json.Object
+read_manifest_hooks (DB db, string package, string? version)
+	throws DatabaseError
+{
+	var manifest = read_manifest (db, package, version);
+	if (! manifest.has_member ("hooks"))
+		return new Json.Object ();
+	var hooks = manifest.get_object_member ("hooks");
+	return hooks.ref ();
+}
+
 private class PreviousEntry : Object, Gee.Hashable<PreviousEntry> {
 	public string path { get; construct; }
 	public string package { get; construct; }
@@ -889,10 +937,16 @@
 		var ret = new List<RelevantApp> ();
 		var hook_name = get_hook_name ();
 		foreach (var unpacked in get_all_packages (user_name)) {
-			var manifest = read_manifest_hooks
+			// if the app is not using a valid framework (anymore)
+			// we don't consider it relevant (anymore)
+			if (!validate_framework_for_package 
+				    (db, unpacked.package, unpacked.version))
+				continue;
+
+			var manifest_hooks = read_manifest_hooks
 				(db, unpacked.package, unpacked.version);
-			foreach (var app_name in manifest.get_members ()) {
-				var hooks = manifest.get_object_member
+			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
@@ -960,6 +1014,7 @@
 			unowned string package = app.package;
 			unowned string version = app.version;
 			unowned string app_name = app.app_name;
+
 			seen.add (@"$(package)_$(app_name)_$(version)");
 			if (is_user_level) {
 				var user_db = new User.for_user
@@ -1016,7 +1071,7 @@
  * @new_version: The new version of the package.
  * @user_name: (allow-none): A user name, or null.
  *
- * Run hooks following removal of a Click package.
+ * Run hooks following install of a Click package.
  *
  * If @user_name is null, only run system-level hooks.  If @user_name is not
  * null, only run user-level hooks for that user.