Hi Michael, Nice branch. Good work. I've got some stylistic nitpicks - our guidelines gleefully discard PEP8, hurrah - and I'd like you to do a strip-trailing-whitespace run on the files you've changed (there's not much, but enough for me to notice). Other than that, r=me. > === added file 'lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py' > --- lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py 1970-01-01 00:00:00 +0000 > +++ lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py 2009-12-01 10:00:36 +0000 > @@ -0,0 +1,38 @@ > +# Copyright 2009 Canonical Ltd. This software is licensed under the > +# GNU Affero General Public License version 3 (see the file LICENSE). > + > +# pylint: disable-msg=E0211,E0213 > + > +"""Interface for build farm job behaviors.""" > + > +__metaclass__ = type > + > +__all__ = [ > + 'IBuildFarmJobBehavior', > + ] > + > +from zope.interface import Interface > + > + > +class BuildBehaviorMismatch(Exception): > + """ > + A general exception that can be raised when the builder's current behavior > + does not match the expected behavior. > + """ > + > + > +class IBuildFarmJobBehavior(Interface): > + > + def set_builder(builder): > + """Sets the associated builder reference for this instance.""" > + > + def log_start_build(build_queue_item, logger): > + """Log the start of a specific build queue item. > + > + The form of the log message will vary depending on the type of build. > + :param build_queue_item: A BuildQueueItem to build. > + :param logger: A logger to be used to log diagnostic information. > + """ > + # A number of other methods to go here that can be customised for each of > + # the different build types (branch build, recipe build, translation > + # build etc.) > > === added file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py' > --- lib/lp/buildmaster/model/buildfarmjobbehavior.py 1970-01-01 00:00:00 +0000 > +++ lib/lp/buildmaster/model/buildfarmjobbehavior.py 2009-12-01 10:00:36 +0000 > @@ -0,0 +1,53 @@ > +# Copyright 2009 Canonical Ltd. This software is licensed under the > +# GNU Affero General Public License version 3 (see the file LICENSE). > + > +# pylint: disable-msg=E0211,E0213 > + > +"""Base and idle BuildFarmJobBehavior classes.""" > + > +__metaclass__ = type > + > +__all__ = [ > + 'BuildFarmJobBehaviorBase', > + 'IdleBuildBehavior' > + ] > + > +from zope.interface import implements > + > +from lp.buildmaster.interfaces.buildfarmjobbehavior import ( > + BuildBehaviorMismatch, IBuildFarmJobBehavior) > + > + > +class BuildFarmJobBehaviorBase: > + """Ensures that all behaviors inherit the same initialisation. > + > + All build-farm job behaviors should inherit from this. > + """ > + > + def __init__(self, buildfarmjob): > + """ > + Store a reference to the job_type with which we were created. > + """ > + self.buildfarmjob = buildfarmjob > + self._builder = None > + > + def set_builder(self, builder): This should be setBuilder(), because who needs PEP8? > + """The builder should be set once and not changed.""" > + self._builder = builder > + > + > +class IdleBuildBehavior(BuildFarmJobBehaviorBase): > + > + implements(IBuildFarmJobBehavior) > + > + def __init__(self): > + """ According to our coding standards the docstring should start on the same line as the opening """. > + The idle behavior is special in that a buildfarmjob is not specified > + during initialisation. > + """ > + super(IdleBuildBehavior, self).__init__(None) > + > + def logStartBuild(self, build_queue_item, logger): > + """See `IBuildFarmJobBehavior`.""" > + raise BuildBehaviorMismatch( > + "Builder was idle when asked to log the start of a build.") > > === modified file 'lib/lp/soyuz/configure.zcml' > --- lib/lp/soyuz/configure.zcml 2009-11-16 22:06:14 +0000 > +++ lib/lp/soyuz/configure.zcml 2009-12-01 10:00:36 +0000 > @@ -892,4 +892,11 @@ > interface="lp.soyuz.interfaces.buildpackagejob.IBuildPackageJob"/> > > > + > + + for="lp.soyuz.interfaces.buildpackagejob.IBuildPackageJob" > + provides="lp.buildmaster.interfaces.buildfarmjobbehavior.IBuildFarmJobBehavior" > + factory="lp.soyuz.model.binarypackagebuildbehavior.BinaryPackageBuildBehavior" > + permission="zope.Public" /> > + > > > === modified file 'lib/lp/soyuz/doc/builder.txt' > --- lib/lp/soyuz/doc/builder.txt 2009-11-12 12:52:23 +0000 > +++ lib/lp/soyuz/doc/builder.txt 2009-12-01 10:00:36 +0000 > @@ -40,6 +40,18 @@ > >>> builder.builderok = True > >>> builder.failnotes = None > > +A builder provides a current_build_behavior attribute to which all the > +build-type specific behavior for the current build is delegated. For > +our sample data, the behavior is specifically a behavior for dealing > +with binary packages > + > + >>> from zope.security.proxy import isinstance > + >>> from lp.soyuz.model.binarypackagebuildbehavior import ( > + ... BinaryPackageBuildBehavior) > + >>> isinstance( > + ... builder.current_build_behavior, BinaryPackageBuildBehavior) > + True > + > In case of copy archives the status string will show both the copy > archive owner as well as the copy archive name. > > > === modified file 'lib/lp/soyuz/doc/buildqueue.txt' > --- lib/lp/soyuz/doc/buildqueue.txt 2009-11-13 19:34:17 +0000 > +++ lib/lp/soyuz/doc/buildqueue.txt 2009-12-01 10:00:36 +0000 > @@ -98,6 +98,16 @@ > (True, 1000) > > > +The BuildQueue item is responsible for providing the required build behavior > +for the item. > + > + >>> from zope.security.proxy import isinstance > + >>> from lp.soyuz.model.binarypackagebuildbehavior import ( > + ... BinaryPackageBuildBehavior) > + >>> isinstance(bq.required_build_behavior, BinaryPackageBuildBehavior) > + True > + > + > == Dispatching and Reseting jobs == > > The sampledata contains an active job, being built by the 'bob' > > === modified file 'lib/lp/soyuz/interfaces/builder.py' > --- lib/lp/soyuz/interfaces/builder.py 2009-11-20 18:06:28 +0000 > +++ lib/lp/soyuz/interfaces/builder.py 2009-12-01 10:00:36 +0000 > @@ -19,11 +19,13 @@ > ] > > from zope.interface import Interface, Attribute > -from zope.schema import Choice, TextLine, Text, Bool > +from zope.schema import Bool, Choice, Field, Text, TextLine > > from canonical.launchpad import _ > from canonical.launchpad.fields import Title, Description > from lp.registry.interfaces.role import IHasOwner > +from lp.buildmaster.interfaces.buildfarmjobbehavior import ( > + IBuildFarmJobBehavior) > from canonical.launchpad.validators.name import name_validator > from canonical.launchpad.validators.url import builder_url_validator > > @@ -57,7 +59,7 @@ > """The build slave has suffered an error and cannot be used.""" > > > -class IBuilder(IHasOwner): > +class IBuilder(IHasOwner, IBuildFarmJobBehavior): > """Build-slave information and state. > > Builder instance represents a single builder slave machine within the > @@ -138,6 +140,10 @@ > "new jobs. "), > required=False) > > + current_build_behavior = Field( > + title=u"The current behavior of the builder for the current job.", > + required=False) > + > def cacheFileOnSlave(logger, libraryfilealias): > """Ask the slave to cache a librarian file to its local disk. > > > === modified file 'lib/lp/soyuz/interfaces/buildqueue.py' > --- lib/lp/soyuz/interfaces/buildqueue.py 2009-11-20 18:06:28 +0000 > +++ lib/lp/soyuz/interfaces/buildqueue.py 2009-12-01 10:00:36 +0000 > @@ -13,7 +13,7 @@ > ] > > from zope.interface import Interface, Attribute > -from zope.schema import Choice, Datetime, Timedelta > +from zope.schema import Choice, Datetime, Field, Timedelta > > from lazr.restful.fields import Reference > > @@ -51,6 +51,10 @@ > title=_('Job type'), required=True, vocabulary=BuildFarmJobType, > description=_("The type of this job.")) > > + required_build_behavior = Field( > + title=_('The builder behavior required to run this job.'), > + required=False, readonly=True) > + > estimated_duration = Timedelta( > title=_("Estimated Job Duration"), required=True, > description=_("Estimated job duration interval.")) > > === added file 'lib/lp/soyuz/model/binarypackagebuildbehavior.py' > --- lib/lp/soyuz/model/binarypackagebuildbehavior.py 1970-01-01 00:00:00 +0000 > +++ lib/lp/soyuz/model/binarypackagebuildbehavior.py 2009-12-01 10:00:36 +0000 > @@ -0,0 +1,35 @@ > +# Copyright 2009 Canonical Ltd. This software is licensed under the > +# GNU Affero General Public License version 3 (see the file LICENSE). > + > +# pylint: disable-msg=E0211,E0213 > + > +"""Builder behavior for binary package builds.""" > + > +__metaclass__ = type > + > +__all__ = [ > + 'BinaryPackageBuildBehavior', > + ] > + > +from lp.buildmaster.interfaces.buildfarmjobbehavior import ( > + IBuildFarmJobBehavior) > +from lp.buildmaster.model.buildfarmjobbehavior import ( > + BuildFarmJobBehaviorBase) > +from lp.soyuz.interfaces.build import IBuildSet > + > +from zope.component import getUtility > +from zope.interface import implements > + > + > +class BinaryPackageBuildBehavior(BuildFarmJobBehaviorBase): > + """Define the behavior of binary package builds.""" > + > + implements(IBuildFarmJobBehavior) > + > + def log_start_build(self, build_queue_item, logger): > + """See `IBuildFarmJobBehavior`.""" > + build = getUtility(IBuildSet).getByQueueEntry(build_queue_item) > + spr = build.sourcepackagerelease > + > + logger.info("startBuild(%s, %s, %s, %s)", self._builder.url, > + spr.name, spr.version, build.pocket.title) > > === modified file 'lib/lp/soyuz/model/builder.py' > --- lib/lp/soyuz/model/builder.py 2009-11-20 18:06:28 +0000 > +++ lib/lp/soyuz/model/builder.py 2009-12-01 10:00:36 +0000 > @@ -20,6 +20,8 @@ > import urllib2 > import xmlrpclib > > +from lazr.delegates import delegates > + > from zope.interface import implements > from zope.component import getUtility > > @@ -31,7 +33,10 @@ > from canonical.cachedproperty import cachedproperty > from canonical.config import config > from canonical.buildd.slave import BuilderStatus > +from lp.buildmaster.interfaces.buildfarmjobbehavior import ( > + BuildBehaviorMismatch, IBuildFarmJobBehavior) > from lp.buildmaster.master import BuilddMaster > +from lp.buildmaster.model.buildfarmjobbehavior import IdleBuildBehavior > from canonical.database.sqlbase import SQLBase, sqlvalues > from lp.soyuz.adapters.archivedependencies import ( > get_primary_current_component, get_sources_list_for_building) > @@ -54,6 +59,7 @@ > PackagePublishingStatus) > from lp.soyuz.model.buildpackagejob import BuildPackageJob > from canonical.launchpad.webapp import urlappend > +from canonical.lazr.utils import safe_hasattr > from canonical.librarian.utils import copy_and_close > > > @@ -114,6 +120,7 @@ > class Builder(SQLBase): > > implements(IBuilder, IHasBuildRecords) > + delegates(IBuildFarmJobBehavior, context="current_build_behavior") > _table = 'Builder' > > _defaultOrder = ['id'] > @@ -135,6 +142,51 @@ > vm_host = StringCol(dbName='vm_host') > active = BoolCol(dbName='active', notNull=True, default=True) > > + def _get_current_build_behavior(self): Should be _getCurrentBuildBehavior(). Yes, I know it's hateful, but them's the guidelines. > + """Return the current build behavior.""" > + if not safe_hasattr(self, '_current_build_behavior'): > + self._current_build_behavior = None > + > + if (self._current_build_behavior is None or > + isinstance(self._current_build_behavior, IdleBuildBehavior)): > + # If we don't currently have a current build behavior set, > + # or we are currently idle, then... > + currentjob = self.currentjob > + if currentjob is not None: > + # ...we'll set it based on our current job. > + self._current_build_behavior = ( > + currentjob.required_build_behavior) > + self._current_build_behavior.set_builder(self) > + return self._current_build_behavior > + elif self._current_build_behavior is None: > + # If we don't have a current job or an idle behavior > + # already set, then we just set the idle behavior > + # before returning. > + self._current_build_behavior = IdleBuildBehavior() > + return self._current_build_behavior > + > + else: > + # We did have a current non-idle build behavior set, so > + # we just return it. > + return self._current_build_behavior > + > + > + def _set_current_build_behavior(self, new_behavior): _setCurrentBuildBehavior(). > + """Set the current build behavior.""" > + > + if self.currentjob is not None: > + # We do not allow the current build behavior to be reset if we > + # have a current job. > + raise BuildBehaviorMismatch( > + "Attempt to reset builder behavior while a current build " > + "exists.") > + else: > + self._current_build_behavior = new_behavior > + self._current_build_behavior.set_builder(self) > + > + current_build_behavior = property( > + _get_current_build_behavior, _set_current_build_behavior) > + > def cacheFileOnSlave(self, logger, libraryfilealias): > """See `IBuilder`.""" > url = libraryfilealias.http_url > @@ -351,10 +403,9 @@ > > def startBuild(self, build_queue_item, logger): > """See IBuilder.""" > - build = getUtility(IBuildSet).getByQueueEntry(build_queue_item) > - spr = build.sourcepackagerelease > - logger.info("startBuild(%s, %s, %s, %s)", self.url, > - spr.name, spr.version, build.pocket.title) > + # Set the build behavior depending on the provided build queue item. > + self.current_build_behavior = build_queue_item.required_build_behavior > + self.log_start_build(build_queue_item, logger) > > # Make sure the request is valid; an exception is raised if it's not. > self._verifyBuildRequest(build_queue_item, logger) > > === modified file 'lib/lp/soyuz/model/buildqueue.py' > --- lib/lp/soyuz/model/buildqueue.py 2009-11-20 18:06:28 +0000 > +++ lib/lp/soyuz/model/buildqueue.py 2009-12-01 10:00:36 +0000 > @@ -24,6 +24,8 @@ > from canonical.database.sqlbase import SQLBase, sqlvalues > from canonical.launchpad.webapp.interfaces import NotFoundError > from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType > +from lp.buildmaster.interfaces.buildfarmjobbehavior import ( > + IBuildFarmJobBehavior) > from lp.services.job.interfaces.job import JobStatus > from lp.services.job.model.job import Job > from lp.soyuz.interfaces.build import BuildStatus, IBuildSet > @@ -49,6 +51,11 @@ > estimated_duration = IntervalCol() > > @property > + def required_build_behavior(self): > + """See `IBuildQueue`.""" > + return IBuildFarmJobBehavior(self.specific_job) > + > + @property > def specific_job(self): > """See `IBuildQueue`.""" > store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) > > === modified file 'lib/lp/soyuz/tests/test_builder.py' > --- lib/lp/soyuz/tests/test_builder.py 2009-11-13 16:37:05 +0000 > +++ lib/lp/soyuz/tests/test_builder.py 2009-12-01 10:00:36 +0000 > @@ -8,10 +8,15 @@ > from zope.component import getUtility > > from canonical.testing import LaunchpadZopelessLayer > +from lp.buildmaster.interfaces.buildfarmjobbehavior import ( > + BuildBehaviorMismatch, IBuildFarmJobBehavior) > +from lp.buildmaster.model.buildfarmjobbehavior import IdleBuildBehavior > from lp.soyuz.interfaces.archive import ArchivePurpose > from lp.soyuz.interfaces.build import BuildStatus, IBuildSet > from lp.soyuz.interfaces.builder import IBuilderSet > from lp.soyuz.interfaces.publishing import PackagePublishingStatus > +from lp.soyuz.model.binarypackagebuildbehavior import ( > + BinaryPackageBuildBehavior) > from lp.soyuz.tests.test_publishing import SoyuzTestPublisher > from lp.testing import TestCaseWithFactory > > @@ -214,5 +219,72 @@ > self.failUnlessEqual('primary', build.archive.name) > self.failUnlessEqual('firefox', build.sourcepackagerelease.name) > > + > +class TestCurrentBuildBehavior(TestCaseWithFactory): > + """ > + This test ensures the get/set behavior of Builder.current_build_behavior. Docstring should start on a new line. > + """ > + > + layer = LaunchpadZopelessLayer > + > + def setUp(self): > + """Create a new builder ready for testing.""" > + super(TestCurrentBuildBehavior, self).setUp() > + self.builder = self.factory.makeBuilder(name='builder') > + > + # Have a publisher and a ppa handy for some of the tests below. > + self.publisher = SoyuzTestPublisher() > + self.publisher.prepareBreezyAutotest() > + self.ppa_joe = self.factory.makeArchive(name="joesppa") > + > + self.build = self.publisher.getPubSource( > + sourcename="gedit", status=PackagePublishingStatus.PUBLISHED, > + archive=self.ppa_joe).createMissingBuilds()[0] > + > + self.buildfarmjob = self.build.buildqueue_record.specific_job > + > + def test_idle_behavior_when_no_current_build(self): > + """ > + We return an idle behavior when there is no behavior specified > + nor a current build. Same here. > + """ > + self.assertIsInstance( > + self.builder.current_build_behavior, IdleBuildBehavior) > + > + def test_set_behavior_when_no_current_job(self): > + """If a builder is idle then it is possible to set the behavior.""" > + self.builder.current_build_behavior = IBuildFarmJobBehavior( > + self.buildfarmjob) > + > + self.assertIsInstance( > + self.builder.current_build_behavior, BinaryPackageBuildBehavior) > + > + def test_current_job_behavior(self): > + """The current behavior is set automatically from the current job.""" > + # Set the builder attribute on the buildqueue record so that our > + # builder will think it has a current build. > + self.build.buildqueue_record.builder = self.builder > + > + self.assertIsInstance( > + self.builder.current_build_behavior, BinaryPackageBuildBehavior) > + > + def test_set_behavior_when_current_job(self): > + """ > + If a builder has a current job then it's behavior cannot be > + set. Same here. > + """ > + self.build.buildqueue_record.builder = self.builder > + > + # As we can't use assertRaises for a property, we use a try-except > + # instead. > + assertion_raised = False > + try: > + self.builder.current_build_behavior = IBuildFarmJobBehavior( > + self.buildfarmjob) > + except BuildBehaviorMismatch, e: > + assertion_raised = True > + > + self.failUnless(assertion_raised) > + > def test_suite(): > return unittest.TestLoader().loadTestsFromName(__name__) >