Merge lp:~julian-edwards/launchpad/arch-all-domination-bug-34086 into lp:launchpad

[Julian Edwards]

[r=wgrant][bug=34086] Prevent superseding of arch-indep binaries until all binaries from the same source are all also superseded.

Fix problems discovered in the previous branch that was backed out where arch-indep and arch-specific binaries flip.

Since the last branch is being re-applied the diff is large. The actual changes are here: http://pastebin.ubuntu.com/711881/

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/archivepublisher/domination.py'
--- lib/lp/archivepublisher/domination.py	2011-10-18 02:09:38 +0000
+++ lib/lp/archivepublisher/domination.py	2011-10-18 12:00:30 +0000
@@ -153,6 +153,7 @@
     class.
     """
     def __init__(self, is_source=True):
+        self.is_source = is_source
         if is_source:
             self.traits = SourcePublicationTraits
         else:
@@ -204,6 +205,27 @@
         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.
 
@@ -260,7 +282,9 @@
                 pub.supersede(current_dominant, logger=self.logger)
                 self.logger.debug2(
                     "Superseding older publication for version %s.", version)
-            elif version in live_versions:
+            elif (version in live_versions or
+                  (not generalization.is_source and
+                   not self._checkArchIndep(pub))):
                 # 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.
@@ -298,6 +322,7 @@
             # 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)
 
@@ -359,27 +384,6 @@
 
         self.logger.debug("Beginning superseded processing...")
 
-        # XXX: dsilvers 2005-09-22 bug=55030:
-        # Need to make binaries go in groups but for now this'll do.
-        # An example of the concrete problem here is:
-        # - Upload foo-1.0, which builds foo and foo-common (arch all).
-        # - Upload foo-1.1, ditto.
-        # - foo-common-1.1 is built (along with the i386 binary for foo)
-        # - foo-common-1.0 is superseded
-        # Foo is now uninstallable on any architectures which don't yet
-        # have a build of foo-1.1, as the foo-common for foo-1.0 is gone.
-
-        # Essentially we ideally don't want to lose superseded binaries
-        # unless the entire group is ready to be made pending removal.
-        # In this instance a group is defined as all the binaries from a
-        # given build. This assumes we've copied the arch_all binaries
-        # from whichever build provided them into each arch-specific build
-        # which we publish. If instead we simply publish the arch-all
-        # binaries from another build then instead we should scan up from
-        # the binary to its source, and then back from the source to each
-        # binary published in *this* distroarchseries for that source.
-        # if the binaries as a group (in that definition) are all superseded
-        # then we can consider them eligible for removal.
         for pub_record in binary_records:
             binpkg_release = pub_record.binarypackagerelease
             self.logger.debug(
@@ -450,19 +454,19 @@
         generalization = GeneralizedPublication(is_source=False)
 
         for distroarchseries in distroseries.architectures:
-            self.logger.debug(
+            self.logger.info(
                 "Performing domination across %s/%s (%s)",
                 distroseries.name, pocket.title,
                 distroarchseries.architecturetag)
 
-            bpph_location_clauses = And(
+            bpph_location_clauses = [
                 BinaryPackagePublishingHistory.status ==
                     PackagePublishingStatus.PUBLISHED,
                 BinaryPackagePublishingHistory.distroarchseries ==
                     distroarchseries,
                 BinaryPackagePublishingHistory.archive == self.archive,
                 BinaryPackagePublishingHistory.pocket == pocket,
-                )
+                ]
             candidate_binary_names = Select(
                 BinaryPackageName.id,
                 And(
@@ -474,7 +478,7 @@
                 ),
                 group_by=BinaryPackageName.id,
                 having=Count(BinaryPackagePublishingHistory.id) > 1)
-            binaries = IStore(BinaryPackagePublishingHistory).find(
+            main_clauses = [
                 BinaryPackagePublishingHistory,
                 BinaryPackageRelease.id ==
                     BinaryPackagePublishingHistory.binarypackagereleaseID,
@@ -482,10 +486,28 @@
                     candidate_binary_names),
                 BinaryPackageRelease.binpackageformat !=
                     BinaryPackageFormat.DDEB,
-                bpph_location_clauses)
-            self.logger.debug("Dominating binaries...")
-            self._dominatePublications(
-                self._sortPackages(binaries, generalization), generalization)
+                ]
+            main_clauses.extend(bpph_location_clauses)
+
+            def do_domination(pass2_msg=""):
+                msg = "binaries..." + pass2_msg
+                self.logger.info("Finding %s" % msg)
+                bins = IStore(
+                    BinaryPackagePublishingHistory).find(*main_clauses)
+                self.logger.info("Dominating %s" % msg)
+                sorted_packages = self._sortPackages(bins, generalization)
+                self._dominatePublications(sorted_packages, generalization)
+
+            do_domination()
+
+            # We need to make a second pass to cover the cases where:
+            #  * arch-specific binaries were not all dominated before arch-all
+            #    ones that depend on them
+            #  * An arch-all turned into an arch-specific, or vice-versa
+            #  * A package is completely schizophrenic and changes all of
+            #    its binaries between arch-all and arch-any (apparently
+            #    occurs sometimes!)
+            do_domination("(2nd pass)")
 
     def _composeActiveSourcePubsCondition(self, distroseries, pocket):
         """Compose ORM condition for restricting relevant source pubs."""

=== modified file 'lib/lp/archivepublisher/tests/test_dominator.py'
--- lib/lp/archivepublisher/tests/test_dominator.py	2011-10-18 02:20:28 +0000
+++ lib/lp/archivepublisher/tests/test_dominator.py	2011-10-18 12:00:30 +0000
@@ -23,7 +23,9 @@
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.log.logger import DevNullLogger
-from lp.soyuz.enums import PackagePublishingStatus
+from lp.soyuz.enums import (
+    PackagePublishingStatus,
+    )
 from lp.soyuz.interfaces.publishing import ISourcePackagePublishingHistory
 from lp.soyuz.tests.test_publishing import TestNativePublishingBase
 from lp.testing import (
@@ -169,6 +171,136 @@
             dominator._dominatePublications,
             pubs, GeneralizedPublication(True))
 
+    def test_archall_domination(self):
+        # Arch-all binaries should not be dominated when a new source
+        # version builds an updated arch-all binary, because slower builds
+        # of other architectures will leave the previous version
+        # uninstallable if they depend on the arch-all binary.
+        # See https://bugs.launchpad.net/launchpad/+bug/34086
+
+        # Set up a source, "foo" which builds "foo-bin" and foo-common
+        # (which is arch-all).
+        foo_10_src = self.getPubSource(
+            sourcename="foo", version="1.0", architecturehintlist="i386",
+            status=PackagePublishingStatus.PUBLISHED)
+        [foo_10_i386_bin] = self.getPubBinaries(
+            binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
+            architecturespecific=True, version="1.0", pub_source=foo_10_src)
+        [build] = foo_10_src.getBuilds()
+        bpr = self.factory.makeBinaryPackageRelease(
+            binarypackagename="foo-common", version="1.0", build=build,
+            architecturespecific=False)
+        foo_10_all_bins = self.publishBinaryInArchive(
+            bpr, self.ubuntutest.main_archive, pocket=foo_10_src.pocket,
+            status=PackagePublishingStatus.PUBLISHED)
+
+        # Now, make version 1.1 of foo and add a foo-common but not foo-bin
+        # (imagine that it's not finished building yet).
+        foo_11_src = self.getPubSource(
+            sourcename="foo", version="1.1", architecturehintlist="all",
+            status=PackagePublishingStatus.PUBLISHED)
+        foo_11_all_bins = self.getPubBinaries(
+            binaryname="foo-common", status=PackagePublishingStatus.PUBLISHED,
+            architecturespecific=False, version="1.1", pub_source=foo_11_src)
+
+        dominator = Dominator(self.logger, self.ubuntutest.main_archive)
+        dominator.judgeAndDominate(
+            foo_10_src.distroseries, foo_10_src.pocket)
+
+        # The source will be superseded.
+        self.checkPublication(foo_10_src, PackagePublishingStatus.SUPERSEDED)
+        # The arch-specific has no dominant, so it's still published
+        self.checkPublication(
+            foo_10_i386_bin, PackagePublishingStatus.PUBLISHED)
+        # The arch-indep has a dominant but must not be superseded yet
+        # since the arch-specific is still published.
+        self.checkPublications(
+            foo_10_all_bins, PackagePublishingStatus.PUBLISHED)
+
+        # Now creating a newer foo-bin should see those last two
+        # publications superseded.
+        [build2] = foo_11_src.getBuilds()
+        foo_11_bin = self.factory.makeBinaryPackageRelease(
+            binarypackagename="foo-bin", version="1.1", build=build2,
+            architecturespecific=True)
+        self.publishBinaryInArchive(
+            foo_11_bin, self.ubuntutest.main_archive,
+            pocket=foo_10_src.pocket,
+            status=PackagePublishingStatus.PUBLISHED)
+        dominator.judgeAndDominate(
+            foo_10_src.distroseries, foo_10_src.pocket)
+        self.checkPublication(
+            foo_10_i386_bin, PackagePublishingStatus.SUPERSEDED)
+        self.checkPublications(
+            foo_10_all_bins, PackagePublishingStatus.SUPERSEDED)
+
+    def test_any_superseded_by_all(self):
+        # Set up a source, foo, which builds an architecture-dependent
+        # binary, foo-bin.
+        foo_10_src = self.getPubSource(
+            sourcename="foo", version="1.0", architecturehintlist="i386",
+            status=PackagePublishingStatus.PUBLISHED)
+        [foo_10_i386_bin] = self.getPubBinaries(
+            binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
+            architecturespecific=True, version="1.0", pub_source=foo_10_src)
+
+        # Now, make version 1.1 of foo, where foo-bin is now
+        # architecture-independent.
+        foo_11_src = self.getPubSource(
+            sourcename="foo", version="1.1", architecturehintlist="all",
+            status=PackagePublishingStatus.PUBLISHED)
+        [foo_10_all_bin, foo_10_all_bin_2] = self.getPubBinaries(
+            binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
+            architecturespecific=False, version="1.1", pub_source=foo_11_src)
+
+        dominator = Dominator(self.logger, self.ubuntutest.main_archive)
+        dominator.judgeAndDominate(
+            foo_10_src.distroseries, foo_10_src.pocket)
+
+        # The source will be superseded.
+        self.checkPublication(foo_10_src, PackagePublishingStatus.SUPERSEDED)
+        # The arch-specific is superseded by the new arch-indep.
+        self.checkPublication(
+            foo_10_i386_bin, PackagePublishingStatus.SUPERSEDED)
+
+    def test_schitzoid_package(self):
+        # Test domination of a source that produces an arch-indep and an
+        # arch-all, that then switches both on the next version to the
+        # other arch type.
+        foo_10_src = self.getPubSource(
+            sourcename="foo", version="1.0", architecturehintlist="i386",
+            status=PackagePublishingStatus.PUBLISHED)
+        [foo_10_i386_bin] = self.getPubBinaries(
+            binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
+            architecturespecific=True, version="1.0", pub_source=foo_10_src)
+        [build] = foo_10_src.getBuilds()
+        bpr = self.factory.makeBinaryPackageRelease(
+            binarypackagename="foo-common", version="1.0", build=build,
+            architecturespecific=False)
+        foo_10_all_bins = self.publishBinaryInArchive(
+            bpr, self.ubuntutest.main_archive, pocket=foo_10_src.pocket,
+            status=PackagePublishingStatus.PUBLISHED)
+
+        foo_11_src = self.getPubSource(
+            sourcename="foo", version="1.1", architecturehintlist="i386",
+            status=PackagePublishingStatus.PUBLISHED)
+        [foo_11_i386_bin] = self.getPubBinaries(
+            binaryname="foo-common", status=PackagePublishingStatus.PUBLISHED,
+            architecturespecific=True, version="1.1", pub_source=foo_11_src)
+        [build] = foo_11_src.getBuilds()
+        bpr = self.factory.makeBinaryPackageRelease(
+            binarypackagename="foo-bin", version="1.1", build=build,
+            architecturespecific=False)
+        foo_11_all_bins = self.publishBinaryInArchive(
+            bpr, self.ubuntutest.main_archive, pocket=foo_11_src.pocket,
+            status=PackagePublishingStatus.PUBLISHED)
+
+        dominator = Dominator(self.logger, self.ubuntutest.main_archive)
+        dominator.judgeAndDominate(foo_10_src.distroseries, foo_10_src.pocket)
+
+        self.checkPublications(foo_10_all_bins + [foo_10_i386_bin],
+                               PackagePublishingStatus.SUPERSEDED)
+
 
 class TestDomination(TestNativePublishingBase):
     """Test overall domination procedure."""
@@ -223,35 +355,6 @@
         self.ubuntutest['breezy-autotest'].status = (
             SeriesStatus.OBSOLETE)
 
-    def test_any_superseded_by_all(self):
-        # Set up a source, foo, which builds an architecture-dependent
-        # binary, foo-bin.
-        foo_10_src = self.getPubSource(
-            sourcename="foo", version="1.0", architecturehintlist="i386",
-            status=PackagePublishingStatus.PUBLISHED)
-        [foo_10_i386_bin] = self.getPubBinaries(
-            binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
-            architecturespecific=True, version="1.0", pub_source=foo_10_src)
-
-        # Now, make version 1.1 of foo, where foo-bin is now
-        # architecture-independent.
-        foo_11_src = self.getPubSource(
-            sourcename="foo", version="1.1", architecturehintlist="all",
-            status=PackagePublishingStatus.PUBLISHED)
-        [foo_10_all_bin, foo_10_all_bin_2] = self.getPubBinaries(
-            binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
-            architecturespecific=False, version="1.1", pub_source=foo_11_src)
-
-        dominator = Dominator(self.logger, self.ubuntutest.main_archive)
-        dominator.judgeAndDominate(
-            foo_10_src.distroseries, foo_10_src.pocket)
-
-        # The source will be superseded.
-        self.checkPublication(foo_10_src, PackagePublishingStatus.SUPERSEDED)
-        # The arch-specific is superseded by the new arch-indep.
-        self.checkPublication(
-            foo_10_i386_bin, PackagePublishingStatus.SUPERSEDED)
-
 
 def make_spphs_for_versions(factory, versions):
     """Create publication records for each of `versions`.

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2011-10-18 02:09:38 +0000
+++ lib/lp/soyuz/model/publishing.py	2011-10-18 12:00:30 +0000
@@ -33,6 +33,7 @@
     Desc,
     LeftJoin,
     Or,
+    Select,
     Sum,
     )
 from storm.store import Store
@@ -1118,6 +1119,60 @@
                 section=self.section,
                 priority=self.priority)
 
+    def getOtherPublicationsForSameSource(self, include_archindep=False):
+        """Return all the other published or pending binaries for this
+        source.
+
+        For example if source package foo builds:
+        foo - i386
+        foo - amd64
+        foo-common - arch-all (published in i386 and amd64)
+        then if this publication is the arch-all amd64, return foo(i386),
+        foo(amd64). If include_archindep is True then also return
+        foo-common (i386)
+
+        :param include_archindep: If True, return architecture independent
+            publications too. Defaults to False.
+
+        :return: an iterable of `BinaryPackagePublishingHistory`
+        """
+        # Avoid circular wotsits.
+        from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
+        from lp.soyuz.model.distroarchseries import DistroArchSeries
+        from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
+        source_select = Select(
+            SourcePackageRelease.id,
+            And(
+                BinaryPackageBuild.source_package_release_id ==
+                    SourcePackageRelease.id,
+                BinaryPackageRelease.build == BinaryPackageBuild.id,
+                self.binarypackagereleaseID == BinaryPackageRelease.id,
+            ))
+        pubs = [
+            BinaryPackageBuild.source_package_release_id ==
+                SourcePackageRelease.id,
+            SourcePackageRelease.id.is_in(source_select),
+            BinaryPackageRelease.build == BinaryPackageBuild.id,
+            BinaryPackagePublishingHistory.binarypackagereleaseID ==
+                BinaryPackageRelease.id,
+            BinaryPackagePublishingHistory.archiveID == self.archive.id,
+            BinaryPackagePublishingHistory.distroarchseriesID ==
+                DistroArchSeries.id,
+            DistroArchSeries.distroseriesID == self.distroseries.id,
+            BinaryPackagePublishingHistory.pocket == self.pocket,
+            BinaryPackagePublishingHistory.status.is_in(
+                active_publishing_status),
+            BinaryPackagePublishingHistory.id != self.id
+            ]
+
+        if not include_archindep:
+            pubs.append(BinaryPackageRelease.architecturespecific == True)
+
+        return IMasterStore(BinaryPackagePublishingHistory).find(
+            BinaryPackagePublishingHistory,
+            *pubs
+            )
+
     def supersede(self, dominant=None, logger=None):
         """See `IBinaryPackagePublishingHistory`."""
         # At this point only PUBLISHED (ancient versions) or PENDING (

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2011-10-18 02:09:38 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2011-10-18 12:00:30 +0000
@@ -1471,6 +1471,112 @@
         self.assertEquals(spph.ancestor.displayname, ancestor.displayname)
 
 
+class TestGetOtherPublicationsForSameSource(TestNativePublishingBase):
+    """Test parts of the BinaryPackagePublishingHistory model.
+
+    See also lib/lp/soyuz/doc/publishing.txt
+    """
+
+    layer = LaunchpadZopelessLayer
+
+    def _makeMixedSingleBuildPackage(self, version="1.0"):
+        # Set up a source with a build that generated four binaries,
+        # two of them an arch-all.
+        foo_src_pub = self.getPubSource(
+            sourcename="foo", version=version, architecturehintlist="i386",
+            status=PackagePublishingStatus.PUBLISHED)
+        [foo_bin_pub] = self.getPubBinaries(
+            binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
+            architecturespecific=True, version=version,
+            pub_source=foo_src_pub)
+        # Now need to grab the build for the source so we can add
+        # more binaries to it.
+        [build] = foo_src_pub.getBuilds()
+        foo_one_common = self.factory.makeBinaryPackageRelease(
+            binarypackagename="foo-one-common", version=version, build=build,
+            architecturespecific=False)
+        foo_one_common_pubs = self.publishBinaryInArchive(
+            foo_one_common, self.ubuntutest.main_archive,
+            pocket=foo_src_pub.pocket,
+            status=PackagePublishingStatus.PUBLISHED)
+        foo_two_common = self.factory.makeBinaryPackageRelease(
+            binarypackagename="foo-two-common", version=version, build=build,
+            architecturespecific=False)
+        foo_two_common_pubs = self.publishBinaryInArchive(
+            foo_two_common, self.ubuntutest.main_archive,
+            pocket=foo_src_pub.pocket,
+            status=PackagePublishingStatus.PUBLISHED)
+        foo_three = self.factory.makeBinaryPackageRelease(
+            binarypackagename="foo-three", version=version, build=build,
+            architecturespecific=True)
+        [foo_three_pub] = self.publishBinaryInArchive(
+            foo_three, self.ubuntutest.main_archive,
+            pocket=foo_src_pub.pocket,
+            status=PackagePublishingStatus.PUBLISHED)
+        # So now we have source foo, which has arch specific binaries
+        # foo-bin and foo-three, and arch:all binaries foo-one-common and
+        # foo-two-common. The latter two will have multiple publications,
+        # one for each DAS in the series.
+        return (
+            foo_src_pub, foo_bin_pub, foo_one_common_pubs,
+            foo_two_common_pubs, foo_three_pub)
+
+    def test_getOtherPublicationsForSameSource(self):
+        # By default getOtherPublicationsForSameSource should return all
+        # of the other binaries built by the same source as the passed
+        # binary publication, except the arch-indep ones.
+        (foo_src_pub, foo_bin_pub, foo_one_common_pubs, foo_two_common_pubs,
+            foo_three_pub) = self._makeMixedSingleBuildPackage()
+
+        foo_one_common_pub = foo_one_common_pubs[0]
+        others = foo_one_common_pub.getOtherPublicationsForSameSource()
+        others = list(others)
+
+        self.assertContentEqual([foo_three_pub, foo_bin_pub], others)
+
+    def test_getOtherPublicationsForSameSource_include_archindep(self):
+        # Check that the arch-indep binaries are returned if requested.
+        (foo_src_pub, foo_bin_pub, foo_one_common_pubs, foo_two_common_pubs,
+         foo_three_pub) = self._makeMixedSingleBuildPackage()
+
+        foo_one_common_pub = foo_one_common_pubs[0]
+        others = foo_one_common_pub.getOtherPublicationsForSameSource(
+            include_archindep=True)
+        others = list(others)
+
+        # We expect all publications created above to be returned,
+        # except the one we use to call the method on.
+        expected = [foo_three_pub, foo_bin_pub]
+        expected.extend(foo_one_common_pubs[1:])
+        expected.extend(foo_two_common_pubs)
+        self.assertContentEqual(expected, others)
+
+    def test_getOtherPublicationsForSameSource_inactive(self):
+        # Check that inactive publications are not returned.
+        (foo_src_pub, foo_bin_pub, foo_one_common_pubs, foo_two_common_pubs,
+             foo_three_pub) = self._makeMixedSingleBuildPackage()
+        foo_bin_pub.status = PackagePublishingStatus.SUPERSEDED
+        foo_three_pub.status = PackagePublishingStatus.SUPERSEDED
+        foo_one_common_pub = foo_one_common_pubs[0]
+        others = foo_one_common_pub.getOtherPublicationsForSameSource()
+        others = list(others)
+
+        self.assertEqual(0, len(others))
+
+    def test_getOtherPublicationsForSameSource_multiple_versions(self):
+        # Check that publications for only the same version as the
+        # context binary publication are returned.
+        (foo_src_pub, foo_bin_pub, foo_one_common_pubs, foo_two_common_pubs,
+         foo_three_pub) = self._makeMixedSingleBuildPackage(version="1.0")
+        self._makeMixedSingleBuildPackage(version="1.1")
+
+        foo_one_common_pub = foo_one_common_pubs[0]
+        others = foo_one_common_pub.getOtherPublicationsForSameSource()
+        others = list(others)
+
+        self.assertContentEqual([foo_three_pub, foo_bin_pub], others)
+
+
 class TestGetBuiltBinaries(TestNativePublishingBase):
     """Test SourcePackagePublishingHistory.getBuiltBinaries() works."""