Merge lp:~mvo/click/fix-multiple-framework-validation into lp:click/devel

[Michael Vogt]

This branch fixes a error when click encounters multiple frameworks
like "ubuntu-core-15.04-dev1, docker-1.3". The current check will
raise a error because it assumes all base-names are equal. This is
no longer the case in snappy and the new code checks base-name properly
now.

[Barry Warsaw (barry)]

Two minor observations, but otherwise LGTM.

review: Approve

[lp:~mvo/click/fix-multiple-framework-validation]

548. By Michael Vogt on 2015-02-18

address review comments from barry

[Michael Vogt (mvo)]

Hi Barry,

thanks for your review!

On Tue, Feb 17, 2015 at 04:03:34PM -0000, Barry Warsaw wrote:
[..]
> Would it make sense to wrap this in `try/except ImportError` clauses? I know that gets a bit verbose, but at least you wouldn't have these commented out lines and could automatically use the native libs if available.

Thanks, I was thinking about this a little bit and I think this block
can just be removed. Its a goal of click build to run on pure
python. So we need the function anyway and can as well use them :)

[..]
> > + framework_base_name = click_framework_get_base_name(
> > + framework_name)
> > framework_base_version = click_framework_get_base_version(
> > framework_name)
> > - framework_base_versions.add(framework_base_version)
> > + if (framework_base_name in base_name_versions and
> > + framework_base_version != base_name_versions[framework_base_name]):
>
> What about (w/abbreviations due to wrapping):
>
> prev = base_name_versions.get(framework_base_name, framework_base_version)
> if framework_base_version != prev:
>
> I guess it may not save that much.

I like this idea! Commited.

Feel free to set to "approved" if you feel its ready.

Thanks,
 Michael

=== modified file 'click/framework.py'
--- click/framework.py	2014-06-23 21:14:06 +0000
+++ click/framework.py	2015-02-18 09:17:34 +0000
@@ -31,12 +31,6 @@
     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 = {}
@@ -76,6 +70,12 @@
     return deb822.get("base-version", None)
 
 
+# python version of the vala click_framework_get_base_version()
+def click_framework_get_base_name(framework_name):
+    deb822 = parse_deb822_file(get_framework_path(framework_name))
+    return deb822.get("base-name", None)
+
+
 # python version of the vala click_framework_has_framework
 def click_framework_has_framework(framework_name):
     return os.path.exists(get_framework_path(framework_name))
@@ -94,7 +94,7 @@
         raise ClickFrameworkInvalid(
             'Could not parse framework "%s"' % framework_string)
 
-    framework_base_versions = set()
+    base_name_versions = {}
     missing_frameworks = []
     for or_dep in parsed_framework:
         if len(or_dep) > 1:
@@ -110,9 +110,20 @@
         if not click_framework_has_framework(framework_name):
             missing_frameworks.append(framework_name)
             continue
+        # ensure we do not use different base versions for the same base-name
+        framework_base_name = click_framework_get_base_name(
+            framework_name)
         framework_base_version = click_framework_get_base_version(
             framework_name)
-        framework_base_versions.add(framework_base_version)
+        prev =  base_name_versions.get(framework_base_name, None)
+        if prev and prev != framework_base_version:
+            raise ClickFrameworkInvalid(
+                'Multiple frameworks with different base versions are not '
+                'allowed. Found: {} ({} != {})'.format(
+                    framework_base_name,
+                    framework_base_version,
+                    base_name_versions[framework_base_name]))
+        base_name_versions[framework_base_name] = framework_base_version
 
     if not ignore_missing_frameworks:
         if len(missing_frameworks) > 1:
@@ -132,8 +143,3 @@
         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/tests/test_build.py'
--- click/tests/test_build.py	2015-01-19 10:26:26 +0000
+++ click/tests/test_build.py	2015-02-18 09:17:34 +0000
@@ -289,13 +289,15 @@
         self.builder = ClickBuilder()
         for framework_name in ("ubuntu-sdk-13.10",
                                "ubuntu-sdk-14.04-papi",
-                               "ubuntu-sdk-14.04-html"):
+                               "ubuntu-sdk-14.04-html",
+                               "docker-sdk-1.3"):
             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",
+            "ubuntu-sdk-13.10, docker-sdk-1.3",
         )
         for framework in valid_framework_values:
             self.builder._validate_framework(framework)