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

Proposed by Colin Watson on 2014-05-30
Status: Work in progress
Proposed branch: lp:~cjwatson/launchpad/copies-respect-new
Merge into: lp:launchpad
Diff against target: 146 lines (+67/-8)
3 files modified
lib/lp/soyuz/configure.zcml (+1/-2)
lib/lp/soyuz/model/packagecopyjob.py (+27/-6)
lib/lp/soyuz/tests/test_packagecopyjob.py (+39/-0)
To merge this branch: bzr merge lp:~cjwatson/launchpad/copies-respect-new
Reviewer Review Type Date Requested Status
William Grant code 2014-05-30 Approve on 2014-05-30
Review via email: mp+221529@code.launchpad.net

Commit message

Send copies to NEW if they contain binaries without overrides.

Description of the change

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.

To post a comment you must log in.
Adam Conrad (adconrad) wrote :

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) wrote :

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) wrote :

> 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) wrote :

> 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) wrote :

(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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/soyuz/configure.zcml'
2--- lib/lp/soyuz/configure.zcml 2014-02-18 11:40:52 +0000
3+++ lib/lp/soyuz/configure.zcml 2014-05-30 12:14:46 +0000
4@@ -1,4 +1,4 @@
5-<!-- Copyright 2009-2013 Canonical Ltd. This software is licensed under the
6+<!-- Copyright 2009-2014 Canonical Ltd. This software is licensed under the
7 GNU Affero General Public License version 3 (see the file LICENSE).
8 -->
9
10@@ -199,7 +199,6 @@
11 <require
12 permission="launchpad.Edit"
13 attributes="
14- setNew
15 setUnapproved
16 setRejected
17 setAccepted
18
19=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
20--- lib/lp/soyuz/model/packagecopyjob.py 2014-05-15 08:49:44 +0000
21+++ lib/lp/soyuz/model/packagecopyjob.py 2014-05-30 12:14:46 +0000
22@@ -515,8 +515,7 @@
23 raise CannotCopy("Package %r %r not found." % (name, version))
24 return source_package
25
26- def _checkPolicies(self, source_name, source_component=None,
27- auto_approve=False):
28+ def _checkPolicies(self, source_name, source_package, auto_approve=False):
29 # This helper will only return if it's safe to carry on with the
30 # copy, otherwise it raises SuspendJobException to tell the job
31 # runner to suspend the job.
32@@ -532,7 +531,8 @@
33 # metadata.
34 defaults = UnknownOverridePolicy().calculateSourceOverrides(
35 self.target_archive, self.target_distroseries,
36- self.target_pocket, [source_name], source_component)
37+ self.target_pocket, [source_name],
38+ source_package.sourcepackagerelease.component)
39 self.addSourceOverride(defaults[0])
40 if auto_approve:
41 auto_approve = self.target_archive.canAdministerQueue(
42@@ -556,6 +556,29 @@
43 self.requester, self.getSourceOverride().component,
44 self.target_pocket, self.target_distroseries)
45
46+ # Go through the same routine for binary overrides if necessary. We
47+ # assume that the above permission check on the source component is
48+ # sufficient.
49+ if self.include_binaries:
50+ binaries = source_package.getBuiltBinaries()
51+ bpn_archtag = [(
52+ bpph.binarypackagerelease.binarypackagename,
53+ bpph.distroarchseries.architecturetag) for bpph in binaries]
54+ ancestry = override_policy.calculateBinaryOverrides(
55+ self.target_archive, self.target_distroseries,
56+ self.target_pocket, bpn_archtag)
57+ if len(ancestry) != len(binaries):
58+ # XXX cjwatson bug=1079577: Binary overrides for Ubuntu
59+ # should be calculated using the Ubuntu override policy, and
60+ # they should be saved in the metadata with an API for
61+ # overriding them.
62+ approve_new = auto_approve or copy_policy.autoApproveNew(
63+ self.target_archive, self.target_distroseries,
64+ self.target_pocket)
65+ if not approve_new:
66+ self._createPackageUpload()
67+ raise SuspendJobException
68+
69 # The package is not new (it has ancestry) so check the copy
70 # policy for existing packages.
71 approve_existing = auto_approve or copy_policy.autoApprove(
72@@ -643,9 +666,7 @@
73 [self.context.id]).any()
74 if pu is None:
75 source_name = getUtility(ISourcePackageNameSet)[self.package_name]
76- self._checkPolicies(
77- source_name, source_package.sourcepackagerelease.component,
78- self.auto_approve)
79+ self._checkPolicies(source_name, source_package, self.auto_approve)
80
81 # The package is free to go right in, so just copy it now.
82 ancestry = self.target_archive.getPublishedSources(
83
84=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
85--- lib/lp/soyuz/tests/test_packagecopyjob.py 2014-05-15 09:06:24 +0000
86+++ lib/lp/soyuz/tests/test_packagecopyjob.py 2014-05-30 12:14:46 +0000
87@@ -18,6 +18,7 @@
88 from zope.security.proxy import removeSecurityProxy
89
90 from lp.bugs.interfaces.bugtask import BugTaskStatus
91+from lp.buildmaster.enums import BuildStatus
92 from lp.registry.interfaces.pocket import PackagePublishingPocket
93 from lp.registry.interfaces.series import SeriesStatus
94 from lp.registry.model.distroseriesdifferencecomment import (
95@@ -1003,6 +1004,40 @@
96 [removeSecurityProxy(job).context.id]).one()
97 self.assertEqual(PackageUploadStatus.NEW, pu.status)
98
99+ def test_copying_new_binaries(self):
100+ # A copy involving source and new binaries requires NEW queue approval.
101+ target_archive = self.factory.makeArchive(
102+ self.distroseries.distribution, purpose=ArchivePurpose.PRIMARY)
103+ source_archive = self.factory.makeArchive()
104+ old_spph = self.publisher.getPubSource(
105+ distroseries=self.distroseries, sourcename="copyme", version="1.0",
106+ status=PackagePublishingStatus.PUBLISHED, archive=target_archive)
107+ self.publisher.getPubBinaries(
108+ binaryname="copyme", pub_source=old_spph,
109+ distroseries=self.distroseries,
110+ status=PackagePublishingStatus.PUBLISHED)
111+ new_spph = self.publisher.getPubSource(
112+ distroseries=self.distroseries, sourcename="copyme", version="1.1",
113+ status=PackagePublishingStatus.PUBLISHED, archive=source_archive)
114+ for build in new_spph.createMissingBuilds():
115+ build.updateStatus(BuildStatus.FULLYBUILT)
116+ for binaryname in ("copyme", "libcopyme-dev"):
117+ bpr = self.publisher.uploadBinaryForBuild(build, binaryname)
118+ self.publisher.publishBinaryInArchive(
119+ bpr, new_spph.archive,
120+ status=PackagePublishingStatus.PUBLISHED)
121+ pu = self.publisher.addPackageUpload(
122+ new_spph.archive, self.distroseries,
123+ changes_file_name="copyme_1.1_%s.changes" % build.arch_tag)
124+ pu.addBuild(build)
125+ job = self.createCopyJobForSPPH(
126+ new_spph, source_archive, target_archive, include_binaries=True)
127+ self.runJob(job)
128+ self.assertEqual(JobStatus.SUSPENDED, job.status)
129+ pu = getUtility(IPackageUploadSet).getByPackageCopyJobIDs(
130+ [removeSecurityProxy(job).context.id]).one()
131+ self.assertEqual(PackageUploadStatus.NEW, pu.status)
132+
133 def test_copying_to_main_archive_unapproved(self):
134 # Uploading to a series that is in a state that precludes auto
135 # approval will cause the job to suspend and a packageupload
136@@ -1302,6 +1337,10 @@
137 distroseries=self.distroseries, sourcename="copyme",
138 version="2.8-0", status=PackagePublishingStatus.PUBLISHED,
139 component='multiverse', section='web', archive=target_archive)
140+ self.publisher.getPubBinaries(
141+ binaryname="copyme", pub_source=old_spph,
142+ distroseries=self.distroseries,
143+ status=PackagePublishingStatus.PUBLISHED)
144 old_spr = old_spph.sourcepackagerelease
145 diff_file = self.publisher.addMockFile("diff_file", restricted=True)
146 package_diff = old_spr.requestDiffTo(target_archive.owner, spr)