Merge lp:~mvo/pkgme-devportal/more-architectures into lp:pkgme-devportal

[Michael Vogt]

This branch adds a architecture field to the database and make the LP code add this architecture field when scanning.

This branch lacks database migration, so it will require the old db to be destroyed and a new one created unfortunately.

[James Westby (james-w)]

Hi Michael,

This looks sensible. It may be that we want a different schema for best querying,
but at this point I don't think so, and we'd need more information to know. Therefore
I think this is the right approach at this point.

Thanks,

James

[lp:~mvo/pkgme-devportal/more-architectures]

65. By Michael Vogt on 2012-07-24

merged updated lp:~mvo/pkgme-devportal/raise-on-unsupported-architectures

66. By Michael Vogt on 2012-07-24

update update_source_package() for the new api

67. By Michael Vogt on 2012-07-24

merged fix from lp:~mvo/pkgme-devportal/raise-on-unsupported-architectures

[James Westby (james-w)]

Hi,

We'll want to add architecture to the query interface of libdep-service
at some point, but I was wrong that this will break libdep-service I think,
so approving.

Thanks,

James

review: Approve

[James Westby (james-w)]

Oh, but we do need a schema migration to deploy this. We don't have a
framework for that yet.

We can probably do something cheap for this one if we have the SQL needed
to do the migration.

Thanks,

James

[lp:~mvo/pkgme-devportal/more-architectures]

68. By Michael Vogt on 2012-07-24

merged from trunk to fix tests

[James Westby (james-w)]

On Tue, 24 Jul 2012 16:01:18 -0000, James Westby <email address hidden> wrote:
> Oh, but we do need a schema migration to deploy this. We don't have a
> framework for that yet.
>
> We can probably do something cheap for this one if we have the SQL needed
> to do the migration.

To answer your question on IRC, it doesn't have to be a migration
per-se, but we can't rollout until we know how it is handled as all the
queries will break if they don't match the db, so we need an answer
before we can land this unfortunately.

Thanks,

James

[Michael Vogt (mvo)]

Thanks James, so I assume we want something like the following sql:

"""
ALTER TABLE libdep ADD architecture TEXT;
UPDATE libdep SET architecture = "i386";
"""

to be run on the DB when this gets deployed. Would this be something we ask the webops to do ad-hoc?
Or something like https://lists.ubuntu.com/archives/storm/2009-August/001117.html ? Or something else?

[Michael Vogt (mvo)]

Hm, apparently storm has some native support for this too, I look at this now.

[Michael Vogt (mvo)]

Some info on this is here:
http://people.canonical.com/~therve/storm/storm.schema.schema.Schema.html

but its not very well documented and it appears that it will not help with this migration as the DB itself needs to be created with the storm.schema stuff so that it has the "patch" table.

[Michael Vogt (mvo)]

I put the storm version of this migration code into lp:~mvo/pkgme-devportal/schemas-via-storm now.

[James Westby (james-w)]

Hi,

After discussing this more, the mechanism of the schema update isn't the important
thing, it's having the code deal with the fact that we won't do the migration
as well roll it out.

The steps would be

1) Land a change to cope with the architecture column being present or not:
   - if it's there it should insert data in to it
   - if it's there it can use it in queries, if it assumes that NULL == 'i386', or it can not use it in queries.
   - it can add the column to the CREATE TABLE IF NOT EXIST statement, as that won't trigger when pushing out the code.

2) Roll that change out

3) Run SQL to ALTER TABLE ADD COLUMN architecture;

3) Run background job to UPDATE SET architecture = 'i386'; This may want to be batched.

4) Once the job completes ALTER COLUMN ADD CONSTRAINT NOT NULL;

5) The db is now ready, once this is done on production land a change to use the column in queries, expecting it to be there and with no nulls and 'i386' for all current data.

6) Roll that change out.

So, some changes are needed to this branch:

72 + "architecture TEXT, "

Should add a NOT NULL constraint I think, we expect every row to have a non-NULL architecture
eventually.

93 + result = self._store.execute("SELECT dependency FROM libdep WHERE library = ? AND architecture = ?",
94 + (unicode(library_name), unicode(arch)))

It shouldn't change that currently, it should just assume that architecture == 'i386' for all rows for now.

108 + self._store.execute("INSERT INTO libdep VALUES (?, ?, ?, ?)",
109 + (unicode(source_package_name), unicode(library_name), unicode(dependency), unicode(arch)))

That needs to change the INSERT statement depending on whether the architecture column is present or not.

With those changes we can get this landed, rolled out to staging, and start the migration there.

Two things come to mind:

  * A way to test if a column is part of a table will be important to have. jml has written code for this before I think.
  * We may want something like dblooptuner for doing the background migration.

Thanks,

James

[James Westby (james-w)]

lifeless corrected my process:

<lifeless> so for a given refactoring, like 'add a required column', you build that out of those two primitives:
<lifeless> - add a nullable column
<lifeless> - add a backfill job and make your other code populate the column, and handle it being null
<lifeless> - wait for the backfill
<lifeless> - change schema to make the column NOT NULL
<lifeless> done.

Thanks,

James

[James Westby (james-w)]

Hi,

So we need a step before those because we currently do

  "INSERT INTO libdep VALUES (?, ?, ?)"

which won't work with multiple columns I don't think (though
I haven't tested.) That means the first step would be to land
a change that altered that.

I don't want to block you on all of this though Michael, so I'd
be +1 on going ahead with a managed rollout of this change
with lockstep db migration, and to start to work on these
issues so we can do better with the next change.

Thanks,

James

[James Westby (james-w)]

Oh, and I've just realised that I think architecture must be part of the
unique constraint, otherwise it will fail if the same package has the same
library and dependencies on multiple architectures.

Thanks,

James

=== modified file 'acceptance/tests/__init__.py'
--- acceptance/tests/__init__.py	2012-07-17 12:34:24 +0000
+++ acceptance/tests/__init__.py	2012-07-24 17:18:20 +0000
@@ -53,8 +53,8 @@
     def test_gtk(self):
         """Runs successfully for a basic GTK+ application."""
         dep_db = self.useFixture(DatabaseFixture())
-        dep_db.db.update_source_package("pthreads", [[("libpthread.so.0", "libpthread0")]])
-        dep_db.db.update_source_package("eglibc", [[("libc.so.6", "libc6")]])
+        dep_db.db.update_source_package("pthreads", [[("libpthread.so.0", "libpthread0", "i386")]])
+        dep_db.db.update_source_package("eglibc", [[("libc.so.6", "libc6", "i386")]])
         test_data = self.useFixture(TestData("gtk"))
         self.run_pkgme(test_data)
         self.assertThat(
@@ -63,7 +63,7 @@
     def test_bundled_library(self):
         """Runs successfully for a basic bundled libary."""
         dep_db = self.useFixture(DatabaseFixture())
-        dep_db.db.update_source_package("eglibc", [[("libc.so.6", "libc6")]])
+        dep_db.db.update_source_package("eglibc", [[("libc.so.6", "libc6", "i386")]])
         test_data = self.useFixture(TestData("bundled_lib"))
         self.run_pkgme(test_data)
         self.assertThat(

=== modified file 'devportalbinary/binary.py'
--- devportalbinary/binary.py	2012-07-24 17:08:59 +0000
+++ devportalbinary/binary.py	2012-07-24 17:18:20 +0000
@@ -138,6 +138,7 @@
     cmd = [OBJDUMP, '-f', '-p'] + binary_paths
     output = run_subprocess(cmd)
     libraries = {}
+    architecture = None
     last_line_was_blank = True
     for line in output:
         this_line_blank = (line.strip() == '')
@@ -148,7 +149,8 @@
                 break
             libraries[current_binary] = []
         if line.startswith('  NEEDED'):
-            libraries[current_binary].append(line.split()[1])
+            soname = line.split()[1]
+            libraries[current_binary].append(soname)
         elif line.startswith('architecture: '):
             # so... i386 is reported as "i386" 
             # *but* amd64 as "i386:x86-64" 

=== modified file 'devportalbinary/database.py'
--- devportalbinary/database.py	2012-07-19 17:29:48 +0000
+++ devportalbinary/database.py	2012-07-24 17:18:20 +0000
@@ -131,7 +131,7 @@
     def iter_symbols(self):
         contents = self.get_symbol_contents()
         if contents:
-            yield contents
+            yield contents, self.bpph.distro_arch_series.architecture_tag
 
 
 class SourcePackage(object):
@@ -180,7 +180,7 @@
             path = download_file(url, directory)
             symbols_contents = extract_symbols_file(path)
             if symbols_contents:
-                yield symbols_contents
+                yield symbols_contents, architecture
         shutil.rmtree(directory)
 
 
@@ -324,6 +324,7 @@
                 "source_package_name TEXT, "
                 "library TEXT, "
                 "dependency TEXT, "
+                "architecture TEXT, "
                 "CONSTRAINT libdep_uniq UNIQUE (source_package_name, "
                 "library, dependency))")
         elif store_type == 'postgres':
@@ -337,6 +338,7 @@
                     "source_package_name TEXT, "
                     "library TEXT, "
                     "dependency TEXT, "
+                    "architecture TEXT, "
                     "CONSTRAINT libdep_uniq UNIQUE (source_package_name, "
                     "library, dependency))")
         else:
@@ -344,21 +346,22 @@
         store.commit()
         return cls(store)
 
-    def get_dependencies(self, library_name):
+    def get_dependencies(self, library_name, arch='i386'):
         """Get the binary packagaes that provide 'library_name'."""
-        result = self._store.execute("SELECT dependency FROM libdep WHERE library = ?",
-                  (unicode(library_name),))
+        result = self._store.execute("SELECT dependency FROM libdep WHERE library = ? AND architecture = ?",
+                  (unicode(library_name), unicode(arch)))
         return set([dependency for [dependency] in result])
 
-    def insert_new_library(self, source_package_name, library_name, dependency):
+    def insert_new_library(self, source_package_name, library_name, 
+                           dependency, arch):
         """Insert a library and its needed dependency into the database.
 
         :param library_name: A full soname, e.g. libfoo.so.1.
         :param dependency: A binary package dependency, possibly including
             version.
         """
-        self._store.execute("INSERT INTO libdep VALUES (?, ?, ?)",
-                  (unicode(source_package_name), unicode(library_name), unicode(dependency)))
+        self._store.execute("INSERT INTO libdep VALUES (?, ?, ?, ?)",
+                  (unicode(source_package_name), unicode(library_name), unicode(dependency), unicode(arch)))
 
     def update_source_package(self, source_package_name, symbols):
         """Update the database with the symbols from 'source_package_name'.
@@ -372,9 +375,9 @@
         self._store.execute("DELETE FROM libdep WHERE source_package_name = ?",
                   (unicode(source_package_name),))
         for symbols in symbols:
-            for library, dependency in symbols:
+            for library, dependency, architecture in symbols:
                 self.insert_new_library(
-                    source_package_name, library, dependency)
+                    source_package_name, library, dependency, architecture)
         self._store.commit()
 
 
@@ -389,8 +392,8 @@
         if not package:
             return
         symbols = [
-            list(iter_so_names(symbols))
-            for symbols in package.iter_symbols()]
+            (list(iter_so_names(symbols)), architecture)
+            for symbols, architecture in package.iter_symbols()]
     db.update_source_package(package_name, symbols)
 
 

=== modified file 'devportalbinary/tests/test_binary.py'
--- devportalbinary/tests/test_binary.py	2012-07-24 15:05:27 +0000
+++ devportalbinary/tests/test_binary.py	2012-07-24 17:18:20 +0000
@@ -233,7 +233,7 @@
 
     def test_guess_dependencies(self):
         db = self.useFixture(DatabaseFixture()).db
-        db.update_source_package('eglibc', [[('libc.so.6', 'libc6')]])
+        db.update_source_package('eglibc', [[('libc.so.6', 'libc6', 'i386')]])
         deps = guess_dependencies(get_test_data_dir_path('hello'))
         self.assertEqual(set(['libc6']), deps)
 
@@ -302,7 +302,7 @@
         shutil.copy(
             get_test_data_file_path('hello', 'hello'), backend.path)
         db = self.useFixture(DatabaseFixture()).db
-        db.update_source_package('eglibc', [[('libc.so.6', 'libc6')]])
+        db.update_source_package('eglibc', [[('libc.so.6', 'libc6', 'i386')]])
         expected_deps = ', '.join(guess_dependencies(backend.path))
         build_deps = backend.get_build_depends(None)
         self.assertEqual(expected_deps, build_deps)
@@ -337,8 +337,8 @@
         # The configuration file overrides the found library dependencies.
         db = self.useFixture(DatabaseFixture()).db
         db.update_source_package('foo', [
-                [('libasound.so.2', 'libfoo')],
-                [('libasound.so.2', 'libbar')],
+                [('libasound.so.2', 'libfoo', 'i386')],
+                [('libasound.so.2', 'libbar', 'i386')],
                 ])
         self.assertEqual(
             get_packages_for_libraries(["libasound.so.2"], "i386"),

=== modified file 'devportalbinary/tests/test_database.py'
--- devportalbinary/tests/test_database.py	2012-07-17 13:46:56 +0000
+++ devportalbinary/tests/test_database.py	2012-07-24 17:18:20 +0000
@@ -47,28 +47,28 @@
 
     def test_insert_new_library(self):
         db = self.get_package_db()
-        db.insert_new_library('foo-src', 'libfoo.so.0', 'foo')
+        db.insert_new_library('foo-src', 'libfoo.so.0', 'foo', 'i386')
         self.assertThat(
-            "SELECT source_package_name, library, dependency FROM libdep",
-            ResultsIn(db, [('foo-src', 'libfoo.so.0', 'foo')]))
+            "SELECT source_package_name, library, dependency, architecture FROM libdep",
+            ResultsIn(db, [('foo-src', 'libfoo.so.0', 'foo', 'i386')]))
 
     def test_double_insert(self):
         db = self.get_package_db()
-        db.insert_new_library('foo-src', 'libfoo.so.0', 'foo')
+        db.insert_new_library('foo-src', 'libfoo.so.0', 'foo', 'i386')
         self.assertRaises(
             IntegrityError,
-            db.insert_new_library, 'foo-src', 'libfoo.so.0', 'foo')
+            db.insert_new_library, 'foo-src', 'libfoo.so.0', 'foo', 'i386')
 
     def test_differing_dependencies(self):
         db = self.get_package_db()
-        db.insert_new_library('foo-src', 'libfoo.so.0', 'foo')
-        db.insert_new_library('foo-src', 'libfoo.so.0', 'bar')
+        db.insert_new_library('foo-src', 'libfoo.so.0', 'foo', 'i386')
+        db.insert_new_library('foo-src', 'libfoo.so.0', 'bar', 'i386')
         deps = db.get_dependencies('libfoo.so.0')
         self.assertEqual(deps, set(['foo', 'bar']))
 
     def test_get_dependencies(self):
         db = self.get_package_db()
-        db.insert_new_library('foo-src', 'libfoo.so.0', 'foo')
+        db.insert_new_library('foo-src', 'libfoo.so.0', 'foo', 'i386')
         deps = db.get_dependencies('libfoo.so.0')
         self.assertEqual(deps, set(['foo']))
 
@@ -80,13 +80,13 @@
     def test_update_source_package(self):
         db = self.get_package_db()
         db.update_source_package(
-            'foo', [[('libfoo.so.1', 'foo-bin')]])
+            'foo', [[('libfoo.so.1', 'foo-bin', 'i386')]])
         deps = db.get_dependencies('libfoo.so.1')
         self.assertEqual(deps, set(['foo-bin']))
 
     def test_update_existing_source_package_no_libraries(self):
         db = self.get_package_db()
-        db.update_source_package('foo', [[('libfoo.so.1', 'foo-bin')]])
+        db.update_source_package('foo', [[('libfoo.so.1', 'foo-bin', 'i386')]])
         # Run again, this time with no symbols, representing that a newer
         # version of the package no longer exports any libraries.
         db.update_source_package('foo', [])