Merge lp:~cjwatson/launchpad/copy-explicit-pocket into lp:launchpad

[Colin Watson]

Allow specifying source distroseries and pocket in Archive.copyPackage and in PackageCopyJob metadata.

== Summary ==

Bug 1070324: Incremental copying of binaries doesn't work. Fixing this would save us a lot of (variously) manual recovery work and careful attention to avoid manual recovery work, particularly now that we're about to start automatically copying things from PROPOSED to RELEASE.

== Proposed fix ==

Allow optionally passing a source distroseries and pocket to Archive.copyPackage, and pass it all the way down the chain. I didn't bother with Archive.copyPackages as I don't think we need this sort of fine-grained control there, although most of the pieces are there if somebody wants to do that later for some reason.

The underlying copy script workflow is tested in lib/lp/soyuz/scripts/tests/test_copypackage.py:TestCopyBuildRecords.test_incremental_binary_copies.

== Implementation details ==

I moved PCJ's Archive.getPublishedSources call into its own method to make it more conveniently testable.

== LOC Rationale ==

+103. I have 5754 lines of credit at the moment, and I think this is worth it to avoid occasional manual recovery work.

== Tests ==

bin/test -vvct lp.soyuz.tests.test_archive.TestCopyPackage -t lp.soyuz.tests.test_packagecopyjob

== Demo and Q/A ==

A lack of dogfood/staging builders makes it hard to QA anything involving real builds, never mind for multiple architectures; but given the tests and that this is a new optional facility it's probably reasonable to just check that copies without from_series/from_pocket still work, along with copies with from_series/from_pocket matching the SPPH to be copied.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2012-09-21 07:02:26 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2012-10-23 13:31:46 +0000
@@ -1318,8 +1318,9 @@
         version=TextLine(title=_("Version")),
         from_archive=Reference(schema=Interface),
         # Really IArchive, see below
-        to_pocket=TextLine(title=_("Pocket name")),
-        to_series=TextLine(title=_("Distroseries name"), required=False),
+        to_pocket=TextLine(title=_("Target pocket name")),
+        to_series=TextLine(
+            title=_("Target distroseries name"), required=False),
         include_binaries=Bool(
             title=_("Include Binaries"),
             description=_("Whether or not to copy binaries already built for"
@@ -1335,12 +1336,16 @@
             description=_("Automatically approve this copy (queue admins "
                           "only)."),
             required=False),
+        from_pocket=TextLine(title=_("Source pocket name"), required=False),
+        from_series=TextLine(
+            title=_("Source distroseries name"), required=False),
         )
     @export_write_operation()
     @operation_for_version('devel')
     def copyPackage(source_name, version, from_archive, to_pocket,
                     person, to_series=None, include_binaries=False,
-                    sponsored=None, unembargo=False, auto_approve=False):
+                    sponsored=None, unembargo=False, auto_approve=False,
+                    from_pocket=None, from_series=None):
         """Copy a single named source into this archive.
 
         Asynchronously copy a specific version of a named source to the
@@ -1367,6 +1372,10 @@
         :param auto_approve: if True and the `IPerson` requesting the sync
             has queue admin permissions on the target, accept the copy
             immediately rather than setting it to unapproved.
+        :param from_pocket: the source pocket (as a string). If omitted,
+            copy from any pocket with a matching version.
+        :param from_series: the source distroseries (as a string). If
+            omitted, copy from any series with a matching version.
 
         :raises NoSuchSourcePackageName: if the source name is invalid
         :raises PocketNotFound: if the pocket name is invalid
 
=== modified file 'lib/lp/soyuz/interfaces/packagecopyjob.py'
--- lib/lp/soyuz/interfaces/packagecopyjob.py	2012-07-30 16:48:37 +0000
+++ lib/lp/soyuz/interfaces/packagecopyjob.py	2012-10-23 13:31:46 +0000
@@ -128,7 +128,8 @@
                target_archive, target_distroseries, target_pocket,
                include_binaries=False, package_version=None,
                copy_policy=PackageCopyPolicy.INSECURE, requester=None,
-               sponsored=None, unembargo=False, auto_approve=False):
+               sponsored=None, unembargo=False, auto_approve=False,
+               source_distroseries=None, source_pocket=None):
         """Create a new `IPlainPackageCopyJob`.
 
         :param package_name: The name of the source package to copy.
@@ -150,6 +151,12 @@
         :param auto_approve: if True and the user requesting the sync has
             queue admin permissions on the target, accept the copy
             immediately rather than setting it to unapproved.
+        :param source_distroseries: The `IDistroSeries` from which to copy
+            the packages. If omitted, copy from any series with a matching
+            version.
+        :param source_pocket: The pocket from which to copy the packages.
+            Must be a member of `PackagePublishingPocket`. If omitted, copy
+            from any pocket with a matching version.
         """
 
     def createMultiple(target_distroseries, copy_tasks, requester,
@@ -227,6 +234,14 @@
         title=_("Automatic approval"),
         required=False, readonly=True)
 
+    source_distroseries = Reference(
+        schema=IDistroSeries, title=_('Source DistroSeries.'),
+        required=False, readonly=True)
+
+    source_pocket = Int(
+        title=_("Source package publishing pocket"), required=False,
+        readonly=True)
+
     def addSourceOverride(override):
         """Add an `ISourceOverride` to the metadata."""
 
@@ -236,6 +251,9 @@
     def getSourceOverride():
         """Get an `ISourceOverride` from the metadata."""
 
+    def findSourcePublication():
+        """Find the appropriate origin `ISourcePackagePublishingHistory`."""
+
     copy_policy = Choice(
         title=_("Applicable copy policy"),
         values=PackageCopyPolicy, required=True, readonly=True)
 
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-10-19 13:11:51 +0000
+++ lib/lp/soyuz/model/archive.py	2012-10-23 13:31:46 +0000
@@ -1639,13 +1639,15 @@
             sources, to_pocket, to_series, include_binaries,
             person=person)
 
-    def _validateAndFindSource(self, from_archive, source_name, version):
+    def _validateAndFindSource(self, from_archive, source_name, version,
+                               from_series=None, from_pocket=None):
         # Check to see if the source package exists, and raise a useful error
         # if it doesn't.
         getUtility(ISourcePackageNameSet)[source_name]
         # Find and validate the source package version required.
         source = from_archive.getPublishedSources(
-            name=source_name, version=version, exact_match=True).first()
+            name=source_name, version=version, exact_match=True,
+            distroseries=from_series, pocket=from_pocket).first()
         if source is None:
             raise CannotCopy(
                 "%s is not published in %s." %
@@ -1664,15 +1666,21 @@
 
     def copyPackage(self, source_name, version, from_archive, to_pocket,
                     person, to_series=None, include_binaries=False,
-                    sponsored=None, unembargo=False, auto_approve=False):
+                    sponsored=None, unembargo=False, auto_approve=False,
+                    from_pocket=None, from_series=None):
         """See `IArchive`."""
         # Asynchronously copy a package using the job system.
         pocket = self._text_to_pocket(to_pocket)
         series = self._text_to_series(to_series)
+        if from_pocket:
+            from_pocket = self._text_to_pocket(from_pocket)
+        if from_series:
+            from_series = self._text_to_series(from_series)
         # Upload permission checks, this will raise CannotCopy as
         # necessary.
         source = self._validateAndFindSource(
-            from_archive, source_name, version)
+            from_archive, source_name, version, from_series=from_series,
+            from_pocket=from_pocket)
         if series is None:
             series = source.distroseries
         check_copy_permissions(person, self, series, pocket, [source])
@@ -1685,7 +1693,8 @@
             package_version=version, include_binaries=include_binaries,
             copy_policy=PackageCopyPolicy.INSECURE, requester=person,
             sponsored=sponsored, unembargo=unembargo,
-            auto_approve=auto_approve)
+            auto_approve=auto_approve, source_distroseries=from_series,
+            source_pocket=from_pocket)
 
     def copyPackages(self, source_names, from_archive, to_pocket,
                      person, to_series=None, from_series=None,
 
=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2012-10-17 16:05:07 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2012-10-23 13:31:46 +0000
@@ -270,19 +270,19 @@
     @classmethod
     def _makeMetadata(cls, target_pocket, package_version,
                       include_binaries, sponsored=None, unembargo=False,
-                      auto_approve=False):
+                      auto_approve=False, source_distroseries=None,
+                      source_pocket=None):
         """Produce a metadata dict for this job."""
-        if sponsored:
-            sponsored_name = sponsored.name
-        else:
-            sponsored_name = None
         return {
             'target_pocket': target_pocket.value,
             'package_version': package_version,
             'include_binaries': bool(include_binaries),
-            'sponsored': sponsored_name,
+            'sponsored': sponsored.name if sponsored else None,
             'unembargo': unembargo,
             'auto_approve': auto_approve,
+            'source_distroseries':
+                source_distroseries.name if source_distroseries else None,
+            'source_pocket': source_pocket.value if source_pocket else None,
         }
 
     @classmethod
@@ -290,13 +290,14 @@
                target_archive, target_distroseries, target_pocket,
                include_binaries=False, package_version=None,
                copy_policy=PackageCopyPolicy.INSECURE, requester=None,
-               sponsored=None, unembargo=False, auto_approve=False):
+               sponsored=None, unembargo=False, auto_approve=False,
+               source_distroseries=None, source_pocket=None):
         """See `IPlainPackageCopyJobSource`."""
         assert package_version is not None, "No package version specified."
         assert requester is not None, "No requester specified."
         metadata = cls._makeMetadata(
             target_pocket, package_version, include_binaries, sponsored,
-            unembargo, auto_approve)
+            unembargo, auto_approve, source_distroseries, source_pocket)
         job = PackageCopyJob(
             job_type=cls.class_job_type,
             source_archive=source_archive,
@@ -435,6 +436,20 @@
     def auto_approve(self):
         return self.metadata.get('auto_approve', False)
 
+    @property
+    def source_distroseries(self):
+        name = self.metadata.get('source_distroseries')
+        if name is None:
+            return None
+        return self.source_archive.distribution[name]
+
+    @property
+    def source_pocket(self):
+        name = self.metadata.get('source_pocket')
+        if name is None:
+            return None
+        return PackagePublishingPocket.items[name]
+
     def _createPackageUpload(self, unapproved=False):
         pu = self.target_distroseries.createQueueEntry(
             pocket=self.target_pocket, archive=self.target_archive,
@@ -472,6 +487,18 @@
 
         return SourceOverride(source_package_name, component, section)
 
+    def findSourcePublication(self):
+        """Find the appropriate origin `ISourcePackagePublishingHistory`."""
+        name = self.package_name
+        version = self.package_version
+        source_package = self.source_archive.getPublishedSources(
+            name=name, version=version, exact_match=True,
+            distroseries=self.source_distroseries,
+            pocket=self.source_pocket).first()
+        if source_package is None:
+            raise CannotCopy("Package %r %r not found." % (name, version))
+        return source_package
+
     def _checkPolicies(self, source_name, source_component=None,
                        auto_approve=False):
         # This helper will only return if it's safe to carry on with the
@@ -589,13 +616,7 @@
                 raise CannotCopy(
                     "Destination pocket must be 'release' for a PPA.")
 
-        name = self.package_name
-        version = self.package_version
-        source_package = self.source_archive.getPublishedSources(
-            name=name, version=version, exact_match=True).first()
-        if source_package is None:
-            raise CannotCopy("Package %r %r not found." % (name, version))
-        source_name = getUtility(ISourcePackageNameSet)[name]
+        source_package = self.findSourcePublication()
 
         # If there's a PackageUpload associated with this job then this
         # job has just been released by an archive admin from the queue.
@@ -604,13 +625,14 @@
         pu = getUtility(IPackageUploadSet).getByPackageCopyJobIDs(
             [self.context.id]).any()
         if pu is None:
+            source_name = getUtility(ISourcePackageNameSet)[self.package_name]
             self._checkPolicies(
                 source_name, source_package.sourcepackagerelease.component,
                 self.auto_approve)
 
         # The package is free to go right in, so just copy it now.
         ancestry = self.target_archive.getPublishedSources(
-            name=name, distroseries=self.target_distroseries,
+            name=self.package_name, distroseries=self.target_distroseries,
             pocket=self.target_pocket, exact_match=True)
         override = self.getSourceOverride()
         copy_policy = self.getPolicyImplementation()
@@ -698,12 +720,15 @@
             " from %s/%s" % (
                 self.source_archive.distribution.name,
                 self.source_archive.name))
+        if self.source_pocket is not None:
+            parts.append(", %s pocket," % self.source_pocket.name)
+        if self.source_distroseries is not None:
+            parts.append(" in %s" % self.source_distroseries)
         parts.append(
             " to %s/%s" % (
                 self.target_archive.distribution.name,
                 self.target_archive.name))
-        parts.append(
-            ", %s pocket," % self.target_pocket.name)
+        parts.append(", %s pocket," % self.target_pocket.name)
         if self.target_distroseries is not None:
             parts.append(" in %s" % self.target_distroseries)
         if self.include_binaries:
 
=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2012-10-19 13:11:51 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2012-10-23 13:31:46 +0000
@@ -2374,6 +2374,32 @@
                 source_name, version, target_archive, to_pocket.name,
                 target_archive.owner)
 
+    def test_copyPackage_with_source_series_and_pocket(self):
+        # The from_series and from_pocket parameters cause copyPackage to
+        # select a matching source publication.
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, version) = self._setup_copy_data()
+        other_series = self.factory.makeDistroSeries(
+            distribution=source_archive.distribution,
+            status=SeriesStatus.DEVELOPMENT)
+        with person_logged_in(source_archive.owner):
+            source.copyTo(
+                other_series, PackagePublishingPocket.UPDATES, source_archive)
+            source.requestDeletion(source_archive.owner)
+        with person_logged_in(target_archive.owner):
+            target_archive.copyPackage(
+                source_name, version, source_archive, to_pocket.name,
+                include_binaries=False, person=target_archive.owner,
+                from_series=source.distroseries.name,
+                from_pocket=source.pocket.name)
+
+        # There should be one copy job, with the source distroseries and
+        # pocket set.
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        copy_job = job_source.getActiveJobs(target_archive).one()
+        self.assertEqual(source.distroseries, copy_job.source_distroseries)
+        self.assertEqual(source.pocket, copy_job.source_pocket)
+
     def test_copyPackages_with_single_package(self):
         (source, source_archive, source_name, target_archive, to_pocket,
          to_series, version) = self._setup_copy_data()
 
=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2012-10-17 12:33:21 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2012-10-23 13:31:46 +0000
@@ -1478,6 +1478,22 @@
 
         self.assertEqual(override, pcj.getSourceOverride())
 
+    def test_findSourcePublication_with_source_series_and_pocket(self):
+        # The source_distroseries and source_pocket parameters cause
+        # findSourcePublication to select a matching source publication.
+        spph = self.publisher.getPubSource()
+        other_series = self.factory.makeDistroSeries(
+            distribution=spph.distroseries.distribution,
+            status=SeriesStatus.DEVELOPMENT)
+        spph.copyTo(
+            other_series, PackagePublishingPocket.PROPOSED, spph.archive)
+        spph.requestDeletion(spph.archive.owner)
+        job = self.createCopyJobForSPPH(
+            spph, spph.archive, spph.archive,
+            target_pocket=PackagePublishingPocket.UPDATES,
+            source_distroseries=spph.distroseries, source_pocket=spph.pocket)
+        self.assertEqual(spph, job.findSourcePublication())
+
     def test_getPolicyImplementation_returns_policy(self):
         # getPolicyImplementation returns the ICopyPolicy that was
         # chosen for the job.