-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi Michael, thank you for your fix and sorry for the delay... 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? > > 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/configure.zcml' OK. > === modified file 'lib/lp/buildmaster/interfaces/packagebuild.py' OK. > === 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 > @@ -11,6 +11,7 @@ > from lazr.delegates import delegates > > from storm.locals import Int, Reference, Storm, Unicode > +from storm.expr import Desc > > from zope.component import getUtility > from zope.interface import classProvides, implements > @@ -19,13 +20,16 @@ > from canonical.launchpad.browser.librarian import ( > ProxiedLibraryFileAlias) > from canonical.launchpad.interfaces.lpstorm import IMasterStore > +from canonical.launchpad.webapp.interfaces import ( > + IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR) > > from lp.buildmaster.interfaces.buildbase import BuildStatus > from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSource > from lp.buildmaster.interfaces.packagebuild import ( > - IPackageBuild, IPackageBuildSource) > + IPackageBuild, IPackageBuildSet, IPackageBuildSource) > from lp.buildmaster.model.buildbase import handle_status_for_build, BuildBase > -from lp.buildmaster.model.buildfarmjob import BuildFarmJobDerived > +from lp.buildmaster.model.buildfarmjob import ( > + BuildFarmJob, BuildFarmJobDerived) > from lp.registry.interfaces.pocket import PackagePublishingPocket > from lp.soyuz.adapters.archivedependencies import ( > default_component_dependency_name) > @@ -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? > + 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? > + 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"? > + # * FULLYBUILT & FAILURES by -datebuilt I would put this near the "else" to explain which you expect there. > + # 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? > + 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 ... > + 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. > + 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? > + > + 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. > + self.package_builds = [] > + self.package_builds.append( > + self.makePackageBuild(archive=self.archive, > + pocket=PackagePublishingPocket.UPDATES)) > + self.package_builds.append( > + self.makePackageBuild(archive=self.archive, > + status=BuildStatus.BUILDING)) > + self.package_build_set = getUtility(IPackageBuildSet) > + > + def test_getBuildsForArchive_all(self): > + # The default call without arguments returns all builds for the > + # archive. > + self.assertContentEqual( > + self.package_builds, self.package_build_set.getBuildsForArchive( > + self.archive)) > + > + def test_getBuildsForArchive_by_status(self): > + # If the status arg is used, the results will be filtered by > + # status. > + self.assertContentEqual( > + self.package_builds[1:], > + self.package_build_set.getBuildsForArchive( > + self.archive, status=BuildStatus.BUILDING)) > + > + def test_getBuildsForArchive_by_pocket(self): > + # If the pocket arg is used, the results will be filtered by > + # pocket. > + self.assertContentEqual( > + self.package_builds[:1], > + self.package_build_set.getBuildsForArchive( > + self.archive, pocket=PackagePublishingPocket.UPDATES)) > + > + > def test_suite(): > return unittest.TestLoader().loadTestsFromName(__name__) > > === 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 ... > 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. 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... > + 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. > + > + > 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" ? > > > class TestBinaryPackageBuild(TestCaseWithFactory): > @@ -120,6 +121,45 @@ > self.build.build_farm_job.id), > self.build.log_url) > > + def test_adapt_from_build_farm_job(self): > + # An `IBuildFarmJob` can be adapted to an IBinaryPackageBuild > + # if it has the correct job type. > + build_farm_job = self.build.build_farm_job > + store = Store.of(build_farm_job) > + store.flush() > + > + binary_package_build = IBinaryPackageBuild(build_farm_job) > + self.failUnlessEqual(self.build, binary_package_build) > + > + def test_adapt_from_build_farm_job_wrong_type(self): > + # An `IBuildFarmJob` of the wrong type results is None. > + build_farm_job = self.build.build_farm_job > + removeSecurityProxy(build_farm_job).job_type = ( > + BuildFarmJobType.RECIPEBRANCHBUILD) > + > + binary_package_build = IBinaryPackageBuild(build_farm_job) > + self.failUnlessEqual(None, binary_package_build) > + > + def test_adapt_from_build_farm_job_prefetching(self): > + # The package_build is prefetched for efficiency. > + build_farm_job = self.build.build_farm_job > + > + # We clear the cache to avoid getting cached objects where > + # they would normally be queries. > + store = Store.of(build_farm_job) > + store.flush() > + store.reset() > + > + binary_package_build = IBinaryPackageBuild(build_farm_job) > + > + package_build, statements = record_statements( > + getattr, binary_package_build, 'package_build') > + self.failUnlessEqual(0, len(statements)) package_build = self.assertStatementCount(0, getattr, binary_package_build, 'package_build') > + > + build_farm_job, statements = record_statements( > + getattr, package_build, 'build_farm_job') > + self.failUnlessEqual(0, len(statements)) > + > > class TestBuildUpdateDependencies(TestCaseWithFactory): > > > === 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 > + > + 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. ;-) review needs-fixing code Cheers, Henning -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkwXSIQACgkQBT3oW1L17iiXggCdHIb1cM044UWvLzIaPGbHdhS4 axsAnj5XhZibyCFHghnIyw7Ttias9lnr =jMJ3 -----END PGP SIGNATURE-----