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 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 @@ > interface="lp.soyuz.interfaces.buildpackagejob.IBuildPackageJob"/> > > + > + + name="PACKAGEBUILD" > + provides="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJob"/> > > > > === 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? > + > + return dict(job_classes) > + > + @property > def specific_job(self): > """See `IBuildQueue`.""" > + specific_class = self.specific_job_classes[self.job_type] > 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. > > @property > > === modified file 'lib/lp/soyuz/tests/test_buildqueue.py' > --- lib/lp/soyuz/tests/test_buildqueue.py 2010-01-04 10:58:09 +0000 > +++ lib/lp/soyuz/tests/test_buildqueue.py 2010-01-07 08:23:28 +0000 > @@ -12,6 +12,7 @@ > IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR) > from canonical.testing import LaunchpadZopelessLayer > > +from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType > from lp.soyuz.interfaces.archive import ArchivePurpose > from lp.soyuz.interfaces.build import BuildStatus > from lp.soyuz.interfaces.builder import IBuilderSet > @@ -330,3 +331,80 @@ > free_count = bq._freeBuildersCount( > build.processor, build.is_virtualized) > self.assertEqual(free_count, 1) > + > + > +class TestJobClasses(TestCaseWithFactory): > + """Tests covering build farm job type classes.""" > + layer = LaunchpadZopelessLayer > + def setUp(self): > + """Set up a native x86 build for the test archive.""" > + super(TestJobClasses, self).setUp() > + > + self.publisher = SoyuzTestPublisher() > + self.publisher.prepareBreezyAutotest() > + > + # First mark all builds in the sample data as already built. > + store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) > + sample_data = store.find(Build) > + for build in sample_data: > + build.buildstate = BuildStatus.FULLYBUILT > + store.flush() > + > + # We test builds that target a primary archive. > + self.non_ppa = self.factory.makeArchive( > + name="primary", purpose=ArchivePurpose.PRIMARY) > + self.non_ppa.require_virtualized = False > + > + self.builds = [] > + self.builds.extend( > + self.publisher.getPubSource( > + sourcename="gedit", status=PackagePublishingStatus.PUBLISHED, > + archive=self.non_ppa).createMissingBuilds()) > + > + def test_BuildPackageJob(self): > + """`BuildPackageJob` is one of the job type classes.""" > + from lp.soyuz.model.buildpackagejob import BuildPackageJob > + _build, bq = find_job(self, 'gedit') > + > + # This is a binary package build. > + self.assertEqual( > + bq.job_type, BuildFarmJobType.PACKAGEBUILD, > + "This is a binary package build") > + > + # The class registered for 'PACKAGEBUILD' is `BuildPackageJob`. > + self.assertEqual( > + bq.specific_job_classes[BuildFarmJobType.PACKAGEBUILD], > + BuildPackageJob, > + "The class registered for 'PACKAGEBUILD' is `BuildPackageJob`") > + > + # The 'specific_job' object associated with this `BuildQueue` > + # instance is of type `BuildPackageJob`. > + self.assertTrue(bq.specific_job is not None) > + self.assertEqual( > + bq.specific_job.__class__, BuildPackageJob, > + "The 'specific_job' object associated with this `BuildQueue` " > + "instance is of type `BuildPackageJob`") > + > + def test_OtherTypeClasses(self): > + """Other job type classes are picked up as well.""" > + from zope import component > + from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob > + 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_job_classes.get(BuildFarmJobType.BRANCHBUILD) is None) > + > + # Pretend that our `FakeBranchBuild` class implements the > + # `IBuildFarmJob` interface. > + component.provideUtility( > + FakeBranchBuild, IBuildFarmJob, 'BRANCHBUILD') > + > + # Now we should see the `FakeBranchBuild` class "registered" in the > + # `specific_job_classes` dictionary under the 'BRANCHBUILD' key. > + self.assertEqual( > + bq.specific_job_classes[BuildFarmJobType.BRANCHBUILD], > + FakeBranchBuild) >