It's great seeing the pieces of the generalisation falling into place!
I've got one rather large question about this change: Why can't you just
use a normal adapter? It seems that you've gone the long way around to
implement adapter-like functionality.
That is, you've got a BuildFarmJobType specified on IBuildQueue.job_type,
and you want to find the corresponding class. I don't know whether it is
the best solution, but you *could* in this case, adapt the build_queue
entry itself to the correct IBuildFarmJob implementation?
IBuildFarmJob(specific_build_queue_entry)
which would return the specifc build farm job.
I'm wondering if it's really that you need specific_job_classes for further
work in the build estimations, rather than specifically for this branch. That
is, originally when you were chatting with gary about using the class manager,
I had understood that you needed to *iterate* over the different
classes to build a query... in that case you would certainly need something
like the specific_job_classes that you've provided below. But as it is, you are
not needing to iterate them at all for this branch, and looking at this branch
in isolation would suggest that a normal adapter would be more intuitive.
If it is the case that you will need specific_job_classes for a later branch,
then it may indeed make sense to *not* use adaption here and re-use the
specific_job_classes exactly as you've done, as otherwise new types
would need to define both the utility *and* the adapter. I'd like to get
BjornT's thoughts on that point, hence marking as needs information.
Some comments in line below.
> === modified file 'lib/lp/soyuz/configure.zcml'
> --- lib/lp/soyuz/configure.zcml 2010-01-04 21:15:46 +0000
> +++ lib/lp/soyuz/configure.zcml 2010-01-07 08:23:28 +0000
> @@ -897,6 +897,16 @@
> <allow
> interface="lp.soyuz.interfaces.buildpackagejob.IBuildPackageJob"/>
> </class>
> + <!--
> + The registration below is used to discover all build farm job classes
> + that implement the `IBuildFarmJob` interface. Please see bug #503839
> + for more detail.
> + The 'name' attribute needs to be set to the appropriate
> + `BuildFarmJobType` enumeration value.
> + -->
> + <utility component="lp.soyuz.model.buildpackagejob.BuildPackageJob"
> + name="PACKAGEBUILD"
> + provides="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJob"/>
>
> <!-- BinaryPackageBuildBehavior -->
> <adapter
>
> === modified file 'lib/lp/soyuz/interfaces/buildqueue.py'
> --- lib/lp/soyuz/interfaces/buildqueue.py 2009-11-27 13:33:55 +0000
> +++ lib/lp/soyuz/interfaces/buildqueue.py 2010-01-07 08:23:28 +0000
> @@ -55,6 +55,10 @@
> title=_('The builder behavior required to run this job.'),
> required=False, readonly=True)
>
> + specific_job_classes = Field(
> + title=_('Job classes that may run on the build farm.'),
> + required=True, readonly=True)
> +
> estimated_duration = Timedelta(
> title=_("Estimated Job Duration"), required=True,
> description=_("Estimated job duration interval."))
>
> === modified file 'lib/lp/soyuz/model/buildqueue.py'
> --- lib/lp/soyuz/model/buildqueue.py 2010-01-04 14:30:52 +0000
> +++ lib/lp/soyuz/model/buildqueue.py 2010-01-07 08:23:28 +0000
> @@ -57,11 +57,31 @@
> return IBuildFarmJobBehavior(self.specific_job)
>
> @property
> + def specific_job_classes(self):
> + """See `IBuildQueue`."""
> + from zope.component import getSiteManager
> + from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob
Is there a reason for these imports to be inline? If so, please add a comment
or otherwise move them as per the guidelines.
> +
> + job_classes = []
> + components = getSiteManager()
> + # Get all components that implement the `IBuildFarmJob` interface.
> + implementations = sorted(components.getUtilitiesFor(IBuildFarmJob))
> + # The above yields a collection of 2-tuples where the first element
> + # is the name of the `BuildFarmJobType` enum and the second element
> + # is the implementing class respectively.
> + for job_enum_name, job_class in implementations:
> + job_enum = getattr(BuildFarmJobType, job_enum_name)
I was originally going to say that we should be checking first with
safe_hasattr here, but then the logical thing to do would be to throw
an informative exception if it wasn't there (due to a mis-configuration)
and that's exactly what getattr will do :)
> + job_classes.append((job_enum, job_class))
Does job_classes need to be a list? Couldn't it be defined as an empty
dict above and then here simply do job_classes[job_enum] = job_class?
Hi Muharem!
It's great seeing the pieces of the generalisation falling into place!
I've got one rather large question about this change: Why can't you just
use a normal adapter? It seems that you've gone the long way around to
implement adapter-like functionality.
That is, you've got a BuildFarmJobType specified on IBuildQueue. job_type,
and you want to find the corresponding class. I don't know whether it is
the best solution, but you *could* in this case, adapt the build_queue
entry itself to the correct IBuildFarmJob implementation?
IBuildFarmJob( specific_ build_queue_ entry)
which would return the specifc build farm job.
I'm wondering if it's really that you need specific_ job_classes for further job_classes that you've provided below. But as it is, you are
work in the build estimations, rather than specifically for this branch. That
is, originally when you were chatting with gary about using the class manager,
I had understood that you needed to *iterate* over the different
classes to build a query... in that case you would certainly need something
like the specific_
not needing to iterate them at all for this branch, and looking at this branch
in isolation would suggest that a normal adapter would be more intuitive.
If it is the case that you will need specific_ job_classes for a later branch, job_classes exactly as you've done, as otherwise new types
then it may indeed make sense to *not* use adaption here and re-use the
specific_
would need to define both the utility *and* the adapter. I'd like to get
BjornT's thoughts on that point, hence marking as needs information.
Some comments in line below.
> === modified file 'lib/lp/ soyuz/configure .zcml' soyuz/configure .zcml 2010-01-04 21:15:46 +0000 soyuz/configure .zcml 2010-01-07 08:23:28 +0000 "lp.soyuz. interfaces. buildpackagejob .IBuildPackageJ ob"/> "lp.soyuz. model.buildpack agejob. BuildPackageJob " "lp.buildmaster .interfaces. buildfarmjob. IBuildFarmJob" /> ildBehavior --> soyuz/interface s/buildqueue. py' soyuz/interface s/buildqueue. py 2009-11-27 13:33:55 +0000 soyuz/interface s/buildqueue. py 2010-01-07 08:23:28 +0000 job_classes = Field( _("Estimated job duration interval.")) soyuz/model/ buildqueue. py' soyuz/model/ buildqueue. py 2010-01-04 14:30:52 +0000 soyuz/model/ buildqueue. py 2010-01-07 08:23:28 +0000 havior( self.specific_ job) job_classes( self): interfaces. buildfarmjob import IBuildFarmJob
> --- lib/lp/
> +++ lib/lp/
> @@ -897,6 +897,16 @@
> <allow
> interface=
> </class>
> + <!--
> + The registration below is used to discover all build farm job classes
> + that implement the `IBuildFarmJob` interface. Please see bug #503839
> + for more detail.
> + The 'name' attribute needs to be set to the appropriate
> + `BuildFarmJobType` enumeration value.
> + -->
> + <utility component=
> + name="PACKAGEBUILD"
> + provides=
>
> <!-- BinaryPackageBu
> <adapter
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -55,6 +55,10 @@
> title=_('The builder behavior required to run this job.'),
> required=False, readonly=True)
>
> + specific_
> + title=_('Job classes that may run on the build farm.'),
> + required=True, readonly=True)
> +
> estimated_duration = Timedelta(
> title=_("Estimated Job Duration"), required=True,
> description=
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -57,11 +57,31 @@
> return IBuildFarmJobBe
>
> @property
> + def specific_
> + """See `IBuildQueue`."""
> + from zope.component import getSiteManager
> + from lp.buildmaster.
Is there a reason for these imports to be inline? If so, please add a comment
or otherwise move them as per the guidelines.
> + components. getUtilitiesFor (IBuildFarmJob) ) BuildFarmJobTyp e, job_enum_name)
> + job_classes = []
> + components = getSiteManager()
> + # Get all components that implement the `IBuildFarmJob` interface.
> + implementations = sorted(
> + # The above yields a collection of 2-tuples where the first element
> + # is the name of the `BuildFarmJobType` enum and the second element
> + # is the implementing class respectively.
> + for job_enum_name, job_class in implementations:
> + job_enum = getattr(
I was originally going to say that we should be checking first with
safe_hasattr here, but then the logical thing to do would be to throw
an informative exception if it wasn't there (due to a mis-configuration)
and that's exactly what getattr will do :)
> + job_classes. append( (job_enum, job_class))
Does job_classes need to be a list? Couldn't it be defined as an empty job_enum] = job_class?
dict above and then here simply do job_classes[
> + job_classes[ self.job_ type]
> + return dict(job_classes)
> +
> + @property
> def specific_job(self):
> """See `IBuildQueue`."""
> + specific_class = self.specific_
> store = Store.of(self)
> result_set = store.find(
> - BuildPackageJob, BuildPackageJob.job == self.job)
> + specific_class, specific_class.job == self.job)
> return result_set.one()
As mentioned at the top, this *could* simply be:
def specific_job(self):
return IBuildFarmJob(self)
(with most of the above storm code moved into a factory) if you used an
adapter, but there may be reasons for not doing so.
> soyuz/tests/ test_buildqueue .py' soyuz/tests/ test_buildqueue .py 2010-01-04 10:58:09 +0000 soyuz/tests/ test_buildqueue .py 2010-01-07 08:23:28 +0000 ssLayer interfaces. buildfarmjob import BuildFarmJobType interfaces. archive import ArchivePurpose interfaces. build import BuildStatus interfaces. builder import IBuilderSet sCount( virtualized) l(free_ count, 1) TestCaseWithFac tory): ssLayer asses, self).setUp() her() prepareBreezyAu totest( ) IStoreSelector) .get(MAIN_ STORE, DEFAULT_FLAVOR) FULLYBUILT makeArchive( ArchivePurpose. PRIMARY) ppa.require_ virtualized = False getPubSource( PackagePublishi ngStatus. PUBLISHED, self.non_ ppa).createMiss ingBuilds( )) geJob(self) : eJob` is one of the job type classes.""" model.buildpack agejob import BuildPackageJob e.PACKAGEBUILD, job_classes[ BuildFarmJobTyp e.PACKAGEBUILD] , (bq.specific_ job is not None) job.__class_ _, BuildPackageJob, lasses( self): interfaces. buildfarmjob import IBuildFarmJob job_classes. get(BuildFarmJo bType.BRANCHBUI LD) is None) provideUtility( job_classes` dictionary under the 'BRANCHBUILD' key. job_classes[ BuildFarmJobTyp e.BRANCHBUILD] ,
> @property
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -12,6 +12,7 @@
> IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
> from canonical.testing import LaunchpadZopele
>
> +from lp.buildmaster.
> from lp.soyuz.
> from lp.soyuz.
> from lp.soyuz.
> @@ -330,3 +331,80 @@
> free_count = bq._freeBuilder
> build.processor, build.is_
> self.assertEqua
> +
> +
> +class TestJobClasses(
> + """Tests covering build farm job type classes."""
> + layer = LaunchpadZopele
> + def setUp(self):
> + """Set up a native x86 build for the test archive."""
> + super(TestJobCl
> +
> + self.publisher = SoyuzTestPublis
> + self.publisher.
> +
> + # First mark all builds in the sample data as already built.
> + store = getUtility(
> + sample_data = store.find(Build)
> + for build in sample_data:
> + build.buildstate = BuildStatus.
> + store.flush()
> +
> + # We test builds that target a primary archive.
> + self.non_ppa = self.factory.
> + name="primary", purpose=
> + self.non_
> +
> + self.builds = []
> + self.builds.extend(
> + self.publisher.
> + sourcename="gedit", status=
> + archive=
> +
> + def test_BuildPacka
> + """`BuildPackag
> + from lp.soyuz.
> + _build, bq = find_job(self, 'gedit')
> +
> + # This is a binary package build.
> + self.assertEqual(
> + bq.job_type, BuildFarmJobTyp
> + "This is a binary package build")
> +
> + # The class registered for 'PACKAGEBUILD' is `BuildPackageJob`.
> + self.assertEqual(
> + bq.specific_
> + BuildPackageJob,
> + "The class registered for 'PACKAGEBUILD' is `BuildPackageJob`")
> +
> + # The 'specific_job' object associated with this `BuildQueue`
> + # instance is of type `BuildPackageJob`.
> + self.assertTrue
> + self.assertEqual(
> + bq.specific_
> + "The 'specific_job' object associated with this `BuildQueue` "
> + "instance is of type `BuildPackageJob`")
> +
> + def test_OtherTypeC
> + """Other job type classes are picked up as well."""
> + from zope import component
> + from lp.buildmaster.
> + class FakeBranchBuild:
> + pass
> +
> + _build, bq = find_job(self, 'gedit')
> + # First make sure that we don't have a job type class registered for
> + # 'BRANCHBUILD' yet.
> + self.assertTrue(
> + bq.specific_
> +
> + # Pretend that our `FakeBranchBuild` class implements the
> + # `IBuildFarmJob` interface.
> + component.
> + FakeBranchBuild, IBuildFarmJob, 'BRANCHBUILD')
> +
> + # Now we should see the `FakeBranchBuild` class "registered" in the
> + # `specific_
> + self.assertEqual(
> + bq.specific_
> + FakeBranchBuild)
>