Merge lp:~cjwatson/launchpad/gina-dsc-binaries into lp:launchpad

[Colin Watson]

[r=allenap][bug=911943] Separate dsc_binaries correctly in gina, and add a garbo-daily job to clean up existing wreckage.

== Summary ==

There is some wrong data in SourcePackageRelease.dsc_binaries, which I believe is all gina's fault: that field is meant to be separated with comma-space, not space. This breaks lp:~cjwatson/launchpad/germinate-from-db, but would also be a blocker (one of several, but still) to switching to no-more-apt-ftparchive publishing for Ubuntu.

== Proposed fix ==

Fix gina to separate dsc_binaries correctly, and add a garbo-daily job to clean up the wreckage.

== Pre-implementation notes ==

Robert and Stuart educated me a bit on how to write garbo jobs.

== Implementation details ==

I found a few rows on dogfood where the Binary field had been incorrectly written as a dependency relationship field, complete with "(>= ...)". This is wildly wrong and it came from PPA uploads rather than gina; but it's in the past and s/ /, /g would not improve matters, so I just excluded anything containing '(', and included a test that such rows remain unchanged.

== Tests ==

bin/test -vvc -t gina -t garbo

== Demo and Q/A ==

I'd like to run this garbo job on dogfood and check that it really does clean everything up, which is probably most easily done by manual inspection of:

  SELECT dsc_binaries FROM SourcePackageRelease WHERE dsc_binaries ~ '[a-z0-9+.-] ';

== lint ==

./lib/lp/soyuz/doc/gina.txt
     162: want exceeds 78 characters.
     179: want exceeds 78 characters.
     223: want exceeds 78 characters.
     236: want exceeds 78 characters.
     242: want exceeds 78 characters.

Pre-existing, fiddly to fix, and should be ignoreable for the purposes of this branch.

[Gavin Panella (allenap)]

Nice :)

[1]

+ WHERE dsc_binaries ~ '[a-z0-9+.-] '

Could the regex be simplified to '[^,] '?

[2]

Fwiw, an alternative, simpler, implementation might be something like:

    def __init__(self, log, abort_time=None):
        super(SourcePackageReleaseDscBinariesUpdater, self).__init__(
            log, abort_time)
        self.store = IMasterStore(SourcePackageRelease)
        self.ids = list(
            self.store.find(
                SourcePackageRelease.id,
                # Get all SPR IDs which have an incorrectly-separated
                # dsc_binaries value (space rather than comma-space).
                SQL("dsc_binaries ~ '[a-z0-9+.-] '"),
                # Skip rows with dsc_binaries in dependency relationship
                # format. This is a different bug.
                SQL("dsc_binaries NOT LIKE '%(%'")))

    def isDone(self):
        """See `TunableLoop`."""
        return len(self.ids) == 0

    def __call__(self, chunk_size):
        """See `TunableLoop`."""
        chunk_ids = self.ids[:chunk_size]
        del self.ids[:chunk_size]
        self.store.execute("""
            UPDATE SourcePackageRelease
            SET dsc_binaries = REGEXP_REPLACE(
                dsc_binaries, '([a-z0-9+.-]) ', E'\\\\1, ', 'g')
            WHERE id IN %s""" % sqlvalues(chunk_ids), noresult=True)
        transaction.commit()

[3]

+ three = [
+ self.factory.getUniqueString(),
+ self.factory.getUniqueString(),
+ self.factory.getUniqueString(),
+ ]
+ spr_three = self.factory.makeSourcePackageRelease(
+ dsc_binaries=" ".join(three))

Is it important to use unique strings? If not it might be clearer to
make them static:

        spr_three = self.factory.makeSourcePackageRelease("one1 two2 three3")
        ...
        self.assertEqual("one1, two2, three3", spr_three.dsc_binaries)

review: Approve

[Colin Watson (cjwatson)]

On Thu, Jan 05, 2012 at 04:16:30PM -0000, Gavin Panella wrote:
> + WHERE dsc_binaries ~ '[a-z0-9+.-] '
>
> Could the regex be simplified to '[^,] '?

No, because there exist dsc_binaries fields with embedded newlines.

Something like [^\n,] might be possible, but I actually felt the above
was a bit clearer. I checked on dogfood's database to make sure there
were no rows that slipped between the cracks.

> Fwiw, an alternative, simpler, implementation might be something like:

Thanks! This is indeed much easier to read. I've applied that.

> + three = [
> + self.factory.getUniqueString(),
> + self.factory.getUniqueString(),
> + self.factory.getUniqueString(),
> + ]
> + spr_three = self.factory.makeSourcePackageRelease(
> + dsc_binaries=" ".join(three))
>
> Is it important to use unique strings? If not it might be clearer to
> make them static:

Yes, you're probably right; I got a bit carried away with the factory
there. I've simplified this as you suggest.

Do you think you could land this for me, if you're satisfied with my
answer to your first point? TIA.

[Gavin Panella (allenap)]

> Do you think you could land this for me, if you're satisfied with my
> answer to your first point? TIA.

Sure, I'll send it off to ec2 now.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2012-01-05 15:19:43 +0000
+++ database/schema/security.cfg	2012-01-05 17:21:29 +0000
@@ -2157,7 +2157,7 @@
 public.revisionauthor                   = SELECT, UPDATE
 public.revisioncache                    = SELECT, DELETE
 public.sourcepackagename                = SELECT
-public.sourcepackagerelease             = SELECT
+public.sourcepackagerelease             = SELECT, UPDATE
 public.sourcepackagepublishinghistory   = SELECT, UPDATE
 public.suggestivepotemplate             = INSERT, DELETE
 public.teamparticipation                = SELECT, DELETE

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2012-01-04 11:49:08 +0000
+++ lib/lp/scripts/garbo.py	2012-01-05 17:21:29 +0000
@@ -86,6 +86,7 @@
     MAIN_STORE,
     MASTER_FLAVOR,
     )
+from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 from lp.translations.interfaces.potemplate import IPOTemplateSet
 from lp.translations.model.potmsgset import POTMsgSet
 from lp.translations.model.potranslation import POTranslation
@@ -901,6 +902,41 @@
         self._update_oldest()
 
 
+class SourcePackageReleaseDscBinariesUpdater(TunableLoop):
+    """Fix incorrect values for SourcePackageRelease.dsc_binaries."""
+
+    maximum_chunk_size = 1000
+
+    def __init__(self, log, abort_time=None):
+        super(SourcePackageReleaseDscBinariesUpdater, self).__init__(
+            log, abort_time)
+        self.store = IMasterStore(SourcePackageRelease)
+        self.ids = list(
+            self.store.find(
+                SourcePackageRelease.id,
+                # Get all SPR IDs which have an incorrectly-separated
+                # dsc_binaries value (space rather than comma-space).
+                SQL("dsc_binaries ~ '[a-z0-9+.-] '"),
+                # Skip rows with dsc_binaries in dependency relationship
+                # format.  This is a different bug.
+                SQL("dsc_binaries NOT LIKE '%(%'")))
+
+    def isDone(self):
+        """See `TunableLoop`."""
+        return len(self.ids) == 0
+
+    def __call__(self, chunk_size):
+        """See `TunableLoop`."""
+        chunk_ids = self.ids[:chunk_size]
+        del self.ids[:chunk_size]
+        self.store.execute("""
+            UPDATE SourcePackageRelease
+            SET dsc_binaries = regexp_replace(
+                dsc_binaries, '([a-z0-9+.-]) ', E'\\\\1, ', 'g')
+            WHERE id IN %s""" % sqlvalues(chunk_ids), noresult=True)
+        transaction.commit()
+
+
 class SuggestiveTemplatesCacheUpdater(TunableLoop):
     """Refresh the SuggestivePOTemplate cache.
 
@@ -1265,6 +1301,7 @@
         ObsoleteBugAttachmentPruner,
         OldTimeLimitedTokenDeleter,
         RevisionAuthorEmailLinker,
+        SourcePackageReleaseDscBinariesUpdater,
         SuggestiveTemplatesCacheUpdater,
         POTranslationPruner,
         UnusedPOTMsgSetPruner,

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2012-01-04 11:49:08 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2012-01-05 17:21:29 +0000
@@ -996,6 +996,42 @@
         self.runDaily()
         self.assertEqual(0, unreferenced_msgsets.count())
 
+    def test_SourcePackageReleaseDscBinariesUpdater_updates_incorrect(self):
+        # SourcePackageReleaseDscBinariesUpdater fixes incorrectly-separated
+        # dsc_binaries values.
+        LaunchpadZopelessLayer.switchDbUser('testadmin')
+        spr = self.factory.makeSourcePackageRelease(
+            dsc_binaries="one two three")
+        transaction.commit()
+        self.runDaily()
+        self.assertEqual("one, two, three", spr.dsc_binaries)
+
+    def test_SourcePackageReleaseDscBinariesUpdater_skips_correct(self):
+        # SourcePackageReleaseDscBinariesUpdater leaves correct dsc_binaries
+        # values alone.
+        LaunchpadZopelessLayer.switchDbUser('testadmin')
+        spr_one = self.factory.makeSourcePackageRelease(dsc_binaries="one")
+        spr_three = self.factory.makeSourcePackageRelease(
+            dsc_binaries="one, two, three")
+        transaction.commit()
+        self.runDaily()
+        self.assertEqual("one", spr_one.dsc_binaries)
+        self.assertEqual("one, two, three", spr_three.dsc_binaries)
+
+    def test_SourcePackageReleaseDscBinariesUpdater_skips_broken(self):
+        # There have been a few instances of Binary fields in PPA packages
+        # that are formatted like a dependency relationship field, complete
+        # with (>= ...).  This is completely invalid (and failed to build),
+        # but does exist historically, so we have to deal with it.
+        # SourcePackageReleaseDscBinariesUpdater leaves such fields well
+        # alone.
+        LaunchpadZopelessLayer.switchDbUser('testadmin')
+        spr = self.factory.makeSourcePackageRelease(
+            dsc_binaries="one (>= 1), two")
+        transaction.commit()
+        self.runDaily()
+        self.assertEqual("one (>= 1), two", spr.dsc_binaries)
+
 
 class TestGarboTasks(TestCaseWithFactory):
     layer = LaunchpadZopelessLayer

=== modified file 'lib/lp/soyuz/doc/gina.txt'
--- lib/lp/soyuz/doc/gina.txt	2011-12-30 06:14:56 +0000
+++ lib/lp/soyuz/doc/gina.txt	2012-01-05 17:21:29 +0000
@@ -280,6 +280,8 @@
     -----END PGP SIGNATURE-----
     >>> print cap.maintainer.displayname
     Michael Vogt
+    >>> print cap.dsc_binaries
+    libcap-dev, libcap-bin, libcap1
 
 Test ubuntu-meta in breezy, which was forcefully imported.
 

=== modified file 'lib/lp/soyuz/scripts/gina/handlers.py'
--- lib/lp/soyuz/scripts/gina/handlers.py	2011-12-30 06:14:56 +0000
+++ lib/lp/soyuz/scripts/gina/handlers.py	2012-01-05 17:21:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Gina db handlers.
@@ -654,7 +654,7 @@
             dsc_format=src.format,
             dsc_maintainer_rfc822=maintainer_line,
             dsc_standards_version=src.standards_version,
-            dsc_binaries=" ".join(src.binaries),
+            dsc_binaries=", ".join(src.binaries),
             upload_archive=distroseries.main_archive)
         log.info('Source Package Release %s (%s) created' %
                  (name.name, src.version))