Merge lp:~zyga/checkbox/fix-1476678 into lp:checkbox

[Zygmunt Krynicki]

[Zygmunt Krynicki (zyga)]

This is almost, but not quite. I need to see what's missing :-|

[Zygmunt Krynicki (zyga)]

Currently this crashes for me like this:

python3 manage.py packaging
CRITICAL plainbox.crashes: Executable 'manage.py' invoked with Namespace(command=<plainbox.provider_manager.PackagingCommand object at 0x7fa10e173518>, debug_console=False, debug_interrupt=False, log_level=None, pdb=False, trace=[]) has crashed
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/plainbox/impl/clitools.py", line 560, in dispatch_and_catch_exceptions
    return self.dispatch_command(ns)
  File "/usr/lib/python3/dist-packages/plainbox/impl/clitools.py", line 556, in dispatch_command
    return ns.command.invoked(ns)
  File "/usr/lib/python3/dist-packages/plainbox/provider_manager.py", line 1295, in invoked
    driver.inspect_provider(provider)
  File "/usr/lib/python3/dist-packages/plainbox/impl/unit/packaging.py", line 260, in inspect_provider
    if self.is_applicable(unit):
  File "/usr/lib/python3/dist-packages/plainbox/impl/unit/packaging.py", line 250, in is_applicable
    if (not _strategy_id_version(unit, os_release)
  File "/usr/lib/python3/dist-packages/plainbox/impl/unit/packaging.py", line 224, in _strategy_id_version
    and unit.os_version_id == os_release['VERSION_ID'])
KeyError: 'VERSION_ID'

I think it's because 'sid' doesn't have VERSION_ID field. I'll patch the code to fix that.

[Zygmunt Krynicki (zyga)]

Yes, this is from sid:

cat /etc/os-release
PRETTY_NAME="Debian GNU/Linux stretch/sid"
NAME="Debian GNU/Linux"
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support/"
BUG_REPORT_URL="https://bugs.debian.org/"

Sigh :-) I'll report this as a separate bug. Let's land this

[Zygmunt Krynicki (zyga)]

Reported as https://bugs.launchpad.net/plainbox/+bug/1477095

[Zygmunt Krynicki (zyga)]

I'll do some more testing and review this myself. Separate review is welcome but if you don't want to test this all the way I can do that.

[lp:~zyga/checkbox/fix-1476678]

3909. By Sylvain Pineau on 2015-07-22

"Release_2015_Week30 [r=sylvain-pineau][bug=1331302,1341769,1347120,1382321,1383447,1387782,1387843,1388055,1388747,1389253,1399481,1400646,1403933,1406719,1410501,1428615,1451343,1451541][author=checkbox-dev]"

3910. By Zygmunt Krynicki on 2015-07-22

"automatic merge of lp:~zyga/checkbox/fix-1477131/ by tarmac [r=sylvain-pineau][bug=1477131][author=zyga]"

3911. By Zygmunt Krynicki on 2015-07-22

plainbox:unit:packaging: improve documentation

Signed-off-by: Zygmunt Krynicki <email address hidden>

3912. By Zygmunt Krynicki on 2015-07-22

plainbox:unit:packaging: fix parsing of Debian meta-data

This patch fixes how multiple-entry packaging meta-data units are parsed.
The fix makes the example in the documentation behave as intended.

Fixes: https://bugs.launchpad.net/plainbox/+bug/1476678
Signed-off-by: Zygmunt Krynicki <email address hidden>

[Sylvain Pineau (sylvain-pineau)]

+1, thanks for the fix

review: Approve

=== modified file 'plainbox/plainbox/impl/unit/packaging.py'
--- plainbox/plainbox/impl/unit/packaging.py	2015-07-22 12:57:27 +0000
+++ plainbox/plainbox/impl/unit/packaging.py	2015-07-22 13:45:52 +0000
@@ -114,6 +114,7 @@
 import abc
 import errno
 import logging
+import re
 import sys
 
 from plainbox.i18n import gettext as _
@@ -142,12 +143,12 @@
 
     @property
     def os_id(self):
-        """ Identifier of the operating system.  """
+        """Identifier of the operating system."""
         return self.get_record_value(self.Meta.fields.os_id)
 
     @property
     def os_version_id(self):
-        """ Version of the operating system.  """
+        """Version of the operating system."""
         return self.get_record_value(self.Meta.fields.os_version_id)
 
     class Meta:
@@ -156,7 +157,7 @@
 
         class fields(SymbolDef):
 
-            """ Symbols for each field of a packaging meta-data unit. """
+            """Symbols for each field of a packaging meta-data unit."""
 
             os_id = 'os-id'
             os_version_id = 'os-version-id'
@@ -174,46 +175,108 @@
 
 class PackagingDriverError(Exception):
 
-    """ Base for all packaging driver exceptions. """
+    """Base for all packaging driver exceptions."""
 
 
 class NoPackagingDetected(PackagingDriverError):
 
-    """ Exception raised when packaging cannot be found. """
+    """Exception raised when packaging cannot be found."""
 
 
 class NoApplicableBinaryPackages(PackagingDriverError):
 
-    """ Exception raised when no applicable binary packages are found. """
+    """Exception raised when no applicable binary packages are found."""
 
 
 class IPackagingDriver(metaclass=abc.ABCMeta):
 
-    """ Interface for all packaging drivers. """
+    """Interface for all packaging drivers."""
 
     @abc.abstractmethod
     def __init__(self, os_release: 'Dict[str, str]'):
-        pass
+        """
+        Initialize the packaging driver.
+
+        :param os_release:
+            The dictionary that represents the contents of the
+            ``/etc/os-release`` file. Using this file the packaging driver can
+            infer information about the target operating system that the
+            packaging will be built for.
+
+            This assumes that packages are built natively, not through a
+            cross-compiler of some sort where the target distribution is
+            different from the host distribution.
+        """
 
     @abc.abstractmethod
     def inspect_provider(self, provider: 'Provider1') -> None:
-        pass
+        """
+        Inspect a provider looking for packaging meta-data.
+
+        :param provider:
+            A provider object to look at. All of the packaging meta-data units
+            there are inspected, if they are applicable (see
+            :meth:`is_applicable()`. Information from applicable units is
+            collected using the :meth:`collect()` method.
+        """
 
     @abc.abstractmethod
     def is_applicable(self, unit: Unit) -> bool:
-        pass
+        """
+        Check if the given unit is applicable for collecting.
+
+        :param unit:
+            The unit to inspect. This doesn't have to be a packaging meta-data
+            unit. In fact, all units are checked with this method.
+        :returns:
+            True if the unit is applicable for collection.
+
+        Packaging meta-data units that have certain properties are applicable.
+        Refer to the documentation of the module for details.
+        """
 
     @abc.abstractmethod
     def collect(self, unit: Unit) -> None:
-        pass
+        """
+        Collect information from the given applicable unit.
+
+        :param unit:
+            The unit to collect information from. This is usually expressed as
+            additional fields that are specific to the type of native packaging
+            for the system.
+
+        Collected information is recorded and made available for the
+        :meth:`modify_packaging_tree()` method later.
+        """
 
     @abc.abstractmethod
     def inspect_packaging(self) -> None:
-        pass
+        """
+        Inspect the packaging tree for additional information.
+
+        :raises NoPackagingDetected:
+            Exception raised when packaging cannot be found.
+        :raises NoApplicableBinaryPackages:
+            Exception raised when no applicable binary packages are found.
+
+        This method looks at the packaging system located in the current
+        directory. This can be the ``debian/`` directory, a particular
+        ``.spec`` file or anything else. Information obtained from the package
+        is used to infer additional properties that can aid in the packaging
+        process.
+        """
 
     @abc.abstractmethod
     def modify_packaging_tree(self) -> None:
-        pass
+        """
+        Modify the packaging tree with information from the packaging units.
+
+        This method uses all of the available information collected from
+        particular packaging meta-data units and from the native packaging to
+        modify the packaging. Additional dependencies may be injected in
+        appropriate places. Please refer to the documentation specific to your
+        packaging system for details.
+        """
 
 
 def _strategy_id_version(unit, os_release):
@@ -237,7 +300,7 @@
 
 class PackagingDriverBase(IPackagingDriver):
 
-    """ Base implementation of a packaging driver. """
+    """Base implementation of a packaging driver."""
 
     def __init__(self, os_release: 'Dict[str, str]'):
         self.os_release = os_release
@@ -262,7 +325,12 @@
 
 class NullPackagingDriver(PackagingDriverBase):
 
-    """ Null implementation of a packaging driver. """
+    """
+    Null implementation of a packaging driver.
+
+    This driver just does nothing at all. It is used as a fall-back when
+    nothing better is detected.
+    """
 
     def is_applicable(self, unit: Unit) -> bool:
         return False
@@ -316,10 +384,14 @@
     def collect(self, unit: Unit) -> None:
         def rel_list(field):
             relations = unit.get_record_value(field, '').replace('\n', ' ')
-            return ', '.join([rel.strip() for rel in relations.split(',')])
-        self._depends.append(rel_list('Depends'))
-        self._suggests.append(rel_list('Suggests'))
-        self._recommends.append(rel_list('Recommends'))
+            return (
+                rel.strip()
+                for rel in re.split(', *', relations)
+                if rel.strip()
+            )
+        self._depends.extend(rel_list('Depends'))
+        self._suggests.extend(rel_list('Suggests'))
+        self._recommends.extend(rel_list('Recommends'))
 
     def _write_pkg_substvars(self, pkg):
         fname = 'debian/{}.substvars'.format(pkg)
@@ -357,7 +429,7 @@
 
 
 def get_packaging_driver() -> IPackagingDriver:
-    """ Get the packaging driver appropriate for the current platform. """
+    """Get the packaging driver appropriate for the current platform."""
     if sys.platform.startswith("linux"):
         os_release = get_os_release()
         if (os_release.get('ID') == 'debian'
 
=== added file 'plainbox/plainbox/impl/unit/test_packging.py'
--- plainbox/plainbox/impl/unit/test_packging.py	1970-01-01 00:00:00 +0000
+++ plainbox/plainbox/impl/unit/test_packging.py	2015-07-22 13:45:52 +0000
@@ -0,0 +1,54 @@
+# This file is part of Checkbox.
+#
+# Copyright 2015 Canonical Ltd.
+# Written by:
+#   Zygmunt Krynicki <[e-mail]>
+#
+# Checkbox is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3,
+# as published by the Free Software Foundation.
+#
+# Checkbox 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 Checkbox.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Tests for the PackagingMetaDataUnit and friends."""
+
+from unittest import TestCase
+
+from plainbox.impl.unit.packaging import DebianPackagingDriver
+from plainbox.impl.unit.packaging import PackagingMetaDataUnit
+
+
+class DebianPackagingDriverTests(TestCase):
+
+    """Tests for the DebianPackagingDriver class."""
+
+    def test_fix_1476678(self):
+        """Check https://bugs.launchpad.net/plainbox/+bug/1476678."""
+        driver = DebianPackagingDriver({})
+        driver.collect(PackagingMetaDataUnit({
+            'Depends': (
+                'python3-checkbox-support (>= 0.2),\n'
+                'python3 (>= 3.2),\n'),
+            'Recommends': (
+                'dmidecode,\n'
+                'dpkg (>= 1.13),\n'
+                'lsb-release,\n'
+                'wodim')
+        }))
+        self.assertEqual(driver._depends, [
+            'python3-checkbox-support (>= 0.2)',
+            'python3 (>= 3.2)',
+        ])
+        self.assertEqual(driver._recommends, [
+            'dmidecode',
+            'dpkg (>= 1.13)',
+            'lsb-release',
+            'wodim'
+        ])
+        self.assertEqual(driver._suggests, [])