Code review comment for lp:~michael.nelson/launchpad/567922-binarypackagebuild-new-table-1
- 567922-binarypackagebuild-new-table-1
- Merge into db-devel
Revision history for this message
| Michael Nelson (michael.nelson) wrote : | # |
| 1 | === modified file 'lib/lp/buildmaster/interfaces/packagebuild.py' |
| 2 | --- lib/lp/buildmaster/interfaces/packagebuild.py 2010-04-29 15:20:09 +0000 |
| 3 | +++ lib/lp/buildmaster/interfaces/packagebuild.py 2010-05-04 12:07:35 +0000 |
| 4 | @@ -6,7 +6,6 @@ |
| 5 | __all__ = [ |
| 6 | 'IPackageBuild', |
| 7 | 'IPackageBuildSource', |
| 8 | - 'IPackageBuildDerived', |
| 9 | ] |
| 10 | |
| 11 | |
| 12 | @@ -19,6 +18,7 @@ |
| 13 | from canonical.launchpad.interfaces.librarian import ILibraryFileAlias |
| 14 | from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob |
| 15 | from lp.registry.interfaces.distribution import IDistribution |
| 16 | +from lp.registry.interfaces.distroseries import IDistroSeries |
| 17 | from lp.registry.interfaces.pocket import PackagePublishingPocket |
| 18 | from lp.soyuz.interfaces.archive import IArchive |
| 19 | |
| 20 | @@ -76,6 +76,12 @@ |
| 21 | title=_("Distribution"), required=True, |
| 22 | description=_("Shortcut for its distribution."))) |
| 23 | |
| 24 | + distro_series = exported( |
| 25 | + Reference( |
| 26 | + schema=IDistroSeries, |
| 27 | + title=_("Distribution series"), required=True, |
| 28 | + description=_("Shortcut for its distribution series."))) |
| 29 | + |
| 30 | def getUploaderCommand(distro_series, upload_leaf, uploader_logfilename): |
| 31 | """Get the command to run as the uploader. |
| 32 | |
| 33 | @@ -138,6 +144,13 @@ |
| 34 | upload log. |
| 35 | """ |
| 36 | |
| 37 | + def handleStatus(status, librarian, slave_status): |
| 38 | + """Handle a finished build status from a slave. |
| 39 | + |
| 40 | + :param status: Slave build status string with 'BuildStatus.' stripped. |
| 41 | + :param slave_status: A dict as returned by IBuilder.slaveStatus |
| 42 | + """ |
| 43 | + |
| 44 | |
| 45 | class IPackageBuildSource(Interface): |
| 46 | """A utility of this interface used to create _things_.""" |
| 47 | @@ -149,15 +162,3 @@ |
| 48 | :param pocket: An item of `PackagePublishingPocket`. |
| 49 | :param dependencies: An optional debian-like dependency line. |
| 50 | """ |
| 51 | - |
| 52 | - |
| 53 | -class IPackageBuildDerived(Interface): |
| 54 | - """Classes deriving from IPackageBuild inherit the default handleStatus. |
| 55 | - """ |
| 56 | - def handleStatus(status, librarian, slave_status): |
| 57 | - """Handle a finished build status from a slave. |
| 58 | - |
| 59 | - :param status: Slave build status string with 'BuildStatus.' stripped. |
| 60 | - :param slave_status: A dict as returned by IBuilder.slaveStatus |
| 61 | - """ |
| 62 | - |
| 63 | |
| 64 | === modified file 'lib/lp/buildmaster/model/packagebuild.py' |
| 65 | --- lib/lp/buildmaster/model/packagebuild.py 2010-04-30 16:12:05 +0000 |
| 66 | +++ lib/lp/buildmaster/model/packagebuild.py 2010-05-04 12:07:35 +0000 |
| 67 | @@ -23,7 +23,7 @@ |
| 68 | from lp.buildmaster.interfaces.buildbase import BuildStatus |
| 69 | from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSource |
| 70 | from lp.buildmaster.interfaces.packagebuild import ( |
| 71 | - IPackageBuild, IPackageBuildDerived, IPackageBuildSource) |
| 72 | + IPackageBuild, IPackageBuildSource) |
| 73 | from lp.buildmaster.model.buildbase import BuildBase |
| 74 | from lp.buildmaster.model.buildfarmjob import BuildFarmJobDerived |
| 75 | from lp.registry.interfaces.pocket import PackagePublishingPocket |
| 76 | @@ -58,7 +58,11 @@ |
| 77 | build_farm_job = Reference(build_farm_job_id, 'BuildFarmJob.id') |
| 78 | |
| 79 | policy_name = 'buildd' |
| 80 | + |
| 81 | + # The following two properties are part of the IPackageBuild |
| 82 | + # interface, but need to be provided by derived classes. |
| 83 | distribution = None |
| 84 | + distro_series = None |
| 85 | |
| 86 | def __init__(self, build_farm_job, archive, pocket, |
| 87 | dependencies=None): |
| 88 | @@ -150,14 +154,25 @@ |
| 89 | """See `IPackageBuild`.""" |
| 90 | raise NotImplementedError |
| 91 | |
| 92 | + def handleStatus(self, status, librarian, slave_status): |
| 93 | + """See `IPackageBuild`.""" |
| 94 | + raise NotImplementedError |
| 95 | + |
| 96 | |
| 97 | class PackageBuildDerived: |
| 98 | - """See `IPackageBuildDerived`.""" |
| 99 | - implements(IPackageBuildDerived) |
| 100 | delegates(IPackageBuild, context="package_build") |
| 101 | |
| 102 | + |
| 103 | +class PackageBuildStatusHandlerMixin: |
| 104 | + """A mixin to provide the common implementation of handleStatus. |
| 105 | + |
| 106 | + This can be used by classes based on PackageBuild to provide |
| 107 | + the implementation of handleStatus, ensuring that the methods |
| 108 | + called during handleStatus are called on the top-level object |
| 109 | + first. |
| 110 | + """ |
| 111 | def handleStatus(self, status, librarian, slave_status): |
| 112 | - """See `IPackageBuildDerived`.""" |
| 113 | + """See `IPackageBuild`.""" |
| 114 | return BuildBase.handleStatus(self, status, librarian, slave_status) |
| 115 | |
| 116 | # The following private handlers currently re-use the BuildBase |
| 117 | |
| 118 | === modified file 'lib/lp/buildmaster/tests/test_packagebuild.py' |
| 119 | --- lib/lp/buildmaster/tests/test_packagebuild.py 2010-04-30 16:11:03 +0000 |
| 120 | +++ lib/lp/buildmaster/tests/test_packagebuild.py 2010-05-04 12:07:35 +0000 |
| 121 | @@ -89,6 +89,9 @@ |
| 122 | self.assertRaises( |
| 123 | NotImplementedError, self.package_build.verifySuccessfulUpload) |
| 124 | self.assertRaises(NotImplementedError, self.package_build.notify) |
| 125 | + self.assertRaises( |
| 126 | + NotImplementedError, self.package_build.handleStatus, |
| 127 | + None, None, None) |
| 128 | |
| 129 | def test_default_values(self): |
| 130 | # PackageBuild has a number of default values. |
| 131 | @@ -96,6 +99,7 @@ |
| 132 | self.failUnlessEqual( |
| 133 | 'multiverse', self.package_build.current_component.name) |
| 134 | self.failUnlessEqual(None, self.package_build.distribution) |
| 135 | + self.failUnlessEqual(None, self.package_build.distro_series) |
| 136 | |
| 137 | def test_log_url(self): |
| 138 | # The url of the build log file is determined by the PackageBuild. |
| 139 | |
| 140 | === modified file 'lib/lp/soyuz/model/binarypackagebuild.py' |
| 141 | --- lib/lp/soyuz/model/binarypackagebuild.py 2010-04-29 08:20:20 +0000 |
| 142 | +++ lib/lp/soyuz/model/binarypackagebuild.py 2010-05-04 09:45:26 +0000 |
| 143 | @@ -199,8 +199,8 @@ |
| 144 | def was_built(self): |
| 145 | """See `IBuild`""" |
| 146 | return self.status not in [BuildStatus.NEEDSBUILD, |
| 147 | - BuildStatus.BUILDING, |
| 148 | - BuildStatus.SUPERSEDED] |
| 149 | + BuildStatus.BUILDING, |
| 150 | + BuildStatus.SUPERSEDED] |
| 151 | |
| 152 | @property |
| 153 | def arch_tag(self): |
On Mon, May 3, 2010 at 5:55 PM, Aaron Bentley <email address hidden> wrote:
> distro_series should be defined on IPackageBuild, because it's common to all packages, even though the implementation will differ for ISourcePackageR
Thanks - I added it but returned None on PackageBuild (similar to the
other attributes) as raising NotImplementedError for attributes means
that the exception is raised during assertProvides(), or is there a
better way?
>
> diff lines 342-343 need to be re-indented.
Done.
>
> I'm also concerned about the split between IPackageBuild and IPackageBuildDe
Thanks for taking the extra time to improve the design beyond the
actual review itself! You were spot on... I'd started getting mixed up
while trying to find places to move methods to. I've removed
IPackageBuildDe
instead created a mixin class PackageBuildHan
be used to provide an implementation on derived classes (ie. this
implementation can be shared between BinaryPackageBuilds and
SPRBuilds, as it is currently with BuildBase).
I've attached the incremental of the above, but if it's ok with you,
I'll also do a second incremental to get rid of IBuildFarmJobDe
for the same reason you've outlined above.