Merge lp:~salgado/launchpad/upload-policy-utility into lp:launchpad

[Guilherme Salgado]

Use named utilities for archive upload policies instead of implementing our own global registry for that.

Use named utilities for archive upload policies instead of implementing our
own global registry for that.

Also define the name for these utilities as constants/class-variables so that
they can be accessed externally and don't need to be hard-coded in multiple
places.

[Julian Edwards (julian-edwards)]

Really nice branch! I have a couple of comments:

> def callable():
> global called
> called += 1

Can this be lp.testing.fakemethod.FakeMethod instead?

Also, you've un-factored findPolicyByName(). Why didn't you fix it in one place for all the existing call sites to use instead of changing all the call sites?

review: Needs Information (code)

[Guilherme Salgado (salgado)]

On Wed, 2010-08-18 at 08:30 +0000, Julian Edwards wrote:
> Review: Needs Information code
> Really nice branch! I have a couple of comments:
>
> > def callable():
> > global called
> > called += 1
>
> Can this be lp.testing.fakemethod.FakeMethod instead?

Good catch; I keep forgetting about these fake* things that Jeroen
wrote.

>
> Also, you've un-factored findPolicyByName(). Why didn't you fix it in one place for all the existing call sites to use instead of changing all the call sites?

I guess you mean findPolicyByOptions()? I un-factored that because its
new implementation would be just two lines, but more importantly because
one of that method's side effects was setting the options (through
setOptions()) on the retrieved policy (which was in fact retrieved by
name and not by any of its options), and I since explicit is better than
implicit<tm> I thought it was a good idea to have the call sites call
setOptions() themselves.

--
Guilherme Salgado <https://launchpad.net/~salgado>

[Julian Edwards (julian-edwards)]

Well I guess I meant both :)

So considering:

- policy = findPolicyByName(name)
+ policy = getUtility(IArchiveUploadPolicy, name)()

I find the former far more readable - it actually tells me what it's doing *and* more importantly it's abstracting out *how* it works so we can tweak it more easily later if we want. The fact that it's the same line count doesn't really matter - in fact your diff would have been much smaller :)

As for moving out setOptions(), I think that's OK, but given my reasoning above, would you agree it makes sense at least to keep the old names?

review: Needs Information

[Guilherme Salgado (salgado)]

On Wed, 2010-08-18 at 13:13 +0000, Julian Edwards wrote:
> Review: Needs Information
> Well I guess I meant both :)
>
> So considering:
>
> - policy = findPolicyByName(name)
> + policy = getUtility(IArchiveUploadPolicy, name)()
>
> I find the former far more readable - it actually tells me what it's
> doing *and* more importantly it's abstracting out *how* it works so we
> can tweak it more easily later if we want. The fact that it's the
> same line count doesn't really matter - in fact your diff would have
> been much smaller :)

I see; that sounds ok to me. I'll do that change.

>
> As for moving out setOptions(), I think that's OK, but given my
> reasoning above, would you agree it makes sense at least to keep the
> old names?

names? Do you mean to keep findPolicyByOptions() as well? I don't see
any reason for doing that as it just uses options.context to lookup a
policy by name -- I'd rather have the callsites use
findPolicyByName(options.context) instead. How does that sound to you?

--
Guilherme Salgado <https://launchpad.net/~salgado>

[Julian Edwards (julian-edwards)]

 merge approve
 review approve

On Wednesday 18 August 2010 14:25:58 you wrote:
> On Wed, 2010-08-18 at 13:13 +0000, Julian Edwards wrote:
> > Review: Needs Information
> > Well I guess I meant both :)
> >
> > So considering:
> >
> > - policy = findPolicyByName(name)
> > + policy = getUtility(IArchiveUploadPolicy, name)()
> >
> > I find the former far more readable - it actually tells me what it's
> > doing *and* more importantly it's abstracting out *how* it works so we
> > can tweak it more easily later if we want. The fact that it's the
> > same line count doesn't really matter - in fact your diff would have
> > been much smaller :)
>
> I see; that sounds ok to me. I'll do that change.

Great!

>
> > As for moving out setOptions(), I think that's OK, but given my
> > reasoning above, would you agree it makes sense at least to keep the
> > old names?
>
> names? Do you mean to keep findPolicyByOptions() as well? I don't see
> any reason for doing that as it just uses options.context to lookup a
> policy by name -- I'd rather have the callsites use
> findPolicyByName(options.context) instead. How does that sound to you?

Ah yes of course, that's good.

Land it! Thanks for the improvement to Soyuz.

review: Approve

[Guilherme Salgado (salgado)]

Julian, I'd like to include some extra (related) changes on this branch; just moving test-specific stuff to lp/archiveuploader/tests. Ideally I'd have these utilities registered only when running tests, but that's not possible for the Scripts/Zopeless layer, so we have to register them all the time.

[Guilherme Salgado (salgado)]

http://paste.ubuntu.com/479960/ has the diff, btw.

[Julian Edwards (julian-edwards)]

On Wednesday 18 August 2010 16:02:38 you wrote:
> Julian, I'd like to include some extra (related) changes on this branch;
> just moving test-specific stuff to lp/archiveuploader/tests. Ideally I'd
> have these utilities registered only when running tests, but that's not
> possible for the Scripts/Zopeless layer, so we have to register them all
> the time.

I use those policies on the command line when I am doing testing on dogfood,
so I'd like to see them left where they are please.

[Guilherme Salgado (salgado)]

FTR:
<salgado> bigjools, you'll still be able to use them; they'll just live closer to the tests now. although I'd have to change that XXX into a comment explaining why we want the policies registered all the time
<bigjools> salgado: ok that's fine if we can still use them from the command line

=== modified file 'lib/canonical/launchpad/webapp/meta.zcml'
--- lib/canonical/launchpad/webapp/meta.zcml	2010-07-22 02:10:27 +0000
+++ lib/canonical/launchpad/webapp/meta.zcml	2010-08-18 16:15:57 +0000
@@ -26,6 +26,13 @@
       />
 
   <directive
+      namespace="http://namespaces.zope.org/zope"
+      name="call"
+      schema="canonical.launchpad.webapp.metazcml.ICallDirective"
+      handler="canonical.launchpad.webapp.metazcml.call"
+      />
+
+  <directive
       namespace="http://namespaces.zope.org/browser"
       name="feeds"
       schema="canonical.launchpad.webapp.metazcml.IFeedsDirective"

=== modified file 'lib/canonical/launchpad/webapp/metazcml.py'
--- lib/canonical/launchpad/webapp/metazcml.py	2010-08-13 15:26:43 +0000
+++ lib/canonical/launchpad/webapp/metazcml.py	2010-08-18 16:15:57 +0000
@@ -618,6 +618,22 @@
             self)
 
 
+class ICallDirective(Interface):
+    """Call the given callable.
+
+    This is useful when you have something that you want to call at startup
+    but don't want it tied to a specific zope event.  Or when you need to
+    register utilities in python at the time the zcml is processed.
+    """
+
+    callable = GlobalObject(
+        title=u"The thing that will be called.", required=True)
+
+
+def call(_context, callable):
+    callable()
+
+
 class IDefineLaunchpadPermissionDirective(IPermissionDirective):
 
     access_level = TextLine(

=== added file 'lib/canonical/launchpad/webapp/tests/test_metazcml.py'
--- lib/canonical/launchpad/webapp/tests/test_metazcml.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/tests/test_metazcml.py	2010-08-18 16:15:57 +0000
@@ -0,0 +1,30 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from zope.configuration import xmlconfig
+
+
+from lp.testing import TestCase
+from lp.testing.fakemethod import FakeMethod
+
+
+class TestCallDirective(TestCase):
+
+    def test_call(self):
+        directive = """ 
+            <call callable="%(this)s.callable" />
+            """ % dict(this=this)
+        xmlconfig.string(zcml_configure % directive)
+        self.assertEqual(1, callable.call_count)
+
+
+callable = FakeMethod()
+this = "canonical.launchpad.webapp.tests.test_metazcml"
+zcml_configure = """
+    <configure xmlns="http://namespaces.zope.org/zope">
+      <include package="canonical.launchpad.webapp" file="meta.zcml" />
+      %s
+    </configure>
+    """

=== modified file 'lib/lp/archiveuploader/tests/__init__.py'
--- lib/lp/archiveuploader/tests/__init__.py	2010-08-06 13:52:46 +0000
+++ lib/lp/archiveuploader/tests/__init__.py	2010-08-18 16:15:57 +0000
@@ -15,7 +15,10 @@
 import sys
 import traceback
 
-from lp.archiveuploader.uploadpolicy import findPolicyByName
+from zope.component import getGlobalSiteManager
+
+from lp.archiveuploader.uploadpolicy import (
+    AbstractUploadPolicy, findPolicyByName, IArchiveUploadPolicy)
 from lp.soyuz.model.queue import PackageUploadSet
 from canonical.librarian.ftests.harness import fillLibrarianFile
 
@@ -98,6 +101,55 @@
         self.print_traceback(exc_info)
 
 
+class AnythingGoesUploadPolicy(AbstractUploadPolicy):
+    """This policy is invoked when processing uploads from the test process.
+
+    We require a signed changes file but that's it.
+    """
+
+    name = 'anything'
+
+    def __init__(self):
+        AbstractUploadPolicy.__init__(self)
+        # We require the changes to be signed but not the dsc
+        self.unsigned_dsc_ok = True
+
+    def policySpecificChecks(self, upload):
+        """Nothing, let it go."""
+        pass
+
+    def rejectPPAUploads(self, upload):
+        """We allow PPA uploads."""
+        return False
+
+
+class AbsolutelyAnythingGoesUploadPolicy(AnythingGoesUploadPolicy):
+    """This policy is invoked when processing uploads from the test process.
+
+    Absolutely everything is allowed, for when you don't want the hassle
+    of dealing with inappropriate checks in tests.
+    """
+
+    name = 'absolutely-anything'
+
+    def __init__(self):
+        AnythingGoesUploadPolicy.__init__(self)
+        self.unsigned_changes_ok = True
+
+    def policySpecificChecks(self, upload):
+        """Nothing, let it go."""
+        pass
+
+
+def register_archive_upload_policy_adapters():
+    policies = [
+        AnythingGoesUploadPolicy, AbsolutelyAnythingGoesUploadPolicy]
+    sm = getGlobalSiteManager()
+    for policy in policies:
+        sm.registerUtility(
+            component=policy, provided=IArchiveUploadPolicy, name=policy.name)
+
+
 mock_options = MockUploadOptions()
 mock_logger = MockUploadLogger()
 mock_logger_quiet = MockUploadLogger(verbose=False)

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2010-08-13 06:08:36 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2010-08-18 16:15:57 +0000
@@ -19,14 +19,13 @@
 
 from email import message_from_string
 
-from zope.component import getUtility
+from zope.component import getGlobalSiteManager, getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.errors import NotFoundError
-from lp.archiveuploader.uploadpolicy import (AbstractUploadPolicy,
-    findPolicyByOptions)
+from lp.archiveuploader.uploadpolicy import (
+    AbstractUploadPolicy, IArchiveUploadPolicy, findPolicyByName)
 from lp.archiveuploader.uploadprocessor import UploadProcessor
-from lp.buildmaster.interfaces.buildbase import BuildStatus
 from canonical.config import config
 from canonical.database.constants import UTC_NOW
 from lp.soyuz.model.archivepermission import ArchivePermission
@@ -62,7 +61,7 @@
     ISourcePackageNameSet)
 from lp.services.mail import stub
 from canonical.launchpad.testing.fakepackager import FakePackager
-from lp.testing import TestCase, TestCaseWithFactory
+from lp.testing import TestCaseWithFactory
 from lp.testing.mail_helpers import pop_notifications
 from canonical.launchpad.webapp.errorlog import ErrorReportingUtility
 from canonical.testing import LaunchpadZopelessLayer
@@ -146,7 +145,9 @@
     def getUploadProcessor(self, txn):
         def getPolicy(distro):
             self.options.distro = distro.name
-            return findPolicyByOptions(self.options)
+            policy = findPolicyByName(self.options.context)
+            policy.setOptions(self.options)
+            return policy
         return UploadProcessor(
             self.options.base_fsroot, self.options.dryrun,
             self.options.nomails,
@@ -533,9 +534,12 @@
 
         See bug 35965.
         """
-        # Register our broken upload policy
-        AbstractUploadPolicy._registerPolicy(BrokenUploadPolicy)
-        self.options.context = 'broken'
+        self.options.context = u'broken'
+        # Register our broken upload policy.
+        getGlobalSiteManager().registerUtility(
+            component=BrokenUploadPolicy, provided=IArchiveUploadPolicy,
+            name=self.options.context)
+
         uploadprocessor = self.getUploadProcessor(self.layer.txn)
 
         # Upload a package to Breezy.

=== modified file 'lib/lp/archiveuploader/tests/uploadpolicy.txt'
--- lib/lp/archiveuploader/tests/uploadpolicy.txt	2010-07-24 09:12:37 +0000
+++ lib/lp/archiveuploader/tests/uploadpolicy.txt	2010-08-18 16:15:57 +0000
@@ -1,31 +1,39 @@
 == The uploader policies ==
 
-When the uploader is invoked, it is given a policy to work in. This
-governs such things as what tests get run at what stages of the
-upload, and whether or not there is a build to be created, or one in
-existence to be used. These policies are in the
-lp.archiveuploader package, in the uploadpolicy module. They
-are accessed by calling findPolicyByName which will either return a
-policy instance or else raise a KeyError.
-
-XXX: dsilvers: 20051019: Get rid of this registration/lookup system
-when we've fixed bug 3137
-
-  >>> from lp.archiveuploader.uploadpolicy import findPolicyByName
-
-There are two policies defined so far. They are the insecure and
-buildd policies.
+When the uploader is invoked, it is given a policy to work in. This governs
+such things as what tests get run at what stages of the upload, and whether or
+not there is a build to be created, or one in existence to be used. These
+policies are in the lp.archiveuploader package, in the uploadpolicy module,
+and implement IArchiveUploadPolicy. They are registered as named utilities,
+but since the policy themselves need to be changed according to user-specified
+arguments, we register the classes as the component of the utilities, which
+means a call to getUtility(IArchiveUploadPolicy, name) will return the class
+itself rather than an instance of it.
+
+  >>> from lp.archiveuploader.uploadpolicy import (
+  ...     IArchiveUploadPolicy, findPolicyByName)
+  >>> policy_cls = getUtility(IArchiveUploadPolicy, 'insecure')
+  >>> policy_cls
+  <class '...InsecureUploadPolicy'>
+
+There's a helper function which returns an instance of the policy with the
+given name, though, and it's preferred over using getUtility() directly.
 
   >>> insecure_policy = findPolicyByName('insecure')
-  >>> insecure_policy.name == 'insecure'
-  True
+  >>> insecure_policy
+  <lp...InsecureUploadPolicy object...
+
+Two of the policies defined so far are the insecure and buildd policies.
+
+  >>> insecure_policy.name
+  'insecure'
   >>> buildd_policy = findPolicyByName('buildd')
-  >>> buildd_policy.name == 'buildd'
-  True
+  >>> buildd_policy.name
+  'buildd'
   >>> abstract_policy = findPolicyByName('abstract')
   Traceback (most recent call last):
   ...
-  KeyError: 'abstract'
+  ComponentLookupError:...
 
 There is a bunch of attributes which we expect to have and which can vary
 from policy to policy.

=== modified file 'lib/lp/archiveuploader/uploadpolicy.py'
--- lib/lp/archiveuploader/uploadpolicy.py	2010-06-23 15:54:45 +0000
+++ lib/lp/archiveuploader/uploadpolicy.py	2010-08-18 16:15:57 +0000
@@ -6,21 +6,25 @@
 __metaclass__ = type
 
 __all__ = [
+    "AbstractUploadPolicy",
+    "BuildDaemonUploadPolicy",
     "findPolicyByName",
-    "findPolicyByOptions",
+    "IArchiveUploadPolicy",
+    "SOURCE_PACKAGE_RECIPE_UPLOAD_POLICY_NAME",
     "UploadPolicyError",
     ]
 
-from zope.component import getUtility
+from zope.component import getGlobalSiteManager, getUtility
+from zope.interface import implements, Interface
 
 from canonical.launchpad.interfaces import ILaunchpadCelebrities
-from lp.code.interfaces.sourcepackagerecipebuild import (
-    ISourcePackageRecipeBuildSource)
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 
 
+# Defined here so that uploadpolicy.py doesn't depend on lp.code.
+SOURCE_PACKAGE_RECIPE_UPLOAD_POLICY_NAME = 'recipe'
 # Number of seconds in an hour (used later)
 HOURS = 3600
 
@@ -29,6 +33,20 @@
     """Raised when a specific policy violation occurs."""
 
 
+class IArchiveUploadPolicy(Interface):
+    """The policy of an upload to a Launchpad archive.
+
+    This is, in practice, just a marker interface for us to look up upload
+    policies by name.
+
+    If registered as a utility, any classes implementing this must be given as
+    the 'component' argument of the <utility> directive so that a getUtility()
+    call returns the class itself rather than an instance.  This is needed
+    because the callsites usually change the policies (based on user-specified
+    arguments).
+    """
+
+
 class AbstractUploadPolicy:
     """Encapsulate the policy of an upload to a launchpad archive.
 
@@ -38,13 +56,13 @@
     tests themselves and they operate on NascentUpload instances in order
     to verify them.
     """
+    implements(IArchiveUploadPolicy)
 
-    policies = {}
     options = None
+    name = 'abstract'
 
     def __init__(self):
         """Prepare a policy..."""
-        self.name = 'abstract'
         self.distro = None
         self.distroseries = None
         self.pocket = None
@@ -143,39 +161,14 @@
         """Return whether the NEW upload should be automatically approved."""
         return False
 
-    @classmethod
-    def _registerPolicy(cls, policy_type):
-        """Register the given policy type as belonging to its given name."""
-        # XXX: JonathanLange 2010-01-15 bug=510892: This shouldn't instantiate
-        # policy types. They should instead have names as class variables.
-        policy_name = policy_type().name
-        cls.policies[policy_name] = policy_type
-
-    @classmethod
-    def findPolicyByName(cls, policy_name):
-        """Return a new policy instance for the given policy name."""
-        return cls.policies[policy_name]()
-
-    @classmethod
-    def findPolicyByOptions(cls, options):
-        """Return a new policy instance given the options dictionary."""
-        policy = cls.policies[options.context]()
-        policy.setOptions(options)
-        return policy
-
-# XXX: dsilvers 2005-10-19 bug=3373: use the component architecture for
-# these instead of reinventing the registration/finder again?
-# Nice shiny top-level policy finder
-findPolicyByName = AbstractUploadPolicy.findPolicyByName
-findPolicyByOptions = AbstractUploadPolicy.findPolicyByOptions
-
 
 class InsecureUploadPolicy(AbstractUploadPolicy):
     """The insecure upload policy is used by the poppy interface."""
 
+    name = 'insecure'
+
     def __init__(self):
         AbstractUploadPolicy.__init__(self)
-        self.name = 'insecure'
         self.can_upload_binaries = False
         self.can_upload_mixed = False
 
@@ -281,15 +274,13 @@
         return False
 
 
-AbstractUploadPolicy._registerPolicy(InsecureUploadPolicy)
-
-
 class BuildDaemonUploadPolicy(AbstractUploadPolicy):
     """The build daemon upload policy is invoked by the slave scanner."""
 
+    name = 'buildd'
+
     def __init__(self):
         super(BuildDaemonUploadPolicy, self).__init__()
-        self.name = 'buildd'
         # We permit unsigned uploads because we trust our build daemons
         self.unsigned_changes_ok = True
         self.unsigned_dsc_ok = True
@@ -313,37 +304,13 @@
         return False
 
 
-AbstractUploadPolicy._registerPolicy(BuildDaemonUploadPolicy)
-
-
-class SourcePackageRecipeUploadPolicy(BuildDaemonUploadPolicy):
-    """Policy for uploading the results of a source package recipe build."""
-
-    def __init__(self):
-        super(SourcePackageRecipeUploadPolicy, self).__init__()
-        # XXX: JonathanLange 2010-01-15 bug=510894: This has to be exactly the
-        # same string as the one in SourcePackageRecipeBuild.policy_name.
-        # Factor out a shared constant.
-        self.name = 'recipe'
-        self.can_upload_source = True
-        self.can_upload_binaries = False
-
-    def getUploader(self, changes):
-        """Return the person doing the upload."""
-        build_id = int(getattr(self.options, 'buildid'))
-        sprb = getUtility(ISourcePackageRecipeBuildSource).getById(build_id)
-        return sprb.requester
-
-
-AbstractUploadPolicy._registerPolicy(SourcePackageRecipeUploadPolicy)
-
-
 class SyncUploadPolicy(AbstractUploadPolicy):
     """This policy is invoked when processing sync uploads."""
 
+    name = 'sync'
+
     def __init__(self):
         AbstractUploadPolicy.__init__(self)
-        self.name = "sync"
         # We don't require changes or dsc to be signed for syncs
         self.unsigned_changes_ok = True
         self.unsigned_dsc_ok = True
@@ -357,50 +324,6 @@
         # Implement this to check the sync
         pass
 
-AbstractUploadPolicy._registerPolicy(SyncUploadPolicy)
-
-
-class AnythingGoesUploadPolicy(AbstractUploadPolicy):
-    """This policy is invoked when processing uploads from the test process.
-
-    We require a signed changes file but that's it.
-    """
-
-    def __init__(self):
-        AbstractUploadPolicy.__init__(self)
-        self.name = "anything"
-        # We require the changes to be signed but not the dsc
-        self.unsigned_dsc_ok = True
-
-    def policySpecificChecks(self, upload):
-        """Nothing, let it go."""
-        pass
-
-    def rejectPPAUploads(self, upload):
-        """We allow PPA uploads."""
-        return False
-
-AbstractUploadPolicy._registerPolicy(AnythingGoesUploadPolicy)
-
-
-class AbsolutelyAnythingGoesUploadPolicy(AnythingGoesUploadPolicy):
-    """This policy is invoked when processing uploads from the test process.
-
-    Absolutely everything is allowed, for when you don't want the hassle
-    of dealing with inappropriate checks in tests.
-    """
-
-    def __init__(self):
-        AnythingGoesUploadPolicy.__init__(self)
-        self.name = "absolutely-anything"
-        self.unsigned_changes_ok = True
-
-    def policySpecificChecks(self, upload):
-        """Nothing, let it go."""
-        pass
-
-AbstractUploadPolicy._registerPolicy(AbsolutelyAnythingGoesUploadPolicy)
-
 
 class SecurityUploadPolicy(AbstractUploadPolicy):
     """The security-upload policy.
@@ -408,9 +331,10 @@
     It allows unsigned changes and binary uploads.
     """
 
+    name = 'security'
+
     def __init__(self):
         AbstractUploadPolicy.__init__(self)
-        self.name = "security"
         self.unsigned_dsc_ok = True
         self.unsigned_changes_ok = True
         self.can_upload_mixed = True
@@ -422,4 +346,19 @@
             upload.reject(
                 "Not permitted to do security upload to non SECURITY pocket")
 
-AbstractUploadPolicy._registerPolicy(SecurityUploadPolicy)
+
+def findPolicyByName(policy_name):
+    """Return a new policy instance for the given policy name."""
+    return getUtility(IArchiveUploadPolicy, policy_name)()
+
+
+def register_archive_upload_policy_adapters():
+    policies = [
+        BuildDaemonUploadPolicy,
+        InsecureUploadPolicy,
+        SyncUploadPolicy, 
+        SecurityUploadPolicy]
+    sm = getGlobalSiteManager()
+    for policy in policies:
+        sm.registerUtility(
+            component=policy, provided=IArchiveUploadPolicy, name=policy.name)

=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py	2010-08-12 17:54:00 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py	2010-08-18 16:15:57 +0000
@@ -60,6 +60,8 @@
 from lp.archiveuploader.nascentupload import (
     NascentUpload, FatalUploadError, EarlyReturnUploadError)
 from lp.archiveuploader.uploadpolicy import (
+    BuildDaemonUploadPolicy,
+    SOURCE_PACKAGE_RECIPE_UPLOAD_POLICY_NAME,
     UploadPolicyError)
 from lp.soyuz.interfaces.archive import IArchiveSet, NoSuchPPA
 from lp.registry.interfaces.distribution import IDistributionSet
@@ -350,10 +352,10 @@
 
         # Reject source upload to buildd upload paths.
         first_path = relative_path.split(os.path.sep)[0]
-        # XXX: JonathanLange 2010-01-15 bug=510894: We should not be re-using
-        # magical string literals. Zombie Dijkstra will come and kill us in
-        # our sleep.
-        if first_path.isdigit() and policy.name not in ('buildd', 'recipe'):
+        is_not_buildd_nor_recipe_policy = policy.name not in [
+            SOURCE_PACKAGE_RECIPE_UPLOAD_POLICY_NAME,
+            BuildDaemonUploadPolicy.name]
+        if first_path.isdigit() and is_not_buildd_nor_recipe_policy:
             error_message = (
                 "Invalid upload path (%s) for this policy (%s)" %
                 (relative_path, policy.name))
@@ -448,8 +450,6 @@
             self.log.debug("Keeping contents untouched")
             return
 
-        pathname = os.path.basename(upload)
-
         self.log.debug("Removing upload directory %s", upload)
         shutil.rmtree(upload)
 
@@ -547,7 +547,7 @@
 
     suite_name = parts[1]
     try:
-        suite = distribution.getDistroSeriesAndPocket(suite_name)
+        distribution.getDistroSeriesAndPocket(suite_name)
     except NotFoundError:
         raise exc_type("Could not find suite '%s'." % suite_name)
 

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2010-08-13 04:01:57 +0000
+++ lib/lp/code/configure.zcml	2010-08-18 16:15:57 +0000
@@ -999,6 +999,11 @@
   <utility component="lp.code.model.sourcepackagerecipebuild.SourcePackageRecipeBuildJob"
         name="RECIPEBRANCHBUILD"
         provides="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJob"/>
+
+  <call
+    callable="lp.code.model.sourcepackagerecipebuild.register_archive_upload_policy_adapter"
+    />
+
   <webservice:register module="lp.code.interfaces.webservice" />
     <adapter
         provides="lp.buildmaster.interfaces.buildfarmjob.ISpecificBuildFarmJob"

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2010-08-09 20:37:40 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2010-08-18 16:15:57 +0000
@@ -24,12 +24,15 @@
 from storm.locals import Int, Reference, Storm
 from storm.store import Store
 
-from zope.component import getUtility
+from zope.component import getGlobalSiteManager, getUtility
 from zope.interface import classProvides, implements
 from zope.security.proxy import ProxyFactory
 
 from canonical.launchpad.webapp import errorlog
 from lp.app.errors import NotFoundError
+from lp.archiveuploader.uploadpolicy import (
+    BuildDaemonUploadPolicy, IArchiveUploadPolicy,
+    SOURCE_PACKAGE_RECIPE_UPLOAD_POLICY_NAME)
 from lp.buildmaster.model.packagebuild import (
     PackageBuild, PackageBuildDerived)
 from lp.buildmaster.interfaces.buildbase import BuildStatus
@@ -54,11 +57,28 @@
 from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 
 
+class SourcePackageRecipeUploadPolicy(BuildDaemonUploadPolicy):
+    """Policy for uploading the results of a source package recipe build."""
+
+    name = SOURCE_PACKAGE_RECIPE_UPLOAD_POLICY_NAME
+
+    def __init__(self):
+        super(SourcePackageRecipeUploadPolicy, self).__init__()
+        self.can_upload_source = True
+        self.can_upload_binaries = False
+
+    def getUploader(self, changes):
+        """Return the person doing the upload."""
+        build_id = int(getattr(self.options, 'buildid'))
+        sprb = getUtility(ISourcePackageRecipeBuildSource).getById(build_id)
+        return sprb.requester
+
+
 class SourcePackageRecipeBuild(PackageBuildDerived, Storm):
 
     __storm_table__ = 'SourcePackageRecipeBuild'
 
-    policy_name = 'recipe'
+    policy_name = SourcePackageRecipeUploadPolicy.name
 
     implements(ISourcePackageRecipeBuild)
     classProvides(ISourcePackageRecipeBuildSource)
@@ -348,6 +368,13 @@
         return 2405 + self.build.archive.relative_build_score
 
 
+def register_archive_upload_policy_adapter():
+    getGlobalSiteManager().registerUtility(
+        component=SourcePackageRecipeUploadPolicy,
+        provided=IArchiveUploadPolicy,
+        name=SourcePackageRecipeUploadPolicy.name)
+
+
 def get_recipe_build_for_build_farm_job(build_farm_job):
     """Return the SourcePackageRecipeBuild associated with a BuildFarmJob."""
     store = Store.of(build_farm_job)

=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml	2010-08-16 21:34:11 +0000
+++ lib/lp/soyuz/configure.zcml	2010-08-18 16:15:57 +0000
@@ -855,4 +855,15 @@
             interface="lp.soyuz.interfaces.binarypackagerelease.IBinaryPackageReleaseDownloadCount"/>
     </class>
 
+    <call
+      callable="lp.archiveuploader.uploadpolicy.register_archive_upload_policy_adapters"
+      />
+
+    <!-- Although the policies defined in lp.archiveuploader.tests are only
+         required by tests, they're used when testing scripts manually on
+         dogfood/staging, so they're registered here. -->
+    <call
+      callable="lp.archiveuploader.tests.register_archive_upload_policy_adapters"
+      />
+
 </configure>

=== modified file 'lib/lp/soyuz/scripts/soyuz_process_upload.py'
--- lib/lp/soyuz/scripts/soyuz_process_upload.py	2010-08-06 13:52:46 +0000
+++ lib/lp/soyuz/scripts/soyuz_process_upload.py	2010-08-18 16:15:57 +0000
@@ -8,7 +8,7 @@
 
 import os
 
-from lp.archiveuploader.uploadpolicy import findPolicyByOptions
+from lp.archiveuploader.uploadpolicy import findPolicyByName
 from lp.archiveuploader.uploadprocessor import UploadProcessor
 from lp.services.scripts.base import (
     LaunchpadCronScript, LaunchpadScriptFailure)
@@ -77,7 +77,9 @@
         self.logger.debug("Initialising connection.")
         def getPolicy(distro):
             self.options.distro = distro.name
-            return findPolicyByOptions(self.options)
+            policy = findPolicyByName(self.options.context)
+            policy.setOptions(self.options)
+            return policy
         processor = UploadProcessor(self.options.base_fsroot, 
             self.options.dryrun, self.options.nomails, self.options.keep,
             getPolicy, self.txn, self.logger)