Merge lp:~jtv/launchpad/bug-884649-branch-2 into lp:launchpad

[Jeroen T. Vermeulen]

[r=jcsackett,julian-edwards] Move special-casing for publication liveness out of inner domination loop; specialize binary passes.

= Summary =

This is part of the Dominator optimization work needed for bug 884649.

Binary domination needs 2 passes. This is because publications for the "all" architecture, which are copied into each architecture, can't be superseded until all architecture-specific publications coming from the same source package release have been superseded. Only then can we decide whether arch-all publications can be superseded. This is because a binary package from the same source package release may still depend on the arch-all binary package, and may not have finished building yet.

The code for dealing with this subtlety is a bit hard to place, because it's deep down in the inner loop as a special case. The special case is applied to both binary-domination passes, although there's really no point: the first pass may conceivably supersede a few arch-all publications but it won't deal with all of them. Meanwhile we pay a hefty price for the extra check.

== Proposed fix ==

Hoist the decision of which publications must stay live out of dominatePackage. It's a policy decision for the caller; the API lets the caller specify which versions stay live.

With that, we can specialize the passes more, and cut out complexity that is slowing things down.

You'll see three new functions explaining exactly what liveness criteria each domination pass uses. The source pass is easy. The first binary pass deals with architecture-specific publications; arch-all ones must be considered, but they are simply kept alive. And once all architectures have gone through first-pass binary domination, superseding as many arch-specific publications as possible, the second binary-domination pass considers just what needs to happen to those arch-all publications.

== Pre-implementation notes ==

Discussed in detail with Julian; see bug 884649 for the plan we're following in addressing this. We should also be able to re-use liveness decisions across architectures (and probably binary packages building from the same source package).

Raphaël suggested another clever improvement: Steve K. has just initialized a new, denormalized column on BinaryPackagePublishingHistory that contains the package name. That should enable a few shortcuts as well.

By the way, note that we can't just dominate arch-all and arch-specific publications separately. Packages may vacillate between the two, producing successive releases that need to be considered together even if domination of some publications may need to be postponed.

== Implementation details ==

I had to ditch _dominatePublications, because the discovery of live versions went into the middle of it. I reasoned that if removing it introduced too much code duplication, it could return with a callback for live-version discovery. But the loss turned out to be pretty small: it's really just a 3-line loop, including log output.

== Tests ==

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

== Demo and Q/A ==

Dominate Ubuntu and you'll see.

= Launchpad lint =

Checking for conflicts and issues in changed files.

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

[j.c.sackett (jcsackett)]

Whew. Very extensive commenting, and thanks for it or I don't think I would have followed all of that!

This is some very complex work, but I think it looks good to move forward.

review: Approve

[Julian Edwards (julian-edwards)]

(I reviewed this several hours ago by email and only just noticed that LP rejected my email..GRRRRRRRRRRR)

Thanks for doing this Jeroen, it looks pretty good so far. I've just got a
few nits inline:

> === modified file 'lib/lp/archivepublisher/domination.py'
> --- lib/lp/archivepublisher/domination.py 2011-11-02 12:58:31 +0000
> +++ lib/lp/archivepublisher/domination.py 2011-11-03 10:12:17 +0000
> @@ -52,8 +52,12 @@
>
> __all__ = ['Dominator']
>
> +from collections import defaultdict
> from datetime import timedelta
> -from operator import itemgetter
> +from operator import (
> + attrgetter,
> + itemgetter,
> + )
>
> import apt_pkg
> from storm.expr import (
> @@ -72,6 +76,7 @@
> DecoratedResultSet,
> )
> from canonical.launchpad.interfaces.lpstorm import IStore
> +from canonical.launchpad.utilities.orderingcheck import OrderingCheck
> from lp.registry.model.sourcepackagename import SourcePackageName
> from lp.services.database.bulk import load_related
> from lp.soyuz.enums import (
> @@ -192,6 +197,104 @@
> else:
> return version_comparison
>
> + def sortPublications(self, publications):
> + """Sort publications from most to least current versions."""
> + # Listify; we want to iterate this twice, which won't do for a
> + # non-persistent sequence.
> + sorted_publications = list(publications)

This variable is prematurely named, I think I'd just write:
    publications = list(publications)
and return publications.sort(...) later.

> + # Batch-load associated package releases; we'll be needing them
> + # to compare versions.
> + self.load_releases(sorted_publications)
> + # Now sort. This is that second iteration. An in-place sort
> + # won't hurt the original, because we're working on a copy of
> + # the original iterable.
> + sorted_publications.sort(cmp=self.compare, reverse=True)
> + return sorted_publications
> +
> +
> +def find_live_source_versions(publications):
> + """Find versions out of Published `publications` that should stay live.
> +
> + This particular notion of liveness applies to source domination: the
> + latest version stays live, and that's it.
> +
> + :param publications: An iterable of `SourcePackagePublishingHistory`
> + sorted by descending package version.
> + :return: A list of live versions.
> + """
> + # Given the required sort order, the latest version is at the head
> + # of the list.
> + return [publications[0].sourcepackagerelease.version]

Nicely abstracted.

> +
> +
> +def get_binary_versions(binary_publications):
> + """List versions for sequence of `BinaryPackagePublishingHistory`."""
> + return [pub.binarypackagerelease.version for pub in
binary_publications]

If you're going to do :param: and :return: for the other functions, you
should do it here too!

> +
> +
> +def find_live_binary_versions_pass_1(publications):
> + """Find versions out of Published `publications` that should stay live.
> +
> + This particular notion of liveness applies to first-pass binary
> + domination: the latest version stays l...

review: Approve

[Jeroen T. Vermeulen (jtv)]

Jon, thanks for taking on this very tough review. It's a crying shame that Julian had also reviewed it, but the extra pair of eyeballs is still appreciated. Even if Julian knows more of the innards, it's also great to hear that someone less familiar with the intricacies of Soyuz and the archive publisher can make sense at least some of this.

Let's not do this too much though or you'll get sucked into Soyuz-related work like I did. :)

[Jeroen T. Vermeulen (jtv)]

> Thanks for doing this Jeroen, it looks pretty good so far. I've just got a
> few nits inline:

Ah, one of those reviews that actually make my code better. Good!

> > === modified file 'lib/lp/archivepublisher/domination.py'
> > --- lib/lp/archivepublisher/domination.py 2011-11-02 12:58:31 +0000
> > +++ lib/lp/archivepublisher/domination.py 2011-11-03 10:12:17 +0000

> > @@ -192,6 +197,104 @@
> > else:
> > return version_comparison
> >
> > + def sortPublications(self, publications):
> > + """Sort publications from most to least current versions."""
> > + # Listify; we want to iterate this twice, which won't do for a
> > + # non-persistent sequence.
> > + sorted_publications = list(publications)
>
> This variable is prematurely named, I think I'd just write:
> publications = list(publications)
> and return publications.sort(...) later.

Yup, you're right. Done.

(Apart from one nasty little subtlety: surprisingly, list.sort does not return the sorted list. I suppose that's to avoid obscuring the fact that it modifies the list in-place.)

> > +def get_binary_versions(binary_publications):
> > + """List versions for sequence of `BinaryPackagePublishingHistory`."""
> > + return [pub.binarypackagerelease.version for pub in
> binary_publications]
>
> If you're going to do :param: and :return: for the other functions, you
> should do it here too!

Hoist by my own pedantard. Done.

> > +def find_live_binary_versions_pass_1(publications):
> > + """Find versions out of Published `publications` that should stay live.
> > +
> > + This particular notion of liveness applies to first-pass binary
> > + domination: the latest version stays live, and so do publications of
> > + binary packages for the "all" architecture.
> > +
> > + :param publications: An iterable of `BinaryPackagePublishingHistory`,
> > + sorted by descending package version.
> > + :return: A list of live versions.
> > + """
> > + publications = list(publications)
> > + latest = publications.pop(0)
> > + return get_binary_versions(
> > + [latest] + [
> > + pub for pub in publications if not pub.architecture_specific])
>
> pub.architecture_specific masks a query, in case you didn't know. I
> absolutely detest properties that do that, it encourages SQL death-by-a-
> thousand-cuts.

I spotted it through sheer luck. It's just dereferencing a Storm reference though, and I took care to bulk-load the referenced BinaryPackageReleases.

> > +def find_live_binary_versions_pass_2(publications):
> > + """Find versions out of Published `publications` that should stay live.
> > +
> > + This particular notion of liveness applies to second-pass binary
> > + domination: the latest version stays live, and architecture-specific
> > + publications stay live (i.e, ones that are not for the "all"
> > + architecture).
> > +
> > + More importantly, any publication for binary packages of the "all"
> > + architecture stay live if any of the non-"all" binary packages from
> > + the same source package release are still active -- even if they are
> > + f...

[Julian Edwards (julian-edwards)]

On Friday 04 November 2011 03:58:24 you wrote:
> > Thanks for doing this Jeroen, it looks pretty good so far. I've just
> > got a
> > few nits inline:
> Ah, one of those reviews that actually make my code better. Good!

You asked for it!

> (Apart from one nasty little subtlety: surprisingly, list.sort does not
> return the sorted list. I suppose that's to avoid obscuring the fact that
> it modifies the list in-place.)

Right, that's annoying.

> That's what I keep thinking too. But the python documentation says
> otherwise. Perhaps it was some other massively useful function that got
> deprecated, or perhaps people changed their minds.
>
> Anyway, looking this up also made me find ifilter and ifilterfalse (in
> itertools). With those, I can finally say what I need here tersely and
> uniformly:
>
> arch_specific_pubs = list(ifilter(is_arch_specific, publications))
> arch_indep_pubs = list(ifilterfalse(is_arch_specific, publications))

I had not seen those before, very nice.

It would serve us some use to trawl through the Python built-ins at some
point!

> It's not very costly compared to inevitable work we're already doing: for a
> list of n publications, this does n-1 version comparisons. That's
> guaranteed to be no more than the sort operation that produced the
> publications list in the first place.
>
> The loop depends very heavily on the sort order that this verifies, and
> there are several call sites that do the sorting. It'd be nice if we could
> just re-sort to make sure of the right order, but besides being more
> expensive that also wouldn't help the liveness functions which also rely on
> sort order.
>
> Given how important it is to get this right and how easy it is to get it
> wrong, I think verifying the order is worth the small added cost.

I agree that it's important. I have an additional suggestion - rename the
method parameter so it's "sorted_publications" and also update the docstring
to say it *must* be sorted.

> Done. But, er... does this mean I'm condemned to another year of this? :-)

You might think so but I could not possibly comment.

Thanks!

=== modified file 'lib/lp/archivepublisher/domination.py'
--- lib/lp/archivepublisher/domination.py	2011-11-02 12:58:31 +0000
+++ lib/lp/archivepublisher/domination.py	2011-11-04 04:07:32 +0000
@@ -52,8 +52,16 @@
 
 __all__ = ['Dominator']
 
+from collections import defaultdict
 from datetime import timedelta
-from operator import itemgetter
+from itertools import (
+    ifilter,
+    ifilterfalse,
+    )
+from operator import (
+    attrgetter,
+    itemgetter,
+    )
 
 import apt_pkg
 from storm.expr import (
@@ -72,6 +80,7 @@
     DecoratedResultSet,
     )
 from canonical.launchpad.interfaces.lpstorm import IStore
+from canonical.launchpad.utilities.orderingcheck import OrderingCheck
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.services.database.bulk import load_related
 from lp.soyuz.enums import (
@@ -192,6 +201,107 @@
         else:
             return version_comparison
 
+    def sortPublications(self, publications):
+        """Sort publications from most to least current versions."""
+        # Listify; we want to iterate this twice, which won't do for a
+        # non-persistent sequence.
+        publications = list(publications)
+        # Batch-load associated package releases; we'll be needing them
+        # to compare versions.
+        self.load_releases(publications)
+        # Now sort.  This is that second iteration.  An in-place sort
+        # won't hurt the original, because we're working on a copy of
+        # the original iterable.
+        publications.sort(cmp=self.compare, reverse=True)
+        return publications
+
+
+def find_live_source_versions(publications):
+    """Find versions out of Published `publications` that should stay live.
+
+    This particular notion of liveness applies to source domination: the
+    latest version stays live, and that's it.
+
+    :param publications: An iterable of `SourcePackagePublishingHistory`
+        sorted by descending package version.
+    :return: A list of live versions.
+    """
+    # Given the required sort order, the latest version is at the head
+    # of the list.
+    return [publications[0].sourcepackagerelease.version]
+
+
+def get_binary_versions(binary_publications):
+    """List versions for sequence of `BinaryPackagePublishingHistory`.
+
+    :param binary_publications: An iterable of
+        `BinaryPackagePublishingHistory`.
+    :return: A list of the publications' respective versions.
+    """
+    return [pub.binarypackagerelease.version for pub in binary_publications]
+
+
+def find_live_binary_versions_pass_1(publications):
+    """Find versions out of Published `publications` that should stay live.
+
+    This particular notion of liveness applies to first-pass binary
+    domination: the latest version stays live, and so do publications of
+    binary packages for the "all" architecture.
+
+    :param publications: An iterable of `BinaryPackagePublishingHistory`,
+        sorted by descending package version.
+    :return: A list of live versions.
+    """
+    publications = list(publications)
+    latest = publications.pop(0)
+    return get_binary_versions(
+        [latest] + [
+            pub for pub in publications if not pub.architecture_specific])
+
+
+def find_live_binary_versions_pass_2(publications):
+    """Find versions out of Published `publications` that should stay live.
+
+    This particular notion of liveness applies to second-pass binary
+    domination: the latest version stays live, and architecture-specific
+    publications stay live (i.e, ones that are not for the "all"
+    architecture).
+
+    More importantly, any publication for binary packages of the "all"
+    architecture stay live if any of the non-"all" binary packages from
+    the same source package release are still active -- even if they are
+    for other architectures.
+
+    This is the raison d'etre for the two-pass binary domination algorithm:
+    to let us see which architecture-independent binary publications can be
+    superseded without rendering any architecture-specific binaries from the
+    same source package release uninstallable.
+
+    (Note that here, "active" includes Published publications but also
+    Pending ones.  This is standard nomenclature in Soyuz.  Some of the
+    domination code confuses matters by using the term "active" to mean only
+    Published publications).
+
+    :param publications: An iterable of `BinaryPackagePublishingHistory`,
+        sorted by descending package version.
+    :return: A list of live versions.
+    """
+    publications = list(publications)
+    latest = publications.pop(0)
+    is_arch_specific = attrgetter('architecture_specific')
+    arch_specific_pubs = list(ifilter(is_arch_specific, publications))
+    arch_indep_pubs = list(ifilterfalse(is_arch_specific, publications))
+
+    # XXX JeroenVermeulen 2011-11-01 bug=884649: This is likely to be
+    # costly, and the result could be reused for all builds of the same
+    # source package release to all architectures.
+    reprieved_pubs = [
+        pub
+        for pub in arch_indep_pubs
+            if pub.getOtherPublicationsForSameSource().any()]
+
+    return get_binary_versions([latest] + arch_specific_pubs + reprieved_pubs)
+
 
 class Dominator:
     """Manage the process of marking packages as superseded.
@@ -209,27 +319,6 @@
         self.logger = logger
         self.archive = archive
 
-    def _checkArchIndep(self, publication):
-        """Return True if the binary publication can be superseded.
-
-        If the publication is an arch-indep binary, we can only supersede
-        it if all the binaries from the same source are also superseded,
-        else those binaries may become uninstallable.
-        See bug 34086.
-        """
-        binary = publication.binarypackagerelease
-        if not binary.architecturespecific:
-            # getOtherPublicationsForSameSource returns PENDING in
-            # addition to PUBLISHED binaries, and we rely on this since
-            # they must also block domination.
-            others = publication.getOtherPublicationsForSameSource()
-            if others.any():
-                # Don't dominate this arch:all binary as there are
-                # other arch-specific binaries from the same build
-                # that are still active.
-                return False
-        return True
-
     def dominatePackage(self, publications, live_versions, generalization):
         """Dominate publications for a single package.
 
@@ -247,34 +336,33 @@
 
         :param publications: Iterable of publications for the same package,
             in the same archive, series, and pocket, all with status
-            `PackagePublishingStatus.PUBLISHED`.
-        :param live_versions: Iterable of version strings that are still
-            considered live for this package.  The given publications will
-            remain active insofar as they represent any of these versions;
-            older publications will be marked as superseded and newer ones
-            as deleted.
+            `PackagePublishingStatus.PUBLISHED`.  They must be sorted from
+            most current to least current, as would be the result of
+            `generalization.sortPublications`.
+        :param live_versions: Iterable of versions that are still considered
+            "live" for this package.  For any of these, the latest publication
+            among `publications` will remain Published.  Publications for
+            older releases, as well as older publications of live versions,
+            will be marked as Superseded.  Publications of newer versions than
+            are listed in `live_versions` are marked as Deleted.
         :param generalization: A `GeneralizedPublication` helper representing
-            the kind of publications these are--source or binary.
+            the kind of publications these are: source or binary.
         """
-        publications = list(publications)
-        generalization.load_releases(publications)
-
-        # Go through publications from latest version to oldest.  This
-        # makes it easy to figure out which release superseded which:
-        # the dominant is always the oldest live release that is newer
-        # than the one being superseded.  In this loop, that means the
-        # dominant is always the last live publication we saw.
-        publications = sorted(
-            publications, cmp=generalization.compare, reverse=True)
+        live_versions = frozenset(live_versions)
 
         self.logger.debug(
             "Package has %d live publication(s).  Live versions: %s",
             len(publications), live_versions)
 
+        # Verify that the publications are really sorted properly.
+        check_order = OrderingCheck(cmp=generalization.compare, reverse=True)
+
         current_dominant = None
         dominant_version = None
 
         for pub in publications:
+            check_order.check(pub)
+
             version = generalization.getPackageVersion(pub)
             # There should never be two published releases with the same
             # version.  So it doesn't matter whether this comparison is
@@ -295,11 +383,6 @@
                 current_dominant = pub
                 dominant_version = version
                 self.logger.debug2("Keeping version %s.", version)
-            elif not (generalization.is_source or self._checkArchIndep(pub)):
-                # As a special case, we keep this version live as well.
-                current_dominant = pub
-                dominant_version = version
-                self.logger.debug2("Keeping version %s.", version)
             elif current_dominant is None:
                 # This publication is no longer live, but there is no
                 # newer version to supersede it either.  Therefore it
@@ -312,50 +395,32 @@
                 pub.supersede(current_dominant, logger=self.logger)
                 self.logger.debug2("Superseding version %s.", version)
 
-    def _dominatePublications(self, pubs, generalization):
-        """Perform dominations for the given publications.
-
-        Keep the latest published version for each package active,
-        superseding older versions.
-
-        :param pubs: A dict mapping names to a list of publications. Every
-            publication must be PUBLISHED or PENDING, and the first in each
-            list will be treated as dominant (so should be the latest).
-        :param generalization: A `GeneralizedPublication` helper representing
-            the kind of publications these are--source or binary.
-        """
-        self.logger.debug("Dominating packages...")
-        for name, publications in pubs.iteritems():
-            assert publications, "Empty list of publications for %s." % name
-            # Since this always picks the latest version as the live
-            # one, this dominatePackage call will never result in a
-            # deletion.
-            latest_version = generalization.getPackageVersion(publications[0])
-            self.logger.debug2("Dominating %s" % name)
-            self.dominatePackage(
-                publications, [latest_version], generalization)
-
-    def _sortPackages(self, pkglist, generalization):
-        """Map out packages by name, and sort by descending version.
-
-        :param pkglist: An iterable of `SourcePackagePublishingHistory` or
-            `BinaryPackagePublishingHistory`.
-        :param generalization: A `GeneralizedPublication` helper representing
-            the kind of publications these are--source or binary.
-        :return: A dict mapping each package name to a list of publications
-            from `pkglist`, newest first.
-        """
-        self.logger.debug("Sorting packages...")
-
-        outpkgs = {}
-        for inpkg in pkglist:
-            key = generalization.getPackageName(inpkg)
-            outpkgs.setdefault(key, []).append(inpkg)
-
-        for package_pubs in outpkgs.itervalues():
-            package_pubs.sort(cmp=generalization.compare, reverse=True)
-
-        return outpkgs
+    def _sortPackages(self, publications, generalization):
+        """Partition publications by package name, and sort them.
+
+        The publications are sorted from most current to least current,
+        as required by `dominatePackage` etc.
+
+        :param publications: An iterable of `SourcePackagePublishingHistory`
+            or of `BinaryPackagePublishingHistory`.
+        :param generalization: A `GeneralizedPublication` helper representing
+            the kind of publications these are: source or binary.
+        :return: A dict mapping each package name to a sorted list of
+            publications from `publications`.
+        """
+        pubs_by_package = defaultdict(list)
+        for pub in publications:
+            pubs_by_package[generalization.getPackageName(pub)].append(pub)
+
+        # Sort the publication lists.  This is not an in-place sort, so
+        # it involves altering the dict while we iterate it.  Listify
+        # the keys so that we can be sure that we're not altering the
+        # iteration order while iteration is underway.
+        for package in list(pubs_by_package.keys()):
+            pubs_by_package[package] = generalization.sortPublications(
+                pubs_by_package[package])
+
+        return pubs_by_package
 
     def _setScheduledDeletionDate(self, pub_record):
         """Set the scheduleddeletiondate on a publishing record.
@@ -520,7 +585,11 @@
             bins = self.findBinariesForDomination(distroarchseries, pocket)
             sorted_packages = self._sortPackages(bins, generalization)
             self.logger.info("Dominating binaries...")
-            self._dominatePublications(sorted_packages, generalization)
+            for name, pubs in sorted_packages.iteritems():
+                self.logger.debug("Dominating %s" % name)
+                assert len(pubs) > 0, "Dominating zero binaries!"
+                live_versions = find_live_binary_versions_pass_1(pubs)
+                self.dominatePackage(pubs, live_versions, generalization)
 
         # We need to make a second pass to cover the cases where:
         #  * arch-specific binaries were not all dominated before arch-all
@@ -534,7 +603,11 @@
             bins = self.findBinariesForDomination(distroarchseries, pocket)
             sorted_packages = self._sortPackages(bins, generalization)
             self.logger.info("Dominating binaries...(2nd pass)")
-            self._dominatePublications(sorted_packages, generalization)
+            for name, pubs in sorted_packages.iteritems():
+                self.logger.debug("Dominating %s" % name)
+                assert len(pubs) > 0, "Dominating zero binaries in 2nd pass!"
+                live_versions = find_live_binary_versions_pass_2(pubs)
+                self.dominatePackage(pubs, live_versions, generalization)
 
     def _composeActiveSourcePubsCondition(self, distroseries, pocket):
         """Compose ORM condition for restricting relevant source pubs."""
@@ -550,7 +623,16 @@
             )
 
     def findSourcesForDomination(self, distroseries, pocket):
-        """Find binary publications that need dominating."""
+        """Find binary publications that need dominating.
+
+        This is only for traditional domination, where the latest published
+        publication is always kept published.  See `find_live_source_versions`
+        for this logic.
+
+        To optimize for that logic, `findSourcesForDomination` will ignore
+        publications that have no other publications competing for the same
+        binary package.  There'd be nothing to do for those cases.
+        """
         # Avoid circular imports.
         from lp.soyuz.model.publishing import SourcePackagePublishingHistory
 
@@ -589,11 +671,18 @@
             distroseries.name, pocket.title)
 
         generalization = GeneralizedPublication(is_source=True)
+
+        self.logger.debug("Finding sources...")
         sources = self.findSourcesForDomination(distroseries, pocket)
+        sorted_packages = self._sortPackages(sources, generalization)
 
         self.logger.debug("Dominating sources...")
-        self._dominatePublications(
-            self._sortPackages(sources, generalization), generalization)
+        for name, pubs in sorted_packages.iteritems():
+            self.logger.debug("Dominating %s" % name)
+            assert len(pubs) > 0, "Dominating zero sources!"
+            live_versions = find_live_source_versions(pubs)
+            self.dominatePackage(pubs, live_versions, generalization)
+
         flush_database_updates()
 
     def findPublishedSourcePackageNames(self, distroseries, pocket):
@@ -653,6 +742,7 @@
         """
         generalization = GeneralizedPublication(is_source=True)
         pubs = self.findPublishedSPPHs(distroseries, pocket, package_name)
+        pubs = generalization.sortPublications(pubs)
         self.dominatePackage(pubs, live_versions, generalization)
 
     def judge(self, distroseries, pocket):
 
=== modified file 'lib/lp/archivepublisher/tests/test_dominator.py'
--- lib/lp/archivepublisher/tests/test_dominator.py	2011-11-02 10:28:31 +0000
+++ lib/lp/archivepublisher/tests/test_dominator.py	2011-11-04 04:07:32 +0000
@@ -16,6 +16,9 @@
 from canonical.testing.layers import ZopelessDatabaseLayer
 from lp.archivepublisher.domination import (
     Dominator,
+    find_live_binary_versions_pass_1,
+    find_live_binary_versions_pass_2,
+    find_live_source_versions,
     GeneralizedPublication,
     STAY_OF_EXECUTION,
     )
@@ -30,6 +33,7 @@
     StormStatementRecorder,
     TestCaseWithFactory,
     )
+from lp.testing.fakemethod import FakeMethod
 from lp.testing.matchers import HasQueryCount
 
 
@@ -72,13 +76,9 @@
             is_source=ISourcePackagePublishingHistory.providedBy(dominant))
         dominator = Dominator(self.logger, self.ubuntutest.main_archive)
 
-        # The _dominate* test methods require a dictionary where the
-        # package name is the key. The key's value is a list of
-        # source or binary packages representing dominant, the first element
-        # and dominated, the subsequents.
-        pubs = {'foo': [dominant, dominated]}
-
-        dominator._dominatePublications(pubs, generalization)
+        pubs = [dominant, dominated]
+        live_versions = [generalization.getPackageVersion(dominant)]
+        dominator.dominatePackage(pubs, live_versions, generalization)
         flush_database_updates()
 
         # The dominant version remains correctly published.
@@ -158,16 +158,30 @@
             [foo_10_source] + foo_10_binaries,
             PackagePublishingStatus.SUPERSEDED)
 
-    def testEmptyDomination(self):
-        """Domination asserts for not empty input list."""
-        dominator = Dominator(self.logger, self.ubuntutest.main_archive)
-        pubs = {'foo': []}
-        # This isn't a really good exception. It should probably be
-        # something more indicative of bad input.
-        self.assertRaises(
-            AssertionError,
-            dominator._dominatePublications,
-            pubs, GeneralizedPublication(True))
+    def test_dominateBinaries_rejects_empty_publication_list(self):
+        """Domination asserts for non-empty input list."""
+        package = self.factory.makeBinaryPackageName()
+        dominator = Dominator(self.logger, self.ubuntutest.main_archive)
+        dominator._sortPackages = FakeMethod({package.name: []})
+        # This isn't a really good exception. It should probably be
+        # something more indicative of bad input.
+        self.assertRaises(
+            AssertionError,
+            dominator.dominateBinaries,
+            self.factory.makeDistroArchSeries().distroseries,
+            self.factory.getAnyPocket())
+
+    def test_dominateSources_rejects_empty_publication_list(self):
+        """Domination asserts for non-empty input list."""
+        package = self.factory.makeSourcePackageName()
+        dominator = Dominator(self.logger, self.ubuntutest.main_archive)
+        dominator._sortPackages = FakeMethod({package.name: []})
+        # This isn't a really good exception. It should probably be
+        # something more indicative of bad input.
+        self.assertRaises(
+            AssertionError,
+            dominator.dominateSources,
+            self.factory.makeDistroSeries(), self.factory.getAnyPocket())
 
     def test_archall_domination(self):
         # Arch-all binaries should not be dominated when a new source
@@ -358,6 +372,16 @@
             SeriesStatus.OBSOLETE)
 
 
+def remove_security_proxies(proxied_objects):
+    """Return list of `proxied_objects`, without their proxies.
+
+    The dominator runs only in scripts, where security proxies don't get
+    in the way.  To test realistically for this environment, strip the
+    proxies wherever necessary and do as you will.
+    """
+    return [removeSecurityProxy(obj) for obj in proxied_objects]
+
+
 def make_spphs_for_versions(factory, versions):
     """Create publication records for each of `versions`.
 
@@ -400,14 +424,15 @@
     archive = das.distroseries.main_archive
     pocket = factory.getAnyPocket()
     bprs = [
-        factory.makeBinaryPackageRelease(binarypackagename=bpn)
+        factory.makeBinaryPackageRelease(
+            binarypackagename=bpn, version=version)
         for version in versions]
-    return [
+    return remove_security_proxies([
         factory.makeBinaryPackagePublishingHistory(
             binarypackagerelease=bpr, binarypackagename=bpn,
             distroarchseries=das, pocket=pocket, archive=archive,
             sourcepackagename=spn, status=PackagePublishingStatus.PUBLISHED)
-        for bpr in bprs]
+        for bpr in bprs])
 
 
 def list_source_versions(spphs):
@@ -591,9 +616,10 @@
     def test_dominatePackage_supersedes_older_pub_with_newer_live_pub(self):
         # When marking a package as superseded, dominatePackage
         # designates a newer live version as the superseding version.
+        generalization = GeneralizedPublication(True)
         pubs = make_spphs_for_versions(self.factory, ['1.0', '1.1'])
         self.makeDominator(pubs).dominatePackage(
-            pubs, ['1.1'], GeneralizedPublication(True))
+            generalization.sortPublications(pubs), ['1.1'], generalization)
         self.assertEqual(PackagePublishingStatus.SUPERSEDED, pubs[0].status)
         self.assertEqual(pubs[1].sourcepackagerelease, pubs[0].supersededby)
         self.assertEqual(PackagePublishingStatus.PUBLISHED, pubs[1].status)
@@ -601,10 +627,11 @@
     def test_dominatePackage_only_supersedes_with_live_pub(self):
         # When marking a package as superseded, dominatePackage will
         # only pick a live version as the superseding one.
+        generalization = GeneralizedPublication(True)
         pubs = make_spphs_for_versions(
             self.factory, ['1.0', '2.0', '3.0', '4.0'])
         self.makeDominator(pubs).dominatePackage(
-            pubs, ['3.0'], GeneralizedPublication(True))
+            generalization.sortPublications(pubs), ['3.0'], generalization)
         self.assertEqual([
                 pubs[2].sourcepackagerelease,
                 pubs[2].sourcepackagerelease,
@@ -616,23 +643,27 @@
     def test_dominatePackage_supersedes_with_oldest_newer_live_pub(self):
         # When marking a package as superseded, dominatePackage picks
         # the oldest of the newer, live versions as the superseding one.
+        generalization = GeneralizedPublication(True)
         pubs = make_spphs_for_versions(self.factory, ['2.7', '2.8', '2.9'])
         self.makeDominator(pubs).dominatePackage(
-            pubs, ['2.8', '2.9'], GeneralizedPublication(True))
+            generalization.sortPublications(pubs), ['2.8', '2.9'],
+            generalization)
         self.assertEqual(pubs[1].sourcepackagerelease, pubs[0].supersededby)
 
     def test_dominatePackage_only_supersedes_with_newer_live_pub(self):
         # When marking a package as superseded, dominatePackage only
         # considers a newer version as the superseding one.
+        generalization = GeneralizedPublication(True)
         pubs = make_spphs_for_versions(self.factory, ['0.1', '0.2'])
         self.makeDominator(pubs).dominatePackage(
-            pubs, ['0.1'], GeneralizedPublication(True))
+            generalization.sortPublications(pubs), ['0.1'], generalization)
         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.
+        generalization = GeneralizedPublication(True)
         spr = self.factory.makeSourcePackageRelease()
         series = self.factory.makeDistroSeries()
         pocket = PackagePublishingPocket.RELEASE
@@ -649,7 +680,8 @@
             ])
 
         self.makeDominator(pubs).dominatePackage(
-            pubs, [spr.version], GeneralizedPublication(True))
+            generalization.sortPublications(pubs), [spr.version],
+            generalization)
         self.assertEqual([
             PackagePublishingStatus.SUPERSEDED,
             PackagePublishingStatus.SUPERSEDED,
@@ -661,12 +693,13 @@
 
     def test_dominatePackage_is_efficient(self):
         # dominatePackage avoids issuing too many queries.
+        generalization = GeneralizedPublication(True)
         versions = ["1.%s" % revision for revision in xrange(5)]
         pubs = make_spphs_for_versions(self.factory, versions)
         with StormStatementRecorder() as recorder:
             self.makeDominator(pubs).dominatePackage(
-                pubs, versions[2:-1],
-                GeneralizedPublication(True))
+                generalization.sortPublications(pubs), versions[2:-1],
+                generalization)
         self.assertThat(recorder, HasQueryCount(LessThan(5)))
 
     def test_dominatePackage_advanced_scenario(self):
@@ -677,6 +710,7 @@
         # 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.
+        generalization = GeneralizedPublication(True)
         series = self.factory.makeDistroSeries()
         package = self.factory.makeSourcePackageName()
         pocket = PackagePublishingPocket.RELEASE
@@ -723,7 +757,8 @@
 
         all_pubs = sum(pubs_by_version.itervalues(), [])
         Dominator(DevNullLogger(), series.main_archive).dominatePackage(
-            all_pubs, live_versions, GeneralizedPublication(True))
+            generalization.sortPublications(all_pubs), live_versions,
+            generalization)
 
         for version in reversed(versions):
             pubs = pubs_by_version[version]
@@ -1089,3 +1124,82 @@
             published_spphs,
             dominator.findSourcesForDomination(
                 spphs[0].distroseries, spphs[0].pocket))
+
+
+def make_publications_arch_specific(pubs, arch_specific=True):
+    """Set the `architecturespecific` attribute for given SPPHs.
+
+    :param pubs: An iterable of `BinaryPackagePublishingHistory`.
+    :param arch_specific: Whether the binary package releases published
+        by `pubs` are to be architecture-specific.  If not, they will be
+        treated as being for the "all" architecture.
+    """
+    for pub in pubs:
+        bpr = removeSecurityProxy(pub).binarypackagerelease
+        bpr.architecturespecific = arch_specific
+
+
+class TestLivenessFunctions(TestCaseWithFactory):
+    """Tests for the functions that say which versions are live."""
+
+    layer = ZopelessDatabaseLayer
+
+    def test_find_live_source_versions_blesses_latest(self):
+        # find_live_source_versions, assuming that you passed it
+        # publications sorted from most current to least current
+        # version, simply returns the most current version.
+        spphs = make_spphs_for_versions(self.factory, ['1.2', '1.1', '1.0'])
+        self.assertEqual(['1.2'], find_live_source_versions(spphs))
+
+    def test_find_live_binary_versions_pass_1_blesses_latest(self):
+        # find_live_binary_versions_pass_1 always includes the latest
+        # version among the input publications in its result.
+        bpphs = make_bpphs_for_versions(self.factory, ['1.2', '1.1', '1.0'])
+        make_publications_arch_specific(bpphs)
+        self.assertEqual(['1.2'], find_live_binary_versions_pass_1(bpphs))
+
+    def test_find_live_binary_versions_pass_1_blesses_arch_all(self):
+        # find_live_binary_versions_pass_1 includes any
+        # architecture-independent publications among the input in its
+        # result.
+        versions = list(reversed(['1.%d' % version for version in range(3)]))
+        bpphs = make_bpphs_for_versions(self.factory, versions)
+
+        # All of these publications are architecture-specific, except
+        # the last one.  This would happen if the binary package had
+        # just changed from being architecture-specific to being
+        # architecture-independent.
+        make_publications_arch_specific(bpphs, True)
+        make_publications_arch_specific(bpphs[-1:], False)
+        self.assertEqual(
+            versions[:1] + versions[-1:],
+            find_live_binary_versions_pass_1(bpphs))
+
+    def test_find_live_binary_versions_pass_2_blesses_latest(self):
+        # find_live_binary_versions_pass_2 always includes the latest
+        # version among the input publications in its result.
+        bpphs = make_bpphs_for_versions(self.factory, ['1.2', '1.1', '1.0'])
+        make_publications_arch_specific(bpphs, False)
+        self.assertEqual(['1.2'], find_live_binary_versions_pass_2(bpphs))
+
+    def test_find_live_binary_versions_pass_2_blesses_arch_specific(self):
+        # find_live_binary_versions_pass_2 includes any
+        # architecture-specific publications among the input in its
+        # result.
+        versions = list(reversed(['1.%d' % version for version in range(3)]))
+        bpphs = make_bpphs_for_versions(self.factory, versions)
+        make_publications_arch_specific(bpphs)
+        self.assertEqual(versions, find_live_binary_versions_pass_2(bpphs))
+
+    def test_find_live_binary_versions_pass_2_reprieves_arch_all(self):
+        # An arch-all BPPH for a BPR built by an SPR that also still has
+        # active arch-dependent BPPHs gets a reprieve: it can't be
+        # superseded until those arch-dependent BPPHs have been
+        # superseded.
+        bpphs = make_bpphs_for_versions(self.factory, ['1.2', '1.1', '1.0'])
+        make_publications_arch_specific(bpphs, False)
+        dependent = self.factory.makeBinaryPackagePublishingHistory(
+            binarypackagerelease=bpphs[1].binarypackagerelease)
+        make_publications_arch_specific([dependent], True)
+        self.assertEqual(
+            ['1.2', '1.1'], find_live_binary_versions_pass_2(bpphs))