On Tue, Dec 1, 2009 at 4:30 PM, Graham Binns <email address hidden> wrote:

> Review: Approve code
>
> 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
>


gah, I'd thought I was cleaning up old code (too much lazrjs lately).



> strip-trailing-whitespace run on the files you've changed (there's not
> much, but enough for me to notice).
>
>
Sorry - switched editors recently and forgot to setup tailing whitespace
removal. I've updated my .rc file to automatically delete trailing spaces
and fixed all the files. Thanks.



> 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?
>

Done.


>
> > +        """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 """.
>

Right. I'd assumed that if it was too long for the first line then it should
start on the following line. Updated.



>
> > +        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"/>
> >      </class>
> >
> > +    <!-- BinaryPackageBuildBehavior -->
> > +    <adapter
> > +        for="lp.soyuz.interfaces.buildpackagejob.IBuildPackageJob"
> > +
>  provides="lp.buildmaster.interfaces.buildfarmjobbehavior.IBuildFarmJobBehavior"
> > +
>  factory="lp.soyuz.model.binarypackagebuildbehavior.BinaryPackageBuildBehavior"
> > +        permission="zope.Public" />
> > +
> >  </configure>
> >
> > === 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.
>

Done.


>
> > +        """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().
>
>
Done.


> > +        """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.
>
>
Done.



> > +    """
> > +
> > +    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.
>

Done.


>
> > +        """
> > +        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.
>

And done.


>
> > +        """
> > +        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__)
> >
>
>

Thanks Graham. Diff attached.



> --
>
> https://code.edge.launchpad.net/~michael.nelson/launchpad/487009-db-generalise-ibuilder-1b/+merge/15477<https://code.edge.launchpad.net/%7Emichael.nelson/launchpad/487009-db-generalise-ibuilder-1b/+merge/15477>
> Your team Launchpad code reviewers from Canonical is subscribed to branch
> lp:launchpad.
>
> --
> launchpad-reviews mailing list
> <email address hidden>
> https://lists.ubuntu.com/mailman/listinfo/launchpad-reviews
>