Merge lp:~jtv/launchpad/bug-537866 into lp:launchpad

[Jeroen T. Vermeulen]

Fix branch_url passed to the slave; fix slave build id verification for TranslationTemplatesBuildJob.

= Bug 537866 =

Two bugs with generating translation templates in the build farm based on bzr branches are fixed here:

 * Despite the TranslationTemplatesBuildJob having the right XMLRPC parameters for the slave in its metadata (as serialized in json_data), there was still a bit of old code that overrode it in TranslationTemplatesBuildBehavior.dispatchBuildToSlave—and with the wrong branch URL. An existing test proved easily extended to cover this.

 * BuildFarmJobBehavior.verifySlaveBuildID did not accept the format of TranslationTemplatesBuildJob names. I had to override it.

The first of these is a simple matter of using the stored parameters instead of recomputing them. Technically it may be nicer to compute the branch URL in TranslationTemplatesBuildBehavior.dispatchBuildToSlave, i.e. at the last moment. But on the other hand TranslationTemplatesBuildJob inherits the handling of a parameters dict from BranchJob anyway, and seeing the URL there makes these objects a lot easier to recognize and deal with during debugging. Something doesn't work? Snip the URL and try it out in a shell. At the stage we're in, that is invaluable. It may remain so if we have to debug problems with this highly complex infrastructure in the future.

The second happens because the base BuildFarmJobBehavior's verifySlaveBuildID assumed a slave build id composed of a build id and a buildqueue id, separated by a dash. TranslationTemplatesBuildJob does not have a build, so it substitutes the branch name instead. (One test runs a nasty case through verifySlaveBuildID where the branch name has dashes wherever a branch name can have them).

In order to avoid code duplication in doing so, I extracted two helpers from the base verifySlaveBuildID: one verifies the Build id contained in the name, and the other the BuildQueue id. The main method just extracts those two parts, calls the helpers, and finally checks that the objects are properly matched. The two helpers ended up so similar that I couldn't help creating yet a third helper that implements either, depending on some customizing parameters.

Implementing the tests required me to produce a realistic Build object. This hurts, but a _relatively_ easy way to do it was to create a SourcePackageRelease first. The factory did not have a makeSPR, but it had another method that created an SPR in the process of creating something else, so I extracted makeSPR from that.

To test:
{{{
./bin/test -vv -t test_translationtemplatesbuildbehavior -t buildfarmjobbehavior
}}}

For Q/A, go through the instructions at https://dev.launchpad.net/Translations/BuildTemplatesOnLocalBuildFarm (which is a bit of work, but at least it's documented in detail).

No lint.

Jeroen

[Guilherme Salgado (salgado)]

Hi Jeroen,

I found a few issues that I'd like you to address before I do a proper
review of this branch.

 review needsfixing

> === modified file 'lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py'
> --- lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py 2010-01-21 05:03:16 +0000
> +++ lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py 2010-03-12 13:56:29 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> # pylint: disable-msg=E0211,E0213
> @@ -57,6 +57,28 @@
> of IBuilder.slaveStatus().
> """
>
> + def getVerifiedBuild(raw_id):
> + """Verify the `Build` id component of a slave build id.

I find it a bit odd that the method's name implies it's used to get
something but its docstring says it just verifies. How about

    """Return the `Build` object with the given id.

    ...
    """

I'd also like the docstring to state what sort of verification is done.

Oh, and does this need to be part of the public interface? ISTM that
it's only used internally, so it could be left out of the interface, no?

> +
> + By default, a `BuildFarmJob` has an identifying name of the form
> + "b-q", where b is the id of its `Build` and q is the id of its
> + `BuildQueue` record.
> +
> + Use `getVerifiedBuild` to verify the "b" part, and retrieve the
> + associated `Build`.
> + """
> +
> + def getVerifiedBuildQueue(raw_id):
> + """Verify the `BuildQueue` id component of a slave build id.

Same here.

> +
> + By default, a `BuildFarmJob` has an identifying name of the form
> + "b-q", where b is the id of its `Build` and q is the id of its
> + `BuildQueue` record.
> +
> + Use `getVerifiedBuildQueue` to verify the "q" part, and retrieve
> + the associated `BuildQueue` object.
> + """
> +
> def verifySlaveBuildID(slave_build_id):
> """Verify that a slave's build ID shows no signs of corruption.
>
>
> === modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
> --- lib/lp/buildmaster/model/buildfarmjobbehavior.py 2010-03-05 13:52:32 +0000
> +++ lib/lp/buildmaster/model/buildfarmjobbehavior.py 2010-03-12 13:56:29 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> # pylint: disable-msg=E0211,E0213
> @@ -17,17 +17,22 @@
> import xmlrpclib
>
> from sqlobject import SQLObjectNotFound
> +
> from zope.component import getUtility
> from zope.interface import implements
> -from zope.security.proxy import removeSecurityProxy
> +from zope.security.proxy import removeSecurityProxy, isinstance as zisinstance
>
> from canonical import encoding
> from canonical.librarian.interfaces import ILibrarianClient
> +
> +from canonical.launchpad.webapp.interfaces import NotFoundError
> from lp.buildmaster.interfaces.bui...

review: Needs Fixing

[Jeroen T. Vermeulen (jtv)]

This is scary. Some arbitrary characters seem to have changed in the code after I last ran "make lint" and the tests. The duplicate test method names (where the word "Queue" was missing in two places) may have been an omission on my part as I continued adding tests at a late stage, though I think these were from an earlier state that I also ran through lint. The "utem" and missing spaces definitely passed lint since I last worked on them.

I hope it's simply because of the mouse cursor moving and activating a different window while I'm typing (as it likes to do for some reason—I know there's a setting for that but I guess my touchpad driver doesn't obey it) but in the past few hours I've also had unexpected characters appearing when copy/pasting. I'll reboot as soon as possible.

Anyway, I've removed the two helper methods from the interface and updated the doc strings.

[Guilherme Salgado (salgado)]

I was doing a second pass on this but then you told me you've got some
more things you need to change here, so I'm sending what I have so far.

> === modified file 'lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py'
> --- lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py 2010-03-06 04:57:40 +0000
> +++ lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py 2010-03-12 16:54:28 +0000
> @@ -3,10 +3,18 @@
>
> """Unit tests for BuildFarmJobBehaviorBase."""
>
> -from unittest import TestCase, TestLoader
> +from unittest import TestLoader
> +
> +from zope.component import getUtility
> +
> +from canonical.testing.layers import ZopelessDatabaseLayer
> +from lp.testing import TestCaseWithFactory
>
> from lp.buildmaster.interfaces.buildbase import BuildStatus
> +from lp.buildmaster.interfaces.builder import CorruptBuildID
> from lp.buildmaster.model.buildfarmjobbehavior import BuildFarmJobBehaviorBase
> +from lp.registry.interfaces.pocket import PackagePublishingPocket
> +from lp.soyuz.interfaces.processor import IProcessorFamilySet
>
>
> class FakeBuildFarmJob:
> @@ -14,27 +22,90 @@
> pass
>
>
> -class TestBuildFarmJobBehaviorBase(TestCase):
> +class TestBuildFarmJobBehaviorBase(TestCaseWithFactory):
> """Test very small, basic bits of BuildFarmJobBehaviorBase."""
>
> - def setUp(self):
> - self.behavior = BuildFarmJobBehaviorBase(FakeBuildFarmJob())
> + layer = ZopelessDatabaseLayer
> +
> + def _makeBehavior(self, buildfarmjob=None):
> + """Create a `BuildFarmJobBehaviorBase`."""
> + if buildfarmjob is None:
> + buildfarmjob = FakeBuildFarmJob()
> + return BuildFarmJobBehaviorBase(buildfarmjob)
> +
> + def _makeBuild(self):
> + """Create a `Build` object."""
> + x86 = getUtility(IProcessorFamilySet).getByName('x86')
> + distroarchseries = self.factory.makeDistroArchSeries(
> + architecturetag='x86', processorfamily=x86)
> + distroseries = distroarchseries.distroseries
> + archive = self.factory.makeArchive(
> + distribution=distroseries.distribution)
> + pocket = PackagePublishingPocket.RELEASE
> + spr = self.factory.makeSPR(distroseries=distroseries, archive=archive)
> +
> + return spr.createBuild(
> + distroarchseries=distroarchseries, pocket=pocket, archive=archive)
> +
> + def _makeBuildQueue(self):
> + """Create a `BuildQueue` object."""
> + return self.factory.makeSourcePackageRecipeBuildJob()
>
> def test_extractBuildStatus_baseline(self):
> # extractBuildStatus picks the name of the build status out of a
> # dict describing the slave's status.
> slave_status = {'build_status': 'BuildStatus.BUILDING'}
> + behavior = self._makeBehavior()
> self.assertEqual(
> BuildStatus.BUILDING.name,
> - self.behavior.extractBuildStatus(slave_status))
> + behavior.extractBuildStatus(slave_status))
>
> def test_extractBuildStatus_malformed(self):
> # extractBuildStatus errors out when the status string is not
> # of the form it expects.
> slave_status = ...

[Jeroen T. Vermeulen (jtv)]

> I was doing a second pass on this but then you told me you've got some
> more things you need to change here, so I'm sending what I have so far.

That's right. The latest change was in the translationtemplatesbuildbehavior doctest and, with a bit of extra work, allowed me to retire an XXX.

> > === modified file 'lib/lp/testing/factory.py'
> > --- lib/lp/testing/factory.py 2010-03-08 12:35:15 +0000
> > +++ lib/lp/testing/factory.py 2010-03-12 16:54:28 +0000
> > @@ -2007,46 +2007,35 @@
> > def getAnySourcePackageUrgency(self):
> > return SourcePackageUrgency.MEDIUM
> >
> > - def makeSourcePackagePublishingHistory(self, sourcepackagename=None,
> > - distroseries=None,
> maintainer=None,
> > - creator=None, component=None,
> > - section=None, urgency=None,
> > - version=None, archive=None,
> > - builddepends=None,
> > - builddependsindep=None,
> > - build_conflicts=None,
> > - build_conflicts_indep=None,
> > - architecturehintlist='all',
> > - dateremoved=None,
> > - date_uploaded=UTC_NOW,
> > - pocket=None,
> > - status=None,
> > - scheduleddeletiondate=None,
> > - dsc_standards_version='3.6.2',
> > - dsc_format='1.0',
> > - dsc_binaries='foo-bin',
> > - ):
> > - if sourcepackagename is None:
> > - sourcepackagename = self.makeSourcePackageName()
> > - spn = sourcepackagename
> > + def makeSPR(self, archive=None, sourcepackagename=None,
> distroseries=None,
>
> Why not makeSourcePackageRelease()? That seems to be consistent with the
> existing ones and avoids the abbreviation, which are not good anyway.

Sure. It's only 5 lines extra; I thought it would be worse. I admit it: I abbreviated to avoid having to line up the parameters on all the following lines behind the opening parenthesis.

> > + if sourcepackagename is None:
> > + sourcepackagename = self.makeSourcePackageName()
> > + spn = sourcepackagename
>
> Would you mind dropping the line above and using 'sourcepackagename' in the
> single place that uses it below?

Not at all. I was just copying over the original code, but now that it's broken down into more manageable pieces, might as well clean up a bit more.

> > @@ -2064,12 +2053,13 @@
> > if creator is None:
> > creator = self.makePerson()
> >
> > + if dscsigningkey is None:
> > + gpg_key = self.makeGPGKey(creator)
>
> This is broken now -- gpg_key is no longer used, so y...

[Guilherme Salgado (salgado)]

On Fri, 2010-03-12 at 20:24 +0000, Jeroen T. Vermeulen wrote:
> > > + def makeSourcePackagePublishingHistory(self, sourcepackagename=None,
> > > + distroseries=None,
> > maintainer=None,
> > > + creator=None, component=None,
> > > + section=None, urgency=None,
> > > + version=None, archive=None,
> > > + builddepends=None,
> > > + builddependsindep=None,
> > > + build_conflicts=None,
> > > + build_conflicts_indep=None,
> > > + architecturehintlist='all',
> > > + dateremoved=None,
> > > + date_uploaded=UTC_NOW,
> > > + pocket=None,
> > > + status=None,
> > > + scheduleddeletiondate=None,
> > > + dsc_standards_version='3.6.2',
> > > + dsc_format='1.0',
> > > + dsc_binaries='foo-bin',
> > > + ):
> > > + if pocket is None:
> > > + pocket = self.getAnyPocket()
> > > +
> > > + if status is None:
> > > + status = PackagePublishingStatus.PENDING
> > > +
> > > + spr = self.makeSPR(
> > > + archive=archive,
> > > + sourcepackagename=sourcepackagename,
> > > + distroseries=distroseries,
> > > + maintainer=maintainer,
> > > + creator=creator, component=component,
> > > + section=section,
> > > + urgency=urgency,
> > > + version=version,
> > > + builddepends=builddepends,
> > > + builddependsindep=builddependsindep,
> > > + build_conflicts=build_conflicts,
> > > + build_conflicts_indep=build_conflicts_indep,
> > > + architecturehintlist=architecturehintlist,
> > > + dsc_standards_version=dsc_standards_version,
> > > + dsc_format=dsc_format,
> > > + dsc_binaries=dsc_binaries,
> > > + date_uploaded=date_uploaded)
> > >
> > > sspph = SourcePackagePublishingHistory(
> > > - distroseries=distroseries,
> > > + distroseries=spr.distroseries,
> >
> > Is this change needed? Couldn't you keep using the distroseries passed as
> > argument?
>
> Again, you're right. I just need to create a distroseries here if I don't already have one, so it adds a tiny bit of duplication.
>

Oh, it wasn't clear to me that you were doing that because makeSPR would
create the distro when None was given to it. If there was a comment I
wouldn't have complained here.

Anyway, I'm happy if you want to leave this tiny bit of duplication but
I'd also be happy with what you had before, a...

review: Approve

=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py	2010-01-21 05:03:16 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py	2010-03-13 10:43:28 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0211,E0213

=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py	2010-03-05 13:52:32 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py	2010-03-13 10:43:28 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0211,E0213
@@ -17,17 +17,22 @@
 import xmlrpclib
 
 from sqlobject import SQLObjectNotFound
+
 from zope.component import getUtility
 from zope.interface import implements
-from zope.security.proxy import removeSecurityProxy
+from zope.security.proxy import removeSecurityProxy, isinstance as zisinstance
 
 from canonical import encoding
 from canonical.librarian.interfaces import ILibrarianClient
+
+from canonical.launchpad.webapp.interfaces import NotFoundError
 from lp.buildmaster.interfaces.builder import CorruptBuildID
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
     BuildBehaviorMismatch, IBuildFarmJobBehavior)
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
+from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.services.job.interfaces.job import JobStatus
+from lp.soyuz.interfaces.build import IBuildSet
 
 
 class BuildFarmJobBehaviorBase:
@@ -59,22 +64,101 @@
         The default behavior is that we don't add any extra values."""
         return {}
 
+    def _helpVerifyBuildIDComponent(self, raw_id, item_type, finder):
+        """Helper for verifying parts of a `BuildFarmJob` name.
+
+        Different `IBuildFarmJob` implementations can have different
+        ways of constructing their identifying names.  The names are
+        produced by `IBuildFarmJob.getName` and verified by
+        `IBuildFarmJobBehavior.verifySlaveBuildID`.
+
+        This little helper makes it easier to verify an object id
+        embedded in that name, check that it's a valid number, and
+        retrieve the associated database object.
+
+        :param raw_id: An unverified id string as extracted from the
+            build name.  The method will verify that it is a number, and
+            try to retrieve the associated object.
+        :param item_type: The type of object this id represents.  Should
+            be a class.
+        :param finder: A function that, given an integral id, finds the
+            associated object of type `item_type`.
+        :raise CorruptBuildID: If `raw_id` is malformed in some way or
+            the associated `item_type` object is not found.
+        :return: An object that is an instance of `item_type`.
+        """
+        type_name = item_type.__name__
+        try:
+            numeric_id = int(raw_id)
+        except ValueError:
+            raise CorruptBuildID(
+                "%s ID is not a number: '%s'" % (type_name, raw_id))
+
+        try:
+            item = finder(numeric_id)
+        except (NotFoundError, SQLObjectNotFound), reason:
+            raise CorruptBuildID(
+                "%s %d is not available: %s" % (
+                    type_name, numeric_id, reason))
+        except Exception, reason:
+            raise CorruptBuildID(
+                "Error while looking up %s %d: %s" % (
+                    type_name, numeric_id, reason))
+
+        if item is None:
+            raise CorruptBuildID("There is no %s with id %d." % (
+                type_name, numeric_id))
+
+        assert zisinstance(item, item_type), (
+            "Looked for %s, but found %s." % (type_name, repr(item)))
+
+        return item
+
+    def getVerifiedBuild(self, raw_id):
+        """Verify and retrieve the `Build` component of a slave build id.
+
+        This does part of the verification for `verifySlaveBuildID`.
+
+        By default, a `BuildFarmJob` has an identifying name of the form
+        "b-q", where b is the id of its `Build` and q is the id of its
+        `BuildQueue` record.
+
+        Use `getVerifiedBuild` to verify the "b" part, and retrieve the
+        associated `Build`.
+        """
+        # Avoid circular import.
+        from lp.soyuz.model.build import Build
+
+        return self._helpVerifyBuildIDComponent(
+            raw_id, Build, getUtility(IBuildSet).getByBuildID)
+
+    def getVerifiedBuildQueue(self, raw_id):
+        """Verify and retrieve the `BuildQueue` component of a slave build id.
+
+        This does part of the verification for `verifySlaveBuildID`.
+
+        By default, a `BuildFarmJob` has an identifying name of the form
+        "b-q", where b is the id of its `Build` and q is the id of its
+        `BuildQueue` record.
+
+        Use `getVerifiedBuildQueue` to verify the "q" part, and retrieve
+        the associated `BuildQueue` object.
+        """
+        return self._helpVerifyBuildIDComponent(
+            raw_id, BuildQueue, getUtility(IBuildQueueSet).get)
+
     def verifySlaveBuildID(self, slave_build_id):
         """See `IBuildFarmJobBehavior`."""
         # Extract information from the identifier.
         try:
             build_id, queue_item_id = slave_build_id.split('-')
-            build_id = int(build_id)
-            queue_item_id = int(queue_item_id)
         except ValueError:
             raise CorruptBuildID('Malformed build ID')
+            
+        build = self.getVerifiedBuild(build_id)
+        queue_item = self.getVerifiedBuildQueue(queue_item_id)
 
-        try:
-            queue_item = getUtility(IBuildQueueSet).get(queue_item_id)
-            # Check whether build and buildqueue are properly related.
-        except SQLObjectNotFound, reason:
-            raise CorruptBuildID(str(reason))
-        if queue_item.specific_job.build.id != build_id:
+        if build != queue_item.specific_job.build:
             raise CorruptBuildID('Job build entry mismatch')
 
     def updateBuild(self, queueItem):

=== modified file 'lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py'
--- lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py	2010-03-06 04:57:40 +0000
+++ lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py	2010-03-13 10:43:28 +0000
@@ -3,10 +3,18 @@
 
 """Unit tests for BuildFarmJobBehaviorBase."""
 
-from unittest import TestCase, TestLoader
+from unittest import TestLoader
+
+from zope.component import getUtility
+
+from canonical.testing.layers import ZopelessDatabaseLayer
+from lp.testing import TestCaseWithFactory
 
 from lp.buildmaster.interfaces.buildbase import BuildStatus
+from lp.buildmaster.interfaces.builder import CorruptBuildID
 from lp.buildmaster.model.buildfarmjobbehavior import BuildFarmJobBehaviorBase
+from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.soyuz.interfaces.processor import IProcessorFamilySet
 
 
 class FakeBuildFarmJob:
@@ -14,27 +22,91 @@
     pass
 
 
-class TestBuildFarmJobBehaviorBase(TestCase):
+class TestBuildFarmJobBehaviorBase(TestCaseWithFactory):
     """Test very small, basic bits of BuildFarmJobBehaviorBase."""
 
-    def setUp(self):
-        self.behavior = BuildFarmJobBehaviorBase(FakeBuildFarmJob())
+    layer = ZopelessDatabaseLayer
+
+    def _makeBehavior(self, buildfarmjob=None):
+        """Create a `BuildFarmJobBehaviorBase`."""
+        if buildfarmjob is None:
+            buildfarmjob = FakeBuildFarmJob()
+        return BuildFarmJobBehaviorBase(buildfarmjob)
+
+    def _makeBuild(self):
+        """Create a `Build` object."""
+        x86 = getUtility(IProcessorFamilySet).getByName('x86')
+        distroarchseries = self.factory.makeDistroArchSeries(
+            architecturetag='x86', processorfamily=x86)
+        distroseries = distroarchseries.distroseries
+        archive = self.factory.makeArchive(
+            distribution=distroseries.distribution)
+        pocket = PackagePublishingPocket.RELEASE
+        spr = self.factory.makeSourcePackageRelease(
+            distroseries=distroseries, archive=archive)
+
+        return spr.createBuild(
+            distroarchseries=distroarchseries, pocket=pocket, archive=archive)
+
+    def _makeBuildQueue(self):
+        """Create a `BuildQueue` object."""
+        return self.factory.makeSourcePackageRecipeBuildJob()
 
     def test_extractBuildStatus_baseline(self):
         # extractBuildStatus picks the name of the build status out of a
         # dict describing the slave's status.
         slave_status = {'build_status': 'BuildStatus.BUILDING'}
+        behavior = self._makeBehavior()
         self.assertEqual(
             BuildStatus.BUILDING.name,
-            self.behavior.extractBuildStatus(slave_status))
+            behavior.extractBuildStatus(slave_status))
 
     def test_extractBuildStatus_malformed(self):
         # extractBuildStatus errors out when the status string is not
         # of the form it expects.
         slave_status = {'build_status': 'BUILDING'}
-        self.assertRaises(
-            AssertionError,
-            self.behavior.extractBuildStatus, slave_status)
+        behavior = self._makeBehavior()
+        self.assertRaises(
+            AssertionError, behavior.extractBuildStatus, slave_status)
+
+    def test_getVerifiedBuild_success(self):
+        build = self._makeBuild()
+        behavior = self._makeBehavior()
+        raw_id = str(build.id)
+
+        self.assertEqual(build, behavior.getVerifiedBuild(raw_id))
+
+    def test_getVerifiedBuild_malformed(self):
+        behavior = self._makeBehavior()
+        self.assertRaises(CorruptBuildID, behavior.getVerifiedBuild, 'hi!')
+
+    def test_getVerifiedBuild_notfound(self):
+        build = self._makeBuild()
+        behavior = self._makeBehavior()
+        nonexistent_id = str(build.id + 99)
+
+        self.assertRaises(
+            CorruptBuildID, behavior.getVerifiedBuild, nonexistent_id)
+
+    def test_getVerifiedBuildQueue_success(self):
+        buildqueue = self._makeBuildQueue()
+        behavior = self._makeBehavior()
+        raw_id = str(buildqueue.id)
+
+        self.assertEqual(buildqueue, behavior.getVerifiedBuildQueue(raw_id))
+
+    def test_getVerifiedBuildQueue_malformed(self):
+        behavior = self._makeBehavior()
+        self.assertRaises(
+            CorruptBuildID, behavior.getVerifiedBuildQueue, 'bye!')
+
+    def test_getVerifiedBuildQueue_notfound(self):
+        buildqueue = self._makeBuildQueue()
+        behavior = self._makeBehavior()
+        nonexistent_id = str(buildqueue.id + 99)
+
+        self.assertRaises(
+            CorruptBuildID, behavior.getVerifiedBuildQueue, nonexistent_id)
 
 
 def test_suite():

=== modified file 'lib/lp/code/model/recipebuilder.py'
--- lib/lp/code/model/recipebuilder.py	2010-02-05 15:06:28 +0000
+++ lib/lp/code/model/recipebuilder.py	2010-03-13 10:43:28 +0000
@@ -8,7 +8,7 @@
     'RecipeBuildBehavior',
     ]
 
-from zope.component import adapts
+from zope.component import adapts, getUtility
 from zope.interface import implements
 
 from lp.archiveuploader.permission import check_upload_to_pocket
@@ -18,7 +18,8 @@
 from lp.buildmaster.model.buildfarmjobbehavior import (
     BuildFarmJobBehaviorBase)
 from lp.code.interfaces.sourcepackagerecipebuild import (
-    ISourcePackageRecipeBuildJob)
+    ISourcePackageRecipeBuildJob, ISourcePackageRecipeBuildSource)
+from lp.code.model.sourcepackagerecipebuild import SourcePackageRecipeBuild
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.soyuz.adapters.archivedependencies import (
     get_primary_current_component, get_sources_list_for_building)
@@ -168,3 +169,11 @@
                 extra_info['logtail'] = raw_slave_status[2]
 
         return extra_info
+
+    def getVerifiedBuild(self, raw_id):
+        """See `IBuildFarmJobBehavior`."""
+        # This type of job has a build that is of type BuildBase but not
+        # actually a Build.
+        return self._helpVerifyBuildIDComponent(
+            raw_id, SourcePackageRecipeBuild,
+            getUtility(ISourcePackageRecipeBuildSource).getById)

=== modified file 'lib/lp/soyuz/doc/buildd-slavescanner.txt'
--- lib/lp/soyuz/doc/buildd-slavescanner.txt	2010-03-10 12:50:18 +0000
+++ lib/lp/soyuz/doc/buildd-slavescanner.txt	2010-03-13 10:43:28 +0000
@@ -95,15 +95,16 @@
     >>> lostbuilding_builder = MockBuilder(
     ...     'Lost Building Slave', LostBuildingSlave())
     >>> buildergroup.rescueBuilderIfLost(lostbuilding_builder)
-    WARNING:root:Builder 'Lost Building Slave' rescued from
-    '1000-10000': 'Object not found'
+    WARNING:root:Builder 'Lost Building Slave' rescued from '1000-10000':
+    'Build 1000 is not available: 'Object not found''
 
 Then a lost slave in status 'WAITING':
 
     >>> lostwaiting_builder = MockBuilder(
     ...     'Lost Waiting Slave', LostWaitingSlave())
     >>> buildergroup.rescueBuilderIfLost(lostwaiting_builder)
-    WARNING:root:Builder 'Lost Waiting Slave' rescued from '1000-10000': 'Object not found'
+    WARNING:root:Builder 'Lost Waiting Slave' rescued from '1000-10000':
+    'Build 1000 is not available: 'Object not found''
 
 Both got rescued, as expected.
 

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-03-11 20:54:36 +0000
+++ lib/lp/testing/factory.py	2010-03-13 10:43:28 +0000
@@ -2008,6 +2008,81 @@
     def getAnySourcePackageUrgency(self):
         return SourcePackageUrgency.MEDIUM
 
+    def makeSourcePackageRelease(self, archive=None, sourcepackagename=None,
+                                 distroseries=None, maintainer=None,
+                                 creator=None, component=None, section=None,
+                                 urgency=None, version=None,
+                                 builddepends=None, builddependsindep=None,
+                                 build_conflicts=None,
+                                 build_conflicts_indep=None,
+                                 architecturehintlist='all',
+                                 dsc_maintainer_rfc822=None,
+                                 dsc_standards_version='3.6.2',
+                                 dsc_format='1.0', dsc_binaries='foo-bin',
+                                 date_uploaded=UTC_NOW):
+        """Make a `SourcePackageRelease`."""
+        if distroseries is None:
+            if archive is None:
+                distribution = None
+            else:
+                distribution = archive.distribution
+            distroseries = self.makeDistroRelease(distribution=distribution)
+
+        if archive is None:
+            archive = self.makeArchive(
+                distribution=distroseries.distribution,
+                purpose=ArchivePurpose.PRIMARY)
+
+        if sourcepackagename is None:
+            sourcepackagename = self.makeSourcePackageName()
+
+        if component is None:
+            component = self.makeComponent()
+
+        if urgency is None:
+            urgency = self.getAnySourcePackageUrgency()
+
+        if section is None:
+            section = self.getUniqueString('section')
+        section = getUtility(ISectionSet).ensure(section)
+
+        if maintainer is None:
+            maintainer = self.makePerson()
+
+        maintainer_email = '%s <%s>' % (
+            maintainer.displayname,
+            maintainer.preferredemail.email)
+
+        if creator is None:
+            creator = self.makePerson()
+
+        if version is None:
+            version = self.getUniqueString('version')
+
+        return distroseries.createUploadedSourcePackageRelease(
+            sourcepackagename=sourcepackagename,
+            maintainer=maintainer,
+            creator=creator,
+            component=component,
+            section=section,
+            urgency=urgency,
+            version=version,
+            builddepends=builddepends,
+            builddependsindep=builddependsindep,
+            build_conflicts=build_conflicts,
+            build_conflicts_indep=build_conflicts_indep,
+            architecturehintlist=architecturehintlist,
+            changelog_entry=None,
+            dsc=None,
+            copyright=self.getUniqueString(),
+            dscsigningkey=None,
+            dsc_maintainer_rfc822=maintainer_email,
+            dsc_standards_version=dsc_standards_version,
+            dsc_format=dsc_format,
+            dsc_binaries=dsc_binaries,
+            archive=archive,
+            dateuploaded=date_uploaded)
+
     def makeSourcePackagePublishingHistory(self, sourcepackagename=None,
                                            distroseries=None, maintainer=None,
                                            creator=None, component=None,
@@ -2027,54 +2102,31 @@
                                            dsc_format='1.0',
                                            dsc_binaries='foo-bin',
                                            ):
-        if sourcepackagename is None:
-            sourcepackagename = self.makeSourcePackageName()
-        spn = sourcepackagename
-
+        """Make a `SourcePackagePublishingHistory`."""
         if distroseries is None:
-            distroseries = self.makeDistroRelease()
+            if archive is None:
+                distribution = None
+            else:
+                distribution = archive.distribution
+            distroseries = self.makeDistroRelease(distribution=distribution)
 
         if archive is None:
             archive = self.makeArchive(
                 distribution=distroseries.distribution,
                 purpose=ArchivePurpose.PRIMARY)
 
-        if component is None:
-            component = self.makeComponent()
-
         if pocket is None:
             pocket = self.getAnyPocket()
 
         if status is None:
             status = PackagePublishingStatus.PENDING
 
-        if urgency is None:
-            urgency = self.getAnySourcePackageUrgency()
-
-        if section is None:
-            section = self.getUniqueString('section')
-        section = getUtility(ISectionSet).ensure(section)
-
-        if maintainer is None:
-            maintainer = self.makePerson()
-
-        maintainer_email = '%s <%s>' % (
-            maintainer.displayname,
-            maintainer.preferredemail.email)
-
-        if creator is None:
-            creator = self.makePerson()
-
-        if version is None:
-            version = self.getUniqueString('version')
-
-        gpg_key = self.makeGPGKey(creator)
-
-        spr = distroseries.createUploadedSourcePackageRelease(
-            sourcepackagename=spn,
+        spr = self.makeSourcePackageRelease(
+            archive=archive,
+            sourcepackagename=sourcepackagename,
+            distroseries=distroseries,
             maintainer=maintainer,
-            creator=creator,
-            component=component,
+            creator=creator, component=component,
             section=section,
             urgency=urgency,
             version=version,
@@ -2083,15 +2135,10 @@
             build_conflicts=build_conflicts,
             build_conflicts_indep=build_conflicts_indep,
             architecturehintlist=architecturehintlist,
-            changelog_entry=None,
-            dsc=None,
-            copyright=self.getUniqueString(),
-            dscsigningkey=gpg_key,
-            dsc_maintainer_rfc822=maintainer_email,
             dsc_standards_version=dsc_standards_version,
             dsc_format=dsc_format,
             dsc_binaries=dsc_binaries,
-            archive=archive, dateuploaded=date_uploaded)
+            date_uploaded=date_uploaded)
 
         sspph = SourcePackagePublishingHistory(
             distroseries=distroseries,

=== modified file 'lib/lp/translations/doc/translationtemplatesbuildbehavior.txt'
--- lib/lp/translations/doc/translationtemplatesbuildbehavior.txt	2010-03-05 13:52:32 +0000
+++ lib/lp/translations/doc/translationtemplatesbuildbehavior.txt	2010-03-13 10:43:28 +0000
@@ -2,13 +2,21 @@
 
 == Setup ==
 
-Set up build environment.
+Set up build environment.  Clear out the build queue.
 
     >>> import transaction
     >>> import logging
     >>> logger = logging.getLogger()
     >>> logger.setLevel(logging.CRITICAL)
 
+    >>> from canonical.database.sqlbase import quote
+    >>> from canonical.launchpad.interfaces.lpstorm import IMasterStore
+    >>> from lp.services.job.interfaces.job import JobStatus
+    >>> from lp.services.job.model.job import Job
+    >>> store = IMasterStore(Job)
+    >>> query = store.execute("UPDATE Job SET status = %s" % quote(
+    ...     JobStatus.FAILED))
+
     >>> from lp.buildmaster.master import BuilddMaster
     >>> from canonical.buildd.tests import BuilddSlaveTestSetup
     >>> bm = BuilddMaster(logger, transaction)
@@ -39,7 +47,21 @@
 
 Make a builder to process our build request.
 
-    >>> builder = factory.makeBuilder(virtualized=False, processor=processor)
+    >>> builder = factory.makeBuilder(
+    ...     virtualized=True, processor=processor, vm_host='hostname')
+
+The builder doesn't talk to a real slave.  We don't have those in our
+test suite.  But we give it a fake one.
+
+    >>> from lp.testing.fakemethod import FakeMethod
+    >>> class FakeSlave:
+    ...     build = FakeMethod()
+    ...     cacheFile = FakeMethod()
+    ...     resume = FakeMethod(result=('Output here', 'Errors here', 0))
+
+    >>> from zope.security.proxy import removeSecurityProxy
+    >>> slave = FakeSlave()
+    >>> removeSecurityProxy(builder).slave = slave
 
 
 == Get a job! ==
@@ -60,20 +82,27 @@
     >>> print buildqueue.date_started
     None
 
-Dispatch the job to the build slave.  The proper method to call here
-would have been Builder.findAndStartJob, but it has not been generalised
-to handle new job types yet.
-
-# XXX JeroenVermeulen bug=506617: call findAndStartJob instead.
-
-    >>> from zope.security.proxy import removeSecurityProxy
-    >>> removeSecurityProxy(builder)._dispatchBuildCandidate(buildqueue)
-
-The build is now marked as started.
+Our job is now first in line to be executed.
+
+    >>> removeSecurityProxy(builder)._findBuildCandidate() == buildqueue
+    True
+
+Dispatch the first job in line (ours!) to the build slave.
+
+    >>> activated_job = builder.findAndStartJob()
+    >>> activated_job == buildqueue
+    True
+
+Our build is now marked as started.
 
     >>> buildqueue.date_started is None
     False
 
+The slave's build method has been called.
+
+    >>> slave.build.call_count
+    1
+
 
 == Teardown ==
 

=== modified file 'lib/lp/translations/model/translationtemplatesbuildbehavior.py'
--- lib/lp/translations/model/translationtemplatesbuildbehavior.py	2010-02-11 19:11:11 +0000
+++ lib/lp/translations/model/translationtemplatesbuildbehavior.py	2010-03-13 10:43:28 +0000
@@ -17,6 +17,7 @@
 
 from canonical.launchpad.interfaces import ILaunchpadCelebrities
 
+from lp.buildmaster.interfaces.builder import CorruptBuildID
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
     IBuildFarmJobBehavior)
 from lp.buildmaster.model.buildfarmjobbehavior import (
@@ -40,12 +41,27 @@
         self._builder.slave.cacheFile(logger, chroot)
         buildid = self.buildfarmjob.getName()
 
-        args = { 'branch_url': self.buildfarmjob.branch.url }
+        args = self.buildfarmjob.metadata
         filemap = {}
 
         self._builder.slave.build(
             buildid, self.build_type, chroot_sha1, filemap, args)
 
+    def verifySlaveBuildID(self, slave_build_id):
+        """See `IBuildFarmJobBehavior`."""
+        try:
+            branch_name, queue_item_id = slave_build_id.rsplit('-', 1)
+        except ValueError:
+            raise CorruptBuildID(
+                "Malformed translation templates build id: '%s'" % (
+                    slave_build_id))
+
+        buildqueue = self.getVerifiedBuildQueue(queue_item_id)
+        if buildqueue.job != self.buildfarmjob.job:
+            raise CorruptBuildID(
+                "ID mismatch for translation templates build '%s'" % (
+                    slave_build_id))
+
     def _getChroot(self):
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
         return ubuntu.currentseries.nominatedarchindep.getChroot()

=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2010-03-12 08:31:43 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2010-03-13 10:43:28 +0000
@@ -19,6 +19,7 @@
 from lp.buildmaster.interfaces.buildbase import BuildStatus
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
     IBuildFarmJobBehavior)
+from lp.buildmaster.interfaces.builder import CorruptBuildID
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
 from lp.testing import TestCaseWithFactory
 from lp.testing.fakemethod import FakeMethod
@@ -123,6 +124,10 @@
         self.assertEqual(
             'translation-templates', slave_status['test_build_type'])
         self.assertIn('branch_url', slave_status['test_build_args'])
+        # The slave receives the public http URL for the branch.
+        self.assertEqual(
+            behavior.buildfarmjob.branch.composePublicURL(),
+            slave_status['test_build_args']['branch_url'])
 
     def test_getChroot(self):
         # _getChroot produces the current chroot for the current Ubuntu
@@ -170,6 +175,38 @@
         self.assertEqual(1, builder.cleanSlave.call_count)
         self.assertEqual(0, behavior._uploadTarball.call_count)
 
+    def test_verifySlaveBuildID_success(self):
+        # TranslationTemplatesBuildJob.getName generates slave build ids
+        # that TranslationTemplatesBuildBehavior.verifySlaveBuildID
+        # accepts.
+        behavior = self._makeBehavior()
+        buildfarmjob = behavior.buildfarmjob
+        job = buildfarmjob.job
+
+        # The test is that this not raise CorruptBuildID (or anything
+        # else, for that matter).
+        behavior.verifySlaveBuildID(behavior.buildfarmjob.getName())
+
+    def test_verifySlaveBuildID_handles_dashes(self):
+        # TranslationTemplatesBuildBehavior.verifySlaveBuildID can deal
+        # with dashes in branch names.
+        behavior = self._makeBehavior()
+        buildfarmjob = behavior.buildfarmjob
+        job = buildfarmjob.job
+        buildfarmjob.branch.name = 'x-y-z--'
+
+        # The test is that this not raise CorruptBuildID (or anything
+        # else, for that matter).
+        behavior.verifySlaveBuildID(behavior.buildfarmjob.getName())
+
+    def test_verifySlaveBuildID_malformed(self):
+        behavior = self._makeBehavior()
+        self.assertRaises(CorruptBuildID, behavior.verifySlaveBuildID, 'huh?')
+
+    def test_verifySlaveBuildID_notfound(self):
+        behavior = self._makeBehavior()
+        self.assertRaises(CorruptBuildID, behavior.verifySlaveBuildID, '1-1')
+
 
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)