Merge lp:~jml/launchpad/process-upload-checks into lp:launchpad/db-devel

[Jonathan Lange]

Allow uploads from recipe jobs. Upload policies are now specified explicitly by Build objects, rather than in configuration files.

[Jonathan Lange (jml)]

This branch changes the Soyuz uploader to allow uploads from the recipe job.

It has a few stages of work:

1. Tweaks to BuilderBase to be more readily testable
  - Lots of new getUploaderFoo methods. The one that's actually used externally is getUploaderCommand()

2. A new upload policy for recipes

3. Changes to the nascentupload process so that it delegates decisions

4. Change the Build and SourcePackageRecipeBuild objects to specify their upload policy explicitly

5. Change the BuilderBase to use those policies when spawning the upload processor.

6. Change the configs so that policies are not specified in the configs.

[Jonathan Lange (jml)]

I should note also that this has been tested, sort of, on wgrant's laptop, and that the config changes are somewhat in lock-step with the code changes. I'll submit a branch to change the production configs.

[Jonathan Lange (jml)]

https://code.edge.launchpad.net/~jml/lp-production-configs/dont-pass-buildd-policy/+merge/17444

[Julian Edwards (julian-edwards)]

The diff below has got conflict markers in it. Also, when I diffed locally, it's 8000 lines against devel, or 17000 against db-devel. WTF!

So, I'm backing off reviewing this for now. Can you grab me ASAP so we can get this done.

[Jonathan Lange (jml)]

Hi Julian,

I've fixed a whole bunch of conflicts as well as all of the failing tests from my last "fix all the conflicts and run the tests" run. I'm running the tests now, and will no doubt have fixed all of the errors by the time you wake.

jml

[Jonathan Lange (jml)]

The tests do pass.

[Julian Edwards (julian-edwards)]

Thanks for fixing things! Here's my review:

> + # XXX: JonathanLange 2010-01-15: This shouldn't instantiate policy
> + # types. They should instead have names as class variables.

The XXX should reference a bug :) Or better still, fix it, it's an easy change.

> + # XXX: JonathanLange 2010-01-15: This has to be exactly the same
> + # string as the one in SourcePackageRecipeBuild.policy_name. Factor
> + # out a shared constant.

Same here.

> class SourcePackageRecipeUploadPolicy(BuildDaemonUploadPolicy):

Did you double check the inherited policy options for this? I think it looks sane but I have to ask ...

> + # XXX: JonathanLange 2010-01-15: We should not be re-using magical
> + # string literals. Zombie Dijkstra will come and kill us in our sleep.

bug number *cough* :)

> - buildqueue_record = Attribute("Corespondent BuildQueue record")
> + buildqueue_record = Attribute("Corresponding BuildQueue record")

I know you're just fixing the text, but we can surely do better than "Attribute".

> + policy_name = Attribute(
> + "The upload policy to use for handling these builds.")

And here!

> + uploader_argv = list(config.builddmaster.uploader.split())

Can you rename this variable to something like "uploader_command". The "argv" bit made me look twice since I was expecting only parameters to the command but it's actually the base command from the config.

> + # XXX: Duplicated from getUploaderCommand
> uploader_logfilename = os.path.join(upload_dir, 'uploader.log')

Why not turn it into a property on the class then?

> - classProvides(IBuildFarmCandidateJobSelection, ISpecificBuildFarmJobClass)
> + classProvides(ISpecificBuildFarmJobClass)

Why did you remove that? I note that it's there in db-devel but not devel.

> + # XXX: BuildBase is supposed to implement IBuildBase, but doesn't atm.
> + # Since it's not the focus of the branch, we'll postpone the work.

From the code I just looked at, it does! If there's a genuine problem then we need a bug please - tag wellington of course. This is becoming known as the Wellington project I think!

FWIW, I think all classes should have basic doctests, I really don't like this unit testing for their basic features.

What layer is TestBuildBase going to run in if it's not explicitly specified?

> +class TestBuildBaseHarder(TestCaseWithFactory):

Crappy name alert! Perhaps "TestBuildBaseWithDatabase" ?

Or maybe, s/TestBuildBase/TestBuildBase<layer>/ where <layer> is whatever it's really using (and I think it should be explicit) and then s/TestBuildBaseHarder/TestBuildBase/. Your call.

> + <adapter factory="lp.soyuz.model.recipebuilder.RecipeBuildBehavior"
> + permission="zope.Public" />

You can fill a gap in my knowledge here, what does this do when declared without the "provides" and "for" ?

> + build_log_url = exported(

Why is this moved from BuildBase to Build?

> + # JRV 2010-01-15: The database table really should have a pocket
> + # column, although this is not a big problem at the moment as recipe
> + # builds only happen for PPA's (so far). (bug 507307)

Can you XXX this in the usual format please.

I guess I should tell Jelmer how this works too :)

> + distroseries.n...

review: Needs Information (code)

[Jonathan Lange (jml)]

On Fri, Jan 22, 2010 at 3:11 AM, Julian Edwards
<email address hidden> wrote:
> Review: Needs Information code
> Thanks for fixing things!  Here's my review:
>

Hey Julian,

Thanks for the review.

As a meta-comment, I'd really appreciate it if you could leave the
filenames from the diff in your reply. It makes it much, much faster
to find the relevant code in my editor.

>> + # XXX: JonathanLange 2010-01-15: This shouldn't instantiate policy
>> + # types. They should instead have names as class variables.
>
> The XXX should reference a bug :)  Or better still, fix it, it's an easy change.
>

Bug filed. It's an easy change, but the branch is big enough as-is.
I'll make the change in a separate branch.

>> + # XXX: JonathanLange 2010-01-15: This has to be exactly the same
>> + # string as the one in SourcePackageRecipeBuild.policy_name. Factor
>> + # out a shared constant.
>
> Same here.
>

And same reply.

>> class SourcePackageRecipeUploadPolicy(BuildDaemonUploadPolicy):
>
> Did you double check the inherited policy options for this?  I think it looks sane but I have to ask ...
>

        self.unsigned_changes_ok = True
        self.unsigned_dsc_ok = True
        self.can_upload_source = False
        self.can_upload_mixed = False

I don't know if those bits are good. Your call.

>> + # XXX: JonathanLange 2010-01-15: We should not be re-using magical
>> + # string literals. Zombie Dijkstra will come and kill us in our sleep.
>
> bug number *cough* :)
>

Added.

>> - buildqueue_record = Attribute("Corespondent BuildQueue record")
>> + buildqueue_record = Attribute("Corresponding BuildQueue record")
>
> I know you're just fixing the text, but we can surely do better than "Attribute".
>

Changed to Object(...)

>> + policy_name = Attribute(
>> +     "The upload policy to use for handling these builds.")
>
> And here!
>

Changed to TextLine(...)

>> +     uploader_argv = list(config.builddmaster.uploader.split())
>
> Can you rename this variable to something like "uploader_command".  The "argv" bit made me look twice since I was expecting only parameters to the command but it's actually the base command from the config.

Ahh ok. FWIW, I used "argv" specifically because it corresponds to
argv arrays in C convention by including the argument.

Changed to uploader_command.

>
>> + # XXX: Duplicated from getUploaderCommand
>>   uploader_logfilename = os.path.join(upload_dir, 'uploader.log')
>
> Why not turn it into a property on the class then?
>

Because it's parametrized -- it depends on upload_leaf. I've made it
an argument to getUploaderCommand.

>> - classProvides(IBuildFarmCandidateJobSelection, ISpecificBuildFarmJobClass)
>> + classProvides(ISpecificBuildFarmJobClass)
>
> Why did you remove that?  I note that it's there in db-devel but not devel.
>

_Probably_ a bad merge. Restored.

>> + # XXX: BuildBase is supposed to implement IBuildBase, but doesn't atm.
>> + # Since it's not the focus of the branch, we'll postpone the work.
>
>
>From the code I just looked at, it does!  If there's a genuine problem then we need a bug please - tag wellington of course.  This is becoming known as the Wellington project I think!

It didn't when I su...

[Julian Edwards (julian-edwards)]

 review approve
 merge approve

> As a meta-comment, I'd really appreciate it if you could leave the
> filenames from the diff in your reply. It makes it much, much faster
> to find the relevant code in my editor.

Right, sorry, I hate that too.

> Bug filed. It's an easy change, but the branch is big enough as-is.
> I'll make the change in a separate branch.

Cool, thank you.

> self.unsigned_changes_ok = True
> self.unsigned_dsc_ok = True
> self.can_upload_source = False
> self.can_upload_mixed = False
>
> I don't know if those bits are good. Your call.

That looks fine, thanks for checking (again).

> >> - buildqueue_record = Attribute("Corespondent BuildQueue record")
> >> + buildqueue_record = Attribute("Corresponding BuildQueue record")
> >
> > I know you're just fixing the text, but we can surely do better than
> > "Attribute".
>
> Changed to Object(...)

Isn't it a Reference() ?

> Ahh ok. FWIW, I used "argv" specifically because it corresponds to
> argv arrays in C convention by including the argument.
>
> Changed to uploader_command.

True. I always hated that :)

>
> >> + # XXX: Duplicated from getUploaderCommand
> >> uploader_logfilename = os.path.join(upload_dir, 'uploader.log')
> >
> > Why not turn it into a property on the class then?
>
> Because it's parametrized -- it depends on upload_leaf. I've made it
> an argument to getUploaderCommand.

D'oh, right.

> >> - classProvides(IBuildFarmCandidateJobSelection,
> >> ISpecificBuildFarmJobClass) + classProvides(ISpecificBuildFarmJobClass)
> >
> > Why did you remove that? I note that it's there in db-devel but not
> > devel.
>
> _Probably_ a bad merge. Restored.

Phew, thanks ;)

Since it didn't break anything, this indicates a missing test :/

> It didn't when I submitted this branch last week, and when I
> re-enabled the test, it failed. The problem is genuine, so I've filed
> a bug and added it to the comment.

Argh, ok, thanks. I was just looking at the "implements" rather than actually
looking to see what it was implementing!

> > FWIW, I think all classes should have basic doctests, I really don't like
> > this unit testing for their basic features.
>
> OK. I haven't added any doctests in this branch.

Yeah, it's a matter of policy I'll raise in a reviewers' meeting I think.

> > What layer is TestBuildBase going to run in if it's not explicitly
> > specified?
>
> No layer at all. It doesn't need to be run in a layer. We want more
> tests like this, because they are fast :)

Ah! Ok, that's cool. We need more of this! You should totally email the
list about that.

> I forwarded you an email on the subject from the Canonical thread.
> http://www.muthukadan.net/docs/zca.html#zcml has some other notes.

Right, I remember it now, thanks for the pointer.

> >> + build_log_url = exported(
> >
> > Why is this moved from BuildBase to Build?
>
> When I was trying to get the assertProvides(BuildBase, IBuildBase)
> thing to pass, it turned out that BuildBase didn't implement this, so
> I moved the declaration.

Righto.

> I look forward to hearing back from you.

Land it!

Cheers,
J.

review: Approve

=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf	2010-01-19 12:49:35 +0000
+++ configs/development/launchpad-lazr.conf	2010-01-29 17:17:19 +0000
@@ -15,7 +15,7 @@
 
 [builddmaster]
 root: /var/tmp/builddmaster/
-uploader: scripts/process-upload.py -Mvv --context buildd
+uploader: scripts/process-upload.py -Mvv
 
 [buildsequencer]
 mailproblemsto: root

=== modified file 'configs/testrunner/launchpad-lazr.conf'
--- configs/testrunner/launchpad-lazr.conf	2010-01-22 04:59:09 +0000
+++ configs/testrunner/launchpad-lazr.conf	2010-01-29 17:17:19 +0000
@@ -17,7 +17,7 @@
 
 [builddmaster]
 socket_timeout: 10
-uploader: scripts/process-upload.py -Mvv --context buildd
+uploader: scripts/process-upload.py -Mvv
 
 [buildsequencer]
 mailproblemsto: -

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-01-26 02:05:23 +0000
+++ database/schema/security.cfg	2010-01-29 17:17:19 +0000
@@ -835,6 +835,7 @@
 public.archive                                  = SELECT, UPDATE
 public.archivearch                              = SELECT, UPDATE
 public.archivedependency                        = SELECT
+public.branch                                   = SELECT
 public.buildqueue                               = SELECT, INSERT, UPDATE, DELETE
 public.job                                      = SELECT, INSERT, UPDATE, DELETE
 public.buildpackagejob                          = SELECT, INSERT, UPDATE, DELETE
@@ -858,8 +859,15 @@
 public.processor                                = SELECT
 public.processorfamily                          = SELECT
 public.pocketchroot                             = SELECT, INSERT, UPDATE
+public.product                                  = SELECT
+public.productseries                            = SELECT
 public.component                                = SELECT
 public.section                                  = SELECT
+public.sourcepackagerecipe                      = SELECT
+public.sourcepackagerecipebuild                 = SELECT, UPDATE
+public.sourcepackagerecipebuildjob              = SELECT, INSERT, UPDATE, DELETE
+public.sourcepackagerecipedata                  = SELECT
+public.sourcepackagerecipedatainstruction       = SELECT
 public.publishedpackage                         = SELECT
 public.person                                   = SELECT
 public.emailaddress                             = SELECT

=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py	2009-11-24 23:12:23 +0000
+++ lib/lp/archiveuploader/nascentupload.py	2010-01-29 17:17:19 +0000
@@ -477,11 +477,12 @@
             return
 
         # Set up some convenient shortcut variables.
-        signer = self.changes.signer
+
+        uploader = self.policy.getUploader(self.changes)
         archive = self.policy.archive
 
         # If we have no signer, there's no ACL we can apply.
-        if signer is None:
+        if uploader is None:
             self.logger.debug("No signer, therefore ACL not processed")
             return
 
@@ -489,7 +490,7 @@
             ISourcePackageNameSet).queryByName(self.changes.dsc.package)
 
         rejection_reason = check_upload_to_archive(
-            signer, self.policy.distroseries, source_name, archive,
+            uploader, self.policy.distroseries, source_name, archive,
             self.changes.dsc.component, self.policy.pocket, not self.is_new)
 
         if rejection_reason is not None:
@@ -894,7 +895,6 @@
         # Queue entries are created in the NEW state by default; at the
         # end of this method we cope with uploads that aren't new.
         self.logger.debug("Creating queue entry")
-        distroseries = self.policy.distroseries
         self.queue_root = self._createQueueEntry()
 
         # When binaryful and sourceful, we have a mixed-mode upload.

=== modified file 'lib/lp/archiveuploader/uploadpolicy.py'
--- lib/lp/archiveuploader/uploadpolicy.py	2009-12-14 00:04:19 +0000
+++ lib/lp/archiveuploader/uploadpolicy.py	2010-01-29 17:17:19 +0000
@@ -5,7 +5,11 @@
 
 __metaclass__ = type
 
-__all__ = ["findPolicyByName", "findPolicyByOptions", "UploadPolicyError"]
+__all__ = [
+    "findPolicyByName",
+    "findPolicyByOptions",
+    "UploadPolicyError",
+    ]
 
 from zope.component import getUtility
 
@@ -13,6 +17,8 @@
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.soyuz.interfaces.sourcepackagerecipebuild import (
+    ISourcePackageRecipeBuildSource)
 
 
 # Number of seconds in an hour (used later)
@@ -85,6 +91,10 @@
         # The earliest year we accept in a deb's file's mtime
         self.earliest_year = 1984
 
+    def getUploader(self, changes):
+        """Get the person who is doing the uploading."""
+        return changes.signer
+
     def setOptions(self, options):
         """Store the options for later."""
         self.options = options
@@ -167,6 +177,8 @@
     @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
 
@@ -188,6 +200,7 @@
 findPolicyByName = AbstractUploadPolicy.findPolicyByName
 findPolicyByOptions = AbstractUploadPolicy.findPolicyByOptions
 
+
 class InsecureUploadPolicy(AbstractUploadPolicy):
     """The insecure upload policy is used by the poppy interface."""
 
@@ -306,7 +319,7 @@
     """The build daemon upload policy is invoked by the slave scanner."""
 
     def __init__(self):
-        AbstractUploadPolicy.__init__(self)
+        super(BuildDaemonUploadPolicy, self).__init__()
         self.name = 'buildd'
         # We permit unsigned uploads because we trust our build daemons
         self.unsigned_changes_ok = True
@@ -334,6 +347,28 @@
 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."""
 

=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py	2009-06-24 23:33:29 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py	2010-01-29 17:17:19 +0000
@@ -339,7 +339,10 @@
 
         # Reject source upload to buildd upload paths.
         first_path = relative_path.split(os.path.sep)[0]
-        if first_path.isdigit() and policy.name != 'buildd':
+        # 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'):
             error_message = (
                 "Invalid upload path (%s) for this policy (%s)" %
                 (relative_path, policy.name))

=== modified file 'lib/lp/buildmaster/interfaces/buildbase.py'
--- lib/lp/buildmaster/interfaces/buildbase.py	2010-01-20 02:02:02 +0000
+++ lib/lp/buildmaster/interfaces/buildbase.py	2010-01-29 17:17:19 +0000
@@ -19,11 +19,14 @@
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.soyuz.interfaces.archive import IArchive
+from lp.soyuz.interfaces.buildqueue import IBuildQueue
 from canonical.launchpad.interfaces.librarian import ILibraryFileAlias
 from canonical.launchpad import _
 
+
 class IBuildBase(Interface):
     """Common interface shared by farm jobs that build a package."""
+
     # XXX: wgrant 2010-01-20 bug=507712: Most of these attribute names
     # are bad.
     datecreated = exported(
@@ -68,10 +71,16 @@
             description=_("A URL for the build log. None if there is no "
                           "log available.")))
 
-    buildqueue_record = Attribute("Corespondent BuildQueue record")
+    buildqueue_record = Object(
+        schema=IBuildQueue, required=True,
+        title=_("Corresponding BuildQueue record"))
 
     is_private = Attribute("Whether the build should be treated as private.")
 
+    policy_name = TextLine(
+        title=_("Policy name"), required=True,
+        description=_("The upload policy to use for handling these builds."))
+
     archive = exported(
         Reference(
             title=_("Archive"), schema=IArchive,
@@ -100,7 +109,14 @@
             title=_("Distribution"), required=True,
             description=_("Shortcut for its distribution.")))
 
-    def handleStatus(status, queueItem, librarian, slave_status):
+    def getUploaderCommand(upload_leaf):
+        """Get the command to run as the uploader.
+
+        :return: A list of command line arguments, beginning with the
+            executable.
+        """
+
+    def handleStatus(status, librarian, slave_status):
         """Handle a finished build status from a slave.
 
         :param status: Slave build status string with 'BuildStatus.' stripped.

=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjob.py'
--- lib/lp/buildmaster/interfaces/buildfarmjob.py	2010-01-18 22:01:19 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjob.py	2010-01-29 17:17:19 +0000
@@ -158,7 +158,7 @@
 
 class IBuildFarmCandidateJobSelection(Interface):
     """Operations for refining candidate job selection (optional).
-    
+
     Job type classes that do *not* need to refine candidate job selection may
     be derived from `BuildFarmJob` which provides a base implementation of
     this interface.
@@ -178,7 +178,7 @@
             SELECT TRUE
             FROM Archive, Build, BuildPackageJob, DistroArchSeries
             WHERE
-            BuildPackageJob.job = Job.id AND 
+            BuildPackageJob.job = Job.id AND
             ..
 
         :param processor: the type of processor that the candidate jobs are
@@ -192,7 +192,7 @@
     def postprocessCandidate(job, logger):
         """True if the candidate job is fine and should be dispatched
         to a builder, False otherwise.
-        
+
         :param job: The `BuildQueue` instance to be scrutinized.
         :param logger: The logger to use.
 

=== modified file 'lib/lp/buildmaster/model/buildbase.py'
--- lib/lp/buildmaster/model/buildbase.py	2010-01-20 01:24:01 +0000
+++ lib/lp/buildmaster/model/buildbase.py	2010-01-29 17:17:19 +0000
@@ -14,9 +14,9 @@
 import os
 import pytz
 import subprocess
-import time
 
 from storm.store import Store
+from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
@@ -24,12 +24,54 @@
 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)
+
+    policy_name = 'buildd'
+
+    def getUploadLeaf(self, build_id, now=None):
+        """Return a directory name to store build things in.
+
+        :param build_id: The id as returned by the slave, normally
+            $BUILD_ID-$BUILDQUEUE_ID
+        :param now: The `datetime` to use. If not provided, defaults to now.
+        """
+        # UPLOAD_LEAF: <TIMESTAMP>-<BUILD_ID>-<BUILDQUEUE_ID>
+        if now is None:
+            now = datetime.datetime.now()
+        return '%s-%s' % (now.strftime("%Y%m%d-%H%M%S"), build_id)
+
+    def getUploadDir(self, upload_leaf):
+        """Return the directory that things will be stored in."""
+        return os.path.join(config.builddmaster.root, 'incoming', upload_leaf)
+
+    def getUploaderCommand(self, upload_leaf, uploader_logfilename):
+        """See `IBuildBase`."""
+        root = os.path.abspath(config.builddmaster.root)
+        uploader_command = list(config.builddmaster.uploader.split())
+
+        # add extra arguments for processing a binary upload
+        extra_args = [
+            "--log-file", "%s" % uploader_logfilename,
+            "-d", "%s" % self.distribution.name,
+            "-s", "%s" % (self.distroseries.name +
+                          pocketsuffix[self.pocket]),
+            "-b", "%s" % self.id,
+            "-J", "%s" % upload_leaf,
+            '--context=%s' % self.policy_name,
+            "%s" % root,
+            ]
+
+        uploader_command.extend(extra_args)
+        return uploader_command
+
     def _getProxiedFileURL(self, library_file):
         """Return the 'http_url' of a `ProxiedLibraryFileAlias`."""
         # Avoiding circular imports.
@@ -81,12 +123,10 @@
         # ensure we have the correct build root as:
         # <BUILDMASTER_ROOT>/incoming/<UPLOAD_LEAF>/<TARGET_PATH>/[FILES]
         root = os.path.abspath(config.builddmaster.root)
-        incoming = os.path.join(root, 'incoming')
 
         # create a single directory to store build result files
-        # UPLOAD_LEAF: <TIMESTAMP>-<BUILD_ID>-<BUILDQUEUE_ID>
-        upload_leaf = "%s-%s" % (time.strftime("%Y%m%d-%H%M%S"), buildid)
-        upload_dir = os.path.join(incoming, upload_leaf)
+        upload_leaf = self.getUploadLeaf(buildid)
+        upload_dir = self.getUploadDir(upload_leaf)
         logger.debug("Storing build result at '%s'" % upload_dir)
 
         # Build the right UPLOAD_PATH so the distribution and archive
@@ -106,29 +146,16 @@
             out_file = open(out_file_name, "wb")
             copy_and_close(slave_file, out_file)
 
-        uploader_argv = list(config.builddmaster.uploader.split())
         uploader_logfilename = os.path.join(upload_dir, 'uploader.log')
-        logger.debug("Saving uploader log at '%s'"
-                     % uploader_logfilename)
-
-        # add extra arguments for processing a binary upload
-        extra_args = [
-            "--log-file", "%s" %  uploader_logfilename,
-            "-d", "%s" % self.distribution.name,
-            "-s", "%s" % (self.distroseries.name +
-                          pocketsuffix[self.pocket]),
-            "-b", "%s" % self.id,
-            "-J", "%s" % upload_leaf,
-            "%s" % root,
-            ]
-
-        uploader_argv.extend(extra_args)
+        uploader_command = self.getUploaderCommand(
+            upload_leaf, uploader_logfilename)
+        logger.debug("Saving uploader log at '%s'" % uploader_logfilename)
 
         logger.debug("Invoking uploader on %s" % root)
-        logger.debug("%s" % uploader_argv)
+        logger.debug("%s" % uploader_command)
 
         uploader_process = subprocess.Popen(
-            uploader_argv, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+            uploader_command, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 
         # Nothing should be written to the stdout/stderr.
         upload_stdout, upload_stderr = uploader_process.communicate()

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2010-01-21 10:48:25 +0000
+++ lib/lp/buildmaster/model/builder.py	2010-01-29 17:17:18 +0000
@@ -457,7 +457,7 @@
                 AND (
                     -- The processor values either match or the candidate
                     -- job is processor-independent.
-                    buildqueue.processor = %s OR 
+                    buildqueue.processor = %s OR
                     buildqueue.processor IS NULL)
                 AND (
                     -- The virtualized values either match or the candidate
@@ -473,7 +473,6 @@
             self.virtualized)
         order_clause = " ORDER BY buildqueue.lastscore DESC, buildqueue.id"
 
-        extra_tables = set()
         extra_queries = []
         job_classes = specific_job_classes()
         for job_type, job_class in job_classes.iteritems():

=== modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
--- lib/lp/buildmaster/model/buildfarmjob.py	2010-01-20 04:04:15 +0000
+++ lib/lp/buildmaster/model/buildfarmjob.py	2010-01-29 17:17:19 +0000
@@ -19,7 +19,7 @@
 class BuildFarmJob:
     """Mix-in class for `IBuildFarmJob` implementations."""
     implements(IBuildFarmJob)
-    classProvides(IBuildFarmCandidateJobSelection, ISpecificBuildFarmJobClass)
+    classProvides(ISpecificBuildFarmJobClass, IBuildFarmCandidateJobSelection)
 
     def score(self):
         """See `IBuildFarmJob`."""

=== added file 'lib/lp/buildmaster/tests/test_buildbase.py'
--- lib/lp/buildmaster/tests/test_buildbase.py	1970-01-01 00:00:00 +0000
+++ lib/lp/buildmaster/tests/test_buildbase.py	2010-01-29 17:17:19 +0000
@@ -0,0 +1,82 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `IBuildBase`."""
+
+__metaclass__ = type
+
+from datetime import datetime
+import os
+import unittest
+
+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
+
+
+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()
+        now = datetime.now()
+        build_id = self.factory.getUniqueInteger()
+        upload_leaf = build_base.getUploadLeaf(build_id, now=now)
+        self.assertEqual(
+            '%s-%s' % (now.strftime("%Y%m%d-%H%M%S"), build_id), upload_leaf)
+
+    def test_getUploadDir(self):
+        # getUploadDir is the absolute path to the directory in which things
+        # are uploaded to.
+        build_base = BuildBase()
+        build_id = self.factory.getUniqueInteger()
+        upload_leaf = build_base.getUploadLeaf(build_id)
+        upload_dir = build_base.getUploadDir(upload_leaf)
+        self.assertEqual(
+            os.path.join(config.builddmaster.root, 'incoming', upload_leaf),
+            upload_dir)
+
+
+class TestBuildBaseWithDatabase(TestCaseWithFactory):
+    """Tests for `IBuildBase` that need objects from the rest of Launchpad."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_getUploaderCommand(self):
+        build_base = BuildBase()
+        upload_leaf = self.factory.getUniqueString('upload-leaf')
+        build_base.distroseries = self.factory.makeDistroSeries()
+        build_base.distribution = build_base.distroseries.distribution
+        build_base.pocket = self.factory.getAnyPocket()
+        build_base.id = self.factory.getUniqueInteger()
+        build_base.policy_name = self.factory.getUniqueString('policy-name')
+        config_args = list(config.builddmaster.uploader.split())
+        log_file = self.factory.getUniqueString('logfile')
+        config_args.extend(
+            ['--log-file', log_file,
+             '-d', build_base.distribution.name,
+             '-s', (build_base.distroseries.name
+                    + pocketsuffix[build_base.pocket]),
+             '-b', str(build_base.id),
+             '-J', upload_leaf,
+             '--context=%s' % build_base.policy_name,
+             os.path.abspath(config.builddmaster.root),
+             ])
+        uploader_command = build_base.getUploaderCommand(
+            upload_leaf, log_file)
+        self.assertEqual(config_args, uploader_command)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml	2010-01-20 07:47:53 +0000
+++ lib/lp/soyuz/configure.zcml	2010-01-29 17:17:19 +0000
@@ -930,6 +930,39 @@
         permission="zope.Public" />
 
     <!-- SourcePackageRecipe -->
+
+    <securedutility
+       component="lp.soyuz.model.sourcepackagerecipe.SourcePackageRecipe"
+       provides="lp.soyuz.interfaces.sourcepackagerecipe.ISourcePackageRecipeSource">
+      <allow interface="lp.soyuz.interfaces.sourcepackagerecipe.ISourcePackageRecipeSource"/>
+    </securedutility>
+
+    <class
+       class="lp.soyuz.model.sourcepackagerecipebuild.SourcePackageRecipeBuild">
+      <allow interface="lp.soyuz.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuild"/>
+    </class>
+
+    <securedutility
+       component="lp.soyuz.model.sourcepackagerecipebuild.SourcePackageRecipeBuild"
+       provides="lp.soyuz.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildSource">
+      <allow interface="lp.soyuz.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildSource"/>
+    </securedutility>
+
+    <class
+       class="lp.soyuz.model.sourcepackagerecipebuild.SourcePackageRecipeBuildJob">
+      <allow interface="lp.soyuz.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJob"/>
+    </class>
+
+    <securedutility
+       component="lp.soyuz.model.sourcepackagerecipebuild.SourcePackageRecipeBuildJob"
+       provides="lp.soyuz.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJobSource">
+      <allow interface="lp.soyuz.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJobSource"/>
+    </securedutility>
+
+    <adapter factory="lp.soyuz.model.recipebuilder.RecipeBuildBehavior"
+             permission="zope.Public" />
+
+    <!-- SourcePackageRecipe -->
     <class
        class="lp.soyuz.model.sourcepackagerecipe.SourcePackageRecipe">
       <allow interface="lp.soyuz.interfaces.sourcepackagerecipe.ISourcePackageRecipe"/>
@@ -953,37 +986,9 @@
        class="bzrlib.plugins.builder.recipe.NestInstruction">
       <allow attributes="as_tuple recipe_branch nest_path" />
     </class>
-    <securedutility
-       component="lp.soyuz.model.sourcepackagerecipe.SourcePackageRecipe"
-       provides="lp.soyuz.interfaces.sourcepackagerecipe.ISourcePackageRecipeSource">
-      <allow interface="lp.soyuz.interfaces.sourcepackagerecipe.ISourcePackageRecipeSource"/>
-    </securedutility>
-
-    <class
-       class="lp.soyuz.model.sourcepackagerecipebuild.SourcePackageRecipeBuild">
-      <allow interface="lp.soyuz.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuild"/>
-    </class>
-
-    <securedutility
-       component="lp.soyuz.model.sourcepackagerecipebuild.SourcePackageRecipeBuild"
-       provides="lp.soyuz.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildSource">
-      <allow interface="lp.soyuz.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildSource"/>
-    </securedutility>
-
-    <class
-       class="lp.soyuz.model.sourcepackagerecipebuild.SourcePackageRecipeBuildJob">
-      <allow interface="lp.soyuz.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJob"/>
-    </class>
+
     <utility component="lp.soyuz.model.sourcepackagerecipebuild.SourcePackageRecipeBuildJob"
         name="RECIPEBRANCHBUILD"
         provides="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJob"/>
 
-    <securedutility
-       component="lp.soyuz.model.sourcepackagerecipebuild.SourcePackageRecipeBuildJob"
-       provides="lp.soyuz.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJobSource">
-      <allow interface="lp.soyuz.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJobSource"/>
-    </securedutility>
-
-    <adapter factory="lp.soyuz.model.recipebuilder.RecipeBuildBehavior"/>
-
 </configure>

=== modified file 'lib/lp/soyuz/interfaces/sourcepackagerecipebuild.py'
--- lib/lp/soyuz/interfaces/sourcepackagerecipebuild.py	2010-01-18 09:38:12 +0000
+++ lib/lp/soyuz/interfaces/sourcepackagerecipebuild.py	2010-01-29 17:17:19 +0000
@@ -49,6 +49,7 @@
         schema=ISourcePackageRecipe, required=True,
         title=_("The recipe being built."))
 
+
 class ISourcePackageRecipeBuildSource(Interface):
     """A utility of this interface be used to create source package builds."""
 

=== modified file 'lib/lp/soyuz/model/build.py'
--- lib/lp/soyuz/model/build.py	2010-01-20 01:24:01 +0000
+++ lib/lp/soyuz/model/build.py	2010-01-29 17:17:19 +0000
@@ -11,6 +11,9 @@
 import datetime
 import logging
 import operator
+import os
+import subprocess
+import time
 
 from zope.interface import implements
 from zope.component import getUtility
@@ -27,7 +30,8 @@
 from canonical.database.datetimecol import UtcDateTimeCol
 from canonical.database.enumcol import EnumCol
 from canonical.database.sqlbase import (
-    cursor, quote_like, SQLBase, sqlvalues)
+    clear_current_connection_cache, flush_database_updates, cursor,
+    quote_like, SQLBase, sqlvalues)
 from canonical.launchpad.components.decoratedresultset import (
     DecoratedResultSet)
 from canonical.launchpad.database.librarian import (
@@ -43,10 +47,12 @@
 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
+from lp.registry.interfaces.pocket import (PackagePublishingPocket,
+    pocketsuffix)
 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

=== modified file 'lib/lp/soyuz/model/buildpackagejob.py'
--- lib/lp/soyuz/model/buildpackagejob.py	2010-01-20 04:14:23 +0000
+++ lib/lp/soyuz/model/buildpackagejob.py	2010-01-29 17:17:19 +0000
@@ -213,8 +213,8 @@
         sub_query = """
             SELECT TRUE FROM Archive, Build, BuildPackageJob, DistroArchSeries
             WHERE
-            BuildPackageJob.job = Job.id AND 
-            BuildPackageJob.build = Build.id AND 
+            BuildPackageJob.job = Job.id AND
+            BuildPackageJob.build = Build.id AND
             Build.distroarchseries = DistroArchSeries.id AND
             Build.archive = Archive.id AND
             ((Archive.private IS TRUE AND

=== modified file 'lib/lp/soyuz/model/recipebuilder.py'
--- lib/lp/soyuz/model/recipebuilder.py	2010-01-18 09:38:12 +0000
+++ lib/lp/soyuz/model/recipebuilder.py	2010-01-29 17:17:19 +0000
@@ -68,7 +68,7 @@
         args["ogrecomponent"] = get_primary_current_component(
             self.build.archive, self.build.distroseries,
             self.build.sourcepackagename.name)
-        args['archives'] = get_sources_list_for_building(self.build, 
+        args['archives'] = get_sources_list_for_building(self.build,
             distroarchseries, self.build.sourcepackagename.name)
         return args
 
@@ -87,7 +87,7 @@
 
         chroot = distroarchseries.getChroot()
         if chroot is None:
-            raise CannotBuild("Unable to find a chroot for %s" % 
+            raise CannotBuild("Unable to find a chroot for %s" %
                               distroarchseries.displayname)
         self._builder.slave.cacheFile(logger, chroot)
 
@@ -131,10 +131,11 @@
 
         # This should already have been checked earlier, but just check again
         # here in case of programmer errors.
-        reason = check_upload_to_pocket(build.archive, build.distroseries, build.pocket)
+        reason = check_upload_to_pocket(
+            build.archive, build.distroseries, build.pocket)
         assert reason is None, (
                 "%s (%s) can not be built for pocket %s: invalid pocket due "
-                "to the series status of %s." % 
+                "to the series status of %s." %
                     (build.title, build.id, build.pocket.name,
                      build.distroseries.name))
 

=== modified file 'lib/lp/soyuz/model/sourcepackagerecipebuild.py'
--- lib/lp/soyuz/model/sourcepackagerecipebuild.py	2010-01-20 04:14:23 +0000
+++ lib/lp/soyuz/model/sourcepackagerecipebuild.py	2010-01-29 17:17:19 +0000
@@ -39,6 +39,8 @@
 class SourcePackageRecipeBuild(BuildBase, Storm):
     __storm_table__ = 'SourcePackageRecipeBuild'
 
+    policy_name = 'recipe'
+
     implements(ISourcePackageRecipeBuild)
     classProvides(ISourcePackageRecipeBuildSource)
 
@@ -87,9 +89,9 @@
 
     @property
     def pocket(self):
-        # JRV 2010-01-15: The database table really should have a pocket 
-        # column, although this is not a big problem at the moment 
-        # as recipe builds only happen for PPA's (so far). (bug 507307)
+        # XXX: JRV 2010-01-15 bug=507307: The database table really should
+        # have a pocket column, although this is not a big problem at the
+        # moment as recipe builds only happen for PPA's (so far).
         return PackagePublishingPocket.RELEASE
 
     recipe_id = Int(name='recipe', allow_none=False)
@@ -129,8 +131,8 @@
         self.sourcepackagename = sourcepackagename
 
     @classmethod
-    def new(
-        cls, sourcepackage, recipe, requester, archive, date_created=None):
+    def new(cls, sourcepackage, recipe, requester, archive,
+            date_created=None):
         """See `ISourcePackageRecipeBuildSource`."""
         store = IMasterStore(SourcePackageRecipeBuild)
         if date_created is None:

=== modified file 'lib/lp/soyuz/model/sourcepackagerecipedata.py'
--- lib/lp/soyuz/model/sourcepackagerecipedata.py	2010-01-13 02:59:33 +0000
+++ lib/lp/soyuz/model/sourcepackagerecipedata.py	2010-01-29 17:17:19 +0000
@@ -127,8 +127,8 @@
 
     sourcepackage_recipe_build_id = Int(
         name='sourcepackage_recipe_build', allow_none=True)
-    #sourcepackage_recipe_build = Reference(
-    #    sourcepackage_recipe_build_id, 'SourcePackageRecipeBuild.id')
+    sourcepackage_recipe_build = Reference(
+        sourcepackage_recipe_build_id, 'SourcePackageRecipeBuild.id')
 
     def getRecipe(self):
         """The BaseRecipeBranch version of the recipe."""

=== modified file 'lib/lp/soyuz/tests/test_recipebuilder.py'
--- lib/lp/soyuz/tests/test_recipebuilder.py	2010-01-21 04:34:44 +0000
+++ lib/lp/soyuz/tests/test_recipebuilder.py	2010-01-29 17:17:19 +0000
@@ -16,8 +16,8 @@
     IBuildFarmJobBehavior)
 from lp.buildmaster.manager import RecordingSlave
 from lp.soyuz.adapters.archivedependencies import get_sources_list_for_building
+from lp.soyuz.model.processor import ProcessorFamilySet
 from lp.soyuz.model.recipebuilder import RecipeBuildBehavior
-from lp.soyuz.model.processor import ProcessorFamilySet
 from lp.soyuz.model.sourcepackagerecipebuild import (
     SourcePackageRecipeBuild)
 from lp.soyuz.tests.soyuzbuilddhelpers import (MockBuilder,
@@ -48,21 +48,20 @@
         """Create a sample `ISourcePackageRecipeBuildJob`."""
         spn = self.factory.makeSourcePackageName("apackage")
         distro = self.factory.makeDistribution(name="distro")
-        distroseries = self.factory.makeDistroSeries(name="mydistro", 
+        distroseries = self.factory.makeDistroSeries(name="mydistro",
             distribution=distro)
-        processorfamily = ProcessorFamilySet().getByName('x86')
-        distroarchseries = self.factory.makeDistroArchSeries(
-            distroseries=distroseries, architecturetag='i386',
-            processorfamily=processorfamily)
+        processorfamily = ProcessorFamilySet().getByProcessorName('386')
+        distroseries.newArch(
+            'i386', processorfamily, True, self.factory.makePerson())
         sourcepackage = self.factory.makeSourcePackage(spn, distroseries)
         requester = self.factory.makePerson(email="[e-mail]",
             name="joe", displayname="Joe User")
-        somebranch = self.factory.makeBranch(owner=requester, name="pkg", 
+        somebranch = self.factory.makeBranch(owner=requester, name="pkg",
             product=self.factory.makeProduct("someapp"))
-        recipe = self.factory.makeSourcePackageRecipe(requester, requester, 
+        recipe = self.factory.makeSourcePackageRecipe(requester, requester,
              distroseries, spn, u"recept", somebranch)
-        spb = self.factory.makeSourcePackageRecipeBuild(sourcepackage=sourcepackage,
-            recipe=recipe, requester=requester)
+        spb = self.factory.makeSourcePackageRecipeBuild(
+            sourcepackage=sourcepackage, recipe=recipe, requester=requester)
         job = spb.makeJob()
         job = IBuildFarmJobBehavior(job)
         return job
@@ -95,8 +94,7 @@
         # _extraBuildArgs will return a sane set of additional arguments
         job = self.makeJob()
         distroarchseries = job.build.distroseries.architectures[0]
-        distroname = job.build.archive.distribution.name
-        self.assertEquals({
+        self.assertEqual({
            'author_email': u'[e-mail]',
            'suite': u'mydistro',
            'author_name': u'Joe User',
@@ -105,7 +103,7 @@
            'ogrecomponent': 'universe',
            'recipe_text': '# bzr-builder format 0.2 deb-version 1.0\n'
                           'lp://dev/~joe/someapp/pkg\n',
-           'archives': get_sources_list_for_building(job.build, 
+           'archives': get_sources_list_for_building(job.build,
                 distroarchseries, job.build.sourcepackagename.name)
             }, job._extraBuildArgs(distroarchseries))
 
@@ -132,10 +130,10 @@
         self.assertEquals(build_args[1], "sourcepackagerecipe")
         self.assertEquals(build_args[3], {})
         distroarchseries = job.build.distroseries.architectures[0]
-        self.assertEquals(build_args[4], job._extraBuildArgs(distroarchseries))
+        self.assertEqual(build_args[4], job._extraBuildArgs(distroarchseries))
 
     def test_dispatchBuildToSlave_nochroot(self):
-        # dispatchBuildToSlave will fail when there is not chroot tarball 
+        # dispatchBuildToSlave will fail when there is not chroot tarball
         # available for the distroseries to build for.
         job = self.makeJob()
         builder = MockBuilder("bob-de-bouwer", SaneBuildingSlave())
@@ -143,7 +141,7 @@
         builder.processor = processorfamily.processors[0]
         job.setBuilder(builder)
         logger = BufferLogger()
-        self.assertRaises(CannotBuild, job.dispatchBuildToSlave, 
+        self.assertRaises(CannotBuild, job.dispatchBuildToSlave,
             "someid", logger)
 
     def test_getById(self):