Code review comment for lp:~jtv/launchpad/bug-499405-translationtemplates-buildmanager

> 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 very much like to avoid the "nothing works until everything works" kind of situation. Especially since the generalisation work for the buildfarm isn't quite done yet: conflicts and API changes may seep in if I keep this branch unlanded.

> === 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 unittest import TestLoader
> +
> +from lp.testing import TestCase
> +
> +from canonical.buildd.generate_translation_templates import (
> + GenerateTranslationTemplates)
> +
> +from canonical.launchpad.ftests.script import run_script
> +from canonical.testing.layers import ZopelessDatabaseLayer
> +
> +
> +class MockGenerateTranslationTemplates(GenerateTranslationTemplates):
> + """A GenerateTranslationTemplates with mocked _checkout."""
> + checked_out_branch = None
> +
> + def _checkout(self, branch_url):
> + self.checked_out_branch = branch_url
> +
> +
>
> Why does checked_out_branch exist? Is it just to demonstrate the a branch
> would have been checked out? Maybe that needs a comment, and maybe
> checked_out_branch should be a boolean.

Right on all counts. I turned it into a boolean and documented it.

> +class TestGenerateTranslationTemplates(TestCase):
> + """Test slave-side generate-translation-templates script."""
> + def test_getBranch_url(self):
> + # If passed a branch URL, the template generation script will
> + # check out that branch into a directory called "source-tree."
> + branch_url = 'lp://~my/translation/branch'
> +
> + generator = MockGenerateTranslationTemplates(branch_url)
> + generator._getBranch()
> +
> + self.assertEqual(branch_url, generator.checked_out_branch)
> + self.assertTrue(generator.branch_dir.endswith('source-tree'))
> +
> + def test_getBranch_dir(self):
> + # If passed a branch directory, the template generation script
> + # works directly in that directory.
> + branch_dir = '/home/me/branch'
> +
> + generator = MockGenerateTranslationTemplates(branch_dir)
> + generator._getBranch()
> +
> + self.assertEqual(None, generator.checked_out_branch)
> + self.assertEqual(branch_dir, generator.branch_dir)
> +
>
> In this case, why would generator.checked_out_branch be none? Shouldn't it be
> set to the value of branch_dir? _getBranch does call _checkout, and you've
> mocked it in a way that it shouldn't be None.

Not in the case where the branch location is a local directory (as opposed to a URL). In that case the script cd's into the branch and does its work there.

The changes for the checked_out_branch field make this clearer, I think.

Jeroen