> -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Hi Michael, > thank you for your fix and sorry for the delay... No problem... it was worth all the improvements below :) > > Am 14.06.2010 10:35, schrieb Michael Nelson: > > Test command: bin/test -vv -m test_packagebuild -m test_binarypackagebuild > -m > > test_hasbuildrecords > > Yup, they pass. Thanks. > > > This branch does some initial work to enable PPA's to present general > > IPackageBuilds rather than IBinaryPackageBuilds (bug 536700). > > So, I assume that is the reason that no bug is attached to this branch? There are two now :P > > > > > It adds and tests an IPackageBuildSet.getBuildsForArchive(). It adds an > > optional param to the getBuildRecords() method - binary_only - which > defaults > > to true (current behaivour). It then updates IArchive.getBuildRecords() to > > support binary_only=False. > > > > Additionally, it adds an adapter for IBuildFarmJob->IBinaryPackageBuild in > > preparation for the UI (so each build can present its title correctly). > > > > Very clever. ;-) > > Here are my comments: > ... > > === modified file 'lib/lp/buildmaster/model/packagebuild.py' > > --- lib/lp/buildmaster/model/packagebuild.py 2010-06-07 10:43:01 +0000 > > +++ lib/lp/buildmaster/model/packagebuild.py 2010-06-14 08:35:42 +0000 > > @@ -214,3 +218,40 @@ > > def _handleStatus_GIVENBACK(self, librarian, slave_status, logger): > > return BuildBase._handleStatus_GIVENBACK( > > self, librarian, slave_status, logger) > > + > > + > > +class PackageBuildSet: > > + implements(IPackageBuildSet) > > + > > + def getBuildsForArchive(self, archive, status=None, pocket=None): > > + """See `IPackageBuildSet`.""" > > + > > + extra_exprs = [] > > + > > + # Add query clause that filters on build status if the latter is > > + # provided. > > I had been told that comments are not there to reiterate what the code is > doing - the code should speak for itself. This looks like such a case, does it > not? /me must stop doing that. Removed. > > > + if status is not None: > > + extra_exprs.append(BuildFarmJob.status == status) > > + > > + # Add query clause that filters on pocket if the latter is > provided. > > Same here. The comment is just chatter. Maybe its pseudo code that you forgot > in here? Ditto. > > > + if pocket: > > + extra_exprs.append(PackageBuild.pocket == pocket) > > + > > + store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) > > + result_set = store.find(PackageBuild, > > + PackageBuild.archive == archive, > > + PackageBuild.build_farm_job == BuildFarmJob.id, > > + *extra_exprs) > > + > > + # Ordering according status > > + # * SUPERSEDED & All by -datecreated > > What does "& All" refer to"? I updated this to: When we have a set of builds that may include pending or superseded builds, we order by -date_created (as we won't always have a date_finished), otherwise we can order by -date_finished. > > > + # * FULLYBUILT & FAILURES by -datebuilt > I would put this near the "else" to explain which you expect there. See what you think of the above comment - it seems like one statement, but happy to change it if you disagree. > > > + # It should present the builds in a more natural order. > This is a really useful comment line because it explains the "why" while the > "how" can be read from the code directly. > > > + if status == BuildStatus.SUPERSEDED or status is None: > > What does it mean when status is None? That might be worth a comment, don't > you think? Is it clearer now with the above comment? (ie. if the user does not specify a status, we find builds of all statuses). Also, I've updated the implementation to be more specific (and hopefully readable): # When we have a set of builds that may include pending or # superseded builds, we order by -date_created (as we won't # always have a date_finished). Otherwise we can order by # -date_finished. if status is None or status in [ BuildStatus.NEEDSBUILD, BuildStatus.BUILDING, BuildStatus.SUPERSEDED]: > > > + result_set.order_by( > > + Desc(BuildFarmJob.date_created), BuildFarmJob.id) > > + else: > > Or maybe even "elif FULLYBUILT & FAILURES"? Then add an "else" that raises an > exception? But maybe that is too paranoid ... Yeah, given that status=None (ie. find all builds, irregardless of status), I don't think we need to verify with an elif here. > > > > + result_set.order_by( > > + Desc(BuildFarmJob.date_finished), BuildFarmJob.id) > > + > > + return result_set > > > > === modified file 'lib/lp/buildmaster/tests/test_packagebuild.py' > > --- lib/lp/buildmaster/tests/test_packagebuild.py 2010-05-21 12:48:44 > +0000 > > +++ lib/lp/buildmaster/tests/test_packagebuild.py 2010-06-14 08:35:42 > +0000 > > @@ -15,9 +15,10 @@ > > > > from canonical.testing.layers import LaunchpadFunctionalLayer > > > > +from lp.buildmaster.interfaces.buildbase import BuildStatus > > from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType > > from lp.buildmaster.interfaces.packagebuild import ( > > - IPackageBuild, IPackageBuildSource) > > + IPackageBuild, IPackageBuildSet, IPackageBuildSource) > > from lp.buildmaster.model.packagebuild import PackageBuild > > from lp.buildmaster.tests.test_buildbase import TestBuildBaseMixin > > from lp.registry.interfaces.pocket import PackagePublishingPocket > > @@ -31,15 +32,16 @@ > > only classes deriving from PackageBuild should be used. > > """ > > > > - def makePackageBuild(self, archive=None): > > + def makePackageBuild( > > Please move this into the factory. Hrm... did you take into account the comment above that method? It's not in the diff so you may not have seen it: "This is not included in the launchpad test factory because only classes deriving from PackageBuild should be used." Or maybe that's not a good reason to omit it from the factory? My reasoning was that there shouldn't be other tests outside of PackageBuild that need to create one - it should always be a derived class. > > > + self, archive=None, job_type=BuildFarmJobType.PACKAGEBUILD, > > + status=BuildStatus.NEEDSBUILD, > > + pocket=PackagePublishingPocket.RELEASE): > > if archive is None: > > archive = self.factory.makeArchive() > > > > return getUtility(IPackageBuildSource).new( > > - job_type=BuildFarmJobType.PACKAGEBUILD, > > - virtualized=True, > > - archive=archive, > > - pocket=PackagePublishingPocket.RELEASE) > > + job_type=job_type, virtualized=True, archive=archive, > > + status=status, pocket=pocket) > > > > > > class TestBuildBaseMethods(TestPackageBuildBase, TestBuildBaseMixin): > > @@ -176,5 +178,46 @@ > > u'My deps', self.package_build.dependencies) > > > > > > +class TestPackageBuildSet(TestPackageBuildBase): > > It's missing from __all__. Is that intentional? Because it's not intended to be exported (none of the test cases in this module are exported). Similar to lp.code.model.tests.test_sourcepackagerecipebuild. Or did I miss something? > > > + > > + layer = LaunchpadFunctionalLayer > > + > > + def setUp(self): > > + super(TestPackageBuildSet, self).setUp() > > + joe = self.factory.makePerson(name="joe") > > + self.archive = self.factory.makeArchive(owner=joe, name="ppa") > > What are the "joe" and "ppa" names for? You never mention them again. Removed (re-used from previous test case that tested urls). > > === modified file 'lib/lp/registry/model/sourcepackage.py' > > --- lib/lp/registry/model/sourcepackage.py 2010-06-09 08:26:26 +0000 > > +++ lib/lp/registry/model/sourcepackage.py 2010-06-14 08:35:42 +0000 > > @@ -508,13 +508,15 @@ > > return not self.__eq__(other) > > > > def getBuildRecords(self, build_state=None, name=None, pocket=None, > > - arch_tag=None, user=None): > > + arch_tag=None, user=None, binary_only=True): > > # Ignore "user", since it would not make any difference to the > > # records returned here (private builds are only in PPA right > > # now and this method only returns records for SPRs in a > > # distribution). > > # We also ignore the name parameter (required as part of the > > - # IHasBuildRecords interface) and use our own name. > > + # IHasBuildRecords interface) and use our own name and the > > + # binary_only parameter as a source package can only have > > + # binary builds. > > Thanks for updating the comment. > > > > > """See `IHasBuildRecords`""" > > I know you did not introduce this but shouldn't the doc string come directly > after the definition line? I have just never seen it this way round ... Me either. Updated. > > > clauseTables = ['SourcePackageRelease', > > > > > === modified file 'lib/lp/soyuz/configure.zcml' > > OK. Adapters are kinda cool. ;) > > > === modified file 'lib/lp/soyuz/interfaces/archive.py' > > OK. > > > === modified file 'lib/lp/soyuz/interfaces/buildrecords.py' > > OK. > > > === modified file 'lib/lp/soyuz/model/archive.py' > > --- lib/lp/soyuz/model/archive.py 2010-06-08 11:30:29 +0000 > > +++ lib/lp/soyuz/model/archive.py 2010-06-14 08:35:42 +0000 > > @@ -34,6 +34,7 @@ > > cursor, quote, quote_like, sqlvalues, SQLBase) > > from canonical.launchpad.interfaces.lpstorm import ISlaveStore > > from lp.buildmaster.interfaces.buildbase import BuildStatus > > +from lp.buildmaster.interfaces.packagebuild import IPackageBuildSet > > from lp.buildmaster.model.buildfarmjob import BuildFarmJob > > from lp.buildmaster.model.packagebuild import PackageBuild > > from lp.services.job.interfaces.job import JobStatus > > @@ -65,7 +66,7 @@ > > ArchiveNotPrivate, ArchivePurpose, ArchiveStatus, CannotCopy, > > CannotSwitchPrivacy, CannotUploadToPPA, CannotUploadToPocket, > > DistroSeriesNotFound, IArchive, IArchiveSet, IDistributionArchive, > > - InsufficientUploadRights, InvalidPocketForPPA, > > + IncompatibleArguments, InsufficientUploadRights, InvalidPocketForPPA, > > InvalidPocketForPartnerArchive, InvalidComponent, IPPA, > > MAIN_ARCHIVE_PURPOSES, NoRightsForArchive, NoRightsForComponent, > > NoSuchPPA, NoTokensForTeams, PocketNotFound, VersionRequiresName, > > @@ -366,12 +367,21 @@ > > return None > > > > def getBuildRecords(self, build_state=None, name=None, pocket=None, > > - arch_tag=None, user=None): > > + arch_tag=None, user=None, binary_only=True): > > """See IHasBuildRecords""" > > # Ignore "user", since anyone already accessing this archive > > # will implicitly have permission to see it. > > - return getUtility(IBinaryPackageBuildSet).getBuildsForArchive( > > - self, build_state, name, pocket, arch_tag) > > + > > + if binary_only: > > + return getUtility(IBinaryPackageBuildSet).getBuildsForArchive( > > + self, build_state, name, pocket, arch_tag) > > + else: > > + if arch_tag is not None or name is not None: > > + raise IncompatibleArguments( > > + "The 'arch_tag' and 'name' parameters can be used only > " > > + "with binary_only=True.") > > I am not very good at webservice design but couldn't you also just ignore > those parameters that are not used? And document that in the interface. Yes you can, but here the inteface (and API) is defined once in IHasBuildRecords.getBuildRecords() and included for multiple implementations. > > Of course, this makes the webservice easier to debug, so maybe this is > great... > > > + return getUtility(IPackageBuildSet).getBuildsForArchive( > > + self, status=build_state, pocket=pocket) > > > > def getPublishedSources(self, name=None, version=None, status=None, > > distroseries=None, pocket=None, > > > > === modified file 'lib/lp/soyuz/model/binarypackagebuild.py' > > --- lib/lp/soyuz/model/binarypackagebuild.py 2010-06-09 12:13:53 +0000 > > +++ lib/lp/soyuz/model/binarypackagebuild.py 2010-06-14 08:35:42 +0000 > > @@ -65,6 +65,26 @@ > > PackageUpload, PackageUploadBuild) > > > > > > +def get_binary_build_for_build_farm_job(build_farm_job): > > + """Factory method to returning a binary for a build farm job.""" > > + # No need to query the db if the build_farm_job doesn't have > > + # the correct job type. > > + if build_farm_job.job_type != BuildFarmJobType.PACKAGEBUILD: > > + return None > > + > > + store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) > > + resulting_tuple = store.find( > > + (BinaryPackageBuild, PackageBuild, BuildFarmJob), > > I think you mean this: > resulting_tuple = store.using( > BinaryPackageBuild, PackageBuild, BuildFarmJob).find( > BinaryPackageBuild, > > Saves some serialization... They are actually different. I'm specifying the find-spec above (ie. the tables/expressions that are *returned* by the find, where as aiui 'using' is for explicitly declaring joins (usually when you need a LeftJoin or similar). At least, that's the only time I've needed to use using. Anyway, to make this difference clearer, I've done: find_spec = (BinaryPackageBuild, PackageBuild, BuildFarmJob) resulting_tuple = store.find( find_spec, BinaryPackageBuild.package_build == PackageBuild.id, PackageBuild.build_farm_job == BuildFarmJob.id, BuildFarmJob.id == build_farm_job.id).one() See what you think. > > > + BinaryPackageBuild.package_build == PackageBuild.id, > > + PackageBuild.build_farm_job == BuildFarmJob.id, > > + BuildFarmJob.id == build_farm_job.id).one() > > + > > + if resulting_tuple is None: > > + return None > > + > > + return resulting_tuple[0] > > ... and the [0] can go away. erm, really? I'm confused now. The whole point is to pre-load the data... see the test: test_adapt_from_build_farm_job_prefetching If there's a way to get that test to past without that find_spec, I'm keen to hear about it! :) > > > + > > + > > class BinaryPackageBuild(PackageBuildDerived, SQLBase): > > implements(IBinaryPackageBuild) > > _table = 'BinaryPackageBuild' > > > > === modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py' > > --- lib/lp/soyuz/tests/test_binarypackagebuild.py 2010-06-09 12:07:58 > +0000 > > +++ lib/lp/soyuz/tests/test_binarypackagebuild.py 2010-06-14 08:35:42 > +0000 > > @@ -15,6 +15,7 @@ > > from canonical.testing import LaunchpadZopelessLayer > > from lp.services.job.model.job import Job > > from lp.buildmaster.interfaces.buildbase import BuildStatus > > +from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType > > from lp.buildmaster.interfaces.builder import IBuilderSet > > from lp.buildmaster.interfaces.buildqueue import IBuildQueue > > from lp.buildmaster.interfaces.packagebuild import IPackageBuild > > @@ -31,7 +32,7 @@ > > from lp.soyuz.model.processor import ProcessorFamilySet > > from lp.soyuz.tests.soyuzbuilddhelpers import WaitingSlave > > from lp.soyuz.tests.test_publishing import SoyuzTestPublisher > > -from lp.testing import TestCaseWithFactory > > +from lp.testing import record_statements, TestCaseWithFactory > > Ah, I see you may not know about "assertStatementCount" ? No - I'd only used record_statements... great, and updated. > > === modified file 'lib/lp/soyuz/tests/test_hasbuildrecords.py' > > --- lib/lp/soyuz/tests/test_hasbuildrecords.py 2010-05-06 10:35:17 > +0000 > > +++ lib/lp/soyuz/tests/test_hasbuildrecords.py 2010-06-14 08:35:42 > +0000 > > @@ -12,6 +12,11 @@ > > > > from lp.registry.model.sourcepackage import SourcePackage > > from lp.buildmaster.interfaces.builder import IBuilderSet > > +from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType > > +from lp.buildmaster.interfaces.packagebuild import ( > > + IPackageBuildSource) > > +from lp.registry.interfaces.pocket import PackagePublishingPocket > > +from lp.soyuz.interfaces.archive import IncompatibleArguments > > from lp.soyuz.interfaces.buildrecords import IHasBuildRecords > > from lp.soyuz.model.processor import ProcessorFamilySet > > from lp.soyuz.tests.test_binarypackagebuild import ( > > @@ -81,6 +86,28 @@ > > > > self.context = self.publisher.distroseries.main_archive > > > > + def test_binary_only_false(self): > > + # An archive can optionally return the more general > > + # package build objects. > > + spr_build = getUtility(IPackageBuildSource).new( > > + job_type=BuildFarmJobType.RECIPEBRANCHBUILD, virtualized=True, > > + archive=self.context, pocket=PackagePublishingPocket.RELEASE) > > Good thing you have a method in LaunchpadObjectFactory for this now, is it > not? :-P Gar - sprung! I'm still not sure that it belongs in the factory as we shouldn't ever be creating package builds on their own (without their related BinaryPackageBuild or SPRecipeBuild) (for the same reason, there's no makeBuildFarmJob in the factory. The reason this test does so is because currently there are no other implementations to use. I've added a comment to that effect: # Until we have different IBuildFarmJob types implemented, we # can only test this by creating a lone PackageBuild of a # different type. But let me know if you're still keen for it to be a factory method. > > > + > > + builds = self.context.getBuildRecords(binary_only=True) > > + self.failUnlessEqual(3, builds.count()) > > + > > + builds = self.context.getBuildRecords(binary_only=False) > > + self.failUnlessEqual(4, builds.count()) > > + > > + def test_incompatible_arguments(self): > > + # binary_only=False is incompatible with arch_tag and name. > > + self.failUnlessRaises( > > + IncompatibleArguments, self.context.getBuildRecords, > > + binary_only=False, arch_tag="anything") > > + self.failUnlessRaises( > > + IncompatibleArguments, self.context.getBuildRecords, > > + binary_only=False, name="anything") > > + > > > > class TestBuilderHasBuildRecords(TestHasBuildRecordsInterface): > > """Test the Builder implementation of IHasBuildRecords.""" > > > > This is a good branch but as I am really interested in your reply I am not > marking this as approved yet. ;-) OK, let me know what you think. Thanks Henning!