Michael Nelson wrote: > Review: Needs Information code > 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. Hello Michael, sorry for the terse answer but, yes, I need to iterate over the farm build job classes. See e.g. lines 48-54 in the enclosed diff which is an excerpt from lp:~al-maisan/launchpad/job-delays-504086, the branch I am currently working on. > 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) >> > Best regards -- Muharem Hrnjadovic