launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #09327
[Merge] lp:~cjwatson/launchpad/copy-packages-with-default-series into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/copy-packages-with-default-series into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #795005 in Launchpad itself: "Asynchronous PPA copying with default series OOPSes"
https://bugs.launchpad.net/launchpad/+bug/795005
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/copy-packages-with-default-series/+merge/112557
== Summary ==
In various places related to copying packages, Launchpad documents that passing a target series of None means that the target series will be the same as the source series (but in the target archive, of course). However, this doesn't actually work everywhere and OOPSes (bug 795005).
== Proposed fix ==
The OOPS itself is in check_copy_permissions when series is None. That function's current interface takes a sequence of SourcePackageNames, and with that interface this bug is impossible to fix because we only do one permissions check per SPN even if that corresponds to the same source package in multiple series. However, all the call sites have convenient access to the SourcePackagePublishingHistory, so I changed the interface to take a sequence of SPPHs instead, making this straightforward.
I moved the bulk loading down from Archive.copyPackages to check_copy_permissions (I don't think there was currently any bulk loading happening when copying via the UI, which can't have helped matters), and added a bit more for good measure.
Once this is fixed, it also becomes necessary to ensure that the PCJs have the correct target series in their metadata, rather than leaving it as None and deferring that decision to when the PCJ is run, since otherwise calls such as getPendingJobsForTargetSeries won't work. I therefore went through all sites where PCJs are created and ensured that they handle the target series being None where applicable.
== LOC Rationale ==
+59. This is part of allowing us to remove synchronous copying from the PPA UI, as well as eventually removing delayed copies. Besides, I have 2976 lines of credit.
== Tests ==
bin/test -vvct test_copyPackage -t test_packagecopyjob
I haven't explicitly added browser tests here. This is mainly because I actually ran into this bug while working on another branch to remove synchronous copying from the PPA UI, which caused a failure in lib/lp/soyuz/browser/tests/archive-views.txt attributable to this bug. So, while that isn't tested right now, it soon will be.
== Demo and Q/A ==
Find a PPA with packages in several series, create a new temporary PPA, and use Archive.copyPackages with the to_series argument omitted to copy everything from the first to the second (with include_binaries=True to avoid unnecessary rebuilds). It should work with all the series information copied over.
Get an admin to set the soyuz.derived_series.max_synchronous_syncs feature flag to something small like 5 on qastaging, and try the same thing, only this time using the +copy-packages web UI. You'll need to copy >5 (or whatever you set) packages for this to work.
== Lint ==
Just one false positive:
./lib/lp/soyuz/model/archive.py
347: redefinition of function 'private' from line 343
--
https://code.launchpad.net/~cjwatson/launchpad/copy-packages-with-default-series/+merge/112557
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/copy-packages-with-default-series into lp:launchpad.
=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py 2012-01-17 21:45:24 +0000
+++ lib/lp/registry/browser/distroseries.py 2012-06-28 12:55:22 +0000
@@ -1293,12 +1293,12 @@
dsd.parent_source_version,
dsd.parent_series.main_archive,
target_distroseries.main_archive,
+ target_distroseries,
PackagePublishingPocket.RELEASE,
)
for dsd in self.getUpgrades()]
getUtility(IPlainPackageCopyJobSource).createMultiple(
- target_distroseries, copies, self.user,
- copy_policy=PackageCopyPolicy.MASS_SYNC)
+ copies, self.user, copy_policy=PackageCopyPolicy.MASS_SYNC)
self.request.response.addInfoNotification(
(u"Upgrades of {context.displayname} packages have been "
=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py 2012-06-14 03:22:43 +0000
+++ lib/lp/soyuz/browser/archive.py 2012-06-28 12:55:22 +0000
@@ -1311,14 +1311,13 @@
not permitted.
"""
if check_permissions:
- spns = [
- spph.sourcepackagerelease.sourcepackagename
- for spph in source_pubs]
check_copy_permissions(
- person, dest_archive, dest_series, dest_pocket, spns)
+ person, dest_archive, dest_series, dest_pocket, source_pubs)
job_source = getUtility(IPlainPackageCopyJobSource)
for spph in source_pubs:
+ if dest_series is None:
+ dest_series = spph.distroseries
job_source.create(
spph.source_package_name, spph.archive, dest_archive, dest_series,
dest_pocket, include_binaries=include_binaries,
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py 2012-06-19 23:49:20 +0000
+++ lib/lp/soyuz/model/archive.py 2012-06-28 12:55:22 +0000
@@ -72,7 +72,6 @@
from lp.registry.model.sourcepackagename import SourcePackageName
from lp.registry.model.teammembership import TeamParticipation
from lp.services.config import config
-from lp.services.database.bulk import load_related
from lp.services.database.constants import UTC_NOW
from lp.services.database.datetimecol import UtcDateTimeCol
from lp.services.database.decoratedresultset import DecoratedResultSet
@@ -1641,11 +1640,12 @@
series = self._text_to_series(to_series)
# Upload permission checks, this will raise CannotCopy as
# necessary.
- sourcepackagename = getUtility(ISourcePackageNameSet)[source_name]
- check_copy_permissions(
- person, self, series, pocket, [sourcepackagename])
+ source = self._validateAndFindSource(
+ from_archive, source_name, version)
+ if series is None:
+ series = source.distroseries
+ check_copy_permissions(person, self, series, pocket, [source])
- self._validateAndFindSource(from_archive, source_name, version)
job_source = getUtility(IPlainPackageCopyJobSource)
job_source.create(
package_name=source_name, source_archive=from_archive,
@@ -1667,20 +1667,10 @@
raise CannotCopy(
"None of the supplied package names are published")
- # Bulk-load the sourcepackagereleases so that the list
- # comprehension doesn't generate additional queries. The
- # sourcepackagenames themselves will already have been loaded when
- # generating the list of source publications in "sources".
- load_related(
- SourcePackageRelease, sources, ["sourcepackagereleaseID"])
- sourcepackagenames = [source.sourcepackagerelease.sourcepackagename
- for source in sources]
-
# Now do a mass check of permissions.
pocket = self._text_to_pocket(to_pocket)
series = self._text_to_series(to_series)
- check_copy_permissions(
- person, self, series, pocket, sourcepackagenames)
+ check_copy_permissions(person, self, series, pocket, sources)
# If we get this far then we can create the PackageCopyJob.
copy_tasks = []
@@ -1690,14 +1680,14 @@
source.sourcepackagerelease.version,
from_archive,
self,
+ series if series is not None else source.distroseries,
PackagePublishingPocket.RELEASE
)
copy_tasks.append(task)
job_source = getUtility(IPlainPackageCopyJobSource)
job_source.createMultiple(
- series, copy_tasks, person,
- copy_policy=PackageCopyPolicy.MASS_SYNC,
+ copy_tasks, person, copy_policy=PackageCopyPolicy.MASS_SYNC,
include_binaries=include_binaries, sponsored=sponsored,
unembargo=unembargo)
=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py 2012-06-24 01:56:36 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py 2012-06-28 12:55:22 +0000
@@ -287,9 +287,8 @@
return derived
@classmethod
- def _composeJobInsertionTuple(cls, target_distroseries, copy_policy,
- include_binaries, job_id, copy_task,
- sponsored, unembargo):
+ def _composeJobInsertionTuple(cls, copy_policy, include_binaries, job_id,
+ copy_task, sponsored, unembargo):
"""Create an SQL fragment for inserting a job into the database.
:return: A string representing an SQL tuple containing initializers
@@ -301,6 +300,7 @@
package_version,
source_archive,
target_archive,
+ target_distroseries,
target_pocket,
) = copy_task
metadata = cls._makeMetadata(
@@ -313,7 +313,7 @@
return data
@classmethod
- def createMultiple(cls, target_distroseries, copy_tasks, requester,
+ def createMultiple(cls, copy_tasks, requester,
copy_policy=PackageCopyPolicy.INSECURE,
include_binaries=False, sponsored=None,
unembargo=False):
@@ -322,8 +322,8 @@
job_ids = Job.createMultiple(store, len(copy_tasks), requester)
job_contents = [
cls._composeJobInsertionTuple(
- target_distroseries, copy_policy, include_binaries, job_id,
- task, sponsored, unembargo)
+ copy_policy, include_binaries, job_id, task, sponsored,
+ unembargo)
for job_id, task in zip(job_ids, copy_tasks)]
return bulk.create(
(PackageCopyJob.job_type, PackageCopyJob.target_distroseries,
=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py 2012-06-27 17:20:45 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py 2012-06-28 12:55:22 +0000
@@ -16,6 +16,7 @@
'update_files_privacy',
]
+from operator import attrgetter
import os
import tempfile
@@ -26,6 +27,7 @@
from lp.app.errors import NotFoundError
from lp.buildmaster.enums import BuildStatus
+from lp.services.database.bulk import load_related
from lp.services.librarian.interfaces import ILibraryFileAliasSet
from lp.services.librarian.utils import copy_and_close
from lp.soyuz.adapters.notification import notify
@@ -204,39 +206,64 @@
return {'status': BuildSetStatus.NEEDSBUILD}
-def check_copy_permissions(person, archive, series, pocket,
- sourcepackagenames):
+def check_copy_permissions(person, archive, series, pocket, sources):
"""Check that `person` has permission to copy a package.
:param person: User attempting the upload.
:param archive: Destination `Archive`.
:param series: Destination `DistroSeries`.
:param pocket: Destination `Pocket`.
- :param sourcepackagenames: Sequence of `SourcePackageName`s for the
+ :param sources: Sequence of `SourcePackagePublishingHistory`s for the
packages to be copied.
:raises CannotCopy: If the copy is not allowed.
"""
+ # Circular imports.
+ from lp.registry.model.distroseries import DistroSeries
+ from lp.registry.model.sourcepackagename import SourcePackageName
+ from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
+
if person is None:
raise CannotCopy("Cannot check copy permissions (no requester).")
+ # Bulk-load the data we'll need from each source publication.
+ load_related(SourcePackageRelease, sources, ["sourcepackagereleaseID"])
+ load_related(DistroSeries, sources, ["distroseriesID"])
+ load_related(
+ SourcePackageName, map(attrgetter("sourcepackagerelease"), sources),
+ ["sourcepackagenameID"])
+
# If there is a requester, check that he has upload permission into
# the destination (archive, component, pocket). This check is done
# here rather than in the security adapter because it requires more
# info than is available in the security adapter.
- for spn in set(sourcepackagenames):
- package = series.getSourcePackage(spn)
- destination_component = package.latest_published_component
-
- # If destination_component is not None, make sure the person
- # has upload permission for this component. Otherwise, any
- # upload permission on this archive will do.
- strict_component = destination_component is not None
- reason = archive.checkUpload(
- person, series, spn, destination_component, pocket,
- strict_component=strict_component)
-
- if reason is not None:
- raise CannotCopy(reason)
+ if series is None:
+ # Use each source's series as the destination for that source.
+ for source in sources:
+ reason = archive.checkUpload(
+ person, source.distroseries,
+ source.sourcepackagerelease.sourcepackagename,
+ source.component, pocket, strict_component=True)
+
+ if reason is not None:
+ raise CannotCopy(reason)
+ else:
+ sourcepackagenames = [
+ source.sourcepackagerelease.sourcepackagename
+ for source in sources]
+ for spn in set(sourcepackagenames):
+ package = series.getSourcePackage(spn)
+ destination_component = package.latest_published_component
+
+ # If destination_component is not None, make sure the person
+ # has upload permission for this component. Otherwise, any
+ # upload permission on this archive will do.
+ strict_component = destination_component is not None
+ reason = archive.checkUpload(
+ person, series, spn, destination_component, pocket,
+ strict_component=strict_component)
+
+ if reason is not None:
+ raise CannotCopy(reason)
class CopyChecker:
@@ -463,8 +490,7 @@
"""
if check_permissions:
check_copy_permissions(
- person, self.archive, series, pocket,
- [source.sourcepackagerelease.sourcepackagename])
+ person, self.archive, series, pocket, [source])
if series not in self.archive.distribution.series:
raise CannotCopy(
=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py 2012-06-19 23:49:20 +0000
+++ lib/lp/soyuz/tests/test_archive.py 2012-06-28 12:55:22 +0000
@@ -2368,6 +2368,22 @@
self.assertEqual(target_archive, copy_job.target_archive)
self.assertTrue(copy_job.unembargo)
+ def test_copyPackage_with_default_distroseries(self):
+ # If to_series is None, copyPackage copies into the same series as
+ # the source in the target archive.
+ (source, source_archive, source_name, target_archive, to_pocket,
+ to_series, version) = self._setup_copy_data()
+ 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)
+
+ # There should be one copy job with the correct target series.
+ job_source = getUtility(IPlainPackageCopyJobSource)
+ copy_jobs = job_source.getActiveJobs(target_archive)
+ self.assertEqual(1, copy_jobs.count())
+ self.assertEqual(source.distroseries, copy_jobs[0].target_distroseries)
+
def test_copyPackages_with_single_package(self):
(source, source_archive, source_name, target_archive, to_pocket,
to_series, version) = self._setup_copy_data()
@@ -2505,6 +2521,33 @@
self.assertEqual(copy_job, copy_jobs[0])
self.assertEqual(new_version, copy_jobs[1].package_version)
+ def test_copyPackages_with_default_distroseries(self):
+ # If to_series is None, copyPackages copies into the same series as
+ # each source in the target archive.
+ (source, source_archive, source_name, target_archive, to_pocket,
+ to_series, version) = self._setup_copy_data()
+ sources = [source]
+ other_series = self.factory.makeDistroSeries(
+ distribution=target_archive.distribution)
+ sources.append(self.factory.makeSourcePackagePublishingHistory(
+ distroseries=other_series, archive=source_archive,
+ status=PackagePublishingStatus.PUBLISHED))
+ names = [source.sourcepackagerelease.sourcepackagename.name
+ for source in sources]
+
+ with person_logged_in(target_archive.owner):
+ target_archive.copyPackages(
+ names, source_archive, to_pocket.name, include_binaries=False,
+ person=target_archive.owner)
+
+ # There should be two copy jobs with the correct target series.
+ job_source = getUtility(IPlainPackageCopyJobSource)
+ copy_jobs = job_source.getActiveJobs(target_archive)
+ self.assertEqual(2, copy_jobs.count())
+ self.assertContentEqual(
+ [source.distroseries for source in sources],
+ [copy_job.target_distroseries for copy_job in copy_jobs])
+
class TestgetAllPublishedBinaries(TestCaseWithFactory):
=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py 2012-06-27 00:49:47 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py 2012-06-28 12:55:22 +0000
@@ -243,6 +243,7 @@
"1.5mother1",
mother.parent_series.main_archive,
derived_series.main_archive,
+ derived_series,
PackagePublishingPocket.RELEASE,
),
(
@@ -250,12 +251,11 @@
"0.9father1",
father.parent_series.main_archive,
derived_series.main_archive,
+ derived_series,
PackagePublishingPocket.UPDATES,
),
]
- job_ids = list(
- job_source.createMultiple(mother.derived_series, copy_tasks,
- requester))
+ job_ids = list(job_source.createMultiple(copy_tasks, requester))
jobs = list(job_source.getActiveJobs(derived_series.main_archive))
self.assertContentEqual(job_ids, [job.id for job in jobs])
self.assertEqual(len(copy_tasks), len(set([job.job for job in jobs])))
@@ -269,6 +269,7 @@
job.package_version,
job.source_archive,
job.target_archive,
+ job.target_distroseries,
job.target_pocket,
)
for job in jobs]
Follow ups
