launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/attemptcopy-no-source into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1061374 in Launchpad itself: "PlainPackageCopyJob.attemptCopy() crashes if only binaries were copied"
  https://bugs.launchpad.net/launchpad/+bug/1061374

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/attemptcopy-no-source/+merge/127932

When a PCJ is created to copy just binaries from one pocket to another, the PCJ machinery attempts to update package diffs for the copied source publication. Except there isn't one and so it OOPSes.

I have corrected the lie that was the variable 'copied_sources' and now iterate all copied publications looking for sources and only perform package diff things if we found one.
-- 
https://code.launchpad.net/~stevenk/launchpad/attemptcopy-no-source/+merge/127932
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/attemptcopy-no-source into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2012-08-20 18:20:36 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2012-10-04 06:00:30 +0000
@@ -76,6 +76,7 @@
     PackageCopyJobType,
     )
 from lp.soyuz.interfaces.packagediff import PackageDiffAlreadyRequested
+from lp.soyuz.interfaces.publishing import ISourcePackagePublishingHistory
 from lp.soyuz.interfaces.queue import IPackageUploadSet
 from lp.soyuz.interfaces.section import ISectionSet
 from lp.soyuz.model.archive import Archive
@@ -602,7 +603,7 @@
         override = self.getSourceOverride()
         copy_policy = self.getPolicyImplementation()
         send_email = copy_policy.send_email(self.target_archive)
-        copied_sources = do_copy(
+        copied_publications = do_copy(
             sources=[source_package], archive=self.target_archive,
             series=self.target_distroseries, pocket=self.target_pocket,
             include_binaries=self.include_binaries, check_permissions=True,
@@ -612,16 +613,21 @@
             unembargo=self.unembargo)
 
         # Add a PackageDiff for this new upload if it has ancestry.
-        if copied_sources and not ancestry.is_empty():
-            from_spr = copied_sources[0].sourcepackagerelease
-            for ancestor in ancestry:
-                to_spr = ancestor.sourcepackagerelease
-                if from_spr != to_spr:
-                    try:
-                        to_spr.requestDiffTo(self.requester, from_spr)
-                    except PackageDiffAlreadyRequested:
-                        pass
+        if copied_publications and not ancestry.is_empty():
+            from_spr = None
+            for publication in copied_publications:
+                if ISourcePackagePublishingHistory.providedBy(publication):
+                    from_spr = publication.sourcepackagerelease
                     break
+            if from_spr:
+                for ancestor in ancestry:
+                    to_spr = ancestor.sourcepackagerelease
+                    if from_spr != to_spr:
+                        try:
+                            to_spr.requestDiffTo(self.requester, from_spr)
+                        except PackageDiffAlreadyRequested:
+                            pass
+                        break
 
         if pu is not None:
             # A PackageUpload will only exist if the copy job had to be

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2012-09-27 02:53:00 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2012-10-04 06:00:30 +0000
@@ -1494,6 +1494,38 @@
         # The job should have set the PU status to REJECTED.
         self.assertEqual(PackageUploadStatus.REJECTED, pu.status)
 
+    def test_diffs_are_not_created_when_only_copying_binaries(self):
+        # The job will not fail because a packagediff from a source that wasn't
+        # copied could not be created.
+        archive = self.distroseries.distribution.main_archive
+        source = self.publisher.getPubSource(
+            distroseries=self.distroseries, sourcename="copyme",
+            version="2.8-1", status=PackagePublishingStatus.PUBLISHED,
+            component='multiverse', section='web',
+            pocket=PackagePublishingPocket.RELEASE, archive=archive)
+        spph = self.factory.makeSourcePackagePublishingHistory(
+            status=PackagePublishingStatus.PUBLISHED,
+            pocket=PackagePublishingPocket.UPDATES, archive=archive,
+            distroseries=self.distroseries,
+            sourcepackagerelease=source.sourcepackagerelease)
+        self.publisher.getPubBinaries(
+            binaryname="copyme", pub_source=spph,
+            distroseries=self.distroseries, archive=archive,
+            pocket=PackagePublishingPocket.UPDATES,
+            status=PackagePublishingStatus.PUBLISHED)
+        requester = self.factory.makePerson()
+        with person_logged_in(archive.owner):
+            archive.newComponentUploader(requester, 'multiverse')
+        source = getUtility(IPlainPackageCopyJobSource)
+        job = source.create(
+            package_name="copyme", package_version="2.8-1",
+            source_archive=archive, target_archive=archive,
+            target_distroseries=self.distroseries,
+            target_pocket=PackagePublishingPocket.RELEASE,
+            include_binaries=True, requester=requester)
+        self.runJob(job)
+        self.assertEqual(JobStatus.COMPLETED, job.status)
+
 
 class TestViaCelery(TestCaseWithFactory):
     """PackageCopyJob runs under Celery."""