Landscape Client

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

  1. package-description-decode-fix
  2. Merge into trunk
Proposed by Alberto Donato
Status: Merged
Approved by: Alberto Donato
Approved revision: 929
Merged at revision: 928
Proposed branch: lp:~ack/landscape-client/package-description-decode-fix
Merge into: lp:~landscape/landscape-client/trunk
Diff against target: 238 lines (+46/-29)
4 files modified
landscape/package/skeleton.py (+4/-2)
landscape/package/tests/helpers.py (+12/-6)
landscape/package/tests/test_skeleton.py (+29/-20)
landscape/tests/helpers.py (+1/-1)
To merge this branch: bzr merge lp:~ack/landscape-client/package-description-decode-fix
Reviewer Review Type Date Requested Status
Simon Poirier (community) Approve
🤖 Landscape Builder test results Approve
Free Ekanayaka (community) Approve
Review via email: mp+314444@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
🤖 Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :

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)
Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Nice, +1

review: Approve
lp:~ack/landscape-client/package-description-decode-fix updated
928. By Alberto Donato

Add comment.

Revision history for this message
🤖 Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :

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)
Revision history for this message
Simon Poirier (simpoir) wrote :

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 updated
929. By Alberto Donato

Fix test (Simon's review)

Revision history for this message
Alberto Donato (ack) wrote :

> 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!

Revision history for this message
🤖 Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :

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)
Revision history for this message
🤖 Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :

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)
Revision history for this message
Simon Poirier (simpoir) wrote :

+1 thanks

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
67 self._hash = package_hash67 self._hash = package_hash
6868
6969
70
71def relation_to_string(relation_tuple):70def relation_to_string(relation_tuple):
72 """Convert an apt relation to a string representation.71 """Convert an apt relation to a string representation.
7372
@@ -150,5 +149,8 @@
150 if with_unicode:149 if with_unicode:
151 skeleton.section = skeleton.section.decode("utf-8")150 skeleton.section = skeleton.section.decode("utf-8")
152 skeleton.summary = skeleton.summary.decode("utf-8")151 skeleton.summary = skeleton.summary.decode("utf-8")
153 skeleton.description = skeleton.description.decode("utf-8")152 # Avoid double-decoding package descriptions in build_skeleton_apt,
153 # which causes an error with newer python-apt (Xenial onwards)
154 if not isinstance(skeleton.description, unicode):
155 skeleton.description = skeleton.description.decode("utf-8")
154 return skeleton156 return skeleton
155157
=== 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 @@
30 test_case._hash_packages_by_name = self._hash_packages_by_name30 test_case._hash_packages_by_name = self._hash_packages_by_name
3131
32 def _add_package(self, packages_file, name, architecture="all",32 def _add_package(self, packages_file, name, architecture="all",
33 version="1.0", control_fields=None):33 version="1.0", description="description",
34 control_fields=None):
34 if control_fields is None:35 if control_fields is None:
35 control_fields = {}36 control_fields = {}
36 package_stanza = textwrap.dedent("""37 package_stanza = textwrap.dedent("""
@@ -42,9 +43,12 @@
42 Architecture: %(architecture)s43 Architecture: %(architecture)s
43 Source: source44 Source: source
44 Version: %(version)s45 Version: %(version)s
45 Description: description46 Description: short description
46 """ % {"name": name, "version": version,47 %(description)s
47 "architecture": architecture})48 """ % {
49 "name": name, "version": version,
50 "architecture": architecture,
51 "description": description.encode("utf-8")})
48 package_stanza = apt_pkg.rewrite_section(52 package_stanza = apt_pkg.rewrite_section(
49 apt_pkg.TagSection(package_stanza), apt_pkg.REWRITE_PACKAGE_ORDER,53 apt_pkg.TagSection(package_stanza), apt_pkg.REWRITE_PACKAGE_ORDER,
50 control_fields.items())54 control_fields.items())
@@ -72,7 +76,8 @@
72 append_file(self.dpkg_status, status + "\n\n")76 append_file(self.dpkg_status, status + "\n\n")
7377
74 def _add_package_to_deb_dir(self, path, name, architecture="all",78 def _add_package_to_deb_dir(self, path, name, architecture="all",
75 version="1.0", control_fields=None):79 version="1.0", description="description",
80 control_fields=None):
76 """Add fake package information to a directory.81 """Add fake package information to a directory.
7782
78 There will only be basic information about the package83 There will only be basic information about the package
@@ -83,7 +88,8 @@
83 control_fields = {}88 control_fields = {}
84 self._add_package(89 self._add_package(
85 os.path.join(path, "Packages"), name, architecture=architecture,90 os.path.join(path, "Packages"), name, architecture=architecture,
86 version=version, control_fields=control_fields)91 version=version, description=description,
92 control_fields=control_fields)
8793
88 def _touch_packages_file(self, deb_dir):94 def _touch_packages_file(self, deb_dir):
89 """Make sure the Packages file gets a newer mtime value.95 """Make sure the Packages file gets a newer mtime value.
9096
=== 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 @@
59 package = self.facade._cache[name]59 package = self.facade._cache[name]
60 return package.candidate60 return package.candidate
6161
62 def build_skeleton(self, *args, **kwargs):
63 """Build the skeleton to be tested."""
64 return build_skeleton_apt(*args, **kwargs)
65
66 def test_build_skeleton(self):62 def test_build_skeleton(self):
67 """63 """
68 C{build_skeleton} builds a C{PackageSkeleton} from a package. If64 build_skeleton_apt builds a C{PackageSkeleton} from a package. If
69 with_info isn't passed, C{section}, C{summary}, C{description},65 with_info isn't passed, C{section}, C{summary}, C{description},
70 C{size} and C{installed_size} will be C{None}.66 C{size} and C{installed_size} will be C{None}.
71 """67 """
72 pkg1 = self.get_package("name1")68 pkg1 = self.get_package("name1")
73 skeleton = self.build_skeleton(pkg1)69 skeleton = build_skeleton_apt(pkg1)
74 self.assertEqual("name1", skeleton.name)70 self.assertEqual("name1", skeleton.name)
75 self.assertEqual("version1-release1", skeleton.version)71 self.assertEqual("version1-release1", skeleton.version)
76 self.assertEqual(None, skeleton.section)72 self.assertEqual(None, skeleton.section)
@@ -90,35 +86,35 @@
9086
91 def test_build_skeleton_without_unicode(self):87 def test_build_skeleton_without_unicode(self):
92 """88 """
93 If C{with_unicode} isn't passed to C{build_skeleton}, the name89 If C{with_unicode} isn't passed to build_skeleton_apt, the name
94 and version of the skeleton are byte strings. The hash doesn't90 and version of the skeleton are byte strings. The hash doesn't
95 change, though.91 change, though.
96 """92 """
97 pkg1 = self.get_package("name1")93 pkg1 = self.get_package("name1")
98 skeleton = self.build_skeleton(pkg1)94 skeleton = build_skeleton_apt(pkg1)
99 self.assertTrue(isinstance(skeleton.name, str))95 self.assertTrue(isinstance(skeleton.name, str))
100 self.assertTrue(isinstance(skeleton.version, str))96 self.assertTrue(isinstance(skeleton.version, str))
101 self.assertEqual(HASH1, skeleton.get_hash())97 self.assertEqual(HASH1, skeleton.get_hash())
10298
103 def test_build_skeleton_with_unicode(self):99 def test_build_skeleton_with_unicode(self):
104 """100 """
105 If C{with_unicode} is passed to C{build_skeleton}, the name101 If C{with_unicode} is passed to build_skeleton_apt, the name
106 and version of the skeleton are unicode strings.102 and version of the skeleton are unicode strings.
107 """103 """
108 pkg1 = self.get_package("name1")104 pkg1 = self.get_package("name1")
109 skeleton = self.build_skeleton(pkg1, with_unicode=True)105 skeleton = build_skeleton_apt(pkg1, with_unicode=True)
110 self.assertTrue(isinstance(skeleton.name, unicode))106 self.assertTrue(isinstance(skeleton.name, unicode))
111 self.assertTrue(isinstance(skeleton.version, unicode))107 self.assertTrue(isinstance(skeleton.version, unicode))
112 self.assertEqual(HASH1, skeleton.get_hash())108 self.assertEqual(HASH1, skeleton.get_hash())
113109
114 def test_build_skeleton_with_info(self):110 def test_build_skeleton_with_info(self):
115 """111 """
116 If C{with_info} is passed to C{build_skeleton}, C{section},112 If C{with_info} is passed to build_skeleton_apt, C{section},
117 C{summary}, C{description} and the size fields will be extracted113 C{summary}, C{description} and the size fields will be extracted
118 from the package.114 from the package.
119 """115 """
120 pkg1 = self.get_package("name1")116 pkg1 = self.get_package("name1")
121 skeleton = self.build_skeleton(pkg1, with_info=True)117 skeleton = build_skeleton_apt(pkg1, with_info=True)
122 self.assertEqual("Group1", skeleton.section)118 self.assertEqual("Group1", skeleton.section)
123 self.assertEqual("Summary1", skeleton.summary)119 self.assertEqual("Summary1", skeleton.summary)
124 self.assertEqual("Description1", skeleton.description)120 self.assertEqual("Description1", skeleton.description)
@@ -128,11 +124,11 @@
128 def test_build_skeleton_with_unicode_and_extra_info(self):124 def test_build_skeleton_with_unicode_and_extra_info(self):
129 """125 """
130 If C{with_unicode} and C{with_info} are passed to126 If C{with_unicode} and C{with_info} are passed to
131 C{build_skeleton}, the name, version and the extra info of the127 build_skeleton_apt, the name, version and the extra info of the
132 skeleton are unicode strings.128 skeleton are unicode strings.
133 """129 """
134 pkg1 = self.get_package("name1")130 pkg1 = self.get_package("name1")
135 skeleton = self.build_skeleton(pkg1, with_unicode=True, with_info=True)131 skeleton = build_skeleton_apt(pkg1, with_unicode=True, with_info=True)
136 self.assertTrue(isinstance(skeleton.name, unicode))132 self.assertTrue(isinstance(skeleton.name, unicode))
137 self.assertTrue(isinstance(skeleton.version, unicode))133 self.assertTrue(isinstance(skeleton.version, unicode))
138 self.assertTrue(isinstance(skeleton.section, unicode))134 self.assertTrue(isinstance(skeleton.section, unicode))
@@ -140,13 +136,26 @@
140 self.assertTrue(isinstance(skeleton.description, unicode))136 self.assertTrue(isinstance(skeleton.description, unicode))
141 self.assertEqual(HASH1, skeleton.get_hash())137 self.assertEqual(HASH1, skeleton.get_hash())
142138
139 def test_build_skeleton_with_unicode_and_non_ascii(self):
140 """
141 If with_unicode and with_info are passed to build_skeleton_apt,
142 the description is decoded and non-ascii chars replaced.
143 """
144 self._add_package_to_deb_dir(
145 self.skeleton_repository_dir, "pkg", description=u"T\xe9st")
146 self.facade._cache.update(None)
147 self.facade._cache.open(None)
148 pkg = self.get_package("pkg")
149 skeleton = build_skeleton_apt(pkg, with_unicode=True, with_info=True)
150 self.assertEqual(u"T?st", skeleton.description)
151
143 def test_build_skeleton_minimal(self):152 def test_build_skeleton_minimal(self):
144 """153 """
145 A package that has only the required fields will still have some154 A package that has only the required fields will still have some
146 relations defined.155 relations defined.
147 """156 """
148 minimal_package = self.get_package("minimal")157 minimal_package = self.get_package("minimal")
149 skeleton = self.build_skeleton(minimal_package)158 skeleton = build_skeleton_apt(minimal_package)
150 self.assertEqual("minimal", skeleton.name)159 self.assertEqual("minimal", skeleton.name)
151 self.assertEqual("1.0", skeleton.version)160 self.assertEqual("1.0", skeleton.version)
152 self.assertEqual(None, skeleton.section)161 self.assertEqual(None, skeleton.section)
@@ -166,7 +175,7 @@
166 be either an empty string or None, depending on which field.175 be either an empty string or None, depending on which field.
167 """176 """
168 package = self.get_package("minimal")177 package = self.get_package("minimal")
169 skeleton = self.build_skeleton(package, True)178 skeleton = build_skeleton_apt(package, True)
170 self.assertEqual("", skeleton.section)179 self.assertEqual("", skeleton.section)
171 self.assertEqual(180 self.assertEqual(
172 "A minimal package with no dependencies or other relations.",181 "A minimal package with no dependencies or other relations.",
@@ -181,7 +190,7 @@
181 simple, i.e. not specifying a version.190 simple, i.e. not specifying a version.
182 """191 """
183 package = self.get_package("simple-relations")192 package = self.get_package("simple-relations")
184 skeleton = self.build_skeleton(package)193 skeleton = build_skeleton_apt(package)
185 self.assertEqual("simple-relations", skeleton.name)194 self.assertEqual("simple-relations", skeleton.name)
186 self.assertEqual("1.0", skeleton.version)195 self.assertEqual("1.0", skeleton.version)
187 relations = [196 relations = [
@@ -201,7 +210,7 @@
201 version dependent.210 version dependent.
202 """211 """
203 package = self.get_package("version-relations")212 package = self.get_package("version-relations")
204 skeleton = self.build_skeleton(package)213 skeleton = build_skeleton_apt(package)
205 self.assertEqual("version-relations", skeleton.name)214 self.assertEqual("version-relations", skeleton.name)
206 self.assertEqual("1.0", skeleton.version)215 self.assertEqual("1.0", skeleton.version)
207 relations = [216 relations = [
@@ -222,7 +231,7 @@
222 skeleton.231 skeleton.
223 """232 """
224 package = self.get_package("multiple-relations")233 package = self.get_package("multiple-relations")
225 skeleton = self.build_skeleton(package)234 skeleton = build_skeleton_apt(package)
226 self.assertEqual("multiple-relations", skeleton.name)235 self.assertEqual("multiple-relations", skeleton.name)
227 self.assertEqual("1.0", skeleton.version)236 self.assertEqual("1.0", skeleton.version)
228 relations = [237 relations = [
@@ -248,7 +257,7 @@
248 is considered to be a single relation, with a special type.257 is considered to be a single relation, with a special type.
249 """258 """
250 package = self.get_package("or-relations")259 package = self.get_package("or-relations")
251 skeleton = self.build_skeleton(package)260 skeleton = build_skeleton_apt(package)
252 self.assertEqual("or-relations", skeleton.name)261 self.assertEqual("or-relations", skeleton.name)
253 self.assertEqual("1.0", skeleton.version)262 self.assertEqual("1.0", skeleton.version)
254 relations = [263 relations = [
255264
=== 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 @@
94 "%s" % (diff, extra))94 "%s" % (diff, extra))
9595
9696
97class LandscapeTest(MessageTestCase, HelperTestCase, TestCase, ):97class LandscapeTest(MessageTestCase, HelperTestCase, TestCase):
9898
99 def setUp(self):99 def setUp(self):
100 self._old_config_filenames = BaseConfiguration.default_config_filenames100 self._old_config_filenames = BaseConfiguration.default_config_filenames

Subscribers

People subscribed via source and target branches

to all changes: