Merge lp:~henninge/launchpad/bug-568355-package-name into lp:launchpad

[Henning Eggers]

Made pottery translation domain detection smarter.

= Bug 568355 =

The function "get_translation_domain" scans a package directory to determine the translation domain in an intltool setup. It turned out that these setups can have more variations and complications than orginially expected. This fix addresses the now known cases.

== Proposed fix ==

The files can be ordered into a substitution hierarchy where the bottom file may include definitions from all files above it. The old implemntation kind of did this the wrong way round. The new order of processing is:

- go through the list of config files top to bottom
- try to read the variable from that config file, store its value
- if the value contains a substitution, try to fulfill that from this file or all previous files.
  - if the substitution cannot be satisfied, fail
- if the variable may also be found in the next file, keep searching
  - otherwise, the search is completed.

A new feature is to search AC_INIT parameter list for the domain.

== Tests ==

The test was greatly improved to remove the necessity to create an extra tarball for each test. Also, all tarballs were moved to their own directories.

bin/text -vvt intltool

== QA ==

QA would have to done on dogfood. The package "eog" should now be processed correctly.

[Abel Deuring (adeuring)]

(11:11:38) adeuring: henninge: lines 51-58 of the diff change the order of locations. The doc string of the method says "This function goes through the ordered list of these possible locations, the order having been copied from intltool-update". My question: Was the old order plain wrong or did the intltools chnage. IOW: Is the new ordering always correct?
(11:12:01) adeuring: or can it be wrong for some versions of intltools?
(11:12:17) henninge: adeuring: the ordering is correct, the comment is not valid any more.
(11:12:56) adeuring: henninge: so, the old ordering was simply based on a misinterpretation of what intltools are doing?
(11:13:19) henninge: adeuring: no, the original order was taken from intltool-update
(11:14:06) henninge: adeuring: basically, what is says "use GETTEXT_PACKGE from Makefile.in.in or else from configure.ac or else from configure.in"
(11:14:15) adeuring: henninge: ok... but the substitution order changed, right?
(11:14:32) henninge: adeuring: this is now reflected in the "keep_trying" value.
(11:14:58) adeuring: henninge: ah, I see. So, can you update the comment? otherwise, r=me
(11:15:14) henninge: adeuring: yes, thanks

review: Approve (code)

[Abel Deuring (adeuring)]

forgot a nitpick:

+ return [
+ param.strip()
+ for param in result.group(1).strip().split(',')]

I think it is sufficient to use result.group(1).split(','), since you call strip() for the result of split()

[Henning Eggers (henninge)]

Thank you for the review. Here is the incremental diff.

=== modified file 'lib/canonical/buildd/pottery/intltool.py'
--- lib/canonical/buildd/pottery/intltool.py	2010-03-10 20:19:39 +0000
+++ lib/canonical/buildd/pottery/intltool.py	2010-05-07 09:57:31 +0000
@@ -75,13 +75,38 @@
     return sorted(filter(check_potfiles_in, find_potfiles_in()))
 
 
-def _try_substitution(path, substitution):
-    """Try to find a substitution in the given config file.
+def _get_AC_PACKAGE_NAME(config_file):
+    """Get the value of AC_PACKAGE_NAME from function parameters.
+
+    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 [].
+    """
+    params = config_file.getFunctionParams("AC_INIT")
+    if params is None or len(params) < 2:
+        return None
+    if len(params) < 4:
+        value = params[0]
+    else:
+        value = params[3]
+    return value.strip("[]")
+
+
+def _try_substitution(config_files, varname, substitution):
+    """Try to find a substitution in the config files.
 
     :returns: The completed substitution or None if none was found.
     """
-    subst_value = ConfigFile(path).getVariable(substitution.name)
-    if subst_value is None:
+    subst_value = None
+    if varname == substitution.name:
+        # Do not look for the same name in the current file.
+        config_files = config_files[:-1]
+    for config_file in reversed(config_files):
+        subst_value = config_file.getVariable(substitution.name)
+        if subst_value is not None:
+            # Substitution found.
+            break
+    else:
         # No substitution found.
         return None
     return substitution.replace(subst_value)
@@ -94,49 +119,50 @@
     translation domain the build environment provides. The domain is usually
     defined in the GETTEXT_PACKAGE variable in one of the build files. Another
     variant is DOMAIN in the Makevars file. This function goes through the
-    ordered list of these possible locations, the order having been copied
-    from intltool-update, and tries to find a valid value.
+    ordered list of these possible locations, top to bottom, and tries to
+    find a valid value. Since the same variable name may be defined in
+    multiple files (usually configure.ac and Makefile.in.in), it needs to
+    keep trying with the next file, until it finds the most specific
+    definition.
 
     If the found value contains a substitution, either autoconf style (@...@)
-    or make style ($(...)), the search is continued in the same file and down
-    the list of files, now searching for the substitution. Multiple
+    or make style ($(...)), the search is continued in the same file and back
+    up the list of files, now searching for the substitution. Multiple
     substitutions or multi-level substitutions are not supported.
     """
     locations = [
-        ('Makefile.in.in', 'GETTEXT_PACKAGE'),
-        ('../configure.ac', 'GETTEXT_PACKAGE'),
-        ('../configure.in', 'GETTEXT_PACKAGE'),
-        ('Makevars', 'DOMAIN'),
+        ('../configure.ac', 'GETTEXT_PACKAGE', True),
+        ('../configure.in', 'GETTEXT_PACKAGE', True),
+        ('Makefile.in.in', 'GETTEXT_PACKAGE', False),
+        ('Makevars', 'DOMAIN', False),
     ]
     value = None
     substitution = None
-    for filename, varname in locations:
+    config_files = []
+    for filename, varname, keep_trying in locations:
         path = os.path.join(dirname, filename)
         if not os.access(path, os.R_OK):
             # Skip non-existent files.
             continue
-        if substitution is None:
-            value = ConfigFile(path).getVariable(varname)
-            if value is not None:
+        config_files.append(ConfigFile(path))
+        new_value = config_files[-1].getVariable(varname)
+        if new_value is not None:
+            value = new_value
+            if value == "AC_PACKAGE_NAME":
+                value = _get_AC_PACKAGE_NAME(config_files[-1])
+            else:
                 # Check if the value need a substitution.
                 substitution = Substitution.get(value)
                 if substitution is not None:
-                    # Try to substitute with value from current file but
-                    # avoid recursion.
-                    if substitution.name != varname:
-                        value = _try_substitution(path, substitution)
-                    else:
-                        # The value has not been found yet but is now stored
-                        # in the Substitution instance.
-                        value = None
-        else:
-            value = _try_substitution(path, substitution)
-        if value 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.
+                        break
+        if value is not None and not keep_trying:
             # A value has been found.
             break
-    if substitution is not None and not substitution.replaced:
-        # Substitution failed.
-        return None
     return value
 
 
@@ -185,17 +211,25 @@
             conf_file = file(file_or_name)
         else:
             conf_file = file_or_name
-        self.content_lines = conf_file.readlines()
+        self.content = conf_file.read()
 
     def getVariable(self, name):
         """Search the file for a variable definition with this name."""
-        pattern = re.compile("^%s[ \t]*=[ \t]*([^\s]*)" % re.escape(name))
-        variable = None
-        for line in self.content_lines:
-            result = pattern.match(line)
-            if result is not None:
-                variable = result.group(1)
-        return variable
+        pattern = re.compile(
+            "^%s[ \t]*=[ \t]*([^\s]*)" % re.escape(name), re.M)
+        result = pattern.search(self.content)
+        if result is None:
+            return None
+        return result.group(1)
+
+    def getFunctionParams(self, name):
+        """Search file for a function call with this name, return parameters.
+        """
+        pattern = re.compile("^%s\(([^)]*)\)" % re.escape(name), re.M)
+        result = pattern.search(self.content)
+        if result is None:
+            return None
+        return [param.strip() for param in result.group(1).split(',')]
 
 
 class Substitution(object):

=== added directory 'lib/lp/translations/tests/pottery_test_data'
=== renamed file 'lib/lp/translations/tests/intltool_POTFILES_in_1.tar.bz2' => 'lib/lp/translations/tests/pottery_test_data/intltool_POTFILES_in_1.tar.bz2'
=== renamed file 'lib/lp/translations/tests/intltool_POTFILES_in_2.tar.bz2' => 'lib/lp/translations/tests/pottery_test_data/intltool_POTFILES_in_2.tar.bz2'
=== added file 'lib/lp/translations/tests/pottery_test_data/intltool_domain_base.tar.bz2'
Binary files lib/lp/translations/tests/pottery_test_data/intltool_domain_base.tar.bz2	1970-01-01 00:00:00 +0000 and lib/lp/translations/tests/pottery_test_data/intltool_domain_base.tar.bz2	2010-05-07 09:57:31 +0000 differ
=== renamed file 'lib/lp/translations/tests/intltool_domain_configure_ac.tar.bz2' => 'lib/lp/translations/tests/pottery_test_data/intltool_domain_configure_ac.tar.bz2'
=== renamed file 'lib/lp/translations/tests/intltool_domain_configure_in.tar.bz2' => 'lib/lp/translations/tests/pottery_test_data/intltool_domain_configure_in.tar.bz2'
=== renamed file 'lib/lp/translations/tests/intltool_domain_configure_in_substitute_version.tar.bz2' => 'lib/lp/translations/tests/pottery_test_data/intltool_domain_configure_in_substitute_version.tar.bz2'
=== renamed file 'lib/lp/translations/tests/intltool_domain_makefile_in_in.tar.bz2' => 'lib/lp/translations/tests/pottery_test_data/intltool_domain_makefile_in_in.tar.bz2'
=== renamed file 'lib/lp/translations/tests/intltool_domain_makefile_in_in_substitute.tar.bz2' => 'lib/lp/translations/tests/pottery_test_data/intltool_domain_makefile_in_in_substitute.tar.bz2'
=== renamed file 'lib/lp/translations/tests/intltool_domain_makefile_in_in_substitute_broken.tar.bz2' => 'lib/lp/translations/tests/pottery_test_data/intltool_domain_makefile_in_in_substitute_broken.tar.bz2'
=== renamed file 'lib/lp/translations/tests/intltool_domain_makefile_in_in_substitute_same_file.tar.bz2' => 'lib/lp/translations/tests/pottery_test_data/intltool_domain_makefile_in_in_substitute_same_file.tar.bz2'
=== renamed file 'lib/lp/translations/tests/intltool_domain_makefile_in_in_substitute_same_name.tar.bz2' => 'lib/lp/translations/tests/pottery_test_data/intltool_domain_makefile_in_in_substitute_same_name.tar.bz2'
=== renamed file 'lib/lp/translations/tests/intltool_domain_makevars.tar.bz2' => 'lib/lp/translations/tests/pottery_test_data/intltool_domain_makevars.tar.bz2'
=== renamed file 'lib/lp/translations/tests/intltool_full_ok.tar.bz2' => 'lib/lp/translations/tests/pottery_test_data/intltool_full_ok.tar.bz2'
=== renamed file 'lib/lp/translations/tests/intltool_single_ok.tar.bz2' => 'lib/lp/translations/tests/pottery_test_data/intltool_single_ok.tar.bz2'
=== modified file 'lib/lp/translations/tests/test_pottery_detect_intltool.py'
--- lib/lp/translations/tests/test_pottery_detect_intltool.py	2010-03-10 20:19:39 +0000
+++ lib/lp/translations/tests/test_pottery_detect_intltool.py	2010-05-07 09:57:31 +0000
@@ -1,6 +1,8 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from __future__ import with_statement
+
 import os
 import tarfile
 import unittest
@@ -19,21 +21,44 @@
 
 class SetupTestPackageMixin(object):
 
-    def prepare_package(self, packagename):
+    test_data_dir = "pottery_test_data"
+
+    def prepare_package(self, packagename, buildfiles=None):
         """Unpack the specified package in a temporary directory.
 
         Change into the package's directory.
+
+        :param packagename: The name of the package to prepare.
+        :param buildfiles: A dictionary of path:content describing files to
+            add to the package.
         """
         # First build the path for the package.
         packagepath = os.path.join(
-            os.getcwd(), os.path.dirname(__file__), packagename + ".tar.bz2")
+            os.getcwd(), os.path.dirname(__file__),
+            self.test_data_dir, packagename + ".tar.bz2")
         # Then change into the temporary directory and unpack it.
         self.useTempDir()
-        tar = tarfile.open(packagepath, "r:bz2")
+        tar = tarfile.open(packagepath, "r|bz2")
         tar.extractall()
         tar.close()
         os.chdir(packagename)
 
+        if buildfiles is None:
+            return
+
+        # Add files as requested.
+        for path, content in buildfiles.items():
+            directory = os.path.dirname(path)
+            if directory != '':
+                try:
+                    os.makedirs(directory)
+                except OSError, e:
+                    # Doesn't matter if it already exists.
+                    if e.errno != 17:
+                        raise
+            with open(path, 'w') as the_file:
+                the_file.write(content)
+
 
 class TestDetectIntltool(TestCase, SetupTestPackageMixin):
 
@@ -91,6 +116,9 @@
         self.assertEqual(
             ["./po-module2"], find_intltool_dirs())
 
+
+class TestIntltoolDomain(TestCase, SetupTestPackageMixin):
+
     def test_get_translation_domain_makevars(self):
         # Find a translation domain in Makevars.
         self.prepare_package("intltool_domain_makevars")
@@ -98,6 +126,33 @@
             "translationdomain",
             get_translation_domain("po"))
 
+    def test_get_translation_domain_makevars_subst_1(self):
+        # Find a translation domain in Makevars, substituted from
+        # Makefile.in.in.
+        self.prepare_package(
+            "intltool_domain_base",
+            {
+                "po/Makefile.in.in": "PACKAGE=packagename-in-in\n",
+                "po/Makevars": "DOMAIN = $(PACKAGE)\n",
+            })
+        self.assertEqual(
+            "packagename-in-in",
+            get_translation_domain("po"))
+
+    def test_get_translation_domain_makevars_subst_2(self):
+        # Find a translation domain in Makevars, substituted from
+        # configure.ac.
+        self.prepare_package(
+            "intltool_domain_base",
+            {
+                "configure.ac": "PACKAGE=packagename-ac\n",
+                "po/Makefile.in.in": "# No domain here.\n",
+                "po/Makevars": "DOMAIN = $(PACKAGE)\n",
+            })
+        self.assertEqual(
+            "packagename-ac",
+            get_translation_domain("po"))
+
     def test_get_translation_domain_makefile_in_in(self):
         # Find a translation domain in Makefile.in.in.
         self.prepare_package("intltool_domain_makefile_in_in")
@@ -112,6 +167,64 @@
             "packagename-ac",
             get_translation_domain("po"))
 
+    def prepare_ac_init(self, parameters):
+        # Prepare test for various permutations of AC_INIT parameters
+        configure_ac_content = dedent("""
+            AC_INIT(%s)
+            GETTEXT_PACKAGE=AC_PACKAGE_NAME
+            """) % parameters
+        self.prepare_package(
+            "intltool_domain_base",
+            {
+                "configure.ac": configure_ac_content,
+            })
+
+    def test_get_translation_domain_configure_ac_init(self):
+        # Find a translation domain in configure.ac in AC_INIT.
+        self.prepare_ac_init("packagename-ac-init, 1.0, http://bug.org")
+        self.assertEqual(
+            "packagename-ac-init",
+            get_translation_domain("po"))
+
+    def test_get_translation_domain_configure_ac_init_single_param(self):
+        # Find a translation domain in configure.ac in AC_INIT.
+        self.prepare_ac_init("[Just 1 param]")
+        self.assertIs(None, get_translation_domain("po"))
+
+    def test_get_translation_domain_configure_ac_init_brackets(self):
+        # Find a translation domain in configure.ac in AC_INIT with brackets.
+        self.prepare_ac_init("[packagename-ac-init], 1.0, http://bug.org")
+        self.assertEqual(
+            "packagename-ac-init",
+            get_translation_domain("po"))
+
+    def test_get_translation_domain_configure_ac_init_tarname(self):
+        # Find a translation domain in configure.ac in AC_INIT tar name
+        # parameter.
+        self.prepare_ac_init(
+            "[Package name], 1.0, http://bug.org, [package-tarname]")
+        self.assertEqual(
+            "package-tarname",
+            get_translation_domain("po"))
+
+    def test_get_translation_domain_configure_ac_init_multiline(self):
+        # Find a translation domain in configure.ac in AC_INIT when it
+        # spans multiple lines.
+        self.prepare_ac_init(
+            "[packagename-ac-init],\n    1.0,\n    http://bug.org")
+        self.assertEqual(
+            "packagename-ac-init",
+            get_translation_domain("po"))
+
+    def test_get_translation_domain_configure_ac_init_multiline_tarname(self):
+        # Find a translation domain in configure.ac in AC_INIT tar name
+        # parameter that is on a different line.
+        self.prepare_ac_init(
+            "[Package name], 1.0,\n    http://bug.org, [package-tarname]")
+        self.assertEqual(
+            "package-tarname",
+            get_translation_domain("po"))
+
     def test_get_translation_domain_configure_in(self):
         # Find a translation domain in configure.in.
         self.prepare_package("intltool_domain_configure_in")
@@ -271,22 +384,57 @@
             # 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 test_getVariable_exists(self):
+        self.assertEqual('dd.d', self.configfile.getVariable('DDD'))
+
+    def test_getVariable_exists_spaces_comment(self):
         self.assertEqual('ccc', self.configfile.getVariable('CCC'))
-        self.assertEqual('dd.d', self.configfile.getVariable('DDD'))
 
     def test_getVariable_empty(self):
         self.assertEqual('', self.configfile.getVariable('AAA'))
+
+    def test_getVariable_empty_spaces(self):
         self.assertEqual('', self.configfile.getVariable('BBB'))
 
     def test_getVariable_nonexistent(self):
         self.assertIs(None, self.configfile.getVariable('FFF'))
 
+    def test_getVariable_broken(self):
+        self.assertIs(None, self.configfile.getVariable('EEE'))
+
+    def test_getFunctionParams_single(self):
+        self.assertEqual(
+            ['param1'], self.configfile.getFunctionParams('FUNC_1'))
+
+    def test_getFunctionParams_multiple(self):
+        self.assertEqual(
+            ['param1', 'param2', 'param3'],
+            self.configfile.getFunctionParams('FUNC_2'))
+
+    def test_getFunctionParams_multiline_indented(self):
+        self.assertEqual(
+            ['param1', 'param2', 'param3'],
+            self.configfile.getFunctionParams('ML_FUNC_1'))
+
+    def test_getFunctionParams_multiline_not_indented(self):
+        self.assertEqual(
+            ['param1', 'param2'],
+            self.configfile.getFunctionParams('ML_FUNC_2'))
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)