Merge lp:~ack/landscape-client/package-description-decode-fix into lp:~landscape/landscape-client/trunk

[Alberto Donato]

Avoid double-decoding package descriptions in build_skeleton_apt, which causes an error with Xenial python-apt.

Avoid double-decoding package descriptions in build_skeleton_apt, which causes an error with Xenial python-apt.

Note that python-apt replaces non-ascii chars in the description with "?".
This happens because no locale is set, but we can't arbitrarily set one in the client and we don't want to report localized versions of package descriptions.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 927
Branch: lp:~ack/landscape-client/package-description-decode-fix
Jenkins: https://ci.lscape.net/job/latch-test-precise/845/

review: Approve (test results)

[Free Ekanayaka (free.ekanayaka)]

Nice, +1

review: Approve

[lp:~ack/landscape-client/package-description-decode-fix]

928. By Alberto Donato on 2017-01-11

Add comment.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 928
Branch: lp:~ack/landscape-client/package-description-decode-fix
Jenkins: https://ci.lscape.net/job/latch-test-precise/846/

review: Approve (test results)

[Simon Poirier (simpoir)]

The change looks good, but your test doesn't seem to be calling build_skeleton(). And quick coverage run tells me your code is not exercised through test.

review: Needs Fixing

[lp:~ack/landscape-client/package-description-decode-fix]

929. By Alberto Donato on 2017-01-12

Fix test (Simon's review)

[Alberto Donato (ack)]

> The change looks good, but your test doesn't seem to be calling
> build_skeleton(). And quick coverage run tells me your code is not exercised
> through test.

Good catch, I think something got messed up while I was experimenting with the test.
It's fixed now. I also dropped the self.build_skeleton() helper in the test class, since it's a usless indirection.
Thanks!

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Fail
Revno: 929
Branch: lp:~ack/landscape-client/package-description-decode-fix
Jenkins: https://ci.lscape.net/job/latch-test-precise/848/

review: Needs Fixing (test results)

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 929
Branch: lp:~ack/landscape-client/package-description-decode-fix
Jenkins: https://ci.lscape.net/job/latch-test-precise/849/

review: Approve (test results)

[Simon Poirier (simpoir)]

+1 thanks

review: Approve

=== modified file 'landscape/package/skeleton.py'
--- landscape/package/skeleton.py	2013-05-31 12:27:27 +0000
+++ landscape/package/skeleton.py	2017-01-12 08:01:46 +0000
@@ -67,7 +67,6 @@
         self._hash = package_hash
 
 
-
 def relation_to_string(relation_tuple):
     """Convert an apt relation to a string representation.
 
@@ -150,5 +149,8 @@
         if with_unicode:
             skeleton.section = skeleton.section.decode("utf-8")
             skeleton.summary = skeleton.summary.decode("utf-8")
-            skeleton.description = skeleton.description.decode("utf-8")
+            # Avoid double-decoding package descriptions in build_skeleton_apt,
+            # which causes an error with newer python-apt (Xenial onwards)
+            if not isinstance(skeleton.description, unicode):
+                skeleton.description = skeleton.description.decode("utf-8")
     return skeleton

=== modified file 'landscape/package/tests/helpers.py'
--- landscape/package/tests/helpers.py	2016-07-06 11:06:09 +0000
+++ landscape/package/tests/helpers.py	2017-01-12 08:01:46 +0000
@@ -30,7 +30,8 @@
         test_case._hash_packages_by_name = self._hash_packages_by_name
 
     def _add_package(self, packages_file, name, architecture="all",
-                     version="1.0", control_fields=None):
+                     version="1.0", description="description",
+                     control_fields=None):
         if control_fields is None:
             control_fields = {}
         package_stanza = textwrap.dedent("""
@@ -42,9 +43,12 @@
                 Architecture: %(architecture)s
                 Source: source
                 Version: %(version)s
-                Description: description
-                """ % {"name": name, "version": version,
-                       "architecture": architecture})
+                Description: short description
+                 %(description)s
+                """ % {
+                    "name": name, "version": version,
+                    "architecture": architecture,
+                    "description": description.encode("utf-8")})
         package_stanza = apt_pkg.rewrite_section(
             apt_pkg.TagSection(package_stanza), apt_pkg.REWRITE_PACKAGE_ORDER,
             control_fields.items())
@@ -72,7 +76,8 @@
         append_file(self.dpkg_status, status + "\n\n")
 
     def _add_package_to_deb_dir(self, path, name, architecture="all",
-                                version="1.0", control_fields=None):
+                                version="1.0", description="description",
+                                control_fields=None):
         """Add fake package information to a directory.
 
         There will only be basic information about the package
@@ -83,7 +88,8 @@
             control_fields = {}
         self._add_package(
             os.path.join(path, "Packages"), name, architecture=architecture,
-            version=version, control_fields=control_fields)
+            version=version, description=description,
+            control_fields=control_fields)
 
     def _touch_packages_file(self, deb_dir):
         """Make sure the Packages file gets a newer mtime value.

=== modified file 'landscape/package/tests/test_skeleton.py'
--- landscape/package/tests/test_skeleton.py	2013-05-31 07:57:06 +0000
+++ landscape/package/tests/test_skeleton.py	2017-01-12 08:01:46 +0000
@@ -59,18 +59,14 @@
         package = self.facade._cache[name]
         return package.candidate
 
-    def build_skeleton(self, *args, **kwargs):
-        """Build the skeleton to be tested."""
-        return build_skeleton_apt(*args, **kwargs)
-
     def test_build_skeleton(self):
         """
-        C{build_skeleton} builds a C{PackageSkeleton} from a package. If
+        build_skeleton_apt builds a C{PackageSkeleton} from a package. If
         with_info isn't passed, C{section}, C{summary}, C{description},
         C{size} and C{installed_size} will be C{None}.
         """
         pkg1 = self.get_package("name1")
-        skeleton = self.build_skeleton(pkg1)
+        skeleton = build_skeleton_apt(pkg1)
         self.assertEqual("name1", skeleton.name)
         self.assertEqual("version1-release1", skeleton.version)
         self.assertEqual(None, skeleton.section)
@@ -90,35 +86,35 @@
 
     def test_build_skeleton_without_unicode(self):
         """
-        If C{with_unicode} isn't passed to C{build_skeleton}, the name
+        If C{with_unicode} isn't passed to build_skeleton_apt, the name
         and version of the skeleton are byte strings. The hash doesn't
         change, though.
         """
         pkg1 = self.get_package("name1")
-        skeleton = self.build_skeleton(pkg1)
+        skeleton = build_skeleton_apt(pkg1)
         self.assertTrue(isinstance(skeleton.name, str))
         self.assertTrue(isinstance(skeleton.version, str))
         self.assertEqual(HASH1, skeleton.get_hash())
 
     def test_build_skeleton_with_unicode(self):
         """
-        If C{with_unicode} is passed to C{build_skeleton}, the name
+        If C{with_unicode} is passed to build_skeleton_apt, the name
         and version of the skeleton are unicode strings.
         """
         pkg1 = self.get_package("name1")
-        skeleton = self.build_skeleton(pkg1, with_unicode=True)
+        skeleton = build_skeleton_apt(pkg1, with_unicode=True)
         self.assertTrue(isinstance(skeleton.name, unicode))
         self.assertTrue(isinstance(skeleton.version, unicode))
         self.assertEqual(HASH1, skeleton.get_hash())
 
     def test_build_skeleton_with_info(self):
         """
-        If C{with_info} is passed to C{build_skeleton}, C{section},
+        If C{with_info} is passed to build_skeleton_apt, C{section},
         C{summary}, C{description} and the size fields will be extracted
         from the package.
         """
         pkg1 = self.get_package("name1")
-        skeleton = self.build_skeleton(pkg1, with_info=True)
+        skeleton = build_skeleton_apt(pkg1, with_info=True)
         self.assertEqual("Group1", skeleton.section)
         self.assertEqual("Summary1", skeleton.summary)
         self.assertEqual("Description1", skeleton.description)
@@ -128,11 +124,11 @@
     def test_build_skeleton_with_unicode_and_extra_info(self):
         """
         If C{with_unicode} and C{with_info} are passed to
-        C{build_skeleton}, the name, version and the extra info of the
+        build_skeleton_apt, the name, version and the extra info of the
         skeleton are unicode strings.
         """
         pkg1 = self.get_package("name1")
-        skeleton = self.build_skeleton(pkg1, with_unicode=True, with_info=True)
+        skeleton = build_skeleton_apt(pkg1, with_unicode=True, with_info=True)
         self.assertTrue(isinstance(skeleton.name, unicode))
         self.assertTrue(isinstance(skeleton.version, unicode))
         self.assertTrue(isinstance(skeleton.section, unicode))
@@ -140,13 +136,26 @@
         self.assertTrue(isinstance(skeleton.description, unicode))
         self.assertEqual(HASH1, skeleton.get_hash())
 
+    def test_build_skeleton_with_unicode_and_non_ascii(self):
+        """
+        If with_unicode and with_info are passed to build_skeleton_apt,
+        the description is decoded and non-ascii chars replaced.
+        """
+        self._add_package_to_deb_dir(
+            self.skeleton_repository_dir, "pkg", description=u"T\xe9st")
+        self.facade._cache.update(None)
+        self.facade._cache.open(None)
+        pkg = self.get_package("pkg")
+        skeleton = build_skeleton_apt(pkg, with_unicode=True, with_info=True)
+        self.assertEqual(u"T?st", skeleton.description)
+
     def test_build_skeleton_minimal(self):
         """
         A package that has only the required fields will still have some
         relations defined.
         """
         minimal_package = self.get_package("minimal")
-        skeleton = self.build_skeleton(minimal_package)
+        skeleton = build_skeleton_apt(minimal_package)
         self.assertEqual("minimal", skeleton.name)
         self.assertEqual("1.0", skeleton.version)
         self.assertEqual(None, skeleton.section)
@@ -166,7 +175,7 @@
         be either an empty string or None, depending on which field.
         """
         package = self.get_package("minimal")
-        skeleton = self.build_skeleton(package, True)
+        skeleton = build_skeleton_apt(package, True)
         self.assertEqual("", skeleton.section)
         self.assertEqual(
             "A minimal package with no dependencies or other relations.",
@@ -181,7 +190,7 @@
         simple, i.e. not specifying a version.
         """
         package = self.get_package("simple-relations")
-        skeleton = self.build_skeleton(package)
+        skeleton = build_skeleton_apt(package)
         self.assertEqual("simple-relations", skeleton.name)
         self.assertEqual("1.0", skeleton.version)
         relations = [
@@ -201,7 +210,7 @@
         version dependent.
         """
         package = self.get_package("version-relations")
-        skeleton = self.build_skeleton(package)
+        skeleton = build_skeleton_apt(package)
         self.assertEqual("version-relations", skeleton.name)
         self.assertEqual("1.0", skeleton.version)
         relations = [
@@ -222,7 +231,7 @@
         skeleton.
         """
         package = self.get_package("multiple-relations")
-        skeleton = self.build_skeleton(package)
+        skeleton = build_skeleton_apt(package)
         self.assertEqual("multiple-relations", skeleton.name)
         self.assertEqual("1.0", skeleton.version)
         relations = [
@@ -248,7 +257,7 @@
         is considered to be a single relation, with a special type.
         """
         package = self.get_package("or-relations")
-        skeleton = self.build_skeleton(package)
+        skeleton = build_skeleton_apt(package)
         self.assertEqual("or-relations", skeleton.name)
         self.assertEqual("1.0", skeleton.version)
         relations = [

=== modified file 'landscape/tests/helpers.py'
--- landscape/tests/helpers.py	2016-08-17 20:31:48 +0000
+++ landscape/tests/helpers.py	2017-01-12 08:01:46 +0000
@@ -94,7 +94,7 @@
                                         "%s" % (diff, extra))
 
 
-class LandscapeTest(MessageTestCase, HelperTestCase, TestCase, ):
+class LandscapeTest(MessageTestCase, HelperTestCase, TestCase):
 
     def setUp(self):
         self._old_config_filenames = BaseConfiguration.default_config_filenames