Merge lp:~stevenk/launchpad/fixes-bug-529950 into lp:launchpad

[Steve Kowalik]

Fix scripts/gina.py to be able to import multiple versions of the same source package from a single archive.

This branch fixes gina to be able to import multiple versions of the same source package from a single repository.

[Jelmer Vernooij (jelmer)]

Hi Steve,

Thanks, nice straightforward change.

My only concern is that this would perhaps break spnames_only mode. My understanding is that scripts/gina.py:193 now loops over sets, not dictionaries.

review: Needs Information (code mentat)

[Michael Nelson (michael.nelson)]

Hi Steve and Jelmer,

Nice find Jelmer. It's a shame (but not a surprise :/), that we don't seem to have a test for the sourcepackagenames_only option. It's only referenced in the config. Steve, feel free to add one before fixing the issue :) Actually, I can't see it referenced anywhere in the lp-production-configs? Do we even use it? (if not, we could remove it).

I've just got a few style questions below.

Thanks for the fix!

> === modified file 'lib/lp/soyuz/scripts/gina/archive.py'
> --- lib/lp/soyuz/scripts/gina/archive.py 2009-06-25 04:06:00 +0000
> +++ lib/lp/soyuz/scripts/gina/archive.py 2010-03-17 11:15:37 +0000
> @@ -207,7 +207,7 @@
> log.exception("Invalid Sources stanza in %s" %
> info_set.sources_tagfile)
> continue
> - self.src_map[src_name] = src_tmp
> + self.src_map.setdefault(src_name, []).append(src_tmp)

Not sure what you prefer, but I think this is more readable like this:

http://pastebin.ubuntu.com/399865/

which also passes the test. Up to you.

> === modified file 'scripts/gina.py'
> --- scripts/gina.py 2009-10-13 14:38:07 +0000
> +++ scripts/gina.py 2010-03-17 11:15:37 +0000
> @@ -229,44 +229,45 @@
> npacks = len(packages_map.src_map)
> log.info('%i Source Packages to be imported' % npacks)
>
> - for source in sorted(packages_map.src_map.values(),
> - key=lambda x: x.get("Package")):
> - count += 1
> - package_name = source.get("Package", "unknown")
> - try:
> + for list_source in sorted(packages_map.src_map.values(),
> + key=lambda x: x[0].get("Package")):

The python style guide used to allow the previous indentation, but it now should be:

   for list_source in sorted(
       packages_map.src_map.values(), key=lambda x: x[0].get("Package")):

See https://dev.launchpad.net/PythonStyleGuide#Multiline%20function%20calls

Or another option, if you rename your var 'list_source' to simply 'source_list', then you can create your source_lists prior to the iteration (for source_list in source_lists:). See what you think.

> + for source in list_source:
> + count += 1
> + package_name = source.get("Package", "unknown")
> try:
> - do_one_sourcepackage(source, kdb, package_root, keyrings,
> + try:
> + do_one_sourcepackage(source, kdb, package_root, keyrings,
> importer_handler)

Similar, if you need to wrap the args to a fn call, they should start on the next line.

> - except psycopg2.Error:
> - log.exception("Database error: unable to create "
> + except psycopg2.Error:
> + log.exception("Database error: unable to create "
> "SourcePackage for %s. Retrying once.."
> % package_name)

And again.

> - importer_handler.abort()
> - time.sleep(15)
> - do_one_sourcepackage(source, kdb, package_root, keyrings,
> + import...

review: Needs Information (code)

[Jelmer Vernooij (jelmer)]

Minor comment; lib/canonical/config/schema-lazr.conf and configs/schema-lazr.conf still reference spnamesonly, can you also remove those ?

review: Approve (code mentat)

[Michael Nelson (michael.nelson)]

> Minor comment; lib/canonical/config/schema-lazr.conf and configs/schema-
> lazr.conf still reference spnamesonly, can you also remove those ?

I was trying to figure out why I only found one reference initially... configs/schema-lazr.conf is a link ;)

Also, the earlier code in scripts/gina.py that actually defines the variable from the config should go to.

> spnames_only = target_section.sourcepackagenames_only

Thanks!

review: Approve (code)

[Deryck Hodge (deryck)]

Given that this would have to be CP'ed anyway, I'm fine with letting it in before the current release.

review: Approve (release-critical)

=== modified file 'lib/lp/soyuz/doc/gina.txt'
--- lib/lp/soyuz/doc/gina.txt	2010-01-10 04:58:44 +0000
+++ lib/lp/soyuz/doc/gina.txt	2010-03-25 10:13:54 +0000
@@ -63,6 +63,8 @@
 
    * archive-copier, a source package which generates one udeb
      in debian-installer. Its maintainer has a name which contains a ","
+   * archive-copier, again, with a different version number to see that
+     both versions get correctly imported.
    * db1-compat, source package what generates 1 binary package. The same
      version was included in both hoary and breezy, but its source
      package's section and its binary package's priority were changed in
@@ -317,13 +319,13 @@
       util-linux (again, poor thing)
 
     >>> print SSPPH.select().count() - orig_sspph_count
-    20
+    21
 
     >>> print SSPPH.selectBy(
     ...     componentID=1,
     ...     pocket=PackagePublishingPocket.RELEASE).count() - \
     ...     orig_sspph_main_count
-    20
+    21
 
 === Testing Binary Package Results ===
 
@@ -532,7 +534,7 @@
     >>> SBPPH.select().count() - orig_sbpph_count
     47
     >>> print SSPPH.select().count() - orig_sspph_count
-    22
+    23
 
 Check that the overrides we did were correctly issued. We can't use selectOneBy
 because, of course, there may be multiple rows published for that package --
@@ -607,7 +609,7 @@
 
     >>> source_difference = partner_source_set_after - partner_source_set
     >>> len(source_difference)
-    11
+    12
 
     >>> binary_difference = partner_binary_set_after - partner_binary_set
     >>> len(binary_difference)
@@ -700,7 +702,7 @@
 
     >>> lenny_sources = SSPPH.select("distroseries = %s" % sqlvalues(lenny))
     >>> lenny_sources.count()
-    11
+    12
 
     >>> print set([pub.status.name for pub in lenny_sources])
     set(['PENDING'])

=== modified file 'lib/lp/soyuz/scripts/gina/archive.py'
--- lib/lp/soyuz/scripts/gina/archive.py	2009-06-25 04:06:00 +0000
+++ lib/lp/soyuz/scripts/gina/archive.py	2010-03-25 10:13:54 +0000
@@ -18,6 +18,7 @@
 import apt_pkg
 import tempfile
 import os
+from collections import defaultdict
 
 from canonical.launchpad.scripts import log
 from lp.soyuz.scripts.gina import call
@@ -185,7 +186,7 @@
 
     def create_maps(self, arch_component_items):
         # Create the maps
-        self.src_map = {}
+        self.src_map = defaultdict(list)
         self.bin_map = {}
 
         # Iterate over ArchComponentItems instance to cover
@@ -207,7 +208,7 @@
                     log.exception("Invalid Sources stanza in %s" %
                                   info_set.sources_tagfile)
                     continue
-                self.src_map[src_name] = src_tmp
+                self.src_map[src_name].append(src_tmp)
 
             # Check if it's in source-only mode, if so, skip binary index
             # mapping.

=== modified file 'lib/lp/soyuz/scripts/tests/gina_test_archive/dists/hoary/main/source/Sources'
--- lib/lp/soyuz/scripts/tests/gina_test_archive/dists/hoary/main/source/Sources	2005-12-01 22:49:03 +0000
+++ lib/lp/soyuz/scripts/tests/gina_test_archive/dists/hoary/main/source/Sources	2010-03-25 10:13:54 +0000
@@ -12,6 +12,20 @@
  41b62efd646e5f010d6e34862503e5a5 510 archive-copier_0.1.5.dsc
  cc6132eb7b8bc715ddd1690a68d7df89 16600 archive-copier_0.1.5.tar.gz
 
+Package: archive-copier
+Binary: archive-copier
+Version: 0.3.6
+Section: misc
+Maintainer: Colin Watson <[e-mail]>
+Build-Depends: debhelper (>= 4.2)
+Architecture: any
+Standards-Version: 3.6.2
+Format: 1.0
+Directory: pool/main/a/archive-copier
+Files:
+ 3b6c16962308a4b8b753ec8d5b289506 510 archive-copier_0.3.6.dsc
+ 6ca2e13475a9aa0a694b85330d905dbd 19598 archive-copier_0.3.6.tar.gz
+
 Package: db1-compat
 Binary: libdb1-compat
 Version: 2.1.3-7

=== modified file 'lib/lp/soyuz/scripts/tests/gina_test_archive/dists/hoary/main/source/Sources.gz'
Binary files lib/lp/soyuz/scripts/tests/gina_test_archive/dists/hoary/main/source/Sources.gz	2005-12-01 22:49:03 +0000 and lib/lp/soyuz/scripts/tests/gina_test_archive/dists/hoary/main/source/Sources.gz	2010-03-25 10:13:54 +0000 differ
=== modified file 'scripts/gina.py'
--- scripts/gina.py	2009-10-13 14:38:07 +0000
+++ scripts/gina.py	2010-03-25 10:13:54 +0000
@@ -194,14 +194,6 @@
                                        dry_run, kdb, package_root, keyrings,
                                        pocket, component_override)
 
-    if spnames_only:
-        log.info('Running in SourcePackageName-only mode...')
-        for source in packages_map.src_map.itervalues():
-            log.info('Ensuring %s name' % source['Package'])
-            importer_handler.ensure_sourcepackagename(source['Package'])
-        log.info('done')
-        return
-
     import_sourcepackages(packages_map, kdb, package_root, keyrings,
                           importer_handler)
     importer_handler.commit()
@@ -229,44 +221,48 @@
     npacks = len(packages_map.src_map)
     log.info('%i Source Packages to be imported' % npacks)
 
-    for source in sorted(packages_map.src_map.values(),
-                         key=lambda x: x.get("Package")):
-        count += 1
-        package_name = source.get("Package", "unknown")
-        try:
+    for list_source in sorted(
+        packages_map.src_map.values(), key=lambda x: x[0].get("Package")):
+        for source in list_source:
+            count += 1
+            package_name = source.get("Package", "unknown")
             try:
-                do_one_sourcepackage(source, kdb, package_root, keyrings,
-                                     importer_handler)
-            except psycopg2.Error:
-                log.exception("Database error: unable to create "
-                              "SourcePackage for %s. Retrying once.."
-                              % package_name)
-                importer_handler.abort()
-                time.sleep(15)
-                do_one_sourcepackage(source, kdb, package_root, keyrings,
-                                     importer_handler)
-        except (InvalidVersionError, MissingRequiredArguments,
+                try:
+                    do_one_sourcepackage(
+                        source, kdb, package_root, keyrings, importer_handler)
+                except psycopg2.Error:
+                    log.exception(
+                        "Database error: unable to create SourcePackage "
+                        "for %s. Retrying once.." % package_name)
+                    importer_handler.abort()
+                    time.sleep(15)
+                    do_one_sourcepackage(
+                        source, kdb, package_root, keyrings, importer_handler)
+            except (
+                InvalidVersionError, MissingRequiredArguments,
                 DisplayNameDecodingError):
-            log.exception("Unable to create SourcePackageData for %s" %
-                          package_name)
-            continue
-        except (PoolFileNotFound, ExecutionError):
-            # Problems with katie db stuff of opening files
-            log.exception("Error processing package files for %s" %
-                          package_name)
-            continue
-        except psycopg2.Error:
-            log.exception("Database errors made me give up: unable to create "
-                          "SourcePackage for %s" % package_name)
-            importer_handler.abort()
-            continue
-        except MultiplePackageReleaseError:
-            log.exception("Database duplication processing %s" %
-                          package_name)
-            continue
+                log.exception(
+                    "Unable to create SourcePackageData for %s" %
+                    package_name)
+                continue
+            except (PoolFileNotFound, ExecutionError):
+                # Problems with katie db stuff of opening files
+                log.exception(
+                    "Error processing package files for %s" % package_name)
+                continue
+            except psycopg2.Error:
+                log.exception(
+                    "Database errors made me give up: unable to create "
+                    "SourcePackage for %s" % package_name)
+                importer_handler.abort()
+                continue
+            except MultiplePackageReleaseError:
+                log.exception(
+                    "Database duplication processing %s" % package_name)
+                continue
 
-        if COUNTDOWN and count % COUNTDOWN == 0:
-            log.warn('%i/%i sourcepackages processed' % (count, npacks))
+            if COUNTDOWN and count % COUNTDOWN == 0:
+                log.warn('%i/%i sourcepackages processed' % (count, npacks))
 
 
 def do_one_sourcepackage(source, kdb, package_root, keyrings,