Merge lp:~jtv/launchpad/bug-499405-translationtemplates-buildmanager into lp:launchpad

= Bug 499405 =

This branch implements a build manager for generating translation templates from a branch. It provides no real functionality on its own; rather it's part of an ongoing project. Pre-imp discussions were done IRL at the Wellington build-farm generalisation sprint.

A build manager is the piece of code that runs on a build slave to coordinate its work. It iterates through a finite-state machine describing the job to be done. An important thing to bear in mind is that this is not "Launchpad code": it runs on a slave machine that has neither the Launchpad modules nor access to the database.

For this particular job, the finite-state machine consists of 7 states:
1. INIT: Initialize.
2. UNPACK: Create a chroot directory by unpacking a chroot tarball.
3. MOUNT: Set up a realistic system environment inside the chroot.
4. UPDATE: Get the latest versions of the packages installed inside the chroot.
5. INSTALL: Add a few job-specific packages to the chroot.
6. GENERATE: Check out the branch and produce templates based on it.
7. CLEANUP: Return the slave to its original, clean state.

Of these, step 6 is the "payload." It invokes a separate script, also found in this branch, that checks out a branch and will eventually generate templates inside it. The script is run under a non-privileged user inside the slave's chroot, providing maximum insulation from untrusted code inside the branches.

The implementations for most of the other steps are inherited from the existing BuildSlave base class.

Testing the actual shell commands is rather hard, which is why other build managers seem to go largely untested. I did find a way to unit-test at least the iteration of the state machine: mock up the BuildManager method that normally forks off an action of the state machine and hands it to twistd. Instead of running the action and triggering a state transition in twistd, the test just logs and verifies the command that would have been run in a sub-process.

In order to make this testable, I had to make just one small change in the base class. It made changes to the filesystem in the current user's home directory. Instead, I made it record the current user's home directory in its constructor; the other methods now get the location of the home directory from that same field. This allows the test to substitute a temporary directory of its own.

I also added a manual test to kick off a templates generation build through XMLRPC. I'm told this can't be run as part of the normal test suite, but it can be run manually to see if anything breaks spectacularly. It's a blatant ripoff of a similar test that abentley implemented for his own build-manager class. You "make run" in one shell then while that's active, run the test script in another with one argument: the SHA1 of a chroot tarball that's in your local Librarian.

Finally, you'll notice a helper method makeTemporaryDirectory in the Launchpad version of the TestCase class. This breaks naming convention with the existing useTempDir, but matches the convention of both a similar method in bzr test cases and the naming style in the standard tempfile module. I did update useTempDir to make use o...

Hi Jeroen-

  This branch looks good. I do have some questions though, and would like to have answers before I Approve this branch.

=== added file 'lib/canonical/buildd/generate_translation_templates.py'
--- lib/canonical/buildd/generate_translation_templates.py 1970-01-01 00:00:00 +0000
+++ lib/canonical/buildd/generate_translation_templates.py 2010-01-21 12:50:24 +0000
@@ -0,0 +1,57 @@
+#! /usr/bin/python2.5
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import os.path
+import sys
+from subprocess import call
+
+
+class GenerateTranslationTemplates:
+ """Script to generate translation templates from a branch."""
+
+ def __init__(self, branch_spec):
+ """Prepare to generate templates for a branch.
+
+ :param branch_spec: Either a branch URL or the path of a local
+ branch. URLs are recognized by the occurrence of ':'. In
+ the case of a URL, this will make up a path for the branch
+ and check out the branch to there.
+ """
+ self.home = os.environ['HOME']
+ self.branch_spec = branch_spec
+
+ def _getBranch(self):
+ """Set `self.branch_dir`, and check out branch if needed."""
+ if ':' in self.branch_spec:
+ # This is a branch URL. Check out the branch.
+ self.branch_dir = os.path.join(self.home, 'source-tree')
+ self._checkout(self.branch_spec)
+ else:
+ # This is a local filesystem path. Use the branch in-place.
+ self.branch_dir = self.branch_spec
+
+ def _checkout(self, branch_url):
+ """Check out a source branch to generate from.
+
+ The branch is checked out to the location specified by
+ `self.branch_dir`.
+ """
+ command = ['/usr/bin/bzr', 'checkout', branch_url, self.branch_dir]
+ return call(command, cwd=self.home)

Is there a reason you're not using bzrlib? I know that the build slaves have special security there, but if you have bzr there, you have bzrlib.

+
+ def generate(self):
+ """Do It. Generate templates."""
+ self._getBranch()
+ # XXX JeroenVermeulen 2010-01-19 bug=509557: Actual payload goes here.
+ return 0
+

Does this mean that this is basically a no-op right now? I'm okay with this code, but I'd rather you wait for it to actually do something than land it now.

+
+if __name__ == '__main__':
+ if len(sys.argv) != 2:
+ print "Usage: %s branch" % sys.argv[0]
+ print "Where 'branch' is a branch URL or directory."
+ sys.exit(1)
+ sys.exit(GenerateTranslationTemplates(sys.argv[1]).generate())

=== added file 'lib/canonical/buildd/tests/test_generate_translation_templates.py'
--- lib/canonical/buildd/tests/test_generate_translation_templates.py 1970-01-01 00:00:00 +0000
+++ lib/canonical/buildd/tests/test_generate_translation_templates.py 2010-01-21 12:50:24 +0000
@@ -0,0 +1,57 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+import os
+from unitte...

> This branch looks good. I do have some questions though, and would like to
> have answers before I Approve this branch.

Thanks for the review—glad I could get someone who was involved in the sprint!

> === added file 'lib/canonical/buildd/generate_translation_templates.py'
> --- lib/canonical/buildd/generate_translation_templates.py 1970-01-01
> 00:00:00 +0000
> +++ lib/canonical/buildd/generate_translation_templates.py 2010-01-21
> 12:50:24 +0000
> @@ -0,0 +1,57 @@
> +#! /usr/bin/python2.5
> +# Copyright 2010 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +__metaclass__ = type
> +
> +import os.path
> +import sys
> +from subprocess import call
> +
> +
> +class GenerateTranslationTemplates:
> + """Script to generate translation templates from a branch."""
> +
> + def __init__(self, branch_spec):
> + """Prepare to generate templates for a branch.
> +
> + :param branch_spec: Either a branch URL or the path of a local
> + branch. URLs are recognized by the occurrence of ':'. In
> + the case of a URL, this will make up a path for the branch
> + and check out the branch to there.
> + """
> + self.home = os.environ['HOME']
> + self.branch_spec = branch_spec
> +
> + def _getBranch(self):
> + """Set `self.branch_dir`, and check out branch if needed."""
> + if ':' in self.branch_spec:
> + # This is a branch URL. Check out the branch.
> + self.branch_dir = os.path.join(self.home, 'source-tree')
> + self._checkout(self.branch_spec)
> + else:
> + # This is a local filesystem path. Use the branch in-place.
> + self.branch_dir = self.branch_spec
> +
> + def _checkout(self, branch_url):
> + """Check out a source branch to generate from.
> +
> + The branch is checked out to the location specified by
> + `self.branch_dir`.
> + """
> + command = ['/usr/bin/bzr', 'checkout', branch_url, self.branch_dir]
> + return call(command, cwd=self.home)
>
> Is there a reason you're not using bzrlib? I know that the build slaves have
> special security there, but if you have bzr there, you have bzrlib.

Mainly laziness. But also, this being a corner of LP that's hard to test, I would like to minimize the risk of silly little mistakes that might not be reported back to me very effectively. Instead of writing my own python, I'm producing a command line here that I can just replicate in a shell when debugging.

By the way, I'm replacing the "checkout" with "export." All I need is the stored files; actual revision control is just useless baggage for this particular job.

> + def generate(self):
> + """Do It. Generate templates."""
> + self._getBranch()
> + # XXX JeroenVermeulen 2010-01-19 bug=509557: Actual payload goes
> here.
> + return 0
> +
>
> Does this mean that this is basically a no-op right now? I'm okay with this
> code, but I'd rather you wait for it to actually do something than land it
> now.

This is a pretty big project, so I'd...

>> + def _checkout(self, branch_url):
>> + """Check out a source branch to generate from.
>> +
>> + The branch is checked out to the location specified by
>> + `self.branch_dir`.
>> + """
>> + command = ['/usr/bin/bzr', 'checkout', branch_url, self.branch_dir]
>> + return call(command, cwd=self.home)
>>
>> Is there a reason you're not using bzrlib? I know that the build slaves have
>> special security there, but if you have bzr there, you have bzrlib.
>
>Mainly laziness. But also, this being a corner of LP that's hard to test, I would >like to minimize the risk of silly little mistakes that might not be reported back to >me very effectively. Instead of writing my own python, I'm producing a command line >here that I can just replicate in a shell when debugging.

So there are a few issues with calling the bzr runtime instead of using bzrlib. The first is that we do not want to use the system bzr. Launchpad packages a local copy of bzr for this very purpose. The second is that we aren't guaranteed we'll have the plugins we need on the system, but we make sure they're available to bzrlib. There are also performance advantages to using bzrlib specifically. I really think that bzrlib needs to be used in this situation.

Other than that, I'm happy with your other changes.

> So there are a few issues with calling the bzr runtime instead of using
> bzrlib. The first is that we do not want to use the system bzr. Launchpad
> packages a local copy of bzr for this very purpose. The second is that we
> aren't guaranteed we'll have the plugins we need on the system, but we make
> sure they're available to bzrlib. There are also performance advantages to
> using bzrlib specifically. I really think that bzrlib needs to be used in
> this situation.
>
> Other than that, I'm happy with your other changes.

This code runs on the slave, so Launchpad is not present. I think that also eliminates a lot of other complications there might otherwise be on the system.

I conferenced with abentley about this, so it's not just me saying "please use bzrlib" but also another code folk. If nothing else, it gives us a nice traceback, instead of a user friendly (and rather unhelpful) error message. It basically runs bzr as a subprocess, which could actually make things more complicated.

Alright then... I've replaced the code with an XXX (referring to a bug, of course) to say that in future the code needs to do the equivalent of "bzr export" using bzrlib.