Merge lp:~wgrant/launchpad/ddeb-domination into lp:launchpad/db-devel

[William Grant]

Binary publications now supersede their corresponding debug publications when they are themselves superseded.

This branch alters binary domination to fix bug #604425.

DDEBs need to be as unintrusive as possible -- people shouldn't have to change how they deal with archive administration. To help with this, DDEB publications should be superseded as soon as their DEB is. This will, for example, eliminate the need for archive admins to manually NBS them out if they stop being built.

This is implemented much the same as atomic arch-indep domination: BinaryPackagePublishingHistory.supersede locates corresponding DDEB publications with the same overrides, and supersedes them.

Because of this change, it's no longer important for DDEBs to supersede each other (their DEBs will handle it all). I've filtered them out of the Dominator query, and added an assertion to confirm that no DDEB ever accidentally supersedes something else.

[Māris Fogels (mars)]

Hi William,

This is a very good change! At first I was caught thinking testJudgeAndDominateWithDDEBs() needed refactoring, but was pleasantly surprised to find that you fully tested both DDEB policies in the test_publishing module. I guess testJudgeAndDominate is just checking that the components are integrated correctly.

The only other question is about _getCorrespondingDDEBPublications(): can it use the ISlaveStore instead of the IMasterStore? Check lib/canonical/launchpad/doc/db-policy.txt for the difference.

Otherwise this looks good to me: r=mars

Maris

review: Approve (code)

[William Grant (wgrant)]

Hi Māris,

On Tue, 2010-07-27 at 20:26 +0000, Māris Fogels wrote:
> Review: Approve code
> Hi William,

Thanks for the review.

> This is a very good change! At first I was caught thinking
> testJudgeAndDominateWithDDEBs() needed refactoring, but was pleasantly
> surprised to find that you fully tested both DDEB policies in the
> test_publishing module. I guess testJudgeAndDominate is just checking
> that the components are integrated correctly.

Right. I refactored the related code in earlier branches to make this
style of testing possible. I think it worked pretty well.

> The only other question is about _getCorrespondingDDEBPublications():
> can it use the ISlaveStore instead of the IMasterStore? Check
> lib/canonical/launchpad/doc/db-policy.txt for the difference.

This is the sort of thing where it's absolutely critical that we have
up-to-the-second data. Using the slave store would be dangerous.

> Otherwise this looks good to me: r=mars

Thanks!

[Robert Collins (lifeless)]

there is a typo - 'independet'.

-Rob

[William Grant (wgrant)]

Robert:

145 - # If this is architecture-independet, all publications with the same
        ^ It's already fixed!

=== modified file 'lib/lp/archivepublisher/domination.py'
--- lib/lp/archivepublisher/domination.py	2010-07-11 13:58:56 +0000
+++ lib/lp/archivepublisher/domination.py	2010-07-26 12:04:48 +0000
@@ -66,6 +66,7 @@
     clear_current_connection_cache)
 
 from canonical.launchpad.interfaces import PackagePublishingStatus
+from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
 
 
 def clear_cache():
@@ -310,10 +311,12 @@
                 AND binarypackagepublishinghistory.status = %s AND
                 binarypackagepublishinghistory.binarypackagerelease =
                     binarypackagerelease.id
+                AND binarypackagerelease.binpackageformat != %s
                 AND binarypackagerelease.binarypackagename IN (
                     SELECT name FROM PubDomHelper WHERE count > 1)"""
                 % sqlvalues(distroarchseries, self.archive,
-                            pocket, PackagePublishingStatus.PUBLISHED),
+                            pocket, PackagePublishingStatus.PUBLISHED,
+                            BinaryPackageFormat.DDEB),
                 clauseTables=['BinaryPackageRelease'])
 
             self.debug("Dominating binaries...")

=== modified file 'lib/lp/archivepublisher/tests/test_dominator.py'
--- lib/lp/archivepublisher/tests/test_dominator.py	2010-07-22 11:59:40 +0000
+++ lib/lp/archivepublisher/tests/test_dominator.py	2010-07-26 12:04:48 +0000
@@ -18,13 +18,14 @@
 class TestDominator(TestNativePublishingBase):
     """Test Dominator class."""
 
-    def createSourceAndBinaries(self, version):
+    def createSourceAndBinaries(self, version, with_debug=False,
+                                archive=None):
         """Create a source and binaries with the given version."""
         source = self.getPubSource(
-            version=version,
+            version=version, archive=archive,
             status=PackagePublishingStatus.PUBLISHED)
         binaries = self.getPubBinaries(
-            pub_source=source,
+            pub_source=source, with_debug=with_debug,
             status=PackagePublishingStatus.PUBLISHED)
         return (source, binaries)
 
@@ -107,6 +108,36 @@
             [foo_10_source] + foo_10_binaries,
             PackagePublishingStatus.SUPERSEDED)
 
+    def testJudgeAndDominateWithDDEBs(self):
+        """Verify that judgeAndDominate ignores DDEBs correctly.
+
+        DDEBs are superseded by their corresponding DEB publications, so they
+        are forbidden from superseding publications (an attempt would result
+        in an AssertionError), and shouldn't be directly considered for
+        superseding either.
+        """
+        ppa = self.factory.makeArchive()
+        foo_10_source, foo_10_binaries = self.createSourceAndBinaries(
+            '1.0', with_debug=True, archive=ppa)
+        foo_11_source, foo_11_binaries = self.createSourceAndBinaries(
+            '1.1', with_debug=True, archive=ppa)
+        foo_12_source, foo_12_binaries = self.createSourceAndBinaries(
+            '1.2', with_debug=True, archive=ppa)
+
+        dominator = Dominator(self.logger, ppa)
+        dominator.judgeAndDominate(
+            foo_10_source.distroseries, foo_10_source.pocket, self.config)
+
+        self.checkPublications(
+            [foo_12_source] + foo_12_binaries,
+            PackagePublishingStatus.PUBLISHED)
+        self.checkPublications(
+            [foo_11_source] + foo_11_binaries,
+            PackagePublishingStatus.SUPERSEDED)
+        self.checkPublications(
+            [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)

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2010-07-21 14:14:54 +0000
+++ lib/lp/soyuz/model/publishing.py	2010-07-26 12:04:48 +0000
@@ -50,6 +50,7 @@
 from lp.registry.interfaces.person import validate_public_person
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.worlddata.model.country import Country
+from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
 from lp.soyuz.model.binarypackagename import BinaryPackageName
 from lp.soyuz.model.binarypackagerelease import (BinaryPackageRelease,
     BinaryPackageReleaseDownloadCount)
@@ -975,6 +976,26 @@
                 section=self.section,
                 priority=self.priority)
 
+    def _getCorrespondingDDEBPublications(self):
+        """Return remaining publications of the corresponding DDEB.
+
+        Only considers binary publications in the corresponding debug
+        archive with the same distroarchseries, pocket, component, section
+        and priority.
+        """
+        return IMasterStore(BinaryPackagePublishingHistory).find(
+                BinaryPackagePublishingHistory,
+                BinaryPackagePublishingHistory.status.is_in(
+                    [PUBLISHED, PENDING]),
+                BinaryPackagePublishingHistory.distroarchseries ==
+                    self.distroarchseries,
+                binarypackagerelease=self.binarypackagerelease.debug_package,
+                archive=self.archive.debug_archive,
+                pocket=self.pocket,
+                component=self.component,
+                section=self.section,
+                priority=self.priority)
+
     def supersede(self, dominant=None, logger=None):
         """See `IBinaryPackagePublishingHistory`."""
         # At this point only PUBLISHED (ancient versions) or PENDING (
@@ -993,6 +1014,16 @@
         super(BinaryPackagePublishingHistory, self).supersede()
 
         if dominant is not None:
+            # DDEBs cannot themselves be dominant; they are always dominated
+            # by their corresponding DEB. Any attempt to dominate with a
+            # dominant DDEB is a bug.
+            assert (
+                dominant.binarypackagerelease.binpackageformat !=
+                    BinaryPackageFormat.DDEB), (
+                "Should not dominate with %s (%s); DDEBs cannot dominate" % (
+                    dominant.binarypackagerelease.title,
+                    dominant.distroarchseries.architecturetag))
+
             dominant_build = dominant.binarypackagerelease.build
             distroarchseries = dominant_build.distro_arch_series
             if logger is not None:
@@ -1010,7 +1041,10 @@
             # between releases.
             self.supersededby = dominant_build
 
-        # If this is architecture-independet, all publications with the same
+        for dominated in self._getCorrespondingDDEBPublications():
+            dominated.supersede(dominant, logger)
+
+        # If this is architecture-independent, all publications with the same
         # context and overrides should be dominated simultaneously.
         if not self.binarypackagerelease.architecturespecific:
             for dominated in self._getOtherPublications():

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2010-07-23 12:19:26 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2010-07-26 12:04:48 +0000
@@ -32,7 +32,7 @@
 from lp.soyuz.model.processor import ProcessorFamily
 from lp.soyuz.model.publishing import (
     SourcePackagePublishingHistory, BinaryPackagePublishingHistory)
-from lp.soyuz.interfaces.archive import ArchivePurpose
+from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
 from lp.soyuz.interfaces.archivearch import IArchiveArchSet
 from lp.soyuz.interfaces.binarypackagename import IBinaryPackageNameSet
 from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
@@ -275,7 +275,8 @@
                        version='666',
                        architecturespecific=False,
                        builder=None,
-                       component='main'):
+                       component='main',
+                       with_debug=False):
         """Return a list of binary publishing records."""
         if distroseries is None:
             distroseries = self.distroseries
@@ -302,11 +303,25 @@
         published_binaries = []
         for build in builds:
             build.builder = builder
+            pub_binaries = []
+            if with_debug:
+                binarypackagerelease_ddeb = self.uploadBinaryForBuild(
+                    build, binaryname + '-dbgsym', filecontent, summary,
+                    description, shlibdep, depends, recommends, suggests,
+                    conflicts, replaces, provides, pre_depends, enhances,
+                    breaks, BinaryPackageFormat.DDEB)
+                pub_binaries += self.publishBinaryInArchive(
+                    binarypackagerelease_ddeb, archive.debug_archive, status,
+                    pocket, scheduleddeletiondate, dateremoved)
+            else:
+                binarypackagerelease_ddeb = None
+
             binarypackagerelease = self.uploadBinaryForBuild(
                 build, binaryname, filecontent, summary, description,
                 shlibdep, depends, recommends, suggests, conflicts, replaces,
-                provides, pre_depends, enhances, breaks, format)
-            pub_binaries = self.publishBinaryInArchive(
+                provides, pre_depends, enhances, breaks, format,
+                binarypackagerelease_ddeb)
+            pub_binaries += self.publishBinaryInArchive(
                 binarypackagerelease, archive, status, pocket,
                 scheduleddeletiondate, dateremoved)
             published_binaries.extend(pub_binaries)
@@ -1144,6 +1159,48 @@
         self.checkSuperseded([bin], super_bin)
         self.assertEquals(super_date, bin.datesuperseded)
 
+    def testSupersedesCorrespondingDDEB(self):
+        """Check that supersede() takes with it any corresponding DDEB.
+
+        DDEB publications should be superseded when their corresponding DEB
+        is.
+        """
+        getUtility(IArchiveSet).new(
+            purpose=ArchivePurpose.DEBUG, owner=self.ubuntutest.owner,
+            distribution=self.ubuntutest)
+
+        # Each of these will return (i386 deb, i386 ddeb, hppa deb,
+        # hppa ddeb).
+        bins = self.getPubBinaries(
+            architecturespecific=True, with_debug=True)
+        super_bins = self.getPubBinaries(
+            architecturespecific=True, with_debug=True)
+
+        bins[0].supersede(super_bins[0])
+        self.checkSuperseded(bins[:2], super_bins[0])
+        self.checkPublications(bins[2:], PackagePublishingStatus.PENDING)
+        self.checkPublications(super_bins, PackagePublishingStatus.PENDING)
+
+        bins[2].supersede(super_bins[2])
+        self.checkSuperseded(bins[:2], super_bins[0])
+        self.checkSuperseded(bins[2:], super_bins[2])
+        self.checkPublications(super_bins, PackagePublishingStatus.PENDING)
+
+    def testDDEBsCannotSupersede(self):
+        """Check that DDEBs cannot supersede other publications.
+
+        Since DDEBs are superseded when their DEBs are, there's no need to
+        for them supersede anything themselves. Any such attempt is an error.
+        """
+        getUtility(IArchiveSet).new(
+            purpose=ArchivePurpose.DEBUG, owner=self.ubuntutest.owner,
+            distribution=self.ubuntutest)
+
+        # This will return (i386 deb, i386 ddeb, hppa deb, hppa ddeb).
+        bins = self.getPubBinaries(
+            architecturespecific=True, with_debug=True)
+        self.assertRaises(AssertionError, bins[0].supersede, bins[1])
+
 
 class TestBinaryGetOtherPublications(TestNativePublishingBase):
     """Test BinaryPackagePublishingHistory._getOtherPublications() works."""