Merge lp:~cjwatson/launchpad/copies-respect-new into lp:launchpad

[Colin Watson]

Send copies to NEW if they contain binaries without overrides.

Make binaryful copies respect NEW. This is not ideal because it results in entries in the NEW queue which don't really explain why they're there; however, it's a first step towards a more reasonable representation of copies. This is particularly important to make sure that there's an opportunity for packages staged in PPAs to receive proper review when copied into the Ubuntu archive.

[Adam Conrad (adconrad)]

So, until the queue redesign copies remain opaque, which means that can't be overridden until they're actually accepted. Will this change at least make sure ancestry does the expected thing (the code certainly seems to mention ancestry) and makes new binaries match the overrides of the source? For most cases, this should end up being the desired behaviour.

[William Grant (wgrant)]

This is going to block any binary copies to series (or distros!) with fewer arches, but otherwise seems fine.

review: Approve (code)

[Colin Watson (cjwatson)]

> So, until the queue redesign copies remain opaque, which means that can't be
> overridden until they're actually accepted.

Right. I know that this is only part of what we need; the patch includes a bug reference for the other major part of the problem. That's bigger, though, and I think that making sure things land in NEW at all is more immediately pressing. It's at least somewhat useful: for example, you can at least reject an upload if it's inappropriate.

William and I think that making binary overrides work on copies may not actually require the full redesign, although that would still be valuable for other reasons (e.g. making diffs work properly would be unutterably tedious with what we have now). We'd stuff the override data into the PU's JSON metadata much as we do now with source uploads, probably in the same format that we currently return from PU.getBinaryProperties, and hook up PU.getBinaryProperties and PU.overrideBinary to that; and we'd have to fish out those overrides in the PCJ, pass them down to the copier, and make the copier handle binary overrides, which is probably the largest chunk of work.

But I didn't want to do that in the same branch as this change, because it'll be a bit longer and I've given William more than enough gigantic branches to review lately ...

> Will this change at least make
> sure ancestry does the expected thing (the code certainly seems to mention
> ancestry) and makes new binaries match the overrides of the source? For most
> cases, this should end up being the desired behaviour.

All this change does is sometimes suspend the copy job and create a queue entry for it in a few more places. If the queue entry is accepted, then the copy job will be reprocessed from the start, applying overrides exactly as before.

When we implement the scheme above, we'll apply the archive's override policy when calculating the overrides that we stuff into the PU, which will result in reasonable defaults.

[Colin Watson (cjwatson)]

> So, until the queue redesign copies remain opaque, which means that can't be
> overridden until they're actually accepted.

Right. I know that this is only part of what we need; the patch includes a bug reference for the other major part of the problem. That's bigger, though, and I think that making sure things land in NEW at all is more immediately pressing. It's at least somewhat useful: for example, you can at least reject an upload if it's inappropriate.

William and I think that making binary overrides work on copies may not actually require the full redesign, although that would still be valuable for other reasons (e.g. making diffs work properly would be unutterably tedious with what we have now). We'd stuff the override data into the PU's JSON metadata much as we do now with source uploads, probably in the same format that we currently return from PU.getBinaryProperties, and hook up PU.getBinaryProperties and PU.overrideBinary to that; and we'd have to fish out those overrides in the PCJ, pass them down to the copier, and make the copier handle binary overrides, which is probably the largest chunk of work.

But I didn't want to do that in the same branch as this change, because it'll be a bit longer and I've given William more than enough gigantic branches to review lately ...

> Will this change at least make
> sure ancestry does the expected thing (the code certainly seems to mention
> ancestry) and makes new binaries match the overrides of the source? For most
> cases, this should end up being the desired behaviour.

All this change does is sometimes suspend the copy job and create a queue entry for it in a few more places. If the queue entry is accepted, then the copy job will be reprocessed from the start, applying overrides exactly as before.

When we implement the scheme above, we'll apply the archive's override policy when calculating the overrides that we stuff into the PU, which will result in reasonable defaults.

[Colin Watson (cjwatson)]

(Sorry for the duplicate. I blame the hotel's captive portal for confusing me.)

Unmerged revisions

17033. By Colin Watson on 2014-05-30

Send copies to NEW if they contain binaries without overrides.

17032. By Colin Watson on 2014-05-30

Remove reference to non-existent setNew attribute.

=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml	2014-02-18 11:40:52 +0000
+++ lib/lp/soyuz/configure.zcml	2014-05-30 12:14:46 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+<!-- Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
      GNU Affero General Public License version 3 (see the file LICENSE).
 -->
 
@@ -199,7 +199,6 @@
         <require
             permission="launchpad.Edit"
             attributes="
-                setNew
                 setUnapproved
                 setRejected
                 setAccepted

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2014-05-15 08:49:44 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2014-05-30 12:14:46 +0000
@@ -515,8 +515,7 @@
             raise CannotCopy("Package %r %r not found." % (name, version))
         return source_package
 
-    def _checkPolicies(self, source_name, source_component=None,
-                       auto_approve=False):
+    def _checkPolicies(self, source_name, source_package, auto_approve=False):
         # This helper will only return if it's safe to carry on with the
         # copy, otherwise it raises SuspendJobException to tell the job
         # runner to suspend the job.
@@ -532,7 +531,8 @@
             # metadata.
             defaults = UnknownOverridePolicy().calculateSourceOverrides(
                 self.target_archive, self.target_distroseries,
-                self.target_pocket, [source_name], source_component)
+                self.target_pocket, [source_name],
+                source_package.sourcepackagerelease.component)
             self.addSourceOverride(defaults[0])
             if auto_approve:
                 auto_approve = self.target_archive.canAdministerQueue(
@@ -556,6 +556,29 @@
                     self.requester, self.getSourceOverride().component,
                     self.target_pocket, self.target_distroseries)
 
+        # Go through the same routine for binary overrides if necessary.  We
+        # assume that the above permission check on the source component is
+        # sufficient.
+        if self.include_binaries:
+            binaries = source_package.getBuiltBinaries()
+            bpn_archtag = [(
+                bpph.binarypackagerelease.binarypackagename,
+                bpph.distroarchseries.architecturetag) for bpph in binaries]
+            ancestry = override_policy.calculateBinaryOverrides(
+                self.target_archive, self.target_distroseries,
+                self.target_pocket, bpn_archtag)
+            if len(ancestry) != len(binaries):
+                # XXX cjwatson bug=1079577: Binary overrides for Ubuntu
+                # should be calculated using the Ubuntu override policy, and
+                # they should be saved in the metadata with an API for
+                # overriding them.
+                approve_new = auto_approve or copy_policy.autoApproveNew(
+                    self.target_archive, self.target_distroseries,
+                    self.target_pocket)
+                if not approve_new:
+                    self._createPackageUpload()
+                    raise SuspendJobException
+
         # The package is not new (it has ancestry) so check the copy
         # policy for existing packages.
         approve_existing = auto_approve or copy_policy.autoApprove(
@@ -643,9 +666,7 @@
             [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)
+            self._checkPolicies(source_name, source_package, self.auto_approve)
 
         # The package is free to go right in, so just copy it now.
         ancestry = self.target_archive.getPublishedSources(

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2014-05-15 09:06:24 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2014-05-30 12:14:46 +0000
@@ -18,6 +18,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.bugs.interfaces.bugtask import BugTaskStatus
+from lp.buildmaster.enums import BuildStatus
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.model.distroseriesdifferencecomment import (
@@ -1003,6 +1004,40 @@
             [removeSecurityProxy(job).context.id]).one()
         self.assertEqual(PackageUploadStatus.NEW, pu.status)
 
+    def test_copying_new_binaries(self):
+        # A copy involving source and new binaries requires NEW queue approval.
+        target_archive = self.factory.makeArchive(
+            self.distroseries.distribution, purpose=ArchivePurpose.PRIMARY)
+        source_archive = self.factory.makeArchive()
+        old_spph = self.publisher.getPubSource(
+            distroseries=self.distroseries, sourcename="copyme", version="1.0",
+            status=PackagePublishingStatus.PUBLISHED, archive=target_archive)
+        self.publisher.getPubBinaries(
+            binaryname="copyme", pub_source=old_spph,
+            distroseries=self.distroseries,
+            status=PackagePublishingStatus.PUBLISHED)
+        new_spph = self.publisher.getPubSource(
+            distroseries=self.distroseries, sourcename="copyme", version="1.1",
+            status=PackagePublishingStatus.PUBLISHED, archive=source_archive)
+        for build in new_spph.createMissingBuilds():
+            build.updateStatus(BuildStatus.FULLYBUILT)
+            for binaryname in ("copyme", "libcopyme-dev"):
+                bpr = self.publisher.uploadBinaryForBuild(build, binaryname)
+                self.publisher.publishBinaryInArchive(
+                    bpr, new_spph.archive,
+                    status=PackagePublishingStatus.PUBLISHED)
+            pu = self.publisher.addPackageUpload(
+                new_spph.archive, self.distroseries,
+                changes_file_name="copyme_1.1_%s.changes" % build.arch_tag)
+            pu.addBuild(build)
+        job = self.createCopyJobForSPPH(
+            new_spph, source_archive, target_archive, include_binaries=True)
+        self.runJob(job)
+        self.assertEqual(JobStatus.SUSPENDED, job.status)
+        pu = getUtility(IPackageUploadSet).getByPackageCopyJobIDs(
+            [removeSecurityProxy(job).context.id]).one()
+        self.assertEqual(PackageUploadStatus.NEW, pu.status)
+
     def test_copying_to_main_archive_unapproved(self):
         # Uploading to a series that is in a state that precludes auto
         # approval will cause the job to suspend and a packageupload
@@ -1302,6 +1337,10 @@
             distroseries=self.distroseries, sourcename="copyme",
             version="2.8-0", status=PackagePublishingStatus.PUBLISHED,
             component='multiverse', section='web', archive=target_archive)
+        self.publisher.getPubBinaries(
+            binaryname="copyme", pub_source=old_spph,
+            distroseries=self.distroseries,
+            status=PackagePublishingStatus.PUBLISHED)
         old_spr = old_spph.sourcepackagerelease
         diff_file = self.publisher.addMockFile("diff_file", restricted=True)
         package_diff = old_spr.requestDiffTo(target_archive.owner, spr)