Merge lp:~jtv/launchpad/validate-translations-file into lp:launchpad

[Jeroen T. Vermeulen]

Validator script for translations files. Also, a leading newline in an XPI manifest is now recognized as an error.

[Jeroen T. Vermeulen (jtv)]

= Bug 503382 =

For Firefox in particular, in order to detect problems with broken translation files early and stay tightly coupled to upstream, the Ubuntu folks need to test translation files for syntax errors and such that would prevent them from importing into Launchpad.

In the case of Firefox (which uses XPI archives for translation, not gettext), a complication is that the upstream files are available as raw files that, on build, would go into an XPI archive. The main type of file to check is DTD files. We need to check these without building full XPI files, but the organization of files within XPI archives can be different from the directory hierarchies in the revision-controlled source tree.

This branch adds a script that parses translations files in a variety of formats. Only DTD and manifest files were requested, but adding a few formats was easy enough and may come in handy.

Note that this is not a full LaunchpadScript, although I did borrow a few snippets of setup code from there. It does not need locking, activity monitoring, etc. but standard options like -v can be useful.

One of the tests re-uses the XPI helpers that currently live in lp.translations.utilities.tests. Maybe those should be moved, though I'm not sure where.

Test with:
{{{
./bin/test -vv -t validate_translations_file
}}}

No lint. To Q/A, run the new script against a variety of files in these formats.

Jeroen

[Brad Crittenden (bac)]

Hi Jeroen,

Thanks for this branch and new tool. It looks good except for a few minor things. r=bac, merge-conditional

> === added file 'lib/lp/translations/scripts/tests/test_validate_translations_file.py'
> --- lib/lp/translations/scripts/tests/test_validate_translations_file.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/translations/scripts/tests/test_validate_translations_file.py 2010-01-05 17:14:16 +0000
> @@ -0,0 +1,133 @@
> +#! /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).

You win brownie points for getting the year right!

> +"""Test the validate-translations-file script."""
> +
> +import logging
> +import os.path
> +from textwrap import dedent
> +from unittest import TestCase, TestLoader
> +
> +from canonical.launchpad.ftests.script import run_script
> +
> +import lp.translations
> +
> +from lp.translations.scripts.validate_translations_file import (
> + UnknownFileType, ValidateTranslationsFile)
> +from lp.translations.utilities.tests.xpi_helpers import (
> + get_en_US_xpi_file_to_import)
> +
> +
> +class TestValidateTranslationsFile(TestCase):
> +
> + def _makeValidator(self, test_args=None):
> + """Produce a ValidateTranslationsFile."""
> + if test_args is None:
> + test_args = []
> + validator = ValidateTranslationsFile(test_args=test_args)
> + validator.logger.setLevel(logging.CRITICAL)
> + return validator
> +
> + def _strip(self, file_contents):
> + """Remove leading newlines & indentation from file_contents."""
> + return dedent(file_contents.strip())
> +
> + def _findTestData(self):
> + """Return base path to this test's test data."""
> + return os.path.join(
> + os.path.dirname(lp.translations.__file__),
> + 'scripts/tests/test-data')
> +
> + def test_validate_unknown(self):
> + # Unknown filename extensions result in UnknownFileType.
> + validator = self._makeValidator(['foo.bar'])

The argument list here is confusing. Does it serve any purpose? If
not please remove it.

> === added file 'lib/lp/translations/scripts/validate_translations_file.py'
> --- lib/lp/translations/scripts/validate_translations_file.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/translations/scripts/validate_translations_file.py 2010-01-05 17:14:16 +0000
> @@ -0,0 +1,131 @@
> +# Copyright 2010 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +__metaclass__ = type
> +
> +__all__ = [
> + 'UnknownFileType',
> + 'ValidateTranslationsFile',
> + ]
> +
> +from cStringIO import StringIO
> +import logging
> +from optparse import OptionParser
> +import os.path
> +
> +from canonical.launchpad import scripts
> +from lp.translations.utilities.gettext_po_parser import POParser
> +from lp.translations.utilities.mozilla_xpi_importer import (
> + DtdFile, MozillaZipImportParser)
> +from lp.translations.utilities.xpi_manifest import XpiManifest
> +
> +
> +class UnknownFileType(Exception):
> + """File's type is not recognized."""...

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

Thank you. You were absolutely right about the confusing test_args; they were mostly meant to be illustrative here but I thought I'd need them at some point. Turns out I don't, and I just removed the entire mechanism. Simple is beautiful.

=== added directory 'lib/lp/translations/scripts/tests/test-data'
=== added file 'lib/lp/translations/scripts/tests/test-data/minimal.pot'
--- lib/lp/translations/scripts/tests/test-data/minimal.pot	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/scripts/tests/test-data/minimal.pot	2010-01-06 06:21:21 +0000
@@ -0,0 +1,8 @@
+msgid ""
+msgstr ""
+"MIME-Version: 1.0\n"
+"Content-Type: text/plan; charset=UTF-8\n"
+"Content-Transfer-Encoding: 8bit\n"
+
+msgid "A translatable string."
+msgstr ""

=== added file 'lib/lp/translations/scripts/tests/test_validate_translations_file.py'
--- lib/lp/translations/scripts/tests/test_validate_translations_file.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/scripts/tests/test_validate_translations_file.py	2010-01-06 06:21:21 +0000
@@ -0,0 +1,131 @@
+#! /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).
+
+"""Test the validate-translations-file script."""
+
+import logging
+import os.path
+from textwrap import dedent
+from unittest import TestCase, TestLoader
+
+from canonical.launchpad.ftests.script import run_script
+
+import lp.translations
+
+from lp.translations.scripts.validate_translations_file import (
+    UnknownFileType, ValidateTranslationsFile)
+from lp.translations.utilities.tests.xpi_helpers import (
+    get_en_US_xpi_file_to_import)
+
+
+class TestValidateTranslationsFile(TestCase):
+
+    def _makeValidator(self):
+        """Produce a ValidateTranslationsFile."""
+        validator = ValidateTranslationsFile(test_args=[])
+        validator.logger.setLevel(logging.CRITICAL)
+        return validator
+
+    def _strip(self, file_contents):
+        """Remove leading newlines & indentation from file_contents."""
+        return dedent(file_contents.strip())
+
+    def _findTestData(self):
+        """Return base path to this test's test data."""
+        return os.path.join(
+            os.path.dirname(lp.translations.__file__),
+            'scripts/tests/test-data')
+
+    def test_validate_unknown(self):
+        # Unknown filename extensions result in UnknownFileType.
+        validator = self._makeValidator()
+        self.assertRaises(
+            UnknownFileType, validator._validateContent, 'foo.bar', 'content')
+
+    def test_validate_dtd_good(self):
+        validator = self._makeValidator()
+        result = validator._validateContent(
+            'test.dtd', '<!ENTITY a.translatable.string "A string">\n')
+        self.assertTrue(result)
+
+    def test_validate_dtd_bad(self):
+        validator = self._makeValidator()
+        result = validator._validateContent(
+            'test.dtd', '<!ENTIT etc.')
+        self.assertFalse(result)
+
+    def test_validate_xpi_manifest_good(self):
+        validator = self._makeValidator()
+        result = validator._validateContent(
+            'chrome.manifest', 'locale foo nl jar:chrome/nl.jar!/foo/')
+        self.assertTrue(result)
+
+    def test_validate_xpi_manifest_bad(self):
+        # XPI manifests must not begin with newline.
+        validator = self._makeValidator()
+        result = validator._validateContent('chrome.manifest', '\nlocale')
+        self.assertFalse(result)
+
+    def test_validate_po_good(self):
+        validator = self._makeValidator()
+        result = validator._validateContent('nl.po', self._strip(r"""
+            msgid ""
+            msgstr ""
+            "MIME-Version: 1.0\n"
+            "Content-Type: text/plan; charset=UTF-8\n"
+            "Content-Transfer-Encoding: 8bit\n"
+
+            msgid "foo"
+            msgstr "bar"
+            """))
+        self.assertTrue(result)
+
+    def test_validate_po_bad(self):
+        validator = self._makeValidator()
+        result = validator._validateContent('nl.po', self._strip("""
+            msgid "no header here"
+            msgstr "hier geen kopje"
+            """))
+        self.assertFalse(result)
+
+    def test_validate_pot_good(self):
+        validator = self._makeValidator()
+        result = validator._validateContent('test.pot', self._strip(r"""
+            msgid ""
+            msgstr ""
+            "MIME-Version: 1.0\n"
+            "Content-Type: text/plan; charset=UTF-8\n"
+            "Content-Transfer-Encoding: 8bit\n"
+
+            msgid "foo"
+            msgstr ""
+            """))
+        self.assertTrue(result)
+
+    def test_validate_pot_bad(self):
+        validator = self._makeValidator()
+        result = validator._validateContent('test.pot', 'garble')
+        self.assertFalse(result)
+
+    def test_validate_xpi_good(self):
+        validator = self._makeValidator()
+        xpi_content = get_en_US_xpi_file_to_import('en-US').read()
+        result = validator._validateContent('pl.xpi', xpi_content)
+        self.assertTrue(result)
+
+    def test_validate_xpi_bad(self):
+        validator = self._makeValidator()
+        result = validator._validateContent('de.xpi', 'garble')
+        self.assertFalse(result)
+
+    def test_script(self):
+        test_input = os.path.join(self._findTestData(), 'minimal.pot')
+        script = 'scripts/rosetta/validate-translations-file.py'
+        result, out, err = run_script(script, [test_input])
+        self.assertEqual(0, result)
+
+
+def test_suite():
+    return TestLoader().loadTestsFromName(__name__)

=== added file 'lib/lp/translations/scripts/validate_translations_file.py'
--- lib/lp/translations/scripts/validate_translations_file.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/scripts/validate_translations_file.py	2010-01-06 06:21:21 +0000
@@ -0,0 +1,130 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+__all__ = [
+    'UnknownFileType',
+    'ValidateTranslationsFile',
+    ]
+
+from cStringIO import StringIO
+import logging
+from optparse import OptionParser
+import os.path
+
+from canonical.launchpad import scripts
+from lp.translations.utilities.gettext_po_parser import POParser
+from lp.translations.utilities.mozilla_xpi_importer import (
+    DtdFile, MozillaZipImportParser)
+from lp.translations.utilities.xpi_manifest import XpiManifest
+
+
+class UnknownFileType(Exception):
+    """File's type is not recognized."""
+
+
+def validate_unknown_file_type(filename, content):
+    """Fail validation: unknown file type."""
+    raise UnknownFileType("Unrecognized file type for '%s'." % filename)
+
+
+def validate_dtd(filename, content):
+    """Validate XPI DTD file."""
+    DtdFile(filename, filename, content)
+
+
+def validate_po(filename, content):
+    """Validate a gettext PO or POT file."""
+    POParser().parse(content)
+
+
+def validate_xpi(filename, content):
+    """Validate an XPI archive."""
+    MozillaZipImportParser(filename, StringIO(content))
+
+
+def validate_xpi_manifest(filename, content):
+    """Validate XPI manifest."""
+    XpiManifest(content)
+
+
+class ValidateTranslationsFile:
+    """Parse translations files to see if they are well-formed."""
+
+    name = 'validate-translations-file'
+
+    validators = {
+	'dtd': validate_dtd,
+	'manifest': validate_xpi_manifest,
+	'po': validate_po,
+	'pot': validate_po,
+	'xpi': validate_xpi,
+	}
+
+    def __init__(self, test_args=None):
+        """Set up basic facilities, similar to `LaunchpadScript`."""
+        self.parser = OptionParser()
+        scripts.logger_options(self.parser, default=logging.INFO)
+        self.options, self.args = self.parser.parse_args(args=test_args)
+        self.logger = scripts.logger(self.options, self.name)
+
+    def main(self):
+        """Validate file(s)."""
+        failures = 0
+        files = len(self.args)
+        self.logger.info("Validating %d file(s)." % files)
+
+        for filename in self.args:
+            if not self._readAndValidate(filename):
+                failures += 1
+
+        if failures == 0:
+            self.logger.info("OK.")
+        elif failures > 1:
+            self.logger.error("%d failures in %d files." % (failures, files))
+        elif files > 1:
+            self.logger.error("1 failure in %d files." % files)
+        else:
+            self.logger.error("Validation failed.")
+
+        if failures == 0:
+            return 0
+        else:
+            return 1
+
+    def _pickValidator(self, filename):
+        """Select the appropriate validator for a file."""
+        base, ext = os.path.splitext(filename)
+        if ext is not None and ext.startswith('.'):
+            ext = ext[1:]
+        return self.validators.get(ext, validate_unknown_file_type)
+
+    def _validateContent(self, filename, content):
+        """Validate in-memory file contents.
+
+        :param filename: Name of this file.
+        :param content: Contents of this file, as raw bytes.
+        :return: Whether the file was parsed successfully.
+        """
+        validator = self._pickValidator(filename)
+        try:
+            validator(filename, content)
+        except (SystemError, AssertionError):
+            raise
+        except UnknownFileType:
+            raise
+        except Exception, e:
+            self.logger.warn("Failure in '%s': %s" % (filename, e))
+            return False
+
+        return True
+
+    def _readAndValidate(self, filename):
+        """Read given file and validate it.
+
+        :param filename: Name of a file to read.
+        :return: Whether the file was parsed successfully.
+        """
+        content = file(filename).read()
+        return self._validateContent(filename, content)

=== modified file 'lib/lp/translations/utilities/mozilla_xpi_importer.py'
--- lib/lp/translations/utilities/mozilla_xpi_importer.py	2009-10-14 18:43:26 +0000
+++ lib/lp/translations/utilities/mozilla_xpi_importer.py	2010-01-06 06:21:21 +0000
@@ -4,7 +4,9 @@
 __metaclass__ = type
 
 __all__ = [
+    'DtdFile',
     'MozillaXpiImporter',
+    'MozillaZipImportParser',
     ]
 
 from cStringIO import StringIO

=== modified file 'lib/lp/translations/utilities/tests/test_xpi_manifest.py'
--- lib/lp/translations/utilities/tests/test_xpi_manifest.py	2009-07-17 00:26:05 +0000
+++ lib/lp/translations/utilities/tests/test_xpi_manifest.py	2010-01-06 06:21:21 +0000
@@ -9,6 +9,9 @@
 
 from lp.translations.utilities.xpi_manifest import XpiManifest
 
+from lp.translations.interfaces.translationimporter import (
+    TranslationFormatSyntaxError)
+
 
 class XpiManifestTestCase(unittest.TestCase):
     """Test `XpiManifest`."""
@@ -38,7 +41,7 @@
             There are no usable
             locale lines
             in this file.
-            """)
+            """.lstrip())
         self.assertEqual(len(manifest._locales), 0)
         chrome_path, locale = manifest.getChromePathAndLocale('lines')
         self.failIf(chrome_path is not None, "Empty manifest matched a path.")
@@ -61,7 +64,7 @@
             locale bar en-US bardir/
             locale ixx en-US ixxdir/
             locale gna en-US gnadir/
-            """)
+            """.lstrip())
         self.assertEqual(len(manifest._locales), 4)
         self._checkSortOrder(manifest)
         for dir in ['gna', 'bar', 'ixx', 'foo']:
@@ -107,7 +110,7 @@
             locale okay fr foodir/
             locale overlong fr foordir/ etc. etc. etc.
             locale incomplete fr
-            """)
+            """.lstrip())
         self.assertEqual(len(manifest._locales), 1)
         chrome_path, locale = manifest.getChromePathAndLocale('foodir/x')
         self.failIf(chrome_path is None, "Garbage lines messed up match.")
@@ -119,7 +122,7 @@
         manifest = XpiManifest("""
             locale dup fy boppe
             locale dup fy boppe
-            """)
+            """.lstrip())
         self.assertEqual(len(manifest._locales), 1)
 
     def _checkLookup(self, manifest, path, chrome_path, locale):
@@ -162,7 +165,7 @@
         manifest = XpiManifest("""
             locale short el /ploink/squit
             locale long he /ploink/squittle
-            """)
+            """.lstrip())
         self._checkSortOrder(manifest)
         self._checkLookup(manifest, 'ploink/squit/x', 'short/x', 'el')
         self._checkLookup(manifest, '/ploink/squittle/x', 'long/x', 'he')
@@ -175,7 +178,7 @@
             locale foo2 ca a/b/
             locale foo3 ca a/b/c/x1
             locale foo4 ca a/b/c/x2
-            """)
+            """.lstrip())
         self._checkSortOrder(manifest)
         self._checkLookup(manifest, 'a/bb', 'foo1/bb', 'ca')
         self._checkLookup(manifest, 'a/bb/c', 'foo1/bb/c', 'ca')
@@ -190,7 +193,7 @@
         manifest = XpiManifest("""
             locale foo en_GB jar:foo.jar!/dir/
             locale bar id jar:bar.jar!/
-            """)
+            """.lstrip())
         self._checkSortOrder(manifest)
         self._checkLookup(
             manifest, 'jar:foo.jar!/dir/file', 'foo/file', 'en_GB')
@@ -227,7 +230,7 @@
             locale croatian hr jar:translations.jar!/hr/
             locale docs sr jar:docs.jar!/sr/
             locale docs hr jar:docs.jar!/hr/
-            """)
+            """.lstrip())
         self._checkSortOrder(manifest)
         self._checkLookup(
             manifest, 'jar:translations.jar!/sr/x', 'serbian/x', 'sr')
@@ -242,7 +245,7 @@
             locale x it jar:dir/x.jar!/subdir/y.jar!/
             locale y it jar:dir/x.jar!/subdir/y.jar!/deep/
             locale z it jar:dir/x.jar!/subdir/z.jar!/
-            """)
+            """.lstrip())
         self._checkSortOrder(manifest)
         self._checkLookup(
             manifest, 'jar:dir/x.jar!/subdir/y.jar!/foo', 'x/foo', 'it')
@@ -291,10 +294,18 @@
             locale browser en-US jar:locales/
             locale browser en-US jar:locales/en-US.jar!/chrome/
             locale browser en-US jar:locales/en-US.jar!/
-            """)
+            """.lstrip())
         path = manifest.findMatchingXpiPath('browser/gui/print.dtd', 'en-US')
         self.assertEqual(path, "jar:locales/en-US.jar!/chrome/gui/print.dtd")
 
+    def test_blank_line(self):
+        # Manifests must not begin with newline.
+        self.assertRaises(
+            TranslationFormatSyntaxError,
+            XpiManifest, """
+            locale browser en-US jar:locales
+            """)
+
 
 def test_suite():
     return unittest.defaultTestLoader.loadTestsFromName(__name__)

=== modified file 'lib/lp/translations/utilities/xpi_manifest.py'
--- lib/lp/translations/utilities/xpi_manifest.py	2009-07-17 00:26:05 +0000
+++ lib/lp/translations/utilities/xpi_manifest.py	2010-01-06 06:21:21 +0000
@@ -9,6 +9,9 @@
 import logging
 import re
 
+from lp.translations.interfaces.translationimporter import (
+    TranslationFormatSyntaxError)
+
 
 def normalize_path(path):
     """Normalize filesystem path within XPI file."""
@@ -127,6 +130,10 @@
 
     def __init__(self, content):
         """Initialize: parse `content` as a manifest file."""
+        if content.startswith('\n'):
+            raise TranslationFormatSyntaxError(
+                message="Manifest begins with newline.")
+
         locales = []
         for line in content.splitlines():
             words = line.split()

=== added file 'scripts/rosetta/validate-translations-file.py'
--- scripts/rosetta/validate-translations-file.py	1970-01-01 00:00:00 +0000
+++ scripts/rosetta/validate-translations-file.py	2010-01-06 06:21:21 +0000
@@ -0,0 +1,18 @@
+#! /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).
+
+# pylint: disable-msg=W0403
+
+__metaclass__ = type
+
+import _pythonpath
+
+import sys
+
+from lp.translations.scripts.validate_translations_file import (
+    ValidateTranslationsFile)
+
+
+if __name__ == "__main__":
+    sys.exit(ValidateTranslationsFile().main())