Merge lp:~michael.nelson/launchpad/create-source-recipe-build2 into lp:launchpad

[Michael Nelson]

Adds ISourcePackageRecipe.requestBuild(), and ensures that classes that say they implement IBuildBase actually do (bug 510919)

[Michael Nelson (michael.nelson)]

= Summary =
This branch builds on the work that james_w had approved at:

https://code.edge.launchpad.net/~james-w/launchpad/create-source-recipe-build/+merge/17435

I merged devel, so am not able to specify it as the pre-requisite branch (without showing tons of diff), so you can check my specific changes at:

http://pastebin.ubuntu.com/368292/

or with bzr diff -r 10188..10194

It does the following:
 1. Deals with the review feedback from the previous branch,
 2. Ensures that IBuildBase is provided by both SourcePackageRecipeBuild and (Binary)Build (bug 510919)
 3. Cleans some lint.

== Proposed fix ==

== Pre-implementation notes ==

Chatted with james_w briefly about what needed to be done.

== Implementation details ==

== Tests ==

bin/test -vv -t test_requestBuild -t TestBuildInterface

== Demo and Q/A ==

= Launchpad lint =

I'm not sure why lint is complaining about 'self' being missing from interface declarations?

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/soyuz/tests/test_sourcepackagerecipebuild.py
  lib/lp/archiveuploader/uploadprocessor.py
  lib/lp/archiveuploader/permission.py
  lib/lp/buildmaster/model/buildbase.py
  lib/lp/soyuz/tests/test_build.py
  lib/lp/soyuz/tests/test_sourcepackagerecipe.py
  lib/lp/buildmaster/tests/test_buildbase.py
  lib/lp/soyuz/model/sourcepackagerecipebuild.py
  lib/lp/archiveuploader/tests/test_permission.py
  lib/lp/soyuz/model/sourcepackagerecipe.py
  lib/lp/buildmaster/interfaces/buildbase.py
  lib/lp/soyuz/interfaces/sourcepackagerecipe.py
  lib/lp/soyuz/model/build.py

== Pylint notices ==

lib/lp/soyuz/tests/test_sourcepackagerecipe.py
    11: [F0401] Unable to import 'bzrlib.plugins.builder.recipe' (No module named builder)

lib/lp/buildmaster/interfaces/buildbase.py
    14: [F0401] Unable to import 'lazr.enum' (No module named enum)
    15: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    16: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)

lib/lp/soyuz/interfaces/sourcepackagerecipe.py
    14: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    77: [E0211, ISourcePackageRecipe.getReferencedBranches] Method has no argument
    80: [E0213, ISourcePackageRecipe.requestBuild] Method should have "self" as first argument
    95: [E0213, ISourcePackageRecipeSource.new] Method should have "self" as first argument

[Muharem Hrnjadovic (al-maisan)]

Looks good.

review: Approve

=== modified file 'lib/lp/archiveuploader/permission.py'
--- lib/lp/archiveuploader/permission.py	2010-01-15 01:37:51 +0000
+++ lib/lp/archiveuploader/permission.py	2010-02-05 10:56:18 +0000
@@ -21,24 +21,21 @@
 from lp.soyuz.interfaces.archive import ArchivePurpose
 
 
-class CannotUploadToArchive:
+class CannotUploadToArchive(Exception):
     """A reason for not being able to upload to an archive."""
 
     _fmt = '%(person)s has no upload rights to %(archive)s.'
 
     def __init__(self, **args):
         """Construct a `CannotUploadToArchive`."""
-        self._message = self._fmt % args
-
-    def __str__(self):
-        return self._message
-
-
-class CannotUploadToPocket:
+        Exception.__init__(self, self._fmt % args)
+
+
+class CannotUploadToPocket(Exception):
     """Returned when a pocket is closed for uploads."""
 
     def __init__(self, distroseries, pocket):
-        super(CannotUploadToPocket, self).__init__(
+        Exception.__init__(self,
             "Not permitted to upload to the %s pocket in a series in the "
             "'%s' state." % (pocket.name, distroseries.status.name))
 
@@ -73,7 +70,7 @@
         "Signer is not permitted to upload to the component '%(component)s'.")
 
     def __init__(self, component):
-        super(NoRightsForComponent, self).__init__(component=component.name)
+        CannotUploadToArchive.__init__(self, component=component.name)
 
 
 class InvalidPocketForPPA(CannotUploadToArchive):
@@ -88,6 +85,15 @@
     _fmt = "Partner uploads must be for the RELEASE or PROPOSED pocket."
 
 
+class ArchiveDisabled(CannotUploadToArchive):
+    """Uploading to a disabled archive is not allowed."""
+
+    _fmt = ("%(archive_name)s is disabled.")
+
+    def __init__(self, archive_name):
+        CannotUploadToArchive.__init__(self, archive_name=archive_name)
+
+
 def components_valid_for(archive, person):
     """Return the components that 'person' can upload to 'archive'.
 
@@ -202,6 +208,9 @@
     :return: CannotUploadToArchive if 'person' cannot upload to the archive,
         None otherwise.
     """
+    if not archive.enabled:
+        return ArchiveDisabled(archive.displayname)
+
     # For PPAs...
     if archive.is_ppa:
         if not archive.canUpload(person):

=== modified file 'lib/lp/archiveuploader/tests/test_permission.py'
--- lib/lp/archiveuploader/tests/test_permission.py	2009-12-13 11:55:40 +0000
+++ lib/lp/archiveuploader/tests/test_permission.py	2010-02-05 10:56:18 +0000
@@ -241,6 +241,14 @@
         self.assertCanUpload(
             person, spn, archive, component_a, strict_component=False)
 
+    def test_cannot_upload_to_disabled_archive(self):
+        spn = self.factory.makeSourcePackageName()
+        archive = self.factory.makeArchive()
+        removeSecurityProxy(archive).disable()
+        component = self.factory.makeComponent()
+        self.assertCannotUpload(u"%s is disabled." % (archive.displayname),
+            archive.owner, spn, archive, component)
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/buildmaster/interfaces/buildbase.py'
--- lib/lp/buildmaster/interfaces/buildbase.py	2010-01-22 04:11:36 +0000
+++ lib/lp/buildmaster/interfaces/buildbase.py	2010-02-05 10:56:18 +0000
@@ -109,7 +109,7 @@
             title=_("Distribution"), required=True,
             description=_("Shortcut for its distribution.")))
 
-    def getUploaderCommand(upload_leaf):
+    def getUploaderCommand(upload_leaf, uploader_logfilename):
         """Get the command to run as the uploader.
 
         :return: A list of command line arguments, beginning with the

=== modified file 'lib/lp/buildmaster/model/buildbase.py'
--- lib/lp/buildmaster/model/buildbase.py	2010-01-21 22:09:51 +0000
+++ lib/lp/buildmaster/model/buildbase.py	2010-02-05 10:56:18 +0000
@@ -16,7 +16,6 @@
 import subprocess
 
 from storm.store import Store
-from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
@@ -24,16 +23,21 @@
 from canonical.database.sqlbase import (
     clear_current_connection_cache, cursor, flush_database_updates)
 from canonical.librarian.utils import copy_and_close
-from lp.buildmaster.interfaces.buildbase import IBuildBase
 from lp.registry.interfaces.pocket import pocketsuffix
 from lp.soyuz.interfaces.build import BuildStatus
 from lp.soyuz.model.buildqueue import BuildQueue
 
 
 class BuildBase:
-
-    implements(IBuildBase)
-
+    """A mixin class providing functionality for farm jobs that build a
+    package.
+
+    Note: this class does not implement IBuildBase as we currently duplicate
+    the properties defined on IBuildBase on the inheriting class tables. 
+    BuildBase cannot therefore implement IBuildBase itself, as storm requires
+    that the corresponding __storm_table__ be defined for the class. Instead,
+    the classes using the BuildBase mixin must ensure that they implement IBuildBase.
+    """
     policy_name = 'buildd'
 
     def getUploadLeaf(self, build_id, now=None):

=== modified file 'lib/lp/buildmaster/tests/test_buildbase.py'
--- lib/lp/buildmaster/tests/test_buildbase.py	2010-01-21 22:35:21 +0000
+++ lib/lp/buildmaster/tests/test_buildbase.py	2010-02-05 10:56:18 +0000
@@ -11,7 +11,6 @@
 
 from canonical.config import config
 from canonical.testing.layers import DatabaseFunctionalLayer
-from lp.buildmaster.interfaces.buildbase import IBuildBase
 from lp.buildmaster.model.buildbase import BuildBase
 from lp.registry.interfaces.pocket import pocketsuffix
 from lp.testing import TestCase, TestCaseWithFactory
@@ -20,13 +19,6 @@
 class TestBuildBase(TestCase):
     """Tests for `IBuildBase`."""
 
-    def disabled_test_build_base_provides_interface(self):
-        # XXX: JonathanLange 2010-01-22 bug=510919: BuildBase is supposed to
-        # implement IBuildBase, but doesn't atm. Since it's not the focus of
-        # the branch, we'll postpone the work.
-        build_base = BuildBase()
-        self.assertProvides(build_base, IBuildBase)
-
     def test_getUploadLeaf(self):
         # getUploadLeaf returns the current time, followed by the build id.
         build_base = BuildBase()

=== modified file 'lib/lp/soyuz/interfaces/sourcepackagerecipe.py'
--- lib/lp/soyuz/interfaces/sourcepackagerecipe.py	2010-01-14 21:31:28 +0000
+++ lib/lp/soyuz/interfaces/sourcepackagerecipe.py	2010-02-05 10:56:18 +0000
@@ -29,6 +29,7 @@
     """A forbidden instruction was found in the recipe."""
 
     def __init__(self, instruction_name):
+        super(ForbiddenInstruction, self).__init__()
         self.instruction_name = instruction_name
 
 
@@ -36,6 +37,7 @@
     """The format of the recipe supplied was too new."""
 
     def __init__(self, supplied_format, newest_supported):
+        super(TooNewRecipeFormat, self).__init__()
         self.supplied_format = supplied_format
         self.newest_supported = newest_supported
 
@@ -75,6 +77,16 @@
     def getReferencedBranches():
         """An iterator of the branches referenced by this recipe."""
 
+    def requestBuild(archive, distroseries, requester, pocket):
+        """Request that the recipe be built in to the specified archive.
+
+        :param archive: The IArchive which you want the build to end up in.
+        :param requester: the person requesting the build.
+        :param pocket: the pocket that should be targeted.
+        :raises: various specific upload errors if the requestor is not
+            able to upload to the archive.
+        """
+
 
 class ISourcePackageRecipeSource(Interface):
     """A utility of this interface can be used to create and access recipes.

=== modified file 'lib/lp/soyuz/model/build.py'
--- lib/lp/soyuz/model/build.py	2010-01-20 22:09:26 +0000
+++ lib/lp/soyuz/model/build.py	2010-02-05 10:56:18 +0000
@@ -11,9 +11,6 @@
 import datetime
 import logging
 import operator
-import os
-import subprocess
-import time
 
 from zope.interface import implements
 from zope.component import getUtility
@@ -30,8 +27,7 @@
 from canonical.database.datetimecol import UtcDateTimeCol
 from canonical.database.enumcol import EnumCol
 from canonical.database.sqlbase import (
-    clear_current_connection_cache, flush_database_updates, cursor,
-    quote_like, SQLBase, sqlvalues)
+    cursor, quote_like, SQLBase, sqlvalues)
 from canonical.launchpad.components.decoratedresultset import (
     DecoratedResultSet)
 from canonical.launchpad.database.librarian import (
@@ -47,17 +43,16 @@
 from canonical.launchpad.webapp.interfaces import (
     IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
 from canonical.launchpad.webapp.tales import DurationFormatterAPI
-from canonical.librarian.utils import copy_and_close
 from lp.archivepublisher.utils import get_ppa_reference
 from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType
 from lp.buildmaster.model.buildbase import BuildBase
-from lp.registry.interfaces.pocket import (PackagePublishingPocket,
-    pocketsuffix)
+from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.job.model.job import Job
 from lp.soyuz.adapters.archivedependencies import get_components_for_building
 from lp.soyuz.interfaces.archive import ArchivePurpose
 from lp.soyuz.interfaces.build import (
     BuildStatus, BuildSetStatus, CannotBeRescored, IBuild, IBuildSet)
+from lp.buildmaster.interfaces.buildbase import IBuildBase
 from lp.buildmaster.interfaces.builder import IBuilderSet
 from lp.soyuz.interfaces.publishing import active_publishing_status
 from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
@@ -71,7 +66,7 @@
 
 
 class Build(BuildBase, SQLBase):
-    implements(IBuild)
+    implements(IBuildBase, IBuild)
     _table = 'Build'
     _defaultOrder = 'id'
 

=== modified file 'lib/lp/soyuz/model/sourcepackagerecipe.py'
--- lib/lp/soyuz/model/sourcepackagerecipe.py	2010-01-13 03:18:49 +0000
+++ lib/lp/soyuz/model/sourcepackagerecipe.py	2010-02-05 10:56:18 +0000
@@ -10,16 +10,27 @@
 
 from storm.locals import Int, Reference, Store, Storm, Unicode
 
+from zope.component import getUtility
 from zope.interface import classProvides, implements
 
 from canonical.database.datetimecol import UtcDateTimeCol
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
 
+from lp.soyuz.interfaces.archive import ArchivePurpose
+from lp.soyuz.interfaces.component import IComponentSet
+from lp.archiveuploader.permission import check_upload_to_archive
 from lp.soyuz.interfaces.sourcepackagerecipe import (
     ISourcePackageRecipe, ISourcePackageRecipeSource)
+from lp.soyuz.interfaces.sourcepackagerecipebuild import (
+    ISourcePackageRecipeBuildSource)
 from lp.soyuz.model.sourcepackagerecipedata import _SourcePackageRecipeData
 
 
+class NonPPABuildRequest(Exception):
+    """A build was requested to a non-PPA and this is currently
+    unsupported."""
+
+
 class SourcePackageRecipe(Storm):
     """See `ISourcePackageRecipe` and `ISourcePackageRecipeSource`."""
 
@@ -82,3 +93,21 @@
         sprecipe.name = name
         store.add(sprecipe)
         return sprecipe
+
+    def requestBuild(self, archive, requester, pocket):
+        """See `ISourcePackageRecipe`."""
+        if archive.purpose != ArchivePurpose.PPA:
+            raise NonPPABuildRequest
+        component = getUtility(IComponentSet)["multiverse"]
+        reject_reason = check_upload_to_archive(
+            requester, self.distroseries, self.sourcepackagename,
+            archive, component, pocket)
+        if reject_reason is not None:
+            raise reject_reason
+
+        sourcepackage = self.distroseries.getSourcePackage(
+            self.sourcepackagename)
+        build = getUtility(ISourcePackageRecipeBuildSource).new(sourcepackage,
+            self, requester, archive)
+        build.queueBuild()
+        return build

=== modified file 'lib/lp/soyuz/model/sourcepackagerecipebuild.py'
--- lib/lp/soyuz/model/sourcepackagerecipebuild.py	2010-01-22 04:01:17 +0000
+++ lib/lp/soyuz/model/sourcepackagerecipebuild.py	2010-02-05 10:56:18 +0000
@@ -21,6 +21,7 @@
 from zope.component import getUtility
 from zope.interface import classProvides, implements
 
+from lp.buildmaster.interfaces.buildbase import IBuildBase
 from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType
 from lp.buildmaster.model.buildbase import BuildBase
 from lp.buildmaster.model.packagebuildfarmjob import PackageBuildFarmJob
@@ -41,7 +42,7 @@
 
     policy_name = 'recipe'
 
-    implements(ISourcePackageRecipeBuild)
+    implements(IBuildBase, ISourcePackageRecipeBuild)
     classProvides(ISourcePackageRecipeBuildSource)
 
     build_farm_job_type = BuildFarmJobType.RECIPEBRANCHBUILD

=== modified file 'lib/lp/soyuz/tests/test_build.py'
--- lib/lp/soyuz/tests/test_build.py	2010-01-11 23:43:59 +0000
+++ lib/lp/soyuz/tests/test_build.py	2010-02-05 10:56:18 +0000
@@ -3,6 +3,8 @@
 
 """Test Build features."""
 
+from datetime import datetime, timedelta
+import pytz
 import unittest
 
 from storm.store import Store
@@ -10,9 +12,10 @@
 
 from canonical.testing import LaunchpadZopelessLayer
 from lp.services.job.model.job import Job
+from lp.buildmaster.interfaces.buildbase import IBuildBase
 from lp.buildmaster.interfaces.builder import IBuilderSet
 from lp.soyuz.interfaces.component import IComponentSet
-from lp.soyuz.interfaces.build import BuildStatus, IBuildSet
+from lp.soyuz.interfaces.build import BuildStatus, IBuild, IBuildSet
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
 from lp.soyuz.model.buildqueue import BuildQueue
 from lp.soyuz.model.buildpackagejob import BuildPackageJob
@@ -21,6 +24,26 @@
 from lp.testing import TestCaseWithFactory
 
 
+class TestBuildInterface(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def test_providesInterfaces(self):
+        # Build provides IBuildBase and IBuild.
+        publisher = SoyuzTestPublisher()
+        publisher.prepareBreezyAutotest()
+        gedit_src_hist = publisher.getPubSource(
+            sourcename="gedit", status=PackagePublishingStatus.PUBLISHED)
+        build = gedit_src_hist.createMissingBuilds()[0]
+
+        # The IBuild.calculated_buildstart property asserts
+        # that both datebuilt and buildduration are set.
+        build.datebuilt = datetime.now(pytz.UTC)
+        build.buildduration = timedelta(0, 1)
+
+        self.assertProvides(build, IBuildBase)
+        self.assertProvides(build, IBuild)
+
 class TestBuildUpdateDependencies(TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer

=== modified file 'lib/lp/soyuz/tests/test_sourcepackagerecipe.py'
--- lib/lp/soyuz/tests/test_sourcepackagerecipe.py	2010-01-13 03:18:49 +0000
+++ lib/lp/soyuz/tests/test_sourcepackagerecipe.py	2010-02-05 10:56:18 +0000
@@ -10,14 +10,33 @@
 
 from bzrlib.plugins.builder.recipe import RecipeParser
 
+from storm.locals import Store
+
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.testing.layers import DatabaseFunctionalLayer
 
+from lp.archiveuploader.permission import (
+    ArchiveDisabled, CannotUploadToArchive, InvalidPocketForPPA)
+from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.job.interfaces.job import (
+    IJob, JobStatus)
+from lp.soyuz.interfaces.archive import ArchivePurpose
+from lp.soyuz.interfaces.buildqueue import (
+    IBuildQueue)
 from lp.soyuz.interfaces.sourcepackagerecipe import (
     ForbiddenInstruction, ISourcePackageRecipe, ISourcePackageRecipeSource,
     TooNewRecipeFormat)
+from lp.soyuz.interfaces.sourcepackagerecipebuild import (
+    ISourcePackageRecipeBuild, ISourcePackageRecipeBuildJob)
+from lp.soyuz.model.buildqueue import (
+    BuildQueue)
+from lp.soyuz.model.sourcepackagerecipebuild import (
+    SourcePackageRecipeBuildJob)
+from lp.soyuz.model.sourcepackagerecipe import (
+    NonPPABuildRequest)
 from lp.testing import login_person, TestCaseWithFactory
 
 
@@ -154,6 +173,54 @@
             TooNewRecipeFormat,
             self.makeSourcePackageRecipeFromBuilderRecipe, builder_recipe)
 
+    def test_requestBuild(self):
+        recipe = self.factory.makeSourcePackageRecipe()
+        ppa = self.factory.makeArchive()
+        build = recipe.requestBuild(ppa, ppa.owner,
+                PackagePublishingPocket.RELEASE)
+        self.assertProvides(build, ISourcePackageRecipeBuild)
+        self.assertEqual(build.archive, ppa)
+        self.assertEqual(build.distroseries, recipe.distroseries)
+        self.assertEqual(build.requester, ppa.owner)
+        store = Store.of(build)
+        store.flush()
+        build_job = store.find(SourcePackageRecipeBuildJob,
+                SourcePackageRecipeBuildJob.build_id==build.id).one()
+        self.assertProvides(build_job, ISourcePackageRecipeBuildJob)
+        self.assertTrue(build_job.virtualized)
+        job = build_job.job
+        self.assertProvides(job, IJob)
+        self.assertEquals(job.status, JobStatus.WAITING)
+        build_queue = store.find(BuildQueue, BuildQueue.job==job.id).one()
+        self.assertProvides(build_queue, IBuildQueue)
+        self.assertTrue(build_queue.virtualized)
+
+    def test_requestBuildRejectsNotPPA(self):
+        recipe = self.factory.makeSourcePackageRecipe()
+        not_ppa = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY)
+        self.assertRaises(NonPPABuildRequest, recipe.requestBuild, not_ppa,
+                not_ppa.owner, PackagePublishingPocket.RELEASE)
+
+    def test_requestBuildRejectsNoPermission(self):
+        recipe = self.factory.makeSourcePackageRecipe()
+        ppa = self.factory.makeArchive()
+        requester = self.factory.makePerson()
+        self.assertRaises(CannotUploadToArchive, recipe.requestBuild, ppa,
+                requester, PackagePublishingPocket.RELEASE)
+
+    def test_requestBuildRejectsInvalidPocket(self):
+        recipe = self.factory.makeSourcePackageRecipe()
+        ppa = self.factory.makeArchive()
+        self.assertRaises(InvalidPocketForPPA, recipe.requestBuild, ppa,
+                ppa.owner, PackagePublishingPocket.BACKPORTS)
+
+    def test_requestBuildRejectsDisabledArchive(self):
+        recipe = self.factory.makeSourcePackageRecipe()
+        ppa = self.factory.makeArchive()
+        removeSecurityProxy(ppa).disable()
+        self.assertRaises(ArchiveDisabled, recipe.requestBuild, ppa,
+                ppa.owner, PackagePublishingPocket.RELEASE)
+
 
 class TestRecipeBranchRoundTripping(TestCaseWithFactory):
 

=== modified file 'lib/lp/soyuz/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/soyuz/tests/test_sourcepackagerecipebuild.py	2010-01-20 01:24:01 +0000
+++ lib/lp/soyuz/tests/test_sourcepackagerecipebuild.py	2010-02-05 10:56:18 +0000
@@ -13,6 +13,7 @@
 
 from canonical.testing.layers import DatabaseFunctionalLayer
 
+from lp.buildmaster.interfaces.buildbase import IBuildBase
 from lp.soyuz.interfaces.buildqueue import IBuildQueue
 from lp.soyuz.interfaces.sourcepackagerecipebuild import (
     ISourcePackageRecipeBuildJob, ISourcePackageRecipeBuild,
@@ -33,9 +34,11 @@
             archive=self.factory.makeArchive(),
             requester=self.factory.makePerson())
 
-    def test_providesInterface(self):
-        # SourcePackageRecipeBuild provides ISourcePackageRecipeBuild.
+    def test_providesInterfaces(self):
+        # SourcePackageRecipeBuild provides IBuildBase and
+        # ISourcePackageRecipeBuild.
         spb = self.makeSourcePackageRecipeBuild()
+        self.assertProvides(spb, IBuildBase)
         self.assertProvides(spb, ISourcePackageRecipeBuild)
 
     def test_saves_record(self):