Merge lp:~jtv/launchpad/bug-845326 into lp:launchpad

[Jeroen T. Vermeulen]

[r=julian-edwards][bug=845326] Make newer [BS]PPHs supersede older ones for same version.

= Summary =

I'm re-doing the domination process so that one Dominator can serve the needs of traditional, latest-publication-record-wins domination; versions-we-see-in-Sources-lists-live domination as needed in Gina; and the as-yet unspecified multiple-versions-can-live-side-by-side domination we are supposed to get in the future.

My code for this was not a full replacement for traditional domination. A single package release can have multiple active publications in the same archive, distroseries, and pocket (e.g. when the package is being re-published into a different component) and traditional domination would mark the newer publication as superseding the older one. The challenge for this branch was to fix that, in a way that was still general enough to serve Gina's needs. All that Gina really knows is what version numbers should survive.

== Proposed fix ==

Extend the new-style domination algorithm: when domination finds multiple publication records for the same package (and archive etc.), for a version that should stay published, then have the newest one supersede the older ones.

(My previous generalized code would keep all publications for the version published. The classic dominator code would only keep the newest publication of the very latest version published and supersede all older ones.)

== Pre-implementation notes ==

William seems to think the solution is sane. As far as we're aware right now, this is the last step before we can land and deploy transitional Gina domination.

== Implementation details ==

The fix itself is fairly straightforward. It's in the first file in the diff. It involves one redundant variable and an extra case in an if/elif cadence, but overall I think it works out pretty cleanly.

== Tests ==

Besides a unit test for the new behaviour, for good measure I also added a massive test for complex combined data. This is not meant to replace proper unit tests; it's too complex for that. But it may reveal any breakage that the unit tests might miss.

{{{
./bin/test -vvc lp.archivepublisher.tests.test_dominator
}}}

== Demo and Q/A ==

A bunch of branches are going to land together. We'll have to make sure that domination still works; that Gina still works; and that Gina now does proper domination.

= Launchpad lint =

There's some pre-existing lint in the dependent branches that either can't be fixed, or would increase the risk of conflicts too much. I did not create any lint of my own, however, and left less than I found.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archivepublisher/domination.py
  lib/lp/soyuz/scripts/gina/dominate.py
  lib/lp/soyuz/model/publishing.py
  lib/lp/soyuz/doc/gina.txt
  lib/lp/soyuz/interfaces/publishing.py
  scripts/gina.py
  lib/lp/archivepublisher/tests/test_dominator.py
  lib/lp/soyuz/scripts/tests/test_gina.py
  lib/lp/soyuz/scripts/gina/handlers.py

./lib/lp/soyuz/doc/gina.txt
     113: narrative exceeds 78 characters.
     162: want exceeds 78 characters.
     179: want exceeds 78 characters.
     189: narrative uses a moin header.
     221: want exceeds 78 characters.
     234: want exceeds 78 characters.
     240: want exceeds 78 characters.
     295: source exceeds 78 characters.
     324: narrative uses a moin header.
     342: narrative exceeds 78 characters.
     354: narrative uses a moin header.
     360: narrative exceeds 78 characters.
     361: narrative exceeds 78 characters.
     459: narrative uses a moin header.
     461: narrative exceeds 78 characters.
     462: narrative exceeds 78 characters.
     477: narrative uses a moin header.
     563: narrative exceeds 78 characters.
     600: narrative uses a moin header.
     657: narrative uses a moin header.
     746: narrative uses a moin header.
     767: narrative uses a moin header.
     780: narrative uses a moin header.
./lib/lp/soyuz/interfaces/publishing.py
     381: E261 at least two spaces before inline comment
     478: E261 at least two spaces before inline comment
     511: E261 at least two spaces before inline comment
     681: E261 at least two spaces before inline comment
     767: E261 at least two spaces before inline comment
./scripts/gina.py
      26: '_pythonpath' imported but unused

[Julian Edwards (julian-edwards)]

review: Approve

=== modified file 'lib/lp/archivepublisher/domination.py'
--- lib/lp/archivepublisher/domination.py	2011-09-09 18:57:14 +0000
+++ lib/lp/archivepublisher/domination.py	2011-09-09 18:57:18 +0000
@@ -193,9 +193,11 @@
     def dominatePackage(self, publications, live_versions, generalization):
         """Dominate publications for a single package.
 
-        Active publications for versions in `live_versions` stay active.
-        Any older versions are marked as superseded by the respective
-        oldest live versions that are newer than the superseded ones.
+        The latest publication for any version in `live_versions` stays
+        active.  Any older publications (including older publications for
+        live versions with multiple publications) are marked as superseded by
+        the respective oldest live releases that are newer than the superseded
+        ones.
 
         Any versions that are newer than anything in `live_versions` are
         marked as deleted.  This should not be possible in Soyuz-native
@@ -222,12 +224,25 @@
             publications, cmp=generalization.compare, reverse=True)
 
         current_dominant = None
+        dominant_version = None
+
         for pub in publications:
-            if generalization.getPackageVersion(pub) in live_versions:
+            version = generalization.getPackageVersion(pub)
+            # There should never be two published releases with the same
+            # version.  So this comparison is really a string
+            # comparison, not a version comparison: if the versions are
+            # equal by either measure, they're from the same release.
+            if dominant_version is not None and version == dominant_version:
+                # This publication is for a live version, but has been
+                # superseded by a newer publication of the same version.
+                # Supersede it.
+                pub.supersede(current_dominant, logger=self.logger)
+            elif version in live_versions:
                 # This publication stays active; if any publications
                 # that follow right after this are to be superseded,
                 # this is the release that they are superseded by.
                 current_dominant = pub
+                dominant_version = version
             elif current_dominant is None:
                 # This publication is no longer live, but there is no
                 # newer version to supersede it either.  Therefore it

=== modified file 'lib/lp/archivepublisher/tests/test_dominator.py'
--- lib/lp/archivepublisher/tests/test_dominator.py	2011-09-09 18:57:14 +0000
+++ lib/lp/archivepublisher/tests/test_dominator.py	2011-09-09 18:57:18 +0000
@@ -5,9 +5,9 @@
 
 __metaclass__ = type
 
+import apt_pkg
 import datetime
-
-import apt_pkg
+from operator import attrgetter
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.database.sqlbase import flush_database_updates
@@ -248,24 +248,25 @@
     return [spph.sourcepackagerelease.version for spph in spphs]
 
 
+def alter_creation_dates(spphs, ages):
+    """Set `datecreated` on each of `spphs` according to `ages`.
+
+    :param spphs: Iterable of `SourcePackagePublishingHistory`.  Their
+        respective creation dates will be offset by the respective ages found
+        in `ages` (with the two being matched up in the same order).
+    :param ages: Iterable of ages.  Must provide the same number of items as
+        `spphs`.  Ages are `timedelta` objects that will be subtracted from
+        the creation dates on the respective records in `spph`.
+    """
+    for spph, age in zip(spphs, ages):
+        spph.datecreated -= age
+
+
 class TestGeneralizedPublication(TestCaseWithFactory):
     """Test publication generalization helpers."""
 
     layer = ZopelessDatabaseLayer
 
-    def alterCreationDates(self, spphs, ages):
-        """Set `datecreated` on each of `spphs` according to `ages`.
-
-        :param spphs: Iterable of `SourcePackagePublishingHistory`.  Their
-            respective creation dates will be offset by the respective ages
-            found in `ages` (with the two being matched up in the same order).
-        :param ages: Iterable of ages.  Must provide the same number of items
-            as `spphs`.  Ages are `timedelta` objects that will be subtracted
-            from the creation dates on the respective records in `spph`.
-        """
-        for spph, age in zip(spphs, ages):
-            spph.datecreated -= age
-
     def test_getPackageVersion_gets_source_version(self):
         spph = self.factory.makeSourcePackagePublishingHistory()
         self.assertEqual(
@@ -327,7 +328,7 @@
                 sourcepackagerelease=spr, distroseries=distroseries,
                 pocket=pocket)
             for counter in xrange(len(ages))]
-        self.alterCreationDates(spphs, ages)
+        alter_creation_dates(spphs, ages)
 
         self.assertEqual(
             [spphs[2], spphs[0], spphs[1]],
@@ -351,13 +352,27 @@
                 sourcepackagerelease=self.factory.makeSourcePackageRelease(
                     version=version))
             for counter in xrange(len(ages))]
-        self.alterCreationDates(spphs, ages)
+        alter_creation_dates(spphs, ages)
 
         self.assertEqual(
             [spphs[2], spphs[0], spphs[1]],
             sorted(spphs, cmp=GeneralizedPublication().compare))
 
 
+def jumble(ordered_list):
+    """Jumble the elements of `ordered_list` into a weird order.
+
+    Ordering is very important in domination.  We jumble some of our lists to
+    insure against "lucky coincidences" that might give our tests the right
+    answers for the wrong reasons.
+    """
+    even = [
+        item for offset, item in enumerate(ordered_list) if offset % 2 == 0]
+    odd = [
+        item for offset, item in enumerate(ordered_list) if offset % 2 != 0]
+    return list(reversed(odd)) + even
+
+
 class TestDominatorMethods(TestCaseWithFactory):
 
     layer = ZopelessDatabaseLayer
@@ -435,6 +450,126 @@
         self.assertEqual(None, pubs[1].supersededby)
         self.assertEqual(PackagePublishingStatus.DELETED, pubs[1].status)
 
+    def test_dominatePackage_supersedes_replaced_pub_for_live_version(self):
+        # Even if a publication record is for a live version, a newer
+        # one for the same version supersedes it.
+        spr = self.factory.makeSourcePackageRelease()
+        series = self.factory.makeDistroSeries()
+        pocket = PackagePublishingPocket.RELEASE
+        pubs = [
+            self.factory.makeSourcePackagePublishingHistory(
+                archive=series.main_archive, distroseries=series,
+                pocket=pocket, status=PackagePublishingStatus.PUBLISHED,
+                sourcepackagerelease=spr)
+            for counter in xrange(3)]
+        alter_creation_dates(pubs, [
+            datetime.timedelta(3),
+            datetime.timedelta(2),
+            datetime.timedelta(1),
+            ])
+
+        self.makeDominator(pubs).dominatePackage(
+            pubs, [spr.version], GeneralizedPublication(True))
+        self.assertEqual([
+            PackagePublishingStatus.SUPERSEDED,
+            PackagePublishingStatus.SUPERSEDED,
+            PackagePublishingStatus.PUBLISHED,
+            ],
+            [pub.status for pub in pubs])
+        self.assertEqual(
+            [spr, spr, None], [pub.supersededby for pub in pubs])
+
+    def test_dominatePackage_advanced_scenario(self):
+        # Put dominatePackage through its paces with complex combined
+        # data.
+        # This test should be redundant in theory (which in theory
+        # equates practice but in practice does not).  If this fails,
+        # don't just patch up the code or this test.  Create unit tests
+        # that specifically cover the difference, then change the code
+        # and/or adapt this test to return to harmony.
+        series = self.factory.makeDistroSeries()
+        package = self.factory.makeSourcePackageName()
+        pocket = PackagePublishingPocket.RELEASE
+
+        versions = ["1.%d" % number for number in xrange(4)]
+
+        # We have one package releases for each version.
+        relevant_releases = dict(
+            (version, self.factory.makeSourcePackageRelease(
+                sourcepackagename=package, version=version))
+            for version in jumble(versions))
+
+        # Each of those releases is subsequently published in
+        # different components.
+        components = jumble(
+            [self.factory.makeComponent() for version in versions])
+
+        # Map versions to lists of publications for that version, from
+        # oldest to newest.  Each re-publishing into a different
+        # component is meant to supersede publication into the previous
+        # component.
+        pubs_by_version = dict(
+            (version, [
+                self.factory.makeSourcePackagePublishingHistory(
+                    archive=series.main_archive, distroseries=series,
+                    pocket=pocket, status=PackagePublishingStatus.PUBLISHED,
+                    sourcepackagerelease=relevant_releases[version],
+                    component=component)
+                for component in components])
+            for version in jumble(versions))
+
+        ages = jumble(
+            [datetime.timedelta(age) for age in xrange(len(versions))])
+
+        # Actually the "oldest to newest" order on the publications only
+        # applies to their creation dates.  Their creation orders are
+        # irrelevant.
+        for pubs_list in pubs_by_version.itervalues():
+            alter_creation_dates(pubs_list, ages)
+            pubs_list.sort(key=attrgetter('datecreated'))
+
+        live_versions = ["1.1", "1.2"]
+        last_version_alive = sorted(live_versions)[-1]
+
+        all_pubs = sum(pubs_by_version.itervalues(), [])
+        Dominator(DevNullLogger(), series.main_archive).dominatePackage(
+            all_pubs, live_versions, GeneralizedPublication(True))
+
+        for version in reversed(versions):
+            pubs = pubs_by_version[version]
+
+            if version in live_versions:
+                # Beware: loop-carried variable.  Used locally as well,
+                # but tells later iterations what the highest-versioned
+                # release so far was.  This is used in tracking
+                # supersededby links.
+                superseding_release = pubs[-1].sourcepackagerelease
+
+            if version in live_versions:
+                # The live versions' latest publications are Published,
+                # their older ones Superseded.
+                expected_status = (
+                    [PackagePublishingStatus.SUPERSEDED] * (len(pubs) - 1) +
+                    [PackagePublishingStatus.PUBLISHED])
+                expected_supersededby = (
+                    [superseding_release] * (len(pubs) - 1) + [None])
+            elif version < last_version_alive:
+                # The superseded versions older than the last live
+                # version have all been superseded.
+                expected_status = (
+                    [PackagePublishingStatus.SUPERSEDED] * len(pubs))
+                expected_supersededby = [superseding_release] * len(pubs)
+            else:
+                # Versions that are newer than any live release have
+                # been deleted.
+                expected_status = (
+                    [PackagePublishingStatus.DELETED] * len(pubs))
+                expected_supersededby = [None] * len(pubs)
+
+            self.assertEqual(expected_status, [pub.status for pub in pubs])
+            self.assertEqual(
+                expected_supersededby, [pub.supersededby for pub in pubs])
+
     def test_dominateRemovedSourceVersions_dominates_publications(self):
         # dominateRemovedSourceVersions finds the publications for a
         # package and calls dominatePackage on them.

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2011-09-09 18:57:14 +0000
+++ lib/lp/soyuz/model/publishing.py	2011-09-09 18:57:18 +0000
@@ -1966,12 +1966,17 @@
             ]
         assert publication_class in permitted_classes, "Deleting wrong type."
 
+        if removed_by is None:
+            removed_by_id = None
+        else:
+            removed_by_id = removed_by.id
+
         affected_pubs = IMasterStore(publication_class).find(
             publication_class, publication_class.id.is_in(ids))
         affected_pubs.set(
             status=PackagePublishingStatus.DELETED,
             datesuperseded=UTC_NOW,
-            removed_byID=removed_by.id,
+            removed_byID=removed_by_id,
             removal_comment=removal_comment)
 
     def requestDeletion(self, sources, removed_by, removal_comment=None):