launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/python-debian-parse-depends into lp:launchpad.

Commit message:
Use python-debian rather than python-apt to parse dependencies.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/python-debian-parse-depends/+merge/266276

Use python-debian rather than python-apt to parse dependencies.

Upgrading python-apt to handle newer syntax is cumbersome.  I did look at excising all use of python-apt in favour of python-debian, which in theory looks possible, but replacing all the uses of apt_pkg.TagFile in a way that's exactly compatible is quite fiddly and best done separately.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/python-debian-parse-depends into lp:launchpad.

=== modified file 'lib/lp/archiveuploader/dscfile.py'
--- lib/lp/archiveuploader/dscfile.py	2015-07-29 04:37:25 +0000
+++ lib/lp/archiveuploader/dscfile.py	2015-07-29 17:19:38 +0000
@@ -24,9 +24,12 @@
 import os
 import shutil
 import tempfile
+import warnings
 
-import apt_pkg
-from debian.deb822 import Deb822Dict
+from debian.deb822 import (
+    Deb822Dict,
+    PkgRelation,
+    )
 from zope.component import getUtility
 
 from lp.app.errors import NotFoundError
@@ -394,16 +397,12 @@
                         "%s: invalid %s field produced by a broken version "
                         "of dpkg-dev (1.10.11)" % (self.filename, field_name))
                 try:
-                    apt_pkg.parse_src_depends(field)
-                except (SystemExit, KeyboardInterrupt):
-                    raise
-                except Exception as error:
-                    # Swallow everything apt_pkg throws at us because
-                    # it is not desperately pythonic and can raise odd
-                    # or confusing exceptions at times and is out of
-                    # our control.
+                    with warnings.catch_warnings():
+                        warnings.simplefilter("error")
+                        PkgRelation.parse_relations(field)
+                except Warning as error:
                     yield UploadError(
-                        "%s: invalid %s field; cannot be parsed by apt: %s"
+                        "%s: invalid %s field; cannot be parsed by deb822: %s"
                         % (self.filename, field_name, error))
 
         # Verify if version declared in changesfile is the same than that

=== modified file 'lib/lp/registry/browser/sourcepackage.py'
--- lib/lp/registry/browser/sourcepackage.py	2015-01-29 16:28:30 +0000
+++ lib/lp/registry/browser/sourcepackage.py	2015-07-29 17:19:38 +0000
@@ -21,7 +21,6 @@
 import urllib
 
 from apt_pkg import (
-    parse_src_depends,
     upstream_version,
     version_compare,
     )
@@ -505,12 +504,10 @@
     def _relationship_parser(self, content):
         """Wrap the relationship_builder for SourcePackages.
 
-        Define apt_pkg.parse_src_depends as a relationship 'parser' and
-        IDistroSeries.getBinaryPackage as 'getter'.
+        Define IDistroSeries.getBinaryPackage as a relationship 'getter'.
         """
         getter = self.context.distroseries.getBinaryPackage
-        parser = parse_src_depends
-        return relationship_builder(content, parser=parser, getter=getter)
+        return relationship_builder(content, getter=getter)
 
     @property
     def builddepends(self):

=== modified file 'lib/lp/soyuz/browser/binarypackagerelease.py'
--- lib/lp/soyuz/browser/binarypackagerelease.py	2012-02-17 04:09:06 +0000
+++ lib/lp/soyuz/browser/binarypackagerelease.py	2015-07-29 17:19:38 +0000
@@ -8,8 +8,6 @@
     'BinaryPackageView',
     ]
 
-from apt_pkg import parse_depends
-
 from lp.services.webapp import Navigation
 from lp.services.webapp.publisher import LaunchpadView
 from lp.soyuz.browser.packagerelationship import relationship_builder
@@ -26,12 +24,10 @@
     def _relationship_parser(self, content):
         """Wrap the relationship_builder for BinaryPackages.
 
-        Define apt_pkg.ParseDep as a relationship 'parser' and
-        IDistroArchSeries.getBinaryPackage as 'getter'.
+        Define IDistroArchSeries.getBinaryPackage as a relationship 'getter'.
         """
         getter = self.context.build.distro_arch_series.getBinaryPackage
-        parser = parse_depends
-        return relationship_builder(content, parser=parser, getter=getter)
+        return relationship_builder(content, getter=getter)
 
     def depends(self):
         return self._relationship_parser(self.context.depends)

=== modified file 'lib/lp/soyuz/browser/packagerelationship.py'
--- lib/lp/soyuz/browser/packagerelationship.py	2015-07-08 16:05:11 +0000
+++ lib/lp/soyuz/browser/packagerelationship.py	2015-07-29 17:19:38 +0000
@@ -12,6 +12,7 @@
 
 import operator as std_operator
 
+from debian.deb822 import PkgRelation
 from zope.interface import implementer
 
 from lp.services.webapp import canonical_url
@@ -21,11 +22,11 @@
     )
 
 
-def relationship_builder(relationship_line, parser, getter):
+def relationship_builder(relationship_line, getter):
     """Parse relationship_line into a IPackageRelationshipSet.
 
-    'relationship_line' is parsed via given 'parser' funcion
-    It also lookup the corresponding URL via the given 'getter'.
+    'relationship_line' is parsed via PkgRelation.parse_relations.
+    It also looks up the corresponding URL via the given 'getter'.
     Return empty list if no line is given.
     """
     relationship_set = PackageRelationshipSet()
@@ -34,20 +35,20 @@
         return relationship_set
 
     parsed_relationships = [
-        token[0] for token in parser(relationship_line)]
+        token[0] for token in PkgRelation.parse_relations(relationship_line)]
 
-    for name, version, operator in parsed_relationships:
+    for rel in parsed_relationships:
+        name = rel['name']
         target_object = getter(name)
         if target_object is not None:
             url = canonical_url(target_object)
         else:
             url = None
-        # The apt_pkg 0.8 API returns '<' and '>' rather than the '<<' and
-        # '>>' form used in control files.
-        if operator == '<':
-            operator = '<<'
-        elif operator == '>':
-            operator = '>>'
+        if rel['version'] is None:
+            operator = ''
+            version = ''
+        else:
+            operator, version = rel['version']
         relationship_set.add(name, operator, version, url)
 
     return relationship_set

=== modified file 'lib/lp/soyuz/doc/package-relationship.txt'
--- lib/lp/soyuz/doc/package-relationship.txt	2014-01-17 02:02:31 +0000
+++ lib/lp/soyuz/doc/package-relationship.txt	2015-07-29 17:19:38 +0000
@@ -29,25 +29,27 @@
   ...    'gcc-3.4-base, libc6 (>= 2.3.2.ds1-4), gcc-3.4 ( = 3.4.1-4sarge1)')
 
 Launchpad models package relationship elements via the
-IPackageRelationShip instance. We use APT to parse the relationship
+IPackageRelationship instance. We use deb822 to parse the relationship
 lines:
 
-  >>> from apt_pkg import parse_depends
-
-parse_depends returns a 'list of lists of tuples' as:
-
-  [ [('$NAME', '$VERSION', '$OPERATOR')],
-    [('$NAME', '$VERSION', '$OPERATOR')],
+  >>> from debian.deb822 import PkgRelation
+
+PkgRelation.parse_relations returns a 'list of lists of dicts' as:
+
+  [ [{'name': '$NAME', 'version': ('$OPERATOR', '$VERSION')}],
+    [{'name': '$NAME', 'version': ('$OPERATOR', '$VERSION')}],
     ... ]
 
-So we need perform a small treatment on its result in order to have
-*sane* data:
-
-  >>> parsed_relationships = [relationship for (relationship,) in
-  ...                         parse_depends(relationship_line)]
-
-Let's see what we call 'sane':
-
+So we need to massage its result into the form we prefer:
+
+  >>> def parse_relations(line):
+  ...     for (rel,) in PkgRelation.parse_relations(relationship_line):
+  ...         if rel['version'] is None:
+  ...             operator, version = '', ''
+  ...         else:
+  ...             operator, version = rel['version']
+  ...         yield rel['name'], version, operator
+  >>> parsed_relationships = list(parse_relations(relationship_line))
   >>> parsed_relationships
   [('gcc-3.4-base', '', ''), ('libc6', '2.3.2.ds1-4', '>='), ('gcc-3.4', '3.4.1-4sarge1', '=')]
 
@@ -74,8 +76,3 @@
   '2.3.2.ds1-4'
   >>> pkg_relationship.url == fake_url
   True
-
-
-
-
-

=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2015-07-14 10:57:46 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2015-07-29 17:19:38 +0000
@@ -17,8 +17,10 @@
     attrgetter,
     itemgetter,
     )
+import warnings
 
 import apt_pkg
+from debian.deb822 import PkgRelation
 import pytz
 from sqlobject import SQLObjectNotFound
 from storm.expr import (
@@ -508,26 +510,19 @@
         Return a triple containing the corresponding (name, version,
         relation) for the given dependency token.
         """
-        try:
-            name, version, relation = token
-        except ValueError:
-            raise AssertionError(
-                "APT is not dealing correctly with a dependency token "
-                "'%r' from %s (%s) with the following dependencies: %s\n"
-                "It is expected to be a tuple containing only another "
-                "tuple with 3 elements  (name, version, relation)."
-                % (token, self.title, self.id, self.dependencies))
-        # Map relations to the canonical form used in control files.
-        if relation == '<':
-            relation = '<<'
-        elif relation == '>':
-            relation = '>>'
-        return (name, version, relation)
+        assert 'name' in token
+        assert 'version' in token
+        if token['version'] is None:
+            relation = ''
+            version = ''
+        else:
+            relation, version = token['version']
+        return (token['name'], version, relation)
 
     def _checkDependencyVersion(self, available, required, relation):
         """Return True if the available version satisfies the context."""
         # This dict maps the package version relationship syntax in lambda
-        # functions which returns boolean according to the results of
+        # functions which returns boolean according to the results of the
         # apt_pkg.version_compare function (see the order above).
         # For further information about pkg relationship syntax see:
         #
@@ -579,13 +574,6 @@
 
         return False
 
-    def _toAptFormat(self, token):
-        """Rebuild dependencies line in apt format."""
-        name, version, relation = self._parseDependencyToken(token)
-        if relation and version:
-            return '%s (%s %s)' % (name, relation, version)
-        return '%s' % name
-
     def updateDependencies(self):
         """See `IBuild`."""
 
@@ -593,10 +581,12 @@
         # properly.
         apt_pkg.init_system()
 
-        # Check package build dependencies using apt_pkg
+        # Check package build dependencies using debian.deb822
         try:
-            parsed_deps = apt_pkg.parse_depends(self.dependencies)
-        except (ValueError, TypeError):
+            with warnings.catch_warnings():
+                warnings.simplefilter("error")
+                parsed_deps = PkgRelation.parse_relations(self.dependencies)
+        except (AttributeError, Warning):
             raise UnparsableDependencies(
                 "Build dependencies for %s (%s) could not be parsed: '%s'\n"
                 "It indicates that something is wrong in buildd-slaves."
@@ -605,11 +595,10 @@
         remaining_deps = []
         for or_dep in parsed_deps:
             if not any(self._isDependencySatisfied(token) for token in or_dep):
-                remaining_deps.append(
-                    " | ".join(self._toAptFormat(token) for token in or_dep))
+                remaining_deps.append(or_dep)
 
         # Update dependencies line
-        self.dependencies = u", ".join(remaining_deps)
+        self.dependencies = unicode(PkgRelation.str(remaining_deps))
 
     def __getitem__(self, name):
         return self.getBinaryPackageRelease(name)