Michael Nelson wrote: [..] Hello Michael, thanks again for taking the time to review this branch despite your own busy schedule. That's very much appreciated :) > Some comments in line below. Please see my replies as well as the enclosed incremental diff. >> === 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. No other code in the module uses them. I have added a comment to that effect. >> + >> + 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 :) Yup. >> + 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? This is a very nice suggestion that I gladly adopted, see the incremental diff. >> + >> + 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. We have already discussed this at length. >> @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) Best regards -- Muharem Hrnjadovic