Merge lp:~jtv/launchpad/pre-832647 into lp:launchpad

[Jeroen T. Vermeulen]

[r=wgrant][bug=832647] Rearrange bits of domcination and gina.

= Summary =

Moving some code about to prepare for the introduction of domination into gina.

== Pre-implementation notes ==

This is the product of extensive discussions with William and Julian, but I'm sort of empty right now and can't reproduce it all! The upshot of it is that they both think what I'm trying to do in the larger picture would work.

== Implementation details ==

The gina changes aren't very important; they're basically just putting the horse back before the cart.

Then there's the publisher changes: a base class has 2 child classes, BPPH and SPPH. (Or more formally, BinaryPackagePublishingHistory and SourcePackagePublishingHistory). Each of these 3 classes has a method “supersede,” with the two derived-class methods calling the base-class one. The base class is never instantiated, so there's really no reason to give its method the same name. I renamed it “setSuperseded” to match the new structure of requestDeletion calling setDeleted.

But the main course is in the dominator. Code in this class is very aggressively unified between BPPH and SPPH. That makes it a little harder to work with. I extracted the specializations away underneath a separate class, really a sub-module of helper functions that hide the differences between BPPH and SPPH. And finally I extracted a few smaller helpers that exploratory coding reveals a need for, and which I think help make the code more readable.

== Tests ==

{{{
./bin/test -vvc lp.archivepublisher
./bin/test -vvc lp.soyuz
}}}

= Launchpad lint =

Nothing to do about the remaining lint, I think; it was all present before I started. Last I heard, someone or other was working to retract the "two spaces" rule so I did nothing about them.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archivepublisher/domination.py
  lib/lp/soyuz/model/publishing.py
  lib/lp/soyuz/interfaces/publishing.py
  scripts/gina.py
  lib/lp/archivepublisher/tests/test_dominator.py

./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

[William Grant (wgrant)]

189 + def _composeActiveSourcePubsCondition(self, distroseries, pocket):

It seems slightly odd to have this take distroseries and pocket, but grab archive from self. This is a common problem in Soyuz, since archives didn't exist when most of it was written. Something to think about, maybe.

234 + And(join_spr_spn(), join_spph_spr(), spph_location_clauses),

This would possibly be clearer if the order of arguments was reversed. The (SPR, SPN) -> (SPPH, SPR) -> SPPH chaining takes a while to pick up here.

319 + def listCreaitonDates(self, spphs):

Unused and misspelled :(

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

> 189 + def _composeActiveSourcePubsCondition(self, distroseries, pocket):
>
> It seems slightly odd to have this take distroseries and pocket, but grab
> archive from self. This is a common problem in Soyuz, since archives didn't
> exist when most of it was written. Something to think about, maybe.

It did not escape notice. But the API reflects the current API of the dominator itself, and will have to rearranged in conjunction with it.

> 234 + And(join_spr_spn(), join_spph_spr(), spph_location_clauses),
>
> This would possibly be clearer if the order of arguments was reversed. The
> (SPR, SPN) -> (SPPH, SPR) -> SPPH chaining takes a while to pick up here.

True, it helps. I switched the first two around, but kept join conditions before selection conditions.

> 319 + def listCreaitonDates(self, spphs):
>
> Unused and misspelled :(

Removed.

=== modified file 'lib/lp/archivepublisher/domination.py'
--- lib/lp/archivepublisher/domination.py	2011-08-30 06:37:55 +0000
+++ lib/lp/archivepublisher/domination.py	2011-09-05 12:08:08 +0000
@@ -53,8 +53,6 @@
 __all__ = ['Dominator']
 
 from datetime import timedelta
-import functools
-import operator
 
 import apt_pkg
 from storm.expr import (
@@ -68,7 +66,7 @@
     flush_database_updates,
     sqlvalues,
     )
-from canonical.launchpad.interfaces.lpstorm import IMasterStore
+from canonical.launchpad.interfaces.lpstorm import IStore
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.soyuz.enums import (
     BinaryPackageFormat,
@@ -87,17 +85,93 @@
 apt_pkg.InitSystem()
 
 
-def _compare_packages_by_version_and_date(get_release, p1, p2):
-    """Compare publications p1 and p2 by their version; using Debian rules.
-
-    If the publications are for the same package, compare by datecreated
-    instead. This lets newer records win.
-    """
-    if get_release(p1).id == get_release(p2).id:
-        return cmp(p1.datecreated, p2.datecreated)
-
-    return apt_pkg.VersionCompare(get_release(p1).version,
-                                  get_release(p2).version)
+def join_spr_spn():
+    """Join condition: SourcePackageRelease/SourcePackageName."""
+    return (
+        SourcePackageName.id == SourcePackageRelease.sourcepackagenameID)
+
+
+def join_spph_spr():
+    """Join condition: SourcePackageRelease/SourcePackagePublishingHistory.
+    """
+    # Avoid circular imports.
+    from lp.soyuz.model.publishing import SourcePackagePublishingHistory
+
+    return (
+        SourcePackageRelease.id ==
+            SourcePackagePublishingHistory.sourcepackagereleaseID)
+
+
+class SourcePublicationTraits:
+    """Basic generalized attributes for `SourcePackagePublishingHistory`.
+
+    Used by `GeneralizedPublication` to hide the differences from
+    `BinaryPackagePublishingHistory`.
+    """
+    @staticmethod
+    def getPackageName(spph):
+        """Return the name of this publication's source package."""
+        return spph.sourcepackagerelease.sourcepackagename.name
+
+    @staticmethod
+    def getPackageRelease(spph):
+        """Return this publication's `SourcePackageRelease`."""
+        return spph.sourcepackagerelease
+
+
+class BinaryPublicationTraits:
+    """Basic generalized attributes for `BinaryPackagePublishingHistory`.
+
+    Used by `GeneralizedPublication` to hide the differences from
+    `SourcePackagePublishingHistory`.
+    """
+    @staticmethod
+    def getPackageName(bpph):
+        """Return the name of this publication's binary package."""
+        return bpph.binarypackagerelease.binarypackagename.name
+
+    @staticmethod
+    def getPackageRelease(bpph):
+        """Return this publication's `BinaryPackageRelease`."""
+        return bpph.binarypackagerelease
+
+
+class GeneralizedPublication:
+    """Generalize handling of publication records.
+
+    This allows us to write code that can be dealing with either
+    `SourcePackagePublishingHistory`s or `BinaryPackagePublishingHistory`s
+    without caring which.  Differences are abstracted away in a traits
+    class.
+    """
+    def __init__(self, is_source=True):
+        if is_source:
+            self.traits = SourcePublicationTraits
+        else:
+            self.traits = BinaryPublicationTraits
+
+    def getPackageName(self, pub):
+        """Get the package's name."""
+        return self.traits.getPackageName(pub)
+
+    def getPackageVersion(self, pub):
+        """Obtain the version string for a publicaiton record."""
+        return self.traits.getPackageRelease(pub).version
+
+    def compare(self, pub1, pub2):
+        """Compare publications by version.
+
+        If both publications are for the same version, their creation dates
+        break the tie.
+        """
+        version_comparison = apt_pkg.VersionCompare(
+            self.getPackageVersion(pub1), self.getPackageVersion(pub2))
+
+        if version_comparison == 0:
+            # Use dates as tie breaker.
+            return cmp(pub1.datecreated, pub2.datecreated)
+        else:
+            return version_comparison
 
 
 class Dominator:
@@ -125,11 +199,10 @@
         """
         self.logger.debug("Dominating packages...")
 
-        for name in pubs.keys():
-            assert pubs[name], (
-                "Empty list of publications for %s" % name)
-            for pubrec in pubs[name][1:]:
-                pubrec.supersede(pubs[name][0], logger=self.logger)
+        for name, publications in pubs.iteritems():
+            assert publications, "Empty list of publications for %s." % name
+            for pubrec in publications[1:]:
+                pubrec.supersede(publications[0], logger=self.logger)
 
     def _sortPackages(self, pkglist, is_source=True):
         """Map out packages by name, and sort by descending version.
@@ -139,27 +212,20 @@
         :param is_source: Whether this call involves source package
             publications.  If so, work with `SourcePackagePublishingHistory`.
             If not, work with `BinaryPackagepublishingHistory`.
-        :return: A dict mapping each package name (as UTF-8 encoded string)
-            to a list of publications from `pkglist`, newest first.
+        :return: A dict mapping each package name to a list of publications
+            from `pkglist`, newest first.
         """
         self.logger.debug("Sorting packages...")
 
-        if is_source:
-            get_release = operator.attrgetter("sourcepackagerelease")
-            get_name = operator.attrgetter("sourcepackagename")
-        else:
-            get_release = operator.attrgetter("binarypackagerelease")
-            get_name = operator.attrgetter("binarypackagename")
+        generalization = GeneralizedPublication(is_source)
 
         outpkgs = {}
         for inpkg in pkglist:
-            key = get_name(get_release(inpkg)).name.encode('utf-8')
+            key = generalization.getPackageName(inpkg)
             outpkgs.setdefault(key, []).append(inpkg)
 
-        sort_order = functools.partial(
-            _compare_packages_by_version_and_date, get_release)
         for package_pubs in outpkgs.itervalues():
-            package_pubs.sort(cmp=sort_order, reverse=True)
+            package_pubs.sort(cmp=generalization.compare, reverse=True)
 
         return outpkgs
 
@@ -312,7 +378,7 @@
                 ),
                 group_by=BinaryPackageName.id,
                 having=Count(BinaryPackagePublishingHistory.id) > 1)
-            binaries = IMasterStore(BinaryPackagePublishingHistory).find(
+            binaries = IStore(BinaryPackagePublishingHistory).find(
                 BinaryPackagePublishingHistory,
                 BinaryPackageRelease.id ==
                     BinaryPackagePublishingHistory.binarypackagereleaseID,
@@ -324,6 +390,19 @@
             self.logger.debug("Dominating binaries...")
             self._dominatePublications(self._sortPackages(binaries, False))
 
+    def _composeActiveSourcePubsCondition(self, distroseries, pocket):
+        """Compose ORM condition for restricting relevant source pubs."""
+        # Avoid circular imports.
+        from lp.soyuz.model.publishing import SourcePackagePublishingHistory
+
+        return And(
+            SourcePackagePublishingHistory.status ==
+                PackagePublishingStatus.PUBLISHED,
+            SourcePackagePublishingHistory.distroseries == distroseries,
+            SourcePackagePublishingHistory.archive == self.archive,
+            SourcePackagePublishingHistory.pocket == pocket,
+            )
+
     def dominateSources(self, distroseries, pocket):
         """Perform domination on source package publications.
 
@@ -332,34 +411,27 @@
         """
         # Avoid circular imports.
         from lp.soyuz.model.publishing import SourcePackagePublishingHistory
+
         self.logger.debug(
             "Performing domination across %s/%s (Source)",
             distroseries.name, pocket.title)
-        spph_location_clauses = And(
-            SourcePackagePublishingHistory.status ==
-                PackagePublishingStatus.PUBLISHED,
-            SourcePackagePublishingHistory.distroseries == distroseries,
-            SourcePackagePublishingHistory.archive == self.archive,
-            SourcePackagePublishingHistory.pocket == pocket,
-            )
+
+        spph_location_clauses = self._composeActiveSourcePubsCondition(
+            distroseries, pocket)
+        having_multiple_active_publications = (
+            Count(SourcePackagePublishingHistory.id) > 1)
         candidate_source_names = Select(
             SourcePackageName.id,
-            And(
-                SourcePackageRelease.sourcepackagenameID ==
-                    SourcePackageName.id,
-                SourcePackagePublishingHistory.sourcepackagereleaseID ==
-                    SourcePackageRelease.id,
-                spph_location_clauses,
-            ),
+            And(join_spr_spn(), join_spph_spr(), spph_location_clauses),
             group_by=SourcePackageName.id,
-            having=Count(SourcePackagePublishingHistory.id) > 1)
-        sources = IMasterStore(SourcePackagePublishingHistory).find(
+            having=having_multiple_active_publications)
+        sources = IStore(SourcePackagePublishingHistory).find(
             SourcePackagePublishingHistory,
-            SourcePackageRelease.id ==
-                SourcePackagePublishingHistory.sourcepackagereleaseID,
+            join_spph_spr(),
             SourcePackageRelease.sourcepackagenameID.is_in(
                 candidate_source_names),
             spph_location_clauses)
+
         self.logger.debug("Dominating sources...")
         self._dominatePublications(self._sortPackages(sources))
         flush_database_updates()

=== modified file 'lib/lp/archivepublisher/tests/test_dominator.py'
--- lib/lp/archivepublisher/tests/test_dominator.py	2011-02-04 05:11:00 +0000
+++ lib/lp/archivepublisher/tests/test_dominator.py	2011-09-05 12:08:08 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for domination.py."""
@@ -7,12 +7,20 @@
 
 import datetime
 
+import apt_pkg
+
 from canonical.database.sqlbase import flush_database_updates
-from lp.archivepublisher.domination import Dominator, STAY_OF_EXECUTION
+from canonical.testing.layers import ZopelessDatabaseLayer
+from lp.archivepublisher.domination import (
+    Dominator,
+    GeneralizedPublication,
+    STAY_OF_EXECUTION,
+    )
 from lp.archivepublisher.publishing import Publisher
 from lp.registry.interfaces.series import SeriesStatus
 from lp.soyuz.enums import PackagePublishingStatus
 from lp.soyuz.tests.test_publishing import TestNativePublishingBase
+from lp.testing import TestCaseWithFactory
 
 
 class TestDominator(TestNativePublishingBase):
@@ -200,3 +208,143 @@
         TestDomination.setUp(self)
         self.ubuntutest['breezy-autotest'].status = (
             SeriesStatus.OBSOLETE)
+
+
+class TestGeneralizedPublication(TestCaseWithFactory):
+    """Test publication generalization helpers."""
+
+    layer = ZopelessDatabaseLayer
+
+    def makeSPPHsForVersions(self, versions):
+        """Create publication records for each of `versions`.
+
+        They records are created in the same order in which they are
+        specified.  Make the order irregular to prove that version ordering
+        is not a coincidence of object creation order etc.
+
+        Versions may also be identical; each publication record will still
+        have its own package release.
+        """
+        distroseries = self.factory.makeDistroSeries()
+        pocket = self.factory.getAnyPocket()
+        sprs = [
+            self.factory.makeSourcePackageRelease(version=version)
+            for version in versions]
+        return [
+            self.factory.makeSourcePackagePublishingHistory(
+                distroseries=distroseries, pocket=pocket,
+                sourcepackagerelease=spr)
+            for spr in sprs]
+
+    def listSourceVersions(self, spphs):
+        """Extract the versions from `spphs` as a list, in the same order."""
+        return [spph.sourcepackagerelease.version for spph in spphs]
+
+    def listCreaitonDates(self, spphs):
+        """Extract creation dates from `spphs` into a list."""
+        return [spph.datecreated for spph in spphs]
+
+    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(
+            spph.sourcepackagerelease.version,
+            GeneralizedPublication(is_source=True).getPackageVersion(spph))
+
+    def test_getPackageVersion_gets_binary_version(self):
+        bpph = self.factory.makeBinaryPackagePublishingHistory()
+        self.assertEqual(
+            bpph.binarypackagerelease.version,
+            GeneralizedPublication(is_source=False).getPackageVersion(bpph))
+
+    def test_compare_sorts_versions(self):
+        versions = [
+            '1.1v2',
+            '1.1v1',
+            '1.1v3',
+            ]
+        spphs = self.makeSPPHsForVersions(versions)
+        sorted_spphs = sorted(spphs, cmp=GeneralizedPublication().compare)
+        self.assertEqual(
+            sorted(versions),
+            self.listSourceVersions(sorted_spphs))
+
+    def test_compare_orders_versions_by_debian_rules(self):
+        versions = [
+            '1.1.0',
+            '1.10',
+            '1.1',
+            '1.1ubuntu0',
+            ]
+        spphs = self.makeSPPHsForVersions(versions)
+
+        debian_sorted_versions = sorted(versions, cmp=apt_pkg.VersionCompare)
+
+        # Assumption: in this case, Debian version ordering is not the
+        # same as alphabetical version ordering.
+        self.assertNotEqual(sorted(versions), debian_sorted_versions)
+
+        # The compare method produces the Debian ordering.
+        sorted_spphs = sorted(spphs, cmp=GeneralizedPublication().compare)
+        self.assertEqual(
+            sorted(versions, cmp=apt_pkg.VersionCompare),
+            self.listSourceVersions(sorted_spphs))
+
+    def test_compare_breaks_tie_with_creation_date(self):
+        # When two publications are tied for comparison because they are
+        # for the same package release, they are ordered by creation
+        # date.
+        distroseries = self.factory.makeDistroSeries()
+        pocket = self.factory.getAnyPocket()
+        spr = self.factory.makeSourcePackageRelease()
+        ages = [
+            datetime.timedelta(2),
+            datetime.timedelta(1),
+            datetime.timedelta(3),
+            ]
+        spphs = [
+            self.factory.makeSourcePackagePublishingHistory(
+                sourcepackagerelease=spr, distroseries=distroseries,
+                pocket=pocket)
+            for counter in xrange(len(ages))]
+        self.alterCreationDates(spphs, ages)
+
+        self.assertEqual(
+            [spphs[2], spphs[0], spphs[1]],
+            sorted(spphs, cmp=GeneralizedPublication().compare))
+
+    def test_compare_breaks_tie_for_releases_with_same_version(self):
+        # When two publications are tied for comparison because they
+        # belong to releases with the same version string, they are
+        # ordered by creation date.
+        version = "1.%d" % self.factory.getUniqueInteger()
+        ages = [
+            datetime.timedelta(2),
+            datetime.timedelta(1),
+            datetime.timedelta(3),
+            ]
+        distroseries = self.factory.makeDistroSeries()
+        pocket = self.factory.getAnyPocket()
+        spphs = [
+            self.factory.makeSourcePackagePublishingHistory(
+                distroseries=distroseries, pocket=pocket,
+                sourcepackagerelease=self.factory.makeSourcePackageRelease(
+                    version=version))
+            for counter in xrange(len(ages))]
+        self.alterCreationDates(spphs, ages)
+
+        self.assertEqual(
+            [spphs[2], spphs[0], spphs[1]],
+            sorted(spphs, cmp=GeneralizedPublication().compare))

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2011-09-02 04:51:25 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2011-09-05 12:08:08 +0000
@@ -195,9 +195,6 @@
         the field name and value is the value string.
         """
 
-    def supersede():
-        """Supersede this publication."""
-
     def requestObsolescence():
         """Make this publication obsolete.
 

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2011-08-31 04:40:44 +0000
+++ lib/lp/soyuz/model/publishing.py	2011-09-05 12:08:08 +0000
@@ -327,8 +327,8 @@
         fields = self.buildIndexStanzaFields()
         return fields.makeOutput()
 
-    def supersede(self):
-        """See `IPublishing`."""
+    def setSuperseded(self):
+        """Set to SUPERSEDED status."""
         self.status = PackagePublishingStatus.SUPERSEDED
         self.datesuperseded = UTC_NOW
 
@@ -742,7 +742,7 @@
             "Should not dominate unpublished source %s" %
             self.sourcepackagerelease.title)
 
-        super(SourcePackagePublishingHistory, self).supersede()
+        self.setSuperseded()
 
         if dominant is not None:
             if logger is not None:
@@ -1126,7 +1126,7 @@
                 self.distroarchseries.architecturetag))
             return
 
-        super(BinaryPackagePublishingHistory, self).supersede()
+        self.setSuperseded()
 
         if dominant is not None:
             # DDEBs cannot themselves be dominant; they are always dominated

=== modified file 'scripts/gina.py'
--- scripts/gina.py	2011-08-23 08:35:13 +0000
+++ scripts/gina.py	2011-09-05 12:08:08 +0000
@@ -209,9 +209,8 @@
     npacks = len(packages_map.src_map)
     log.info('%i Source Packages to be imported', npacks)
 
-    for list_source in sorted(
-        packages_map.src_map.values(), key=lambda x: x[0].get("Package")):
-        for source in list_source:
+    for package in sorted(packages_map.src_map.iterkeys()):
+        for source in packages_map.src_map[package]:
             count += 1
             attempt_source_package_import(
                 source, kdb, package_root, keyrings, importer_handler)
@@ -244,10 +243,9 @@
         log.info(
             '%i Binary Packages to be imported for %s', npacks, archtag)
         # Go over binarypackages importing them for this architecture
-        for binary in sorted(packages_map.bin_map[archtag].values(),
-                             key=lambda x: x.get("Package")):
+        for package_name in sorted(packages_map.bin_map[archtag].iterkeys()):
+            binary = packages_map.bin_map[archtag][package_name]
             count += 1
-            package_name = binary.get("Package", "unknown")
             try:
                 try:
                     do_one_binarypackage(binary, archtag, kdb, package_root,