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

[Jeroen T. Vermeulen]

Make pottery deal with quotes.

= Bug 580337 =

The pottery code, which runs on build-farm slaves in order to generate translation templates from branches, was failing in some cases. The reason was that it attempts to extract a project name from the configure/autoconf/make setup found in the branch, but fails to deal with quotes around these names.

Here I add a bit of quote-stripping logic. It's pretty naïve, since project names are expected to be simple identifiers. After discussion with Henning, I applied it to both function parameters and variables, and removed the old special-case trick for removing M4-style bracket quotes.

A lot of the diff is actually a cleanup of the unit tests. The existing unit test created a single sample configuration file as part of its setup, for use as implicit state in the tests. Each test then verified one expected result from querying the file. This is poorly maintainable: a lot of what is being tested remains implicit ("the weird bit in this line doesn't affect how we deal with that line"). Such tests are easily broken by future maintenance of the test, or you find you can test one thing or the other but not both because they both need something different to be in the first/last place in a list etc. So I broke it up into individual explicit tests without reference to hidden state.

To exercise the tests:
{{{
./bin/test -vv -m lp.translations -t pottery
}}}

To Q/A, roll out this patch to the dogfood build slaves and generate some templates from branches. This is to verify that the existing functionality is still there. Then, do the same with lp:zeitgeist to see if that one has started working.

No lint.

Jeroen

[Edwin Grubbs (edwin-grubbs)]

Hi Jeroen,

Looks good. Just one comment below.

-Edwin

>=== modified file 'lib/canonical/buildd/pottery/intltool.py'
>--- lib/canonical/buildd/pottery/intltool.py 2010-05-07 09:54:50 +0000
>+++ lib/canonical/buildd/pottery/intltool.py 2010-06-30 14:44:01 +0000
>@@ -86,10 +86,9 @@
> if params is None or len(params) < 2:
> return None
> if len(params) < 4:
>- value = params[0]
>+ return params[0]
> else:
>- value = params[3]
>- return value.strip("[]")
>+ return params[3]
>

The docstring for _get_AC_PACKAGE_NAME still says:
"They may be enclosed in []."

review: Approve

=== modified file 'lib/canonical/buildd/debian/changelog'
--- lib/canonical/buildd/debian/changelog	2010-06-12 03:48:34 +0000
+++ lib/canonical/buildd/debian/changelog	2010-06-30 15:48:33 +0000
@@ -1,3 +1,9 @@
+launchpad-buildd (64) hardy-cat; urgency=low
+
+  * Pottery now strips quotes from variables.
+
+ -- Jeroen Vermeulen <[e-mail]>  Wed, 30 Jun 2010 12:50:59 +0200
+
 launchpad-buildd (63) hardy-cat; urgency=low
 
   * Drop apply-ogre-model, since override-sources-list replaced it three years

=== modified file 'lib/canonical/buildd/pottery/intltool.py'
--- lib/canonical/buildd/pottery/intltool.py	2010-05-07 09:54:50 +0000
+++ lib/canonical/buildd/pottery/intltool.py	2010-06-30 15:48:33 +0000
@@ -80,16 +80,15 @@
 
     The value of AC_PACKAGE_NAME is either the first or the fourth
     parameter of the AC_INIT call if it is called with at least two
-    parameters. They may be enclosed in [].
+    parameters.
     """
     params = config_file.getFunctionParams("AC_INIT")
     if params is None or len(params) < 2:
         return None
     if len(params) < 4:
-        value = params[0]
+        return params[0]
     else:
-        value = params[3]
-    return value.strip("[]")
+        return params[3]
 
 
 def _try_substitution(config_files, varname, substitution):
@@ -151,14 +150,14 @@
             if value == "AC_PACKAGE_NAME":
                 value = _get_AC_PACKAGE_NAME(config_files[-1])
             else:
-                # Check if the value need a substitution.
+                # Check if the value needs a substitution.
                 substitution = Substitution.get(value)
                 if substitution is not None:
                     # Try to substitute with value.
                     value = _try_substitution(
                         config_files, varname, substitution)
                     if value is None:
-                        # No substitution found, the setup is broken.
+                        # No substitution found; the setup is broken.
                         break
         if value is not None and not keep_trying:
             # A value has been found.
@@ -213,6 +212,28 @@
             conf_file = file_or_name
         self.content = conf_file.read()
 
+    def _stripQuotes(self, identifier):
+        """Strip surrounding quotes from `identifier`, if present.
+
+        :param identifier: a string, possibly surrounded by matching
+            'single,' "double," or [bracket] quotes.
+        :return: `identifier` but with the outer pair of matching quotes
+            removed, if they were there.
+        """
+        if len(identifier) < 2:
+            return identifier
+
+        quote_pairs = [
+            ('"', '"'),
+            ("'", "'"),
+            ("[", "]"),
+            ]
+        for (left, right) in quote_pairs:
+            if identifier.startswith(left) and identifier.endswith(right):
+                return identifier[1:-1]
+
+        return identifier
+
     def getVariable(self, name):
         """Search the file for a variable definition with this name."""
         pattern = re.compile(
@@ -220,7 +241,7 @@
         result = pattern.search(self.content)
         if result is None:
             return None
-        return result.group(1)
+        return self._stripQuotes(result.group(1))
 
     def getFunctionParams(self, name):
         """Search file for a function call with this name, return parameters.
@@ -229,7 +250,11 @@
         result = pattern.search(self.content)
         if result is None:
             return None
-        return [param.strip() for param in result.group(1).split(',')]
+        else:
+            return [
+                self._stripQuotes(param.strip())
+                for param in result.group(1).split(',')
+                ]
 
 
 class Substitution(object):

=== modified file 'lib/canonical/buildd/translationtemplates.py'
--- lib/canonical/buildd/translationtemplates.py	2010-04-01 09:48:13 +0000
+++ lib/canonical/buildd/translationtemplates.py	2010-06-30 15:48:33 +0000
@@ -7,6 +7,7 @@
 
 from canonical.buildd.debian import DebianBuildManager, DebianBuildState
 
+
 class TranslationTemplatesBuildState(DebianBuildState):
     INSTALL = "INSTALL"
     GENERATE = "GENERATE"
@@ -65,7 +66,8 @@
         # user. Should be safe to assume the home dirs are named identically.
         assert self.home.startswith('/'), "home directory must be absolute."
 
-        path = os.path.join(self._chroot_path, self.home[1:], self._resultname)
+        path = os.path.join(
+            self._chroot_path, self.home[1:], self._resultname)
         if os.access(path, os.F_OK):
             self._slave.addWaitingFile(path)
 

=== modified file 'lib/lp/translations/tests/test_pottery_detect_intltool.py'
--- lib/lp/translations/tests/test_pottery_detect_intltool.py	2010-05-06 15:48:35 +0000
+++ lib/lp/translations/tests/test_pottery_detect_intltool.py	2010-06-30 15:48:33 +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).
 
 from __future__ import with_statement
@@ -17,6 +17,7 @@
     ConfigFile, check_potfiles_in, find_intltool_dirs, find_potfiles_in,
     generate_pot, generate_pots, get_translation_domain)
 from lp.testing import TestCase
+from lp.testing.fakemethod import FakeMethod
 
 
 class SetupTestPackageMixin(object):
@@ -78,7 +79,7 @@
     def test_check_potfiles_in_content_ok(self):
         # Ideally all files listed in POTFILES.in exist in the source package.
         self.prepare_package("intltool_single_ok")
-        self.assertTrue(check_potfiles_in("./po")) 
+        self.assertTrue(check_potfiles_in("./po"))
 
     def test_check_potfiles_in_content_ok_file_added(self):
         # If a file is not listed in POTFILES.in, the file is still good for
@@ -87,7 +88,7 @@
         added_file = file("./src/sourcefile_new.c", "w")
         added_file.write("/* Test file. */")
         added_file.close()
-        self.assertTrue(check_potfiles_in("./po")) 
+        self.assertTrue(check_potfiles_in("./po"))
 
     def test_check_potfiles_in_content_not_ok_file_removed(self):
         # If a file is missing that is listed in POTFILES.in, the file
@@ -95,13 +96,13 @@
         # our purposes.
         self.prepare_package("intltool_single_ok")
         os.remove("./src/sourcefile1.c")
-        self.assertFalse(check_potfiles_in("./po")) 
+        self.assertFalse(check_potfiles_in("./po"))
 
     def test_check_potfiles_in_wrong_directory(self):
         # Passing in the wrong directory will cause the check to fail
         # gracefully and return False.
         self.prepare_package("intltool_single_ok")
-        self.assertFalse(check_potfiles_in("./foo")) 
+        self.assertFalse(check_potfiles_in("./foo"))
 
     def test_find_intltool_dirs(self):
         # Complete run: find all directories with intltool structure.
@@ -378,62 +379,167 @@
 
 class TestConfigFile(TestCase):
 
-    def setUp(self):
-        super(TestConfigFile, self).setUp()
-        self.configfile = ConfigFile(StringIO(dedent("""\
-            # Demo config file
-            CCC
-            AAA=
-            FUNC_1(param1)
-            BBB = 
-            FUNC_2(param1, param2,param3 )
-            CCC = ccc # comment
-            ML_FUNC_1(param1,
-                param2, param3)
-            DDD=dd.d
-            ML_FUNC_2(
-            param1,
-            param2)
-            EEE 
-            = eee
-            """)))
+    def _makeConfigFile(self, text):
+        """Create a `ConfigFile` containing `text`."""
+        return ConfigFile(StringIO(dedent(text)))
+
+    def test_getVariable_smoke(self):
+        configfile = self._makeConfigFile("""
+            A = 1
+            B = 2
+            C = 3
+            """)
+        self.assertEqual('1', configfile.getVariable('A'))
+        self.assertEqual('2', configfile.getVariable('B'))
+        self.assertEqual('3', configfile.getVariable('C'))
 
     def test_getVariable_exists(self):
-        self.assertEqual('dd.d', self.configfile.getVariable('DDD'))
+        configfile = self._makeConfigFile("DDD=dd.d")
+        self.assertEqual('dd.d', configfile.getVariable('DDD'))
+
+    def test_getVariable_ignores_mere_mention(self):
+        configfile = self._makeConfigFile("""
+            CCC
+            CCC = ccc # (this is the real definition)
+            CCC
+            """)
+        self.assertEqual('ccc', configfile.getVariable('CCC'))
+
+    def test_getVariable_ignores_irrelevancies(self):
+        configfile = self._makeConfigFile("""
+            A = a
+            ===
+            blah
+            FOO(n, m)
+            a = case-insensitive
+
+            Z = z
+            """)
+        self.assertEqual('a', configfile.getVariable('A'))
+        self.assertEqual('z', configfile.getVariable('Z'))
 
     def test_getVariable_exists_spaces_comment(self):
-        self.assertEqual('ccc', self.configfile.getVariable('CCC'))
+        configfile = self._makeConfigFile("CCC = ccc # comment")
+        self.assertEqual('ccc', configfile.getVariable('CCC'))
 
     def test_getVariable_empty(self):
-        self.assertEqual('', self.configfile.getVariable('AAA'))
+        configfile = self._makeConfigFile("AAA=")
+        self.assertEqual('', configfile.getVariable('AAA'))
 
     def test_getVariable_empty_spaces(self):
-        self.assertEqual('', self.configfile.getVariable('BBB'))
+        configfile = self._makeConfigFile("BBB = ")
+        self.assertEqual('', configfile.getVariable('BBB'))
 
     def test_getVariable_nonexistent(self):
-        self.assertIs(None, self.configfile.getVariable('FFF'))
+        configfile = self._makeConfigFile("X = y")
+        self.assertIs(None, configfile.getVariable('FFF'))
 
     def test_getVariable_broken(self):
-        self.assertIs(None, self.configfile.getVariable('EEE'))
+        configfile = self._makeConfigFile("EEE \n= eee")
+        self.assertIs(None, configfile.getVariable('EEE'))
+
+    def test_getVariable_strips_quotes(self):
+        # Quotes get stripped off variables.
+        configfile = self._makeConfigFile("QQQ = 'qqq'")
+        self.assertEqual('qqq', configfile.getVariable('QQQ'))
+
+        # This is done by invoking _stripQuotes (tested separately).
+        configfile._stripQuotes = FakeMethod(result='foo')
+        self.assertEqual('foo', configfile.getVariable('QQQ'))
+        self.assertNotEqual(0, configfile._stripQuotes.call_count)
 
     def test_getFunctionParams_single(self):
-        self.assertEqual(
-            ['param1'], self.configfile.getFunctionParams('FUNC_1'))
+        configfile = self._makeConfigFile("FUNC_1(param1)")
+        self.assertEqual(['param1'], configfile.getFunctionParams('FUNC_1'))
 
     def test_getFunctionParams_multiple(self):
+        configfile = self._makeConfigFile("FUNC_2(param1, param2, param3 )")
         self.assertEqual(
             ['param1', 'param2', 'param3'],
-            self.configfile.getFunctionParams('FUNC_2'))
+            configfile.getFunctionParams('FUNC_2'))
 
     def test_getFunctionParams_multiline_indented(self):
+        configfile = self._makeConfigFile("""
+            ML_FUNC_1(param1,
+                param2, param3)
+            """)
         self.assertEqual(
             ['param1', 'param2', 'param3'],
-            self.configfile.getFunctionParams('ML_FUNC_1'))
+            configfile.getFunctionParams('ML_FUNC_1'))
 
     def test_getFunctionParams_multiline_not_indented(self):
+        configfile = self._makeConfigFile("""
+            ML_FUNC_2(
+            param1,
+            param2)
+            """)
         self.assertEqual(
-            ['param1', 'param2'],
-            self.configfile.getFunctionParams('ML_FUNC_2'))
+            ['param1', 'param2'], configfile.getFunctionParams('ML_FUNC_2'))
+    
+    def test_getFunctionParams_strips_quotes(self):
+        # Quotes get stripped off function parameters.
+        configfile = self._makeConfigFile('FUNC("param")')
+        self.assertEqual(['param'], configfile.getFunctionParams('FUNC'))
+
+        # This is done by invoking _stripQuotes (tested separately).
+        configfile._stripQuotes = FakeMethod(result='arg')
+        self.assertEqual(['arg'], configfile.getFunctionParams('FUNC'))
+        self.assertNotEqual(0, configfile._stripQuotes.call_count)
+
+    def test_stripQuotes_unquoted(self):
+        # _stripQuotes leaves unquoted identifiers intact.
+        configfile = self._makeConfigFile('')
+        self.assertEqual('hello', configfile._stripQuotes('hello'))
+
+    def test_stripQuotes_empty(self):
+        configfile = self._makeConfigFile('')
+        self.assertEqual('', configfile._stripQuotes(''))
+
+    def test_stripQuotes_single_quotes(self):
+        # Single quotes are stripped.
+        configfile = self._makeConfigFile('')
+        self.assertEqual('x', configfile._stripQuotes("'x'"))
+
+    def test_stripQuotes_double_quotes(self):
+        # Double quotes are stripped.
+        configfile = self._makeConfigFile('')
+        self.assertEqual('y', configfile._stripQuotes('"y"'))
+
+    def test_stripQuotes_bracket_quotes(self):
+        # Brackets are stripped.
+        configfile = self._makeConfigFile('')
+        self.assertEqual('z', configfile._stripQuotes('[z]'))
+
+    def test_stripQuotes_opening_brackets(self):
+        # An opening bracket must be matched by a closing one.
+        configfile = self._makeConfigFile('')
+        self.assertEqual('[x[', configfile._stripQuotes('[x['))
+
+    def test_stripQuotes_closing_brackets(self):
+        # A closing bracket is not accepted as an opening quote.
+        configfile = self._makeConfigFile('')
+        self.assertEqual(']x]', configfile._stripQuotes(']x]'))
+
+    def test_stripQuotes_multiple(self):
+        # Only a single layer of quotes is stripped.
+        configfile = self._makeConfigFile('')
+        self.assertEqual('"n"', configfile._stripQuotes("'\"n\"'"))
+
+    def test_stripQuotes_single_quote(self):
+        # A string consisting of just one quote is not stripped.
+        configfile = self._makeConfigFile('')
+        self.assertEqual("'", configfile._stripQuotes("'"))
+
+    def test_stripQuotes_mismatched(self):
+        # Mismatched quotes are not stripped.
+        configfile = self._makeConfigFile('')
+        self.assertEqual("'foo\"", configfile._stripQuotes("'foo\""))
+
+    def test_stripQuotes_unilateral(self):
+        # A quote that's only on one end doesn't get stripped.
+        configfile = self._makeConfigFile('')
+        self.assertEqual('"foo', configfile._stripQuotes('"foo'))
+
 
 
 def test_suite():