Merge lp:~mvo/click/multiple-frameworks into lp:click

[Michael Vogt]

This branch adds support for multiple frameworks in a click manifest.

To support multiple frameworks there is now a "click/framework.py" file that contains a new "validate_framework()" method that is used by build.py and install.py to validate the given framework string.

validate_framework() will ensure that:
- the base version is the same for all frameworks used
- no additional relations like >= or similar is used in the framework declaration string
- no or dependencies (foo|bar) is used in the framework declaration string

It is possible to override if missing_frameworks is a error or a warning. Logging is used when this is overriden. I'm happy to use a different mechanism if logging is inappropriate.

The test helper gets a new click.test.helpers.TestCase._create_mock_framework_{dir,file} that is used in both test_build.py and test_install.py

I currently use the pure-python version of click_framework_get_base_version() - we could use the native version when available but I wonder if that is really needed given that both build and install are already in python so the overhead of using the python version seems to be not a issue here.

If there is anything else you would like me to change or do differently, please just let me know, happy to do it.

Thanks!
 Michael

[Colin Watson (cjwatson)]

We ought to avoid exposing the CLICK_FRAMEWORKS_DIR environment variable interface when it's only for tests; tests should be able to install mock functions as appropriate to override the frameworks directory. And as you say the default framework directory should be generated by configure - I suspect that undoing the change to click/paths.py.in from r372.1.1 would be enough for this.

However, this doesn't need to block merging, and you can clean it up at your leisure. Other than that I see nothing to complain about here. Thanks!

review: Approve

=== modified file 'click/build.py'
--- click/build.py	2014-01-23 17:19:09 +0000
+++ click/build.py	2014-05-05 13:23:45 +0000
@@ -50,6 +50,11 @@
 from click.preinst import static_preinst
 from click.versions import spec_version
 
+from click.framework import (
+    validate_framework,
+    ClickFrameworkInvalid,
+)
+
 
 @contextlib.contextmanager
 def make_temp_dir():
@@ -196,30 +201,13 @@
             package.add_file("control.tar.gz", control_tar_path)
             package.add_file("data.tar.gz", data_tar_path)
 
-    def _validate_framework(self, framework):
+    def _validate_framework(self, framework_string):
         """Apply policy checks to framework declarations."""
         try:
-            apt_pkg
-        except NameError:
-            return
-
-        try:
-            parsed_framework = apt_pkg.parse_depends(framework)
-        except ValueError:
-            raise ClickBuildError('Could not parse framework "%s"' % framework)
-        if len(parsed_framework) > 1:
-            raise ClickBuildError(
-                'Multiple dependencies in framework "%s" not yet allowed' %
-                framework)
-        for or_dep in parsed_framework:
-            if len(or_dep) > 1:
-                raise ClickBuildError(
-                    'Alternative dependencies in framework "%s" not yet '
-                    'allowed' % framework)
-            if or_dep[0][1] or or_dep[0][2]:
-                raise ClickBuildError(
-                    'Version relationship in framework "%s" not yet allowed' %
-                    framework)
+            validate_framework(
+                framework_string, ignore_missing_frameworks=True)
+        except ClickFrameworkInvalid as e:
+            raise ClickBuildError(str(e))
 
     def build(self, dest_dir, manifest_path="manifest.json"):
         with make_temp_dir() as temp_dir:

=== added file 'click/framework.py'
--- click/framework.py	1970-01-01 00:00:00 +0000
+++ click/framework.py	2014-05-05 13:23:45 +0000
@@ -0,0 +1,135 @@
+# Copyright (C) 2014 Canonical Ltd.
+# Author: Michael Vogt <[e-mail]>
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 3 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Pure python click framework handling support."""
+
+import logging
+import os
+import re
+
+try:
+    import apt_pkg
+except:
+    pass
+
+
+class ClickFrameworkInvalid(Exception):
+    pass
+
+
+# FIXME: use native lib if available
+#from gi.repository import Click
+#click_framework_get_base_version = Click.framework_get_base_version
+#click_framework_has_framework = Click.has_framework
+
+
+# python version of the vala parse_deb822_file()
+def parse_deb822_file(filename):
+    data = {}
+    with open(filename) as f:
+        for line in f:
+            line = line.strip()
+            # from deb822.vala
+            field_re_posix = r'^([^:[:space:]]+)[[:space:]]*:[[:space:]]'\
+                       '([^[:space:]].*?)[[:space:]]*$'
+            # python does not do posix char classes
+            field_re = field_re_posix.replace("[:space:]", "\s")
+            blank_re_posix = r'^[[:space:]]*$'
+            blank_re = blank_re_posix.replace("[:space:]", "\s")
+            if re.match(blank_re, line):
+                break
+            match = re.match(field_re, line)
+            if match and match.group(1) and match.group(2):
+                data[match.group(1).lower()] = match.group(2)
+    return data
+
+
+# python version of vala get_frameworks_dir
+def get_frameworks_dir(framework_name):
+    # FIXME: get via configure.in etc?
+    frameworks_dir = os.environ.get(
+        "CLICK_FRAMEWORKS_DIR", "/usr/share/click/frameworks")
+    framework_path = os.path.join(
+        frameworks_dir, framework_name+".framework")
+    return framework_path
+
+
+# python version of the vala click_framework_get_base_version()
+def click_framework_get_base_version(framework_name):
+    deb822 = parse_deb822_file(get_frameworks_dir(framework_name))
+    return deb822.get("base-version", None)
+
+
+# python version of the vala click_framework_has_framework
+def click_framework_has_framework(framework_name):
+    return os.path.exists(get_frameworks_dir(framework_name))
+
+
+def validate_framework(framework_string, ignore_missing_frameworks=False):
+    try:
+        apt_pkg
+    except NameError:
+        logging.warning("No apt_pkg module, skipping validate_framework")
+        return
+
+    try:
+        parsed_framework = apt_pkg.parse_depends(framework_string)
+    except ValueError:
+        raise ClickFrameworkInvalid(
+            'Could not parse framework "%s"' % framework_string)
+
+    framework_base_versions = set()
+    missing_frameworks = []
+    for or_dep in parsed_framework:
+        if len(or_dep) > 1:
+            raise ClickFrameworkInvalid(
+                'Alternative dependencies in framework "%s" not yet '
+                'allowed' % framework_string)
+        if or_dep[0][1] or or_dep[0][2]:
+            raise ClickFrameworkInvalid(
+                'Version relationship in framework "%s" not yet allowed' %
+                framework_string)
+        # now verify that different base versions are not mixed
+        framework_name = or_dep[0][0]
+        if not click_framework_has_framework(framework_name):
+            missing_frameworks.append(framework_name)
+            continue
+        framework_base_version = click_framework_get_base_version(
+                framework_name)
+        framework_base_versions.add(framework_base_version)
+
+    if not ignore_missing_frameworks:
+        if len(missing_frameworks) > 1:
+            raise ClickFrameworkInvalid(
+                'Frameworks %s not present on system (use '
+                '--force-missing-framework option to override)' %
+                ", ".join('"%s"' % f for f in missing_frameworks))
+        elif missing_frameworks:
+            raise ClickFrameworkInvalid(
+                'Framework "%s" not present on system (use '
+                '--force-missing-framework option to override)' %
+                missing_frameworks[0])
+    else:
+        if len(missing_frameworks) > 1:
+            logging.warning("Ignoring missing frameworks %s" % (
+                ", ".join('"%s"' % f for f in missing_frameworks)))
+        elif missing_frameworks:
+            logging.warning('Ignoring missing framework "%s"' % (
+                missing_frameworks[0]))
+
+    if len(framework_base_versions) > 1:
+        raise ClickFrameworkInvalid(
+            'Multiple frameworks with different base versions are not '
+            'allowed. Found: {0}'.format(framework_base_versions))

=== modified file 'click/install.py'
--- click/install.py	2014-04-03 08:52:02 +0000
+++ click/install.py	2014-05-05 13:23:45 +0000
@@ -49,6 +49,11 @@
 from click.preinst import static_preinst_matches
 from click.versions import spec_version
 
+from click.framework import (
+    validate_framework,
+    ClickFrameworkInvalid,
+)
+
 
 try:
     _DebFile.close
@@ -189,34 +194,9 @@
                 raise ClickInstallerAuditError(
                     'No "framework" entry in manifest')
             try:
-                parsed_framework = apt_pkg.parse_depends(framework)
-            except ValueError:
-                raise ClickInstallerAuditError(
-                    'Could not parse framework "%s"' % framework)
-            for or_dep in parsed_framework:
-                if len(or_dep) > 1:
-                    raise ClickInstallerAuditError(
-                        'Alternative dependencies in framework "%s" not yet '
-                        'allowed' % framework)
-                if or_dep[0][1] or or_dep[0][2]:
-                    raise ClickInstallerAuditError(
-                        'Version relationship in framework "%s" not yet '
-                        'allowed' % framework)
-            if not self.force_missing_framework:
-                missing_frameworks = []
-                for or_dep in parsed_framework:
-                    if not Click.Framework.has_framework(or_dep[0][0]):
-                        missing_frameworks.append(or_dep[0][0])
-                if len(missing_frameworks) > 1:
-                    raise ClickInstallerAuditError(
-                        'Frameworks %s not present on system (use '
-                        '--force-missing-framework option to override)' %
-                        ", ".join('"%s"' % f for f in missing_frameworks))
-                elif missing_frameworks:
-                    raise ClickInstallerAuditError(
-                        'Framework "%s" not present on system (use '
-                        '--force-missing-framework option to override)' %
-                        missing_frameworks[0])
+                validate_framework(framework, self.force_missing_framework)
+            except ClickFrameworkInvalid as e:
+                raise ClickInstallerAuditError(str(e))
 
             if check_arch:
                 architecture = manifest.get("architecture", "all")

=== modified file 'click/tests/helpers.py'
--- click/tests/helpers.py	2014-03-10 14:49:27 +0000
+++ click/tests/helpers.py	2014-05-05 13:23:45 +0000
@@ -30,6 +30,7 @@
 
 import contextlib
 import os
+import re
 import shutil
 import sys
 import tempfile
@@ -107,6 +108,32 @@
         self.assertRaisesGError(
             "click_user_error-quark", code, callableObj, *args, **kwargs)
 
+    def _create_mock_framework_dir(self, frameworks_dir=None):
+        if frameworks_dir is None:
+            frameworks_dir = os.path.join(self.temp_dir, "frameworks")
+        Click.ensuredir(frameworks_dir)
+        os.environ["CLICK_FRAMEWORKS_DIR"] = frameworks_dir
+        return frameworks_dir
+
+    def _create_mock_framework_file(self, framework_name):
+        self.use_temp_dir()
+        self._create_mock_framework_dir()
+        r = r'(?P<name>[a-z]+-sdk)-(?P<ver>[0-9.]+)(-[a-z0-9-]+)?'
+        match = re.match(r, framework_name)
+        if match is None:
+            name = "unknown"
+            ver = "1.0"
+        else:
+            name = match.group("name")
+            ver = match.group("ver")
+        framework_filename = os.path.join(
+            self.temp_dir, "frameworks",
+            "{0}.framework".format(framework_name))
+        with open(framework_filename, "w") as f:
+            f.write("Base-Name: {0}\n".format(name))
+            f.write("Base-Version: {0}\n".format(ver))
+
+
 
 if not hasattr(mock, "call"):
     # mock 0.7.2, the version in Ubuntu 12.04 LTS, lacks mock.ANY and

=== modified file 'click/tests/test_build.py'
--- click/tests/test_build.py	2014-01-23 17:19:09 +0000
+++ click/tests/test_build.py	2014-05-05 13:23:45 +0000
@@ -245,6 +245,7 @@
             del target_json["installed-size"]
             self.assertEqual(source_json, target_json)
 
+    # FIXME: DRY violation with test_build_multiple_architectures etc
     def test_build_multiple_frameworks(self):
         self.use_temp_dir()
         scratch = os.path.join(self.temp_dir, "scratch")
@@ -259,11 +260,44 @@
                     "ubuntu-sdk-14.04-basic, ubuntu-sdk-14.04-webapps",
             }, f)
         self.builder.add_file(scratch, "/")
-        self.assertRaisesRegex(
-            ClickBuildError,
-            'Multiple dependencies in framework "ubuntu-sdk-14.04-basic, '
-            'ubuntu-sdk-14.04-webapps" not yet allowed',
-            self.builder.build, self.temp_dir)
+        path = self.builder.build(self.temp_dir)
+        control_path = os.path.join(self.temp_dir, "control")
+        subprocess.check_call(["dpkg-deb", "-e", path, control_path])
+        manifest_path = os.path.join(control_path, "manifest")
+        with open(os.path.join(scratch, "manifest.json")) as source, \
+                open(manifest_path) as target:
+            source_json = json.load(source)
+            target_json = json.load(target)
+            del target_json["installed-size"]
+            self.assertEqual(source_json, target_json)
+
+
+class TestClickFrameworkValidation(TestCase):
+    def setUp(self):
+        super(TestClickFrameworkValidation, self).setUp()
+        self.builder = ClickBuilder()
+        for framework_name in ("ubuntu-sdk-13.10",
+                               "ubuntu-sdk-14.04-papi",
+                               "ubuntu-sdk-14.04-html"):
+            self._create_mock_framework_file(framework_name)
+
+    def test_validate_framework_good(self):
+        valid_framework_values = (
+            "ubuntu-sdk-13.10",
+            "ubuntu-sdk-14.04-papi, ubuntu-sdk-14.04-html",
+        )
+        for framework in valid_framework_values:
+            self.builder._validate_framework(framework)
+
+    def test_validate_framework_bad(self):
+        invalid_framework_values = (
+            "ubuntu-sdk-13.10, ubuntu-sdk-14.04-papi",
+            "ubuntu-sdk-13.10 (>= 13.10)",
+            "ubuntu-sdk-13.10 | ubuntu-sdk-14.04",
+        )
+        for framework in invalid_framework_values:
+            with self.assertRaises(ClickBuildError):
+                self.builder._validate_framework(framework)
 
 
 class TestClickSourceBuilder(TestCase, TestClickBuilderBaseMixin):

=== modified file 'click/tests/test_install.py'
--- click/tests/test_install.py	2014-04-03 08:52:02 +0000
+++ click/tests/test_install.py	2014-05-05 13:23:45 +0000
@@ -104,12 +104,10 @@
         return package_path
 
     def _setup_frameworks(self, preloads, frameworks_dir=None, frameworks=[]):
-        if frameworks_dir is None:
-            frameworks_dir = os.path.join(self.temp_dir, "frameworks")
+        frameworks_dir = self._create_mock_framework_dir(frameworks_dir)
         shutil.rmtree(frameworks_dir, ignore_errors=True)
-        Click.ensuredir(frameworks_dir)
         for framework in frameworks:
-            touch(os.path.join(frameworks_dir, "%s.framework" % framework))
+            self._create_mock_framework_file(framework)
         preloads["click_get_frameworks_dir"].side_effect = (
             lambda: self.make_string(frameworks_dir))