launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13625
[Merge] lp:~cjwatson/launchpad/copy-explicit-pocket into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/copy-explicit-pocket into lp:launchpad.
Commit message:
Allow specifying source distroseries and pocket in Archive.copyPackage and in PackageCopyJob metadata.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1070324 in Launchpad itself: "Incremental copying of binaries doesn't work due to lack of explicit series/pocket selection"
https://bugs.launchpad.net/launchpad/+bug/1070324
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/copy-explicit-pocket/+merge/131002
== 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.
--
https://code.launchpad.net/~cjwatson/launchpad/copy-explicit-pocket/+merge/131002
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/copy-explicit-pocket into lp:launchpad.
=== 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.
Follow ups
