Merge lp:~james-w/pkgme-devportal/dict-api into lp:pkgme-devportal

[James Westby]

Move to a dict based api for libdep information.

Hi,

The list-based API was very difficult to reason about, so I rewrote it
to be dict-based. I think it's now much more obvious what each of the
APIs is, and the data is easier to read if you are debugging.

Thanks,

James

[Jonathan Lange (jml)]

Hey,

Thanks for doing this. The dict-based API is much clearer.

The branch adds a bunch of new functions / methods that don't have direct test coverage AFAICT. Can you please add tests for these before this branch lands?

 libdep_mapping_for_deb
 get_arch_libdep_mapping
 extract_deb_control
 get_file_contents
 libdep_mapping_from_symbols

It might also be a good idea to maybe make some new modules and move things around a bit. Another day, I think.

cheers,
jml

review: Needs Fixing

[lp:~james-w/pkgme-devportal/dict-api]

103. By James Westby on 2012-09-07

Merge trunk.

104. By James Westby on 2012-09-07

Test the testable new functions.

[James Westby (james-w)]

On Fri, 07 Sep 2012 08:21:20 -0000, Jonathan Lange <email address hidden> wrote:
> Review: Needs Fixing
>
> Hey,
>
> Thanks for doing this. The dict-based API is much clearer.
>
> The branch adds a bunch of new functions / methods that don't have direct test coverage AFAICT. Can you please add tests for these before this branch lands?
>
> libdep_mapping_for_deb

This one extracts a .deb to a tempdir, so I'll leave this untested.

> get_arch_libdep_mapping

This downloads a .deb and passes it to the previous, so ditto.

> extract_deb_control

This obviously extracts a .deb also.

> get_file_contents

Done. (Which led to me simplifying the API slightly, to take a path,
rather than two path components which are joined, and to use treeshape
for the first time, which went well)

> libdep_mapping_from_symbols

Done.

> It might also be a good idea to maybe make some new modules and move things around a bit. Another day, I think.

Yeah, it needs some breaking up I think, but really that should be
breaking up in to libdep-service :-)

Thanks,

James

[Jonathan Lange (jml)]

Yay

review: Approve

=== modified file 'devportalbinary/acceptance/tests/__init__.py'
--- devportalbinary/acceptance/tests/__init__.py	2012-09-06 19:53:09 +0000
+++ devportalbinary/acceptance/tests/__init__.py	2012-09-07 15:19:19 +0000
@@ -60,8 +60,10 @@
     def test_gtk(self):
         """Runs successfully for a basic GTK+ application."""
         dep_db = self.useFixture(DatabaseFixture())
-        dep_db.db.update_package("pthreads", [([("libpthread.so.0", "libpthread0")], "i386")])
-        dep_db.db.update_package("eglibc", [([("libc.so.6", "libc6")], "i386")])
+        dep_db.db.update_package("pthreads",
+                {'i386': {"libpthread.so.0": "libpthread0"}})
+        dep_db.db.update_package("eglibc",
+                {'i386': {"libc.so.6": "libc6"}})
         test_data = self.useFixture(TestData("gtk"))
         self.run_pkgme(test_data)
         self.assertThat(test_data.path, HasFileTree({'debian/control': {}}))
@@ -69,7 +71,8 @@
     def test_bundled_library(self):
         """Runs successfully for a basic bundled libary."""
         dep_db = self.useFixture(DatabaseFixture())
-        dep_db.db.update_package("eglibc", [([("libc.so.6", "libc6")], "i386")])
+        dep_db.db.update_package("eglibc",
+                {'i386': {"libc.so.6": "libc6"}})
         test_data = self.useFixture(TestData("bundled_lib"))
         self.run_pkgme(test_data)
         self.assertThat(

=== modified file 'devportalbinary/database.py'
--- devportalbinary/database.py	2012-09-06 19:53:09 +0000
+++ devportalbinary/database.py	2012-09-07 15:19:19 +0000
@@ -1,7 +1,7 @@
 # Copyright 2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-from contextlib import closing
+from contextlib import closing, contextmanager
 import errno
 import gzip
 from itertools import chain
@@ -37,6 +37,8 @@
 APPLICATION_NAME = 'pkgme-binary'
 SERVICE_ROOT = uris.LPNET_SERVICE_ROOT
 
+SYMBOLS_FILENAME = 'symbols'
+
 
 # A list of package names that don't match lib*, but which we want
 # to scan anyway.
@@ -94,6 +96,26 @@
           for series in our_series])
 
 
+def libdep_mapping_for_deb(self, deb_file):
+    """Returns the library -> dependency mapping information from a package.
+
+    The symbols file will be read to get the information. If it is not present
+    an empty dict will be returned indicating that there are no libraries.
+
+    If there are libraries the dict will map from the library names
+    to the package dependencies that should be used to get the
+    library, e.g.
+
+        {'libfoo.so.1': 'libfoo1', ...}
+    """
+    with extract_deb_control(deb_file) as extracted:
+        symbols_path = os.path.join(extracted, SYMBOLS_FILENAME)
+        symbols = get_file_contents(symbols_path)
+        if symbols is not None:
+            return libdep_mapping_from_symbols(symbols)
+        return {}
+
+
 class BinaryPackage(object):
 
     def __init__(self, bpph):
@@ -123,46 +145,58 @@
             arch,
             )
 
-    def get_symbol_contents(self):
+    def get_arch_libdep_mapping(self):
+        """Return the library -> dependency mapping with architecture info.
+
+        :return: a dict mapping architecture tags to dicts mapping library
+            names to dependencies, e.g.
+                {'amd64': {'libfoo.so.1': 'libfoo1', ...}, ...}
+        """
+        arch = self.bpph.distro_arch_series.architecture_tag
+        mapping = {}
         directory = tempfile.mkdtemp()
         try:
             url = self.get_url()
             filename = os.path.splitext(urlutils.basename(url))[0]
-            if not is_library_package(filename):
-                return
-            path = download_file(url, directory)
-            symbols_contents = extract_symbols_file(path)
-            return symbols_contents
+            if is_library_package(filename):
+                deb_file = download_file(url, directory)
+                mapping = libdep_mapping_for_deb(deb_file)
         finally:
             shutil.rmtree(directory)
-
-    def iter_symbols(self):
-        contents = self.get_symbol_contents()
-        if contents:
-            yield contents, self.bpph.distro_arch_series.architecture_tag
-
-
-def extract_symbols_file(binary_package_path):
-    """Return the contents of the symbols file for a binary package.
-
-    :param binary_package_path: The path to a binary package.
+        return {arch: mapping}
+
+
+@contextmanager
+def extract_deb_control(binary_package_path):
+    """Extract a deb control archive, returning the extracted path.
+
+    This is a context manager, and the tempdir will be cleaned up when closed.
     """
     temp_dir = tempfile.mkdtemp()
     try:
         run_subprocess(['dpkg-deb', '-e', binary_package_path, temp_dir])
-        symbol_path = os.path.join(temp_dir, 'symbols')
-        try:
-            return open(symbol_path).read()
-        except (OSError, IOError), e:
-            if e.errno == errno.ENOENT:
-                return
-            raise
+        yield temp_dir
     finally:
         shutil.rmtree(temp_dir)
 
 
-def iter_so_names(symbol_contents):
-    """Yield (library, dependency) from a symbols file.
+def get_file_contents(path):
+    """Read the contents of the file at `path`.
+
+    :return: the contents of the file, or None if there was
+        no such file.
+    :raises: Any errors opening or reading the file.
+    """
+    try:
+        return open(path).read()
+    except (OSError, IOError), e:
+        if e.errno == errno.ENOENT:
+            return
+        raise
+
+
+def libdep_mapping_from_symbols(symbol_contents):
+    """Returns a dict mapping libraries to dependencies based on the symbols
 
     Ignores versions and a whole bunch of other stuff and is probably the
     wrong API even.
@@ -179,6 +213,7 @@
     # This is OK, as we're only processing symbols files from binary packages
     # rather than source packages.
 
+    mapping = {}
     for line in symbol_contents.splitlines():
         if not line:
             # Blank lines are skipped
@@ -207,7 +242,8 @@
             #
             # e.g. (arch=!armel)#include "libdiagnostics0.symbols.backtrace"
             continue
-        yield library, dependency
+        mapping[library] = dependency
+    return mapping
 
 
 def iter_contents_file(contents):
@@ -478,39 +514,29 @@
              unicode(dependency),
              unicode(arch)))
 
-    def update_package(self, package_name, symbol_list):
+    def update_package(self, package_name, arch_libdep_mapping):
         """Update the database with the libdep info from 'package_name'.
 
         :param package_name: The name of the package where the
             symbols came from.
-        :param symbol_list: A list of representations of symbols files, where each
-            representation is a list of tuples of library and
-            dependency and an architecture string.
-            e.g. [([(libfoo, foo), ...], 'i386'), ...].
-            Each architecture should only appear once in the list.
+        :param arch_libdep_mapping: a dict mapping architecture tags to dicts
+            mapping library names to dependencies, e.g.
+                {'amd64': {'libfoo.so.1': 'libfoo1', ...}, ...}
         """
-        for symbols in symbol_list:
-            deps, architecture = symbols
+        for arch, libdep_mapping in arch_libdep_mapping.items():
             self._store.execute(
                     "DELETE FROM libdep WHERE source_package_name = ? "
                     "AND architecture = ?",
-                    (unicode(package_name), unicode(architecture)))
-            for library, dependency in deps:
+                    (unicode(package_name), unicode(arch)))
+            for library, dependency in libdep_mapping.items():
                 self.insert_new_library(
-                    package_name, library, dependency, architecture)
+                    package_name, library, dependency, arch)
         self._store.commit()
 
     def close(self):
         self._store.close()
 
 
-def build_symbol_list(symbols):
-    symbol_list = [
-        (list(iter_so_names(symbols)), architecture)
-        for symbols, architecture in symbols]
-    return symbol_list
-
-
 def fetch_symbol_files(scan_mode, package_name, db):
     """Insert the libdep info for ``package_name`` into ``db``."""
     if scan_mode != 'binary':
@@ -519,8 +545,8 @@
         package = BinaryPackage.find(lp.anonymous, package_name)
         if not package:
             return
-        symbol_list = build_symbol_list(package.iter_symbols())
-    db.update_package(package_name, symbol_list)
+        arch_libdep_mapping = package.get_arch_libdep_mapping()
+    db.update_package(package_name, arch_libdep_mapping)
 
 
 def main():

=== modified file 'devportalbinary/tests/test_binary.py'
--- devportalbinary/tests/test_binary.py	2012-09-06 19:53:09 +0000
+++ devportalbinary/tests/test_binary.py	2012-09-07 15:19:19 +0000
@@ -256,7 +256,7 @@
     def test_guess_dependencies(self):
         self.useFixture(DatabaseConfig(self.db_fixture))
         with closing(PackageDatabase(self.db_fixture.conn)) as db:
-            db.update_package('eglibc', [([('libc.so.6', 'libc6')], 'i386')])
+            db.update_package('eglibc', {'i386': {'libc.so.6': 'libc6'}})
         deps, arch = guess_dependencies(get_test_data_dir_path('hello'))
         self.assertEqual(set(['libc6']), deps)
 
@@ -330,7 +330,7 @@
         shutil.copy(
             get_test_data_file_path('hello', 'hello'), backend.path)
         with closing(PackageDatabase(self.db_fixture.conn)) as db:
-            db.update_package('eglibc', [([('libc.so.6', 'libc6')], 'i386')])
+            db.update_package('eglibc', {'i386': {'libc.so.6': 'libc6'}})
         deps, arch = guess_dependencies(backend.path)
         expected_deps = ', '.join(deps)
         build_deps = backend.get_build_depends(None)
@@ -367,10 +367,7 @@
     def test_get_lib_overrides_for_packages_for_libraries(self):
         # The configuration file overrides the found library dependencies.
         with closing(PackageDatabase(self.db_fixture.conn)) as db:
-            db.update_package('foo', [
-                    ([('libasound.so.2', 'libfoo')], 'i386'),
-                    ([('libasound.so.2', 'libbar')], 'i386'),
-                    ])
+            db.update_package('foo', {'i386': {'libasound.so.2': 'libfoo'}})
         self.assertEqual(
             get_packages_for_libraries(["libasound.so.2"], "i386"),
             set(["libasound2"]))
@@ -384,7 +381,7 @@
         self.useFixture(BinaryFileFixture(path))
         backend = self.make_backend(path)
         with closing(PackageDatabase(self.db_fixture.conn)) as db:
-            db.update_package('eglibc', [([('libc.so.6', 'libc6')], 'i386')])
+            db.update_package('eglibc', {'i386': {'libc.so.6': 'libc6'}})
         conf = load_configuration()
         self.assertEqual(backend.get_architecture(metadata={}), "i386")
 

=== modified file 'devportalbinary/tests/test_database.py'
--- devportalbinary/tests/test_database.py	2012-09-06 19:53:09 +0000
+++ devportalbinary/tests/test_database.py	2012-09-07 15:19:19 +0000
@@ -5,6 +5,7 @@
 from fixtures import (
     TempDir,
     )
+from mock import patch
 from storm.databases.postgres import psycopg2
 from storm.exceptions import ClosedError
 from testresources import ResourcedTestCase
@@ -13,12 +14,16 @@
     Equals,
     Matcher,
     )
-from mock import patch
+from treeshape import (
+    CONTENT,
+    FileTree,
+    )
 
 from devportalbinary.database import (
     AptFilePackageDatabase,
     BinaryPackage,
-    build_symbol_list,
+    get_file_contents,
+    libdep_mapping_from_symbols,
     load_configuration,
     PackageDatabase,
     )
@@ -94,16 +99,16 @@
     def test_update_package(self):
         db = self.get_package_db()
         db.update_package(
-            'foo', [([('libfoo.so.1', 'foo-bin')], 'i386')])
+            'foo', {'i386': {'libfoo.so.1': 'foo-bin'}})
         deps = db.get_dependencies('libfoo.so.1')
         self.assertEqual(deps, set(['foo-bin']))
 
     def test_update_existing_package_no_libraries(self):
         db = self.get_package_db()
-        db.update_package('foo', [([('libfoo.so.1', 'foo-bin')], 'i386')])
+        db.update_package('foo', {'i386': {'libfoo.so.1': 'foo-bin'}})
         # Run again, this time with no symbols, representing that a newer
         # version of the package no longer exports any libraries.
-        db.update_package('foo', [([], 'i386')])
+        db.update_package('foo', {'i386': {}})
         deps = db.get_dependencies('libfoo.so.1')
         self.assertEqual(deps, set())
 
@@ -111,25 +116,11 @@
         # If two architectures are updated separately then they
         # shouldn't interfere
         db = self.get_package_db()
-        db.update_package(
-            'foo', [([('libfoo.so.1', 'foo-bin')], 'i386')])
-        db.update_package(
-            'foo', [([('libfoo.so.1', 'foo-bin-amd64')], 'amd64')])
+        db.update_package('foo', {'i386': {'libfoo.so.1': 'foo-bin'}})
+        db.update_package('foo', {'amd64': {'libfoo.so.1': 'foo-bin-amd64'}})
         deps = db.get_dependencies('libfoo.so.1', arch='i386')
         self.assertEqual(deps, set(['foo-bin']))
 
-    def test_fetch_symbol_files(self):
-        # It's hard to test the function because it uses launchpadlib,
-        # so we'll test the main point, taking the output of
-        # package.iter_symbols(), passing it through build_symbol_list(),
-        # and then to # update_package() to ensure that the signatures
-        # match
-        symbol_list = build_symbol_list([('libfoo.so.1 foo-bin', 'i386')])
-        db = self.get_package_db()
-        db.update_package('foo', symbol_list)
-        deps = db.get_dependencies('libfoo.so.1')
-        self.assertEqual(set(['foo-bin']), deps)
-
     def test_close(self):
         # Test that we can close the package db.
         db = PackageDatabase(self.db_fixture.conn)
@@ -144,16 +135,6 @@
         db.close()
 
 
-class TestBuildSymbolList(TestCase):
-
-    def test_no_symbols(self):
-        self.assertEqual([], build_symbol_list([]))
-
-    def test_some_symbols(self):
-        symbol_list = build_symbol_list([('a b', 'i386')])
-        self.assertEqual([([('a', 'b')], 'i386')], symbol_list)
-
-
 class TestDatabaseConfiguration(TestCase):
 
     def use_database_config(self, **db_settings):
@@ -291,3 +272,52 @@
             '1',
             )
         self.assertEqual(expected_url, BinaryPackage(bpph).get_url())
+
+
+class TestGetFileContents(TestCase):
+
+    def test_exists(self):
+        expected_content = 'boring content\n'
+        filename = 'foo.txt'
+        tree = {filename: {CONTENT: expected_content}}
+        path = self.useFixture(FileTree(tree)).join(filename)
+        self.assertEqual(expected_content, get_file_contents(path))
+
+    def test_not_exists(self):
+        tree = {}
+        path = self.useFixture(FileTree(tree)).join('nothere')
+        self.assertEqual(None, get_file_contents(path))
+
+    def test_directory(self):
+        tree = {}
+        path = self.useFixture(FileTree(tree)).path
+        self.assertRaises(IOError, get_file_contents, path)
+
+
+class TestLibdepMappingFromSymbols(TestCase):
+
+    def test_empty(self):
+        self.assertEqual({}, libdep_mapping_from_symbols(''))
+
+    def test_blank_line_ignored(self):
+        self.assertEqual({}, libdep_mapping_from_symbols('\n'))
+
+    def test_alternate_template_ignored(self):
+        self.assertEqual({}, libdep_mapping_from_symbols('| foo\n'))
+
+    def test_meta_information_ignored(self):
+        self.assertEqual({}, libdep_mapping_from_symbols('* foo\n'))
+
+    def test_symbols_ignored(self):
+        self.assertEqual({}, libdep_mapping_from_symbols(' foo\n'))
+
+    def test_comments_ignored(self):
+        self.assertEqual({}, libdep_mapping_from_symbols('# foo\n'))
+
+    def test_include_ignored(self):
+        self.assertEqual({},
+                libdep_mapping_from_symbols('(arch=!armel)#include foo\n'))
+
+    def test_includes_mapping(self):
+        self.assertEqual({'libfoo.so.1': 'libfoo'},
+                libdep_mapping_from_symbols('libfoo.so.1 libfoo #MINVER#\n'))