launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04042
[Merge] lp:~jtv/launchpad/pre-800634-800652 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/pre-800634-800652 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #800634 in Launchpad itself: "+queue page doesn't show the overrides for syncs"
https://bugs.launchpad.net/launchpad/+bug/800634
Bug #800652 in Launchpad itself: "+queue page broken for accepting syncs"
https://bugs.launchpad.net/launchpad/+bug/800652
For more details, see:
https://code.launchpad.net/~jtv/launchpad/pre-800634-800652/+merge/65657
= Summary =
This lays some groundwork for another branch I have in flight. It introduces IPackageCopyJobSource, which can take a PackageCopyJob and give you a full-fledged IPackageCopyJob implementation of the right concrete type. The only concrete implementation is PlainPackageCopyJob at the moment, but hard-coding that wasn't actually any easier than generalizing it a tiny bit.
I also ran into some circular imports, and even though it's not the minimal change, chose lp.soyuz.adapters.overrides as the place to cut the cycles. It just seems to me that importing lp.soyuz.model classes into an lp.soyuz.adapters module is in some vague way swimming against the flow.
== Tests ==
I ran more, but this is the main one:
{{{
./bin/test -vvc lp.soyuz.tests.test_packagecopyjob
}}}
== Demo and Q/A ==
Will only be interesting as part of my other branch, which fixes up the display of overrides for sync jobs.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/soyuz/model/queue.py
lib/lp/soyuz/interfaces/queue.py
lib/lp/soyuz/model/packagecopyjob.py
lib/lp/soyuz/configure.zcml
lib/lp/soyuz/interfaces/packagecopyjob.py
lib/lp/soyuz/tests/test_packagecopyjob.py
lib/lp/soyuz/adapters/overrides.py
--
https://code.launchpad.net/~jtv/launchpad/pre-800634-800652/+merge/65657
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/pre-800634-800652 into lp:launchpad.
=== modified file 'lib/lp/soyuz/adapters/overrides.py'
--- lib/lp/soyuz/adapters/overrides.py 2011-06-05 08:17:40 +0000
+++ lib/lp/soyuz/adapters/overrides.py 2011-06-23 12:57:43 +0000
@@ -40,13 +40,7 @@
from lp.soyuz.model.binarypackagename import BinaryPackageName
from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
from lp.soyuz.model.component import Component
-from lp.soyuz.model.distroarchseries import DistroArchSeries
-from lp.soyuz.model.publishing import (
- BinaryPackagePublishingHistory,
- SourcePackagePublishingHistory,
- )
from lp.soyuz.model.section import Section
-from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
class IOverride(Interface):
@@ -130,7 +124,7 @@
def __repr__(self):
return ("<BinaryOverride at %x component=%r section=%r "
- "binary_package_name=%r distro_arch_series=%r priority=%r>" %
+ "binary_package_name=%r distro_arch_series=%r priority=%r>" %
(id(self), self.component, self.section, self.binary_package_name,
self.distro_arch_series, self.priority))
@@ -187,7 +181,7 @@
class FromExistingOverridePolicy(BaseOverridePolicy):
"""Override policy that only searches for existing publications.
-
+
Override policy that returns the SourcePackageName, component and
section for the latest published source publication, or the
BinaryPackageName, DistroArchSeries, component, section and priority
@@ -195,10 +189,15 @@
"""
def calculateSourceOverrides(self, archive, distroseries, pocket, spns):
- store = IStore(SourcePackagePublishingHistory)
+ # Avoid circular imports.
+ from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
+ from lp.soyuz.model.publishing import SourcePackagePublishingHistory
+
def eager_load(rows):
bulk.load(Component, (row[1] for row in rows))
bulk.load(Section, (row[2] for row in rows))
+
+ store = IStore(SourcePackagePublishingHistory)
already_published = DecoratedResultSet(
store.find(
(SourcePackageRelease.sourcepackagenameID,
@@ -226,10 +225,15 @@
def calculateBinaryOverrides(self, archive, distroseries, pocket,
binaries):
- store = IStore(BinaryPackagePublishingHistory)
+ # Avoid circular imports.
+ from lp.soyuz.model.distroarchseries import DistroArchSeries
+ from lp.soyuz.model.publishing import BinaryPackagePublishingHistory
+
def eager_load(rows):
bulk.load(Component, (row[2] for row in rows))
bulk.load(Section, (row[3] for row in rows))
+
+ store = IStore(BinaryPackagePublishingHistory)
expanded = calculate_target_das(distroseries, binaries)
candidates = (
make_package_condition(archive, das, bpn)
@@ -266,11 +270,11 @@
class UnknownOverridePolicy(BaseOverridePolicy):
"""Override policy that returns defaults.
-
+
Override policy that assumes everything passed in doesn't exist, so
returns the defaults.
"""
-
+
def calculateSourceOverrides(self, archive, distroseries, pocket,
sources):
default_component = archive.default_component or getUtility(
@@ -291,9 +295,9 @@
class UbuntuOverridePolicy(FromExistingOverridePolicy,
UnknownOverridePolicy):
"""Override policy for Ubuntu.
-
- An override policy that incorporates both the from existing policy
- and the unknown policy.
+
+ An override policy that incorporates both the existing policy and the
+ unknown policy.
"""
def calculateSourceOverrides(self, archive, distroseries, pocket,
@@ -314,9 +318,12 @@
total = set(binaries)
overrides = FromExistingOverridePolicy.calculateBinaryOverrides(
self, archive, distroseries, pocket, binaries)
- existing = set((
- overide.binary_package_name, overide.distro_arch_series.architecturetag)
- for overide in overrides)
+ existing = set(
+ (
+ overide.binary_package_name,
+ overide.distro_arch_series.architecturetag,
+ )
+ for overide in overrides)
missing = total.difference(existing)
if missing:
unknown = UnknownOverridePolicy.calculateBinaryOverrides(
@@ -340,6 +347,8 @@
def make_package_condition(archive, das, bpn):
+ # Avoid circular imports.
+ from lp.soyuz.model.publishing import BinaryPackagePublishingHistory
return And(
BinaryPackagePublishingHistory.archiveID == archive.id,
BinaryPackagePublishingHistory.distroarchseriesID == das.id,
@@ -347,9 +356,13 @@
def id_resolver(lookups):
+ # Avoid circular imports.
+ from lp.soyuz.model.publishing import SourcePackagePublishingHistory
+
def _resolve(row):
store = IStore(SourcePackagePublishingHistory)
return tuple(
(value if cls is None else store.get(cls, value))
for value, cls in zip(row, lookups))
+
return _resolve
=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml 2011-06-15 02:41:34 +0000
+++ lib/lp/soyuz/configure.zcml 2011-06-23 12:57:43 +0000
@@ -914,6 +914,11 @@
<!-- PackageCopyJobSource -->
<securedutility
+ component=".model.packagecopyjob.PackageCopyJob"
+ provides=".interfaces.packagecopyjob.IPackageCopyJobSource">
+ <allow interface=".interfaces.packagecopyjob.IPackageCopyJobSource"/>
+ </securedutility>
+ <securedutility
component=".model.packagecopyjob.PlainPackageCopyJob"
provides=".interfaces.packagecopyjob.IPlainPackageCopyJobSource">
<allow interface=".interfaces.packagecopyjob.IPlainPackageCopyJobSource"/>
=== modified file 'lib/lp/soyuz/interfaces/packagecopyjob.py'
--- lib/lp/soyuz/interfaces/packagecopyjob.py 2011-06-14 06:01:14 +0000
+++ lib/lp/soyuz/interfaces/packagecopyjob.py 2011-06-23 12:57:43 +0000
@@ -5,6 +5,7 @@
__all__ = [
"IPackageCopyJob",
+ "IPackageCopyJobSource",
"IPlainPackageCopyJob",
"IPlainPackageCopyJobSource",
"PackageCopyJobType",
@@ -37,6 +38,21 @@
from lp.soyuz.interfaces.archive import IArchive
+class IPackageCopyJobSource(Interface):
+ """Utility for `IPackageCopyJob`-implementing types."""
+
+ def wrap(package_copy_job):
+ """Wrap a `PackageCopyJob` in its concrete implementation type.
+
+ As a special case, `None` produces `None`.
+
+ :param package_copy_job: A `PackageCopyJob`.
+ :return: An `IPackageCopyJob` implementation based on
+ `package_copy_job`, but of the job's specific concrete type
+ (such as `PlainPackageCopyJob`).
+ """
+
+
class IPackageCopyJob(Interface):
"""A job that copies packages between `IArchive`s."""
@@ -74,6 +90,12 @@
schema=IJob, title=_('The common Job attributes'),
required=True, readonly=True)
+ component_name = TextLine(
+ title=_("Component override name"), required=False, readonly=True)
+
+ section_name = TextLine(
+ title=_("Section override name"), required=False, readonly=True)
+
metadata = Attribute('A dict of data about the job.')
=== modified file 'lib/lp/soyuz/interfaces/queue.py'
--- lib/lp/soyuz/interfaces/queue.py 2011-06-17 05:23:56 +0000
+++ lib/lp/soyuz/interfaces/queue.py 2011-06-23 12:57:43 +0000
@@ -140,6 +140,11 @@
package_copy_job = Reference(
schema=IPackageCopyJob,
description=_("The PackageCopyJob for this upload, if it has one."),
+ title=_("Raw Package Copy Job"), required=False, readonly=True)
+
+ concrete_package_copy_job = Reference(
+ schema=IPackageCopyJob,
+ description=_("Concrete IPackageCopyJob implementation, if any."),
title=_("Package Copy Job"), required=False, readonly=True)
archive = exported(
=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py 2011-06-16 13:21:14 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py 2011-06-23 12:57:43 +0000
@@ -22,6 +22,7 @@
classProvides,
implements,
)
+from zope.security.proxy import removeSecurityProxy
from canonical.database.enumcol import EnumCol
from canonical.database.sqlbase import sqlvalues
@@ -61,6 +62,7 @@
from lp.soyuz.interfaces.copypolicy import ICopyPolicy
from lp.soyuz.interfaces.packagecopyjob import (
IPackageCopyJob,
+ IPackageCopyJobSource,
IPlainPackageCopyJob,
IPlainPackageCopyJobSource,
PackageCopyJobType,
@@ -75,6 +77,7 @@
"""Base class for package copying jobs."""
implements(IPackageCopyJob)
+ classProvides(IPackageCopyJobSource)
__storm_table__ = 'PackageCopyJob'
@@ -99,6 +102,31 @@
_json_data = Unicode('json_data')
+ # Derived concrete classes. The entire class gets one dict for
+ # this; it's not meant to be on an instance.
+ concrete_classes = {}
+
+ @classmethod
+ def registerConcreteClass(cls, new_class):
+ """Register a concrete `IPackageCopyJob`-implementing class."""
+ assert new_class.class_job_type not in cls.concrete_classes, (
+ "Class %s is already registered." % new_class)
+ cls.concrete_classes[new_class.class_job_type] = new_class
+
+ @classmethod
+ def wrap(cls, package_copy_job):
+ """See `IPackageCopyJobSource`."""
+ if package_copy_job is None:
+ return None
+ job_type = removeSecurityProxy(package_copy_job).job_type
+ concrete_class = cls.concrete_classes[job_type]
+ return concrete_class(package_copy_job)
+
+ @classmethod
+ def getByID(cls, pcj_id):
+ """See `IPackageCopyJobSource`."""
+ return cls.wrap(IStore(PackageCopyJob).get(PackageCopyJob, pcj_id))
+
def __init__(self, source_archive, target_archive, target_distroseries,
job_type, metadata, package_name=None, copy_policy=None):
super(PackageCopyJob, self).__init__()
@@ -130,6 +158,16 @@
existing.update(metadata_dict)
self._json_data = self.serializeMetadata(existing)
+ @property
+ def component_name(self):
+ """See `IPackageCopyJob`."""
+ return self.metadata.get("component_override")
+
+ @property
+ def section_name(self):
+ """See `IPackageCopyJob`."""
+ return self.metadata.get("section_override")
+
class PackageCopyJobDerived(BaseRunnableJob):
"""Abstract class for deriving from PackageCopyJob."""
@@ -339,13 +377,13 @@
metadata_dict = dict(
component_override=component,
section_override=section)
- self.context.extendMetadata(metadata_dict)
+ removeSecurityProxy(self.context).extendMetadata(metadata_dict)
def getSourceOverride(self):
"""Fetch an `ISourceOverride` from the metadata."""
name = self.package_name
- component_name = self.metadata.get("component_override")
- section_name = self.metadata.get("section_override")
+ component_name = self.component_name
+ section_name = self.section_name
source_package_name = getUtility(ISourcePackageNameSet)[name]
try:
component = getUtility(IComponentSet)[component_name]
@@ -508,3 +546,6 @@
def getPolicyImplementation(self):
"""Return the `ICopyPolicy` applicable to this job."""
return ICopyPolicy(self.copy_policy)
+
+
+PackageCopyJob.registerConcreteClass(PlainPackageCopyJob)
=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py 2011-06-22 12:03:31 +0000
+++ lib/lp/soyuz/model/queue.py 2011-06-23 12:57:43 +0000
@@ -65,12 +65,14 @@
from lp.registry.model.sourcepackagename import SourcePackageName
from lp.services.mail.signedmessage import strip_pgp_signature
from lp.services.propertycache import cachedproperty
+from lp.soyuz.adapters.overrides import SourceOverride
from lp.soyuz.adapters.notification import notify
from lp.soyuz.enums import (
PackageUploadCustomFormat,
PackageUploadStatus,
)
from lp.soyuz.interfaces.archive import MAIN_ARCHIVE_PURPOSES
+from lp.soyuz.interfaces.packagecopyjob import IPackageCopyJobSource
from lp.soyuz.interfaces.publishing import (
IPublishingSet,
ISourcePackagePublishingHistory,
@@ -853,6 +855,11 @@
existing_components.add(binary.component)
return existing_components
+ @cachedproperty
+ def concrete_package_copy_job(self):
+ """See `IPackageUpload`."""
+ return getUtility(IPackageCopyJobSource).wrap(self.package_copy_job)
+
def overrideSource(self, new_component, new_section, allowed_components):
"""See `IPackageUpload`."""
if new_component is None and new_section is None:
@@ -860,15 +867,9 @@
return False
if self.package_copy_job is not None:
- # We just need to add the required component/section to the
- # job metadata.
- extra_data = {}
- if new_component is not None:
- extra_data['component_override'] = new_component.name
- if new_section is not None:
- extra_data['section_override'] = new_section.name
- self.package_copy_job.extendMetadata(extra_data)
- return
+ self.concrete_package_copy_job.addSourceOverride(SourceOverride(
+ self.package_name, new_component, new_section))
+ return True
if not self.contains_source:
return False
=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py 2011-06-10 11:17:39 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py 2011-06-23 12:57:43 +0000
@@ -17,7 +17,10 @@
from canonical.config import config
from canonical.launchpad.interfaces.lpstorm import IStore
from canonical.launchpad.webapp.testing import verifyObject
-from canonical.testing import LaunchpadZopelessLayer
+from canonical.testing import (
+ LaunchpadZopelessLayer,
+ ZopelessDatabaseLayer,
+ )
from lp.registry.model.distroseriesdifferencecomment import (
DistroSeriesDifferenceComment,
)
@@ -43,6 +46,7 @@
from lp.soyuz.interfaces.component import IComponentSet
from lp.soyuz.interfaces.packagecopyjob import (
IPackageCopyJob,
+ IPackageCopyJobSource,
IPlainPackageCopyJob,
IPlainPackageCopyJobSource,
)
@@ -52,12 +56,14 @@
from lp.soyuz.interfaces.sourcepackageformat import (
ISourcePackageFormatSelectionSet,
)
+from lp.soyuz.model.packagecopyjob import PackageCopyJob
from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
from lp.testing import (
run_script,
TestCaseWithFactory,
)
from lp.testing.mail_helpers import pop_notifications
+from lp.testing.matchers import Provides
from lp.testing.fakemethod import FakeMethod
@@ -298,7 +304,7 @@
archive=breezy_archive)
# The target archive needs ancestry so the package is
# auto-accepted.
- ancestry = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=target_series, sourcename="libc",
version="2.8-0", status=PackagePublishingStatus.PUBLISHED,
archive=target_archive)
@@ -489,7 +495,7 @@
# Publish a package in the source archive with some overridable
# properties set to known values.
- source_package = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=distroseries, sourcename="libc",
component='universe', section='web',
version="2.8-1", status=PackagePublishingStatus.PUBLISHED,
@@ -497,7 +503,7 @@
# Now put the same named package in the target archive with
# different override values.
- ancestry_package = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=distroseries, sourcename="libc",
component='restricted', section='games',
version="2.8-0", status=PackagePublishingStatus.PUBLISHED,
@@ -594,7 +600,7 @@
# Publish a package in the source archive with some overridable
# properties set to known values.
- source_package = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=distroseries, sourcename="copyme",
component='multiverse', section='web',
version="2.8-1", status=PackagePublishingStatus.PUBLISHED,
@@ -645,7 +651,7 @@
source_archive = self.factory.makeArchive()
# Publish a package in the source archive.
- source_package = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=distroseries, sourcename="copyme",
version="2.8-1", status=PackagePublishingStatus.PUBLISHED,
component='multiverse', section='web',
@@ -653,7 +659,7 @@
# Now put the same named package in the target archive so it has
# ancestry.
- ancestry_package = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=distroseries, sourcename="copyme",
version="2.8-0", status=PackagePublishingStatus.PUBLISHED,
component='main', section='games',
@@ -698,7 +704,7 @@
source_archive = self.factory.makeArchive()
# Publish a package in the source archive.
- source_package = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=distroseries, sourcename="copyme",
version="2.8-1", status=PackagePublishingStatus.PUBLISHED,
archive=source_archive)
@@ -838,3 +844,25 @@
transaction.commit()
self.layer.switchDbUser(self.dbuser)
removeSecurityProxy(job).reportFailure(CannotCopy("Mommy it hurts"))
+
+
+class TestPackageCopyJobSource(TestCaseWithFactory):
+ """Test the `IPackageCopyJob` utility."""
+
+ layer = ZopelessDatabaseLayer
+
+ def test_implements_interface(self):
+ job_source = getUtility(IPackageCopyJobSource)
+ self.assertThat(job_source, Provides(IPackageCopyJobSource))
+
+ def test_wrap_accepts_None(self):
+ job_source = getUtility(IPackageCopyJobSource)
+ self.assertIs(None, job_source.wrap(None))
+
+ def test_wrap_wraps_PlainPackageCopyJob(self):
+ original_ppcj = self.factory.makePlainPackageCopyJob()
+ IStore(PackageCopyJob).flush()
+ pcj = IStore(PackageCopyJob).get(PackageCopyJob, original_ppcj.id)
+ self.assertNotEqual(None, pcj)
+ job_source = getUtility(IPackageCopyJobSource)
+ self.assertEqual(original_ppcj, job_source.wrap(pcj))
