Merge lp:~mvo/pkgme-devportal/less-silly-contents-file-based-db-backend into lp:~mvo/pkgme-devportal/run-specific-tests

[Michael Vogt]

This branch makes the apt-file/Conents-$(arch) backend more reasonable
and most importantly fast. With that we could use it to run the scoreboard
once with the regular DB backend and once with the aptfile backend.

[Jonathan Lange (jml)]

Thanks Michael.

The code is very clear and well-structured. I'd draw the lines in slightly different places, but I don't think they are necessary for this to land, or even necessary at all. "Approve with tweaks"

Minor typos:

 * iter_contents_file has a typo in its header, "wiht" vs "with"

 * _download_and_prepare_contents_file_if_needed has a typo in its docstring, "Conents" vs "Contents"

 * "interessted" vs "interested" in comments in same function

Some other suggestions:

 * _get_lib_to_pkgs_mapping
   * 'cachekey' seems unnecessary. Why not use a tuple as the key?
   * Python *almost* has something that does this for you (collections.defaultdict). How annoying.

In case you're curious, here's what I think I'd do (or at least would muck around with):

 * iter_contents_file
   * return a set of pkgs, rather than a comma-separated string
   * muck around with having it return a dict, or wrap it in something that returns a dict
   * split out the filtering code. These three together would make it a proper parser.

 * _get_mapping_from_contents_file
   * after those changes to iter_contents_file, this can be expressed in terms of 'filter' and 'reduce' (well, if only Python had dict versions of basic HOFs)

 * _download_and_prepare_contents_file_if_needed
   * extract the bit inside the 'if' into a function (or method) that downloaded & prepared the file
   * make a new method that tried to open the contents file, and then called the download & prepare method if it didn't exist (if e.errno == ENOENT)
   * change _get_mapping_from_contents_file to use that new 'open' method

Basically, I have a prejudice against look-before-you-leap and another prejudice toward functional programming.

cheers,
jml

review: Approve

[lp:~mvo/pkgme-devportal/less-silly-contents-file-based-db-backend]

81. By Michael Vogt on 2012-08-16

fix typos (thanks to jml)

82. By Michael Vogt on 2012-08-16

devportalbinary/database.py: use (distroseries, arch) as cachekey instead of string, thanks to jml

[Michael Vogt (mvo)]

On Thu, Aug 16, 2012 at 12:01:31PM -0000, Jonathan Lange wrote:
> Review: Approve
>
> Thanks Michael.
>
> The code is very clear and well-structured.

Happy to hear that!

> I'd draw the lines in slightly different places, but I don't think they are necessary for this to land, or even necessary at all. "Approve with tweaks"

Thank you for your excellent review (as always :)

> Minor typos:
>
> * iter_contents_file has a typo in its header, "wiht" vs "with"
>
> * _download_and_prepare_contents_file_if_needed has a typo in its docstring, "Conents" vs "Contents"
>
> * "interessted" vs "interested" in comments in same function

Fixed in bzr now.

> Some other suggestions:
>
> * _get_lib_to_pkgs_mapping
> * 'cachekey' seems unnecessary. Why not use a tuple as the key?

Indeed, fixed too.

> * Python *almost* has something that does this for you
> (collections.defaultdict). How annoying.

:)
>
 In case you're curious, here's what I think I'd do (or at least would muck around with):
[..]
> Basically, I have a prejudice against look-before-you-leap and another prejudice toward functional programming.

I like your suggestions and will play around with them. I'm not sure I
will do all of them, but I want to apply them and see what the code looks
like then :) Unfortunately I did not get to this yet :/ But I will try
again early next week or after feature freeze (latest).

Thanks,
 Michael

[lp:~mvo/pkgme-devportal/less-silly-contents-file-based-db-backend]

83. By Michael Vogt on 2012-08-21

iter_contents_file yields a set of pkgnames now instead of a comma seperated string

84. By Michael Vogt on 2012-08-21

use a filter func for iter_contents_file() instead of the contents path hardcoding

85. By Michael Vogt on 2012-08-21

use the python buildin filter() instead of passing a filter func to iter_contents_file()

[Michael Vogt (mvo)]

I addressed some more points, its not doing all you suggested, but a reeval would be nice.

[Jonathan Lange (jml)]

Thanks! I hope you had fun with this.

jml

review: Approve

[Jonathan Lange (jml)]

I can't approved this for landing because it is proposed into your 'run-specific-tests'. I'm pretty sure tarmac won't pick it up either.

Could you please re-propose against trunk & I'll rubberstamp.

[lp:~mvo/pkgme-devportal/less-silly-contents-file-based-db-backend]

86. By Michael Vogt on 2012-08-23

fix configuration tests as the new default is the AptFilePackageDatabase backend

87. By Michael Vogt on 2012-08-23

devportalbinary/tests/test_database.py: use self.assertIsInstance() instead of trying to do the same manually, thanks to jml

Unmerged revisions

87. By Michael Vogt on 2012-08-23

devportalbinary/tests/test_database.py: use self.assertIsInstance() instead of trying to do the same manually, thanks to jml

86. By Michael Vogt on 2012-08-23

fix configuration tests as the new default is the AptFilePackageDatabase backend

85. By Michael Vogt on 2012-08-21

use the python buildin filter() instead of passing a filter func to iter_contents_file()

84. By Michael Vogt on 2012-08-21

use a filter func for iter_contents_file() instead of the contents path hardcoding

83. By Michael Vogt on 2012-08-21

iter_contents_file yields a set of pkgnames now instead of a comma seperated string

82. By Michael Vogt on 2012-08-16

devportalbinary/database.py: use (distroseries, arch) as cachekey instead of string, thanks to jml

81. By Michael Vogt on 2012-08-16

fix typos (thanks to jml)

80. By Michael Vogt on 2012-08-16

devportalbinary/tests/test_database.py: also test that headers are skipped

79. By Michael Vogt on 2012-08-16

add test docstrings

78. By Michael Vogt on 2012-08-16

another round of refactor/docstring cleanup

=== modified file 'README'
--- README	2012-08-15 14:24:43 +0000
+++ README	2012-08-21 16:41:20 +0000
@@ -39,7 +39,6 @@
 You can override the location of this configuration file by setting the
 environment variable ``PKGME_DEVPORTAL_CONFIG_FILE``.
 
-
 Postgres
 --------
 
@@ -59,15 +58,25 @@
   sudo -u postgres psql -f db/postgres_schema.sql
 
 
+Contents File (apt-file)
+------------------------
+Running in this mode is useful for running locally. It will download the
+archives "Contents-%(architecture)s" file on demand and build a local
+version of that to map from soname to pkgname.
+
+You can run pkgme locally using:
+ $ cd /path/to/binary/thing/you/want/packaged
+ $ echo '{ "package_name" : "mypkg", "description" : "some description" }' > devportal-metadata.json
+ $ PYTHONPATH=/path/to/pkgme:/path/to/pkgme-devport \
+   PKGME_BACKEND_PATHS=/path/to/pkgme-devportal/devportalbinary/backends/ \
+   pkgme
+
+
 Acceptance tests
 ================
 
 There is an acceptance test suite that is not run as part of the
-unit tests. You can run these tests in virtualenv with
-
-  python -m testtools.run acceptance.tests
-
-or in the rootdir of the checkout with
-
-  PYTHONPATH=$(pwd) python -m testtools.run acceptance.tests
+unit tests. You can run these tests with:
+
+ $ make acceptance
 

=== modified file 'devportalbinary/configuration.py'
--- devportalbinary/configuration.py	2012-07-27 13:16:09 +0000
+++ devportalbinary/configuration.py	2012-08-21 16:41:20 +0000
@@ -31,9 +31,9 @@
 
     # database
     database = Section()
-    database.db_type = StringOption(default='postgres',
-            help=('The database to use, "postgres" is the only supported '
-                  'value.'))
+    database.db_type = StringOption(default='aptfile',
+            help=('The database to use, "postgres", "aptfile" are supported '
+                  'values.'))
     database.host = StringOption(default=None,
             help='The database host (for postgres)')
     database.port = IntOption(default=None,
@@ -44,6 +44,8 @@
             help='The database password (for postgres)')
     database.db_name = StringOption(default=None,
             help='The database name (for postgres)')
+    database.aptfile_cachedir = StringOption(default="~/.cache/pkgme-devportal",
+            help='The cache directory for the aptfile backend')
 
     scan_mode = StringOption(
         help='To scan binary or source packages.')

=== modified file 'devportalbinary/database.py'
--- devportalbinary/database.py	2012-08-09 00:04:42 +0000
+++ devportalbinary/database.py	2012-08-21 16:41:20 +0000
@@ -2,10 +2,12 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import errno
+import gzip
 from itertools import chain
 import os
 import shutil
 import tempfile
+import urllib
 
 from bzrlib import urlutils
 from fixtures import (
@@ -267,6 +269,24 @@
         yield library, dependency
 
 
+def iter_contents_file(contents):
+    """ Yield (full-library-path, set-of-pkgnames) from a Contents file.
+    
+    It expects a line starting with "FILE" that tells it when the header ends
+    and the actual content starts. 
+    """
+    found_start_marker = False
+    for line in contents:
+        if not found_start_marker:
+            if line.startswith("FILE"):
+                found_start_marker = True
+            continue
+        (path, sep, pkgs) = [s.strip() for s in line.rpartition(" ")]
+        # pkgs is formated a bit funny, e.g. universe/pkgname
+        pkgs = set([os.path.basename(pkg) for pkg in pkgs.split(",")])
+        yield (path, pkgs)
+
+
 class URI(StormURI):
     """A stand-in for Storm's URI class.
 
@@ -289,10 +309,146 @@
             self.options = dict()
 
 
+class AptFilePackageDatabase(object):
+    """ Really dumb database that just uses apt-file for local testing """
+
+    # we could also read /etc/ld.so.conf.d/*.conf but this maybe different on 
+    # different distroseries especially if
+    #    server-distroseries != target-distroseries
+    #  (I wish there was ldconfig --print-search-dirs)
+    LD_SEARCH_PATH = [
+        # standards
+        "lib",
+        "usr/lib",
+        "usr/local/lib",
+        # old biarch
+        "lib32",
+        "usr/lib32",
+        # new multiarch
+        "lib/i686-linux-gnu",
+        "lib/i386-linux-gnu",
+        "lib/x86_64-linux-gnu",
+        "usr/lib/i386-linux-gnu",
+        "usr/lib/i686-linux-gnu",
+        "usr/lib/x86_64-linux-gnu",
+        # ?
+        "usr/lib/x86_64-linux-gnu/fakechroot",
+        "usr/lib/x86_64-linux-gnu/mesa",
+        "usr/lib/x86_64-linux-gnu/mesa-egl",
+        "usr/lib/i386-linux-gnu/mesa",
+        ]
+    
+    DISTROSERIES = "oneiric"
+
+    CONTENTS_FILE_URL_LOCATION = (
+        "http://archive.ubuntu.com/ubuntu/dists/%(distroseries)s/"
+        "Contents-%(arch)s.gz")
+
+    CONTENTS_FILE = "Contents-%(distroseries)s-%(arch)s"
+
+    def __init__(self, cachedir=None):
+        self.cachedir = os.path.expanduser(cachedir)
+        self._distroseries_arch_cache = {}
+
+    def _get_lib_to_pkgs_mapping(self, distroseries, arch):
+        """ Returns a dict of { library-name : set([pkg1,pkg2])
+
+        This function will return a dict to lookup library-name to package
+        dependencies for the given distroseries and architecture
+        """
+        if not (distroseries, arch) in self._distroseries_arch_cache:
+            self._distroseries_arch_cache[(distroseries, arch)] = \
+                      self._get_mapping_from_contents_file(distroseries, arch)
+        return self._distroseries_arch_cache[(distroseries, arch)]
+    
+    def _get_contents_file_cache_path(self, distroseries, arch):
+        """ Return the path in the cache for the given distroseries, arch """
+        return os.path.join(
+            self.cachedir, self.CONTENTS_FILE % {
+                'distroseries' : distroseries, 'arch' : arch })
+
+    def _get_contents_file_server_url(self, distroseries, arch):
+        """ Return the remote server URL for the given distroseries, arch """
+        return self.CONTENTS_FILE_URL_LOCATION % {
+            'distroseries' : distroseries, 'arch' : arch }
+
+    def _get_mapping_from_contents_file(self, distroseries, arch):
+        """ Return lib,pkgs mapping from contents file for distroseries, arch
+
+        This expects the contents file to be in the cachedir already.
+        """
+        lib_to_pkgs = {}
+        path = self._get_contents_file_cache_path(distroseries, arch)
+        with open(path) as f:
+            for (path, pkgs) in filter(
+                lambda (p, pkgs): os.path.dirname(p) in self.LD_SEARCH_PATH,
+                iter_contents_file(f)):
+                basename = os.path.basename(path)
+                if not basename in lib_to_pkgs:
+                    lib_to_pkgs[basename] = set()
+                lib_to_pkgs[basename] |= pkgs
+        return lib_to_pkgs
+
+    def _download_contents_file_compressed(self, distroseries, arch):
+        """ Downloads the content file for distroseries, arch into target """
+        # XXX: we may eventually want to merge the Contents files from
+        #      the -updates repository too in addition to the main archive
+        url = self._get_contents_file_server_url(distroseries, arch)
+        target = self._get_contents_file_cache_path(distroseries, arch)
+        compressed_target = target + os.path.splitext(url)[1]
+        # download
+        urllib.urlretrieve(url, compressed_target)
+        return compressed_target
+
+    def _prune_contents_gz_file(self, infile, outfile):
+        """ Read a compressed Contents.gz and write out a pruned version.
+
+        This will use iter_contents_file to go over infile and write
+        the relevant lines that are in the LD_SEARCH_PATH to outfile.
+        """
+        with open(outfile, "w") as outf, gzip.open(infile) as inf:
+            # first write the header
+            outf.write("FILE  LOCATION\n")
+            # then iter over all relevant lines and write them out
+            for (path, pkgs) in filter(
+                lambda (p,pkgs): os.path.dirname(p) in self.LD_SEARCH_PATH,
+                iter_contents_file(inf)):
+                outf.write("%s %s\n" % (path, ",".join(pkgs)))
+
+    def _download_and_prepare_contents_file_if_needed(self, distroseries, arch):
+        """ Ensure there is a usable Contents file in the cachedir
+
+        This will download, uncompress and prune a Conents file for
+        distroseries, arch so that get_dependencies works.
+        """
+        # mvo: We can (and should eventually) do etag/if-modified-since
+        #      matching here. But its not really important as long as
+        #      we package for stable distroseries as the Contents file
+        #      will not change
+        path = self._get_contents_file_cache_path(distroseries, arch)
+        if not os.path.exists(path):
+            compressed_contents = self._download_contents_file_compressed(
+                distroseries, arch)
+            # and prune from ~300mb to 1mb uncompressed as we are only
+            # interested in the library path parts
+            self._prune_contents_gz_file(compressed_contents, path)
+            os.remove(compressed_contents)
+
+    def get_dependencies(self, lib, arch="i386"):
+        # do lazy downloading for now, we could also make this part
+        # of bin/fetch-symbols I guess(?)
+        self._download_and_prepare_contents_file_if_needed(
+            self.DISTROSERIES, arch)
+        lib_to_pkgs = self._get_lib_to_pkgs_mapping(self.DISTROSERIES, arch)
+        return lib_to_pkgs.get(lib)
+
+
 class PackageDatabase(object):
 
+    # the various db backends, aptfile is a bit special
     SQLITE = 'sqlite'
     POSTGRES = 'postgres'
+    APTFILE = 'aptfile'
 
     def __init__(self, store):
         self._store = store
@@ -350,6 +506,9 @@
     def create(cls, store=None):
         if store is None:
             options = load_configuration().options
+            # XXX: not elegant
+            if options.database_db_type == cls.APTFILE:
+                return AptFilePackageDatabase(options.database_aptfile_cachedir)
             store = cls.get_store_from_config(options)
         return cls(store)
 

=== added directory 'devportalbinary/tests/data/apt-file-backend'
=== added file 'devportalbinary/tests/data/apt-file-backend/Contents-oneiric-i386'
--- devportalbinary/tests/data/apt-file-backend/Contents-oneiric-i386	1970-01-01 00:00:00 +0000
+++ devportalbinary/tests/data/apt-file-backend/Contents-oneiric-i386	2012-08-21 16:41:20 +0000
@@ -0,0 +1,47 @@
+This file maps each file available in the Ubuntu
+system to the package from which it originates.  It includes packages
+from the DIST distribution for the ARCH architecture.
+
+You can use this list to determine which package contains a specific
+file, or whether or not a specific file is available.  The list is
+updated weekly, each architecture on a different day.
+
+When a file is contained in more than one package, all packages are
+listed.  When a directory is contained in more than one package, only
+the first is listed.
+
+The best way to search quickly for a file is with the Unix `grep'
+utility, as in `grep <regular expression> CONTENTS':
+
+ $ grep nose Contents
+ etc/nosendfile                                          net/sendfile
+ usr/X11R6/bin/noseguy                                   x11/xscreensaver
+ usr/X11R6/man/man1/noseguy.1x.gz                        x11/xscreensaver
+ usr/doc/examples/ucbmpeg/mpeg_encode/nosearch.param     graphics/ucbmpeg
+ usr/lib/cfengine/bin/noseyparker                        admin/cfengine
+
+This list contains files in all packages, even though not all of the
+packages are installed on an actual system at once.  If you want to
+find out which packages on an installed Debian system provide a
+particular file, you can use `dpkg --search <filename>':
+
+ $ dpkg --search /usr/bin/dselect
+ dpkg: /usr/bin/dselect
+
+
+FILE                                                    LOCATION
+bin/afio                                                    universe/utils/afio
+bin/ash                                                     universe/shells/ash
+bin/autopartition                                           admin/ubiquity
+bin/autopartition-loop                                      admin/ubiquity
+bin/bash                                                    shells/bash
+lib/foo.so                                                  foo,bar
+lib/i386-linux-gnu/libx86.so.1                              libs/libx86-1
+lib/i386-linux-gnu/libz.so.1                                libs/zlib1g
+lib/i386-linux-gnu/libz.so.1.2.7                            libs/zlib1g
+lib/i386-linux-gnu/security/pam_access.so                   admin/libpam-modules
+lib/i386-linux-gnu/security/pam_afs_session.so              universe/net/libpam-afs-session
+lib/init/fstab                                              admin/mountall
+lib/init/rw                                                 admin/initscripts
+lib/init/splash-functions-base                              admin/initscripts
+lib/init/upstart-job                                        admin/upstart

=== modified file 'devportalbinary/tests/test_database.py'
--- devportalbinary/tests/test_database.py	2012-07-26 17:51:15 +0000
+++ devportalbinary/tests/test_database.py	2012-08-21 16:41:20 +0000
@@ -1,3 +1,4 @@
+import gzip
 import os
 
 from fixtures import (
@@ -10,8 +11,10 @@
     Equals,
     Matcher,
     )
+from mock import patch
 
 from devportalbinary.database import (
+    AptFilePackageDatabase,
     load_configuration,
     PackageDatabase,
     )
@@ -135,3 +138,43 @@
         self.assertEqual(expected_host, uri.host)
         self.assertEqual(expected_password, uri.password)
         self.assertEqual(expected_username, uri.username)
+
+
+class AptFilePackageDatabaseTestCase(TestCase):
+    
+    # point to our local contents file version that is a tad smaller
+    CONTENTS_CACHE = os.path.join(
+        os.path.dirname(__file__), "data", "apt-file-backend")
+
+    def setUp(self):
+        super(AptFilePackageDatabaseTestCase, self).setUp()
+        self.db = AptFilePackageDatabase(self.CONTENTS_CACHE)
+
+    def test_read_fixture_contents_worked(self):
+        """ test that our fixture Contents file works as expected """
+        # our test DB has 4 entries in the default search path
+        self.assertEqual(
+            len(self.db._get_lib_to_pkgs_mapping("oneiric", "i386")), 4)
+
+    def test_get_dependencies(self):
+        """ Test that data from the fixture dependencies file works """
+        self.assertEqual(
+            self.db.get_dependencies("libz.so.1"), set(["zlib1g"]))
+
+    @patch("urllib.urlretrieve")
+    def test_lazy_downloading(self, mock_urlretrieve):
+        """ test that lazy downloading works """
+        def _put_fixture_contents_file_in_place(url, target):
+            with gzip.open(target, "w") as f:
+                f.write("""
+Some header text that is ignored
+FILE                 LOCATION
+usr/lib/libfoo.so.2  pkgfoo,pkgbar
+""")
+        tempdir = self.useFixture(TempDir())
+        db = AptFilePackageDatabase(tempdir.path)
+        mock_urlretrieve.side_effect = _put_fixture_contents_file_in_place
+        self.assertEqual(
+            db.get_dependencies("libfoo.so.2", arch="i386"),
+            set(["pkgfoo", "pkgbar"]))
+        self.assertEqual(len(db._get_lib_to_pkgs_mapping("oneiric", "i386")), 1)