Merge lp:~gocept/landscape-client/py3-package-store-reporter into lp:~landscape/landscape-client/trunk

[Steffen Allner]

Support py2/3 in landscape.package.store and landscape.package.reporter modules.

We are on the journey to get the Bytes / Unicode story solved for Python 2/3 compatibility.

This MP considers code in landscape.package.store and landscape.package.reporter and their respective tests. A major point was the explicit declaration, that hashes have to be bytes, as the hashing functions in Python do not automatically encode with ascii. Additionally other APIs required to be explicit with unicode and bytes declaration.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[lp:~gocept/landscape-client/py3-package-store-reporter]

982. By Steffen Allner on 2017-03-17

Comply with PEP-8 and coding style.

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 981
Branch: lp:~gocept/landscape-client/py3-package-store-reporter
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3632/

review: Approve (test results)

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 982
Branch: lp:~gocept/landscape-client/py3-package-store-reporter
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3633/

review: Approve (test results)

[Данило Шеган (danilo)]

FWIW, bpickle is now back to landscape.lib.bpickle, so there are a few conflicts in the imports now (Launchpad does not regenerate the diff when the target changes).

[lp:~gocept/landscape-client/py3-package-store-reporter]

983. By Steffen Allner on 2017-03-17

Backmerge from trunk with py2/3 bpickle.

[Steffen Allner (sallner)]

> FWIW, bpickle is now back to landscape.lib.bpickle, so there are a few
> conflicts in the imports now (Launchpad does not regenerate the diff when the
> target changes).

I have merged the trunk back into this branch. Should be updated now.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 983
Branch: lp:~gocept/landscape-client/py3-package-store-reporter
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3639/

review: Approve (test results)

[Данило Шеган (danilo)]

Looks good: a few minor nits inline. I'll just do a test run with this before I mark it approved.

[Данило Шеган (danilo)]

review: Approve

[Steffen Allner (sallner)]

See inline comments. I would especially interested in a direction for the StrictVersion case.

[lp:~gocept/landscape-client/py3-package-store-reporter]

984. By Steffen Allner on 2017-03-21

Remove no longer needed helper method.

985. By Steffen Allner on 2017-03-21

Remove superfluous assignment.

986. By Steffen Allner on 2017-03-21

Use undocumented parameter to ensure bytes as return value.

[Steffen Allner (sallner)]

I cleaned up and documented the mentioned parts.

Additionally, I opened up a new MP which uses only bytes for software comparison[0].

[0] https://code.launchpad.net/~gocept/landscape-client/bytes-versioning/+merge/320494

[Данило Шеган (danilo)]

Replied inline. If you want, we can deal with the is_version_higher() fix in a follow-up branch and get this one landed, but please get a Gocept review as well.

[Данило Шеган (danilo)]

Cool, haven't refreshed with inline comments for a while: there's a conflict in this branch now. Please fix a conflict and get a ~gocept review and I'll get this branch landed.

[Данило Шеган (danilo)]

After looking at the other branch, I guess you might want to land that one first. Then we can merge trunk here, resolve a conflict and land this one too.

[Daniel Havlik (nilo)]

If conflict is fixed: +1

review: Approve

[Steffen Allner (sallner)]

Then let's wait for the other branch to arrive and I can fix both afterwards.

[Данило Шеган (danilo)]

Landed the other branch, please re-merge trunk into this one and resolve the conflict.

[lp:~gocept/landscape-client/py3-package-store-reporter]

987. By Steffen Allner on 2017-03-21

Backmerge from trunk.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Fail
Revno: 987
Branch: lp:~gocept/landscape-client/py3-package-store-reporter
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3689/

review: Needs Fixing (test results)

[Steffen Allner (sallner)]

It seems there is again the same flaky test, we have seen before. Danilo, do you have an idea, how we could make it more robust? I can never reproduce that failre with Python 2. Maybe you want to restart it.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 987
Branch: lp:~gocept/landscape-client/py3-package-store-reporter
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3692/

review: Approve (test results)

[Данило Шеган (danilo)]

Landing.

review: Approve

=== modified file 'landscape/broker/store.py'
--- landscape/broker/store.py	2017-03-17 09:26:45 +0000
+++ landscape/broker/store.py	2017-03-21 17:01:15 +0000
@@ -101,7 +101,7 @@
 
 from landscape import DEFAULT_SERVER_API
 from landscape.lib import bpickle
-from landscape.lib.fs import create_binary_file
+from landscape.lib.fs import create_binary_file, read_binary_file
 from landscape.lib.versioning import sort_versions, is_version_higher
 
 
@@ -260,7 +260,7 @@
         for filename in self._walk_pending_messages():
             if max is not None and len(messages) >= max:
                 break
-            data = self._get_content(self._message_dir(filename))
+            data = read_binary_file(self._message_dir(filename))
             try:
                 message = bpickle.loads(data)
             except ValueError as e:
@@ -436,13 +436,6 @@
     def _message_dir(self, *args):
         return os.path.join(self._directory, *args)
 
-    def _get_content(self, filename):
-        file = open(filename, 'rb')
-        try:
-            return file.read()
-        finally:
-            file.close()
-
     def _reprocess_holding(self):
         """
         Unhold accepted messages left behind, and hold unaccepted
@@ -454,7 +447,7 @@
         for old_filename in self._walk_messages():
             flags = self._get_flags(old_filename)
             try:
-                message = bpickle.loads(self._get_content(old_filename))
+                message = bpickle.loads(read_binary_file(old_filename))
             except ValueError as e:
                 logging.exception(e)
                 if HELD not in flags:

=== modified file 'landscape/compat.py'
--- landscape/compat.py	2017-03-17 09:26:45 +0000
+++ landscape/compat.py	2017-03-21 17:01:15 +0000
@@ -46,16 +46,3 @@
             return s.decode(encoding, errors)
     else:
         return s
-
-
-def convert_buffer_to_string(mem_view):
-    """
-    Converts a buffer in Python 2 or a memoryview in Python 3 to str.
-
-    @param mem_view: The view to convert.
-    """
-    if _PY3:
-        result = mem_view.decode('ascii')
-    else:
-        result = str(mem_view)
-    return result

=== modified file 'landscape/package/facade.py'
--- landscape/package/facade.py	2017-03-13 15:38:09 +0000
+++ landscape/package/facade.py	2017-03-21 17:01:15 +0000
@@ -196,7 +196,8 @@
         process = subprocess.Popen(
             ["dpkg", "--set-selections"] + self._dpkg_args,
             stdin=subprocess.PIPE)
-        process.communicate(selection)
+        # We need bytes here to communicate with the process.
+        process.communicate(selection.encode("utf-8"))
 
     def set_package_hold(self, version):
         """Add a dpkg hold for a package.

=== modified file 'landscape/package/reporter.py'
--- landscape/package/reporter.py	2017-03-20 09:43:08 +0000
+++ landscape/package/reporter.py	2017-03-21 17:01:15 +0000
@@ -20,7 +20,6 @@
 from landscape.lib.fs import touch_file
 from landscape.lib.lsb_release import parse_lsb_release, LSB_RELEASE_FILENAME
 
-from landscape.compat import convert_buffer_to_string
 from landscape.package.taskhandler import (
     PackageTaskHandlerConfiguration, PackageTaskHandler, run_task_handler)
 from landscape.package.store import UnknownHashIDRequest, FakePackageStore
@@ -231,6 +230,9 @@
                 self._reactor.call_later(
                     LOCK_RETRY_DELAYS[retry], self._apt_update, deferred)
                 out, err, code = yield deferred
+                out = out.decode("utf-8")
+                err = err.decode("utf-8")
+
                 timestamp = self._reactor.time()
 
                 touch_file(self._config.update_stamp_filename)
@@ -730,7 +732,7 @@
             messages = global_store.get_messages_by_ids(not_sent)
             sent = []
             for message_id, message in messages:
-                message = bpickle.loads(convert_buffer_to_string(message))
+                message = bpickle.loads(message)
                 if message["type"] not in got_type:
                     got_type.add(message["type"])
                     sent.append(message_id)

=== modified file 'landscape/package/skeleton.py'
--- landscape/package/skeleton.py	2017-03-10 12:19:57 +0000
+++ landscape/package/skeleton.py	2017-03-21 17:01:15 +0000
@@ -50,10 +50,14 @@
         """
         if self._hash is not None:
             return self._hash
-        digest = sha1("[%d %s %s]" % (self.type, self.name, self.version))
+        # We use ascii here as encoding  for backwards compatibility as it was
+        # default encoding for conversion from unicode to bytes in Python 2.7.
+        package_info = b"[%d %s %s]" % (
+            self.type, self.name.encode("ascii"), self.version.encode("ascii"))
+        digest = sha1(package_info)
         self.relations.sort()
         for pair in self.relations:
-            digest.update("[%d %s]" % pair)
+            digest.update(b"[%d %s]" % (pair[0], pair[1].encode("ascii")))
         return digest.digest()
 
     def set_hash(self, package_hash):

=== modified file 'landscape/package/store.py'
--- landscape/package/store.py	2017-03-17 09:26:45 +0000
+++ landscape/package/store.py	2017-03-21 17:01:15 +0000
@@ -7,9 +7,7 @@
     from pysqlite2 import dbapi2 as sqlite3
 
 from twisted.python.compat import iteritems, long
-from twisted.python.compat import StringType as basestring
 
-from landscape.compat import convert_buffer_to_string
 from landscape.lib import bpickle
 from landscape.lib.store import with_cursor
 
@@ -50,7 +48,10 @@
 
     @with_cursor
     def get_hash_id(self, cursor, hash):
-        """Return the id associated to C{hash}, or C{None} if not available."""
+        """Return the id associated to C{hash}, or C{None} if not available.
+
+        @param hash: a C{bytes} representing a hash.
+        """
         cursor.execute("SELECT id FROM hash WHERE hash=?",
                        (sqlite3.Binary(hash),))
         value = cursor.fetchone()
@@ -62,7 +63,7 @@
     def get_hash_ids(self, cursor):
         """Return a C{dict} holding all the available hash=>id mappings."""
         cursor.execute("SELECT hash, id FROM hash")
-        return dict([(str(row[0]), row[1]) for row in cursor.fetchall()])
+        return {bytes(row[0]): row[1] for row in cursor.fetchall()}
 
     @with_cursor
     def get_id_hash(self, cursor, id):
@@ -71,7 +72,7 @@
         cursor.execute("SELECT hash FROM hash WHERE id=?", (id,))
         value = cursor.fetchone()
         if value:
-            return str(value[0])
+            return bytes(value[0])
         return None
 
     @with_cursor
@@ -149,7 +150,7 @@
         the attached lookaside databases, falling back to the main one, as
         described in L{add_hash_id_db}.
         """
-        assert isinstance(hash, basestring)
+        assert isinstance(hash, bytes)
 
         # Check if we can find the hash=>id mapping in the lookaside stores
         for store in self._hash_id_stores:
@@ -332,7 +333,7 @@
         result = cursor.execute(
                 "SELECT id, data FROM message WHERE id IN (%s) "
                 "ORDER BY id" % params, tuple(message_ids)).fetchall()
-        return [(row[0], row[1]) for row in result]
+        return [(row[0], bytes(row[1])) for row in result]
 
 
 class HashIDRequest(object):
@@ -346,7 +347,7 @@
     def hashes(self, cursor):
         cursor.execute("SELECT hashes FROM hash_id_request WHERE id=?",
                        (self.id,))
-        return bpickle.loads(convert_buffer_to_string(cursor.fetchone()[0]))
+        return bpickle.loads(bytes(cursor.fetchone()[0]))
 
     @with_cursor
     def _get_timestamp(self, cursor):
@@ -395,7 +396,7 @@
 
         self.queue = row[0]
         self.timestamp = row[1]
-        self.data = bpickle.loads(convert_buffer_to_string(row[2]))
+        self.data = bpickle.loads(bytes(row[2]))
 
     @with_cursor
     def remove(self, cursor):

=== modified file 'landscape/package/tests/helpers.py'
--- landscape/package/tests/helpers.py	2017-03-13 15:15:46 +0000
+++ landscape/package/tests/helpers.py	2017-03-21 17:01:15 +0000
@@ -6,7 +6,7 @@
 import apt_inst
 import apt_pkg
 
-from landscape.lib.fs import append_binary_file, append_text_file
+from landscape.lib.fs import append_binary_file
 from landscape.lib.fs import create_binary_file
 from landscape.package.facade import AptFacade
 
@@ -49,11 +49,17 @@
                 """ % {
                     "name": name, "version": version,
                     "architecture": architecture,
-                    "description": description.encode("utf-8")})
+                    "description": description}).encode("utf-8")
+        # We want to re-order the TagSection, but it requires bytes as input.
+        # As we also want to write a binary file, we have to explicitly pass
+        # the hardly documented `bytes=True` to TagSection as it would be
+        # returned as unicode in Python 3 otherwise. In future versions of
+        # apt_pkg there should be a TagSection.write() which is recommended.
         package_stanza = apt_pkg.rewrite_section(
-            apt_pkg.TagSection(package_stanza), apt_pkg.REWRITE_PACKAGE_ORDER,
-            control_fields.items())
-        append_binary_file(packages_file, "\n" + package_stanza + "\n")
+            apt_pkg.TagSection(package_stanza, bytes=True),
+            apt_pkg.REWRITE_PACKAGE_ORDER,
+            list(control_fields.items()))
+        append_binary_file(packages_file, b"\n" + package_stanza + b"\n")
 
     def _add_system_package(self, name, architecture="all", version="1.0",
                             control_fields=None):
@@ -72,9 +78,9 @@
         control = deb.control.extractdata("control")
         deb_file.close()
         lines = control.splitlines()
-        lines.insert(1, "Status: install ok installed")
-        status = "\n".join(lines)
-        append_text_file(self.dpkg_status, status + "\n\n")
+        lines.insert(1, b"Status: install ok installed")
+        status = b"\n".join(lines)
+        append_binary_file(self.dpkg_status, status + b"\n\n")
 
     def _add_package_to_deb_dir(self, path, name, architecture="all",
                                 version="1.0", description="description",
@@ -161,7 +167,8 @@
            "Mzepwz6J7y5jpkIOH6sDKssF1rmUqYzBX2piZj9zyFad5RHv8dLoXsqua2spF3v+PQ"
            "ffXIlN8aYepsu3x2u0202VX+QFC10st6vvMfDdacgtdzKtpe5G5tuFYx5elcpXm27O"
            "d8LH7Oj3mqP7VgD8P6dTmJ33dsPnpuBnPO3SvLDNlu6ay9It6yZon0BIZRMApGwSgY"
-           "BaNgFIyCUTAKRsEoGAWjYBSMglEwCkbBKBgFo2AUjIJRMApGAUkAADhX8vgAKAAA ")
+           "BaNgFIyCUTAKRsEoGAWjYBSMglEwCkbBKBgFo2AUjIJRMApGAUkAADhX8vgAKAAA "
+           ).encode("ascii")
 
 PKGDEB2 = ("ITxhcmNoPgpkZWJpYW4tYmluYXJ5ICAgMTE2NjExNDUyMiAgMCAgICAgMCAgICAgMT"
            "AwNjQ0ICA0ICAgICAgICAgYAoyLjAKY29udHJvbC50YXIuZ3ogIDExNjYxMTQ1MjIg"
@@ -183,7 +190,8 @@
            "jR45xB99RGrkMGEq4Pbf0L3UWDL4XIRIk6Hjx7Urzj6SSxS/YTzKbu28sqe/64oPmF"
            "JGPj3lqR1cLMdz12u04rLHp/gM2y0mv3HOc/GqxvCl7PqWh7kbux6VrFk69zlefZsu"
            "v5WPycH/NUv7VgF8N6vfeBcgXp3NlnBFNDw5eZsd1as/aK+JzyvZ0TGEbBKBgFo2AU"
-           "jIJRMApGwSgYBaNgFIyCUTAKRsEoGAWjYBSMglEwCkbBKBgFJAEAu4OlKQAoAAAK")
+           "jIJRMApGwSgYBaNgFIyCUTAKRsEoGAWjYBSMglEwCkbBKBgFJAEAu4OlKQAoAAAK"
+           ).encode("ascii")
 
 PKGDEB3 = ("ITxhcmNoPgpkZWJpYW4tYmluYXJ5ICAgMTE2OTE0ODIwMyAgMCAgICAgMCAgICAgMT"
            "AwNjQ0ICA0ICAgICAgICAgYAoyLjAKY29udHJvbC50YXIuZ3ogIDExNjkxNDgyMDMg"
@@ -206,7 +214,8 @@
            "bOTd7zh0Xz0y5bdGmDrbLp/dbhNtdpU/EFSt9LKe7/xHgzWn4PWcirYXuVsbrlVMeT"
            "pXaZ4t+zkfi5/zY57qTy3Yw7B+XU7g+8L07rmG7Fe2bVxmyHZLZ+0V8Sl2Xj8mMIyC"
            "UTAKRsEoGAWjYBSMglEwCkbBKBgFo2AUjIJRMApGwSgYBaNgFIyCUTAKSAIAY/FOKA"
-           "AoAAAK")
+           "AoAAAK"
+           ).encode("ascii")
 
 PKGDEB4 = ("ITxhcmNoPgpkZWJpYW4tYmluYXJ5ICAgMTI3NjUxMTU3OC41MCAgICAgMCAgICAgNj"
            "Q0ICAgICA0\nICAgICAgICAgYAoyLjAKY29udHJvbC50YXIuZ3ogIDEyNzY1MTE1Nz"
@@ -221,7 +230,8 @@
            "ICAgYAofiwgAWgUWTAL/7dFBCsMgEEDRWfcUniCZ\nsU57kJ5ASJdFSOz9K9kULLQr"
            "C4H/NiPqQvnTLMNpc3XfZ9PPfW2W1JOae9s3i5okuPzBc6t5bU9Z\nS6nf7v067z93"
            "ENO8lcd9fP/LZ/d3f4td/6h+lqD0H+7W6ocl13wSAAAAAAAAAAAAAAAAAAfzAqr5\n"
-           "GFYAKAAACg==\n")
+           "GFYAKAAACg==\n"
+           ).encode("ascii")
 
 PKGDEB_MINIMAL = (
     "ITxhcmNoPgpkZWJpYW4tYmluYXJ5ICAgMTMxNzg5MDQ3OSAgMCAgICAgMCAgICAgMTAwNj"
@@ -234,7 +244,8 @@
     "AAAAAAAAAAAAAAAAAAAAAMBF70s1/foAKAAAZGF0YS50YXIu Z3ogICAgIDEzMTc4OTA0N"
     "zkgIDAgICAgIDAgICAgIDEwMDY0NCAgMTA3ICAgICAgIGAKH4sIAAAA AAACA+3KsQ3CQB"
     "AEwCvlK4D/N4frMSGBkQz0jwmQiHCEo5lkpd09HOPv6mrMfGcbs37nR7R2Pg01"
-    "ew5r32rvNUrGDp73x7SUEpfrbZl//LZ2AAAAAAAAAAAA2NELx33R7wAoAAAK")
+    "ew5r32rvNUrGDp73x7SUEpfrbZl//LZ2AAAAAAAAAAAA2NELx33R7wAoAAAK"
+).encode("ascii")
 
 PKGDEB_SIMPLE_RELATIONS = (
     "ITxhcmNoPgpkZWJpYW4tYmluYXJ5ICAgMTMxODUxNjMyMiAgMCAgICAgMCAgICAgMTAwNj"
@@ -249,7 +260,8 @@
     "EKgcHt1gAoAABkYXRhLnRhci5neiAgICAgMTMxODUxNjMyMiAgMCAgICAgMCAg ICAgMTA"
     "wNjQ0ICAxMDcgICAgICAgYAofiwgAAAAAAAID7cqxDcJQEETBK8UVwH2b+64HQgIjGegf "
     "CJCIIMLRTPKC3d0+/i6f5qpX21z52bdorR+m7Fl9imw5jhVDxQbu19txHYY4nS/r8uX3aw"
-    "cAAAAA AAAAAIANPQALnD6FACgAAAo=")
+    "cAAAAA AAAAAIANPQALnD6FACgAAAo="
+).encode("ascii")
 
 
 PKGDEB_VERSION_RELATIONS = (
@@ -265,7 +277,8 @@
     "AAAACAy/sAwTtOtwAoAABkYXRhLnRhci5neiAgICAgMTMxODUxNjQ5OCAgMCAg ICAgMCA"
     "gICAgMTAwNjQ0ICAxMDcgICAgICAgYAofiwgAAAAAAAID7cqxEcIwEETRK0UVgCT7UD0Q "
     "EpgZA/0DATNEEOHoveQHu7t9/F19GpmvtpH1s2/R2mGeemYfc9RW+9SjZGzgfr0d11LidL"
-    "6sy5ff rx0AAAAAAAAAAAA29AD/ixlwACgAAAo=")
+    "6sy5ff rx0AAAAAAAAAAAA29AD/ixlwACgAAAo="
+).encode("ascii")
 
 
 PKGDEB_MULTIPLE_RELATIONS = (
@@ -282,7 +295,8 @@
     "0YS50YXIuZ3ogICAgIDEzMTg1ODAwNzkgIDAgICAgIDAgICAgIDEwMDY0NCAgMTA3ICAg "
     "ICAgIGAKH4sIAAAAAAACA+3KsRHCMBBE0StFFYBkfFY9EBKYGWP3DwTMEEGEo/eSH+wejv"
     "F39aln vtp61s++RWvTeBpy6tmjtjqMLUrGDrb7el5Kicv1tsxffr92AAAAAAAAAAAA2NE"
-    "Db6L1AQAoAAAK")
+    "Db6L1AQAoAAAK"
+).encode("ascii")
 
 
 PKGDEB_OR_RELATIONS = (
@@ -299,7 +313,8 @@
     "6ICAgICAxMzE3ODg4ODY5ICAwICAgICAwICAgICAxMDA2NDQgIDEwNyAgICAgICBgCh+L "
     "CAAAAAAAAgPtyrsRwjAURNFXiioAfZBcjwkJzIyB/oGAGSIc4eic5Aa7h2P8XX6Zen+3TD"
     "1/9yNK"
-    "GadWR2ltRC651hGpxw4et/u8phTny3Vdfvy2dgAAAAAAAAAAANjRE6Lr2rEAKAAACg==")
+    "GadWR2ltRC651hGpxw4et/u8phTny3Vdfvy2dgAAAAAAAAAAANjRE6Lr2rEAKAAACg=="
+).encode("ascii")
 
 
 HASH1 = base64.decodestring(b"/ezv4AefpJJ8DuYFSq4RiEHJYP4=")

=== modified file 'landscape/package/tests/test_reporter.py'
--- landscape/package/tests/test_reporter.py	2017-03-20 09:43:08 +0000
+++ landscape/package/tests/test_reporter.py	2017-03-21 17:01:15 +0000
@@ -27,7 +27,6 @@
     LandscapeTest, BrokerServiceHelper, EnvironSaverHelper)
 from landscape.reactor import FakeReactor
 
-from landscape.compat import convert_buffer_to_string
 
 SAMPLE_LSB_RELEASE = "DISTRIB_CODENAME=codename\n"
 
@@ -96,21 +95,21 @@
         os.chmod(self.reporter.apt_update_filename, 0o755)
 
     def test_set_package_ids_with_all_known(self):
-        self.store.add_hash_id_request(["hash1", "hash2"])
-        request2 = self.store.add_hash_id_request(["hash3", "hash4"])
-        self.store.add_hash_id_request(["hash5", "hash6"])
+        self.store.add_hash_id_request([b"hash1", b"hash2"])
+        request2 = self.store.add_hash_id_request([b"hash3", b"hash4"])
+        self.store.add_hash_id_request([b"hash5", b"hash6"])
 
         self.store.add_task("reporter",
                             {"type": "package-ids", "ids": [123, 456],
                              "request-id": request2.id})
 
         def got_result(result):
-            self.assertEqual(self.store.get_hash_id("hash1"), None)
-            self.assertEqual(self.store.get_hash_id("hash2"), None)
-            self.assertEqual(self.store.get_hash_id("hash3"), 123)
-            self.assertEqual(self.store.get_hash_id("hash4"), 456)
-            self.assertEqual(self.store.get_hash_id("hash5"), None)
-            self.assertEqual(self.store.get_hash_id("hash6"), None)
+            self.assertEqual(self.store.get_hash_id(b"hash1"), None)
+            self.assertEqual(self.store.get_hash_id(b"hash2"), None)
+            self.assertEqual(self.store.get_hash_id(b"hash3"), 123)
+            self.assertEqual(self.store.get_hash_id(b"hash4"), 456)
+            self.assertEqual(self.store.get_hash_id(b"hash5"), None)
+            self.assertEqual(self.store.get_hash_id(b"hash6"), None)
 
         deferred = self.reporter.handle_tasks()
         return deferred.addCallback(got_result)
@@ -129,7 +128,7 @@
 
         message_store.set_accepted_types(["add-packages"])
 
-        request1 = self.store.add_hash_id_request(["foo", HASH1, "bar"])
+        request1 = self.store.add_hash_id_request([b"foo", HASH1, b"bar"])
 
         self.store.add_task("reporter",
                             {"type": "package-ids",
@@ -184,7 +183,7 @@
 
         message_store.set_accepted_types(["add-packages"])
 
-        request1 = self.store.add_hash_id_request(["foo", HASH1, "bar"])
+        request1 = self.store.add_hash_id_request([b"foo", HASH1, b"bar"])
 
         self.store.add_task("reporter",
                             {"type": "package-ids",
@@ -238,7 +237,7 @@
         deferred = Deferred()
         deferred.errback(Boom())
 
-        request_id = self.store.add_hash_id_request(["foo", HASH1, "bar"]).id
+        request_id = self.store.add_hash_id_request([b"foo", HASH1, b"bar"]).id
 
         self.store.add_task("reporter", {"type": "package-ids",
                                          "ids": [123, None, 456],
@@ -259,7 +258,7 @@
             return result.addCallback(got_result, send_mock)
 
     def test_set_package_ids_removes_request_id_when_done(self):
-        request = self.store.add_hash_id_request(["hash1"])
+        request = self.store.add_hash_id_request([b"hash1"])
         self.store.add_task("reporter", {"type": "package-ids", "ids": [123],
                                          "request-id": request.id})
 
@@ -562,7 +561,7 @@
         self.assertTrue(self.reporter._apt_sources_have_changed())
 
     def test_remove_expired_hash_id_request(self):
-        request = self.store.add_hash_id_request(["hash1"])
+        request = self.store.add_hash_id_request([b"hash1"])
         request.message_id = 9999
 
         request.timestamp -= HASH_ID_REQUEST_TIMEOUT
@@ -575,7 +574,7 @@
         return result.addCallback(got_result)
 
     def test_remove_expired_hash_id_request_wont_remove_before_timeout(self):
-        request1 = self.store.add_hash_id_request(["hash1"])
+        request1 = self.store.add_hash_id_request([b"hash1"])
         request1.message_id = 9999
         request1.timestamp -= HASH_ID_REQUEST_TIMEOUT / 2
 
@@ -592,7 +591,7 @@
         return result.addCallback(got_result)
 
     def test_remove_expired_hash_id_request_updates_timestamps(self):
-        request = self.store.add_hash_id_request(["hash1"])
+        request = self.store.add_hash_id_request([b"hash1"])
         message_store = self.broker_service.message_store
         message_id = message_store.add({"type": "add-packages",
                                         "packages": [],
@@ -607,7 +606,7 @@
         return result.addCallback(got_result)
 
     def test_remove_expired_hash_id_request_removes_when_no_message_id(self):
-        request = self.store.add_hash_id_request(["hash1"])
+        request = self.store.add_hash_id_request([b"hash1"])
 
         def got_result(result):
             self.assertRaises(UnknownHashIDRequest,
@@ -1305,9 +1304,9 @@
         spawn_patcher = mock.patch.object(reporter, "spawn_process",
             side_effect=[
                 # Simulate series of failures to acquire the apt lock.
-                succeed(('', '', 100)),
-                succeed(('', '', 100)),
-                succeed(('', '', 100))])
+                succeed((b'', b'', 100)),
+                succeed((b'', b'', 100)),
+                succeed((b'', b'', 100))])
         spawn_patcher.start()
         self.addCleanup(spawn_patcher.stop)
 
@@ -1343,8 +1342,8 @@
         spawn_patcher = mock.patch.object(reporter, "spawn_process",
             side_effect=[
                 # Simulate a failed apt lock grab then a successful one.
-                succeed(('', '', 100)),
-                succeed(('output', 'error', 0))])
+                succeed((b'', b'', 100)),
+                succeed((b'output', b'error', 0))])
         spawn_patcher.start()
         self.addCleanup(spawn_patcher.stop)
 
@@ -1628,7 +1627,7 @@
         return deferred
 
     @mock.patch("landscape.package.reporter.spawn_process",
-                return_value=succeed(("", "", 0)))
+                return_value=succeed((b"", b"", 0)))
     def test_run_apt_update_honors_http_proxy(self, mock_spawn_process):
         """
         The PackageReporter.run_apt_update method honors the http_proxy
@@ -1647,7 +1646,7 @@
             env={"http_proxy": "http://proxy_server:8080"})
 
     @mock.patch("landscape.package.reporter.spawn_process",
-                return_value=succeed(("", "", 0)))
+                return_value=succeed((b"", b"", 0)))
     def test_run_apt_update_honors_https_proxy(self, mock_spawn_process):
         """
         The PackageReporter.run_apt_update method honors the https_proxy
@@ -1869,8 +1868,7 @@
                     "SELECT id, data FROM message").fetchall())
                 self.assertEqual(1, len(stored))
                 self.assertEqual(1, stored[0][0])
-                self.assertEqual(message,
-                    bpickle.loads(convert_buffer_to_string(stored[0][1])))
+                self.assertEqual(message, bpickle.loads(bytes(stored[0][1])))
             result.addCallback(callback)
             result.chainDeferred(deferred)
 

=== modified file 'landscape/package/tests/test_store.py'
--- landscape/package/tests/test_store.py	2017-01-09 14:29:54 +0000
+++ landscape/package/tests/test_store.py	2017-03-21 17:01:15 +0000
@@ -19,12 +19,12 @@
         self.store2 = HashIdStore(self.filename)
 
     def test_set_and_get_hash_id(self):
-        self.store1.set_hash_ids({"ha\x00sh1": 123, "ha\x00sh2": 456})
-        self.assertEqual(self.store1.get_hash_id("ha\x00sh1"), 123)
-        self.assertEqual(self.store1.get_hash_id("ha\x00sh2"), 456)
+        self.store1.set_hash_ids({b"ha\x00sh1": 123, b"ha\x00sh2": 456})
+        self.assertEqual(self.store1.get_hash_id(b"ha\x00sh1"), 123)
+        self.assertEqual(self.store1.get_hash_id(b"ha\x00sh2"), 456)
 
     def test_get_hash_ids(self):
-        hash_ids = {"hash1": 123, "hash2": 456}
+        hash_ids = {b"hash1": 123, b"hash2": 456}
         self.store1.set_hash_ids(hash_ids)
         self.assertEqual(self.store1.get_hash_ids(), hash_ids)
 
@@ -80,33 +80,33 @@
         self.assertEqual([None], rollbacks)
 
     def test_get_id_hash(self):
-        self.store1.set_hash_ids({"hash1": 123, "hash2": 456})
-        self.assertEqual(self.store2.get_id_hash(123), "hash1")
-        self.assertEqual(self.store2.get_id_hash(456), "hash2")
+        self.store1.set_hash_ids({b"hash1": 123, b"hash2": 456})
+        self.assertEqual(self.store2.get_id_hash(123), b"hash1")
+        self.assertEqual(self.store2.get_id_hash(456), b"hash2")
 
     def test_clear_hash_ids(self):
-        self.store1.set_hash_ids({"ha\x00sh1": 123, "ha\x00sh2": 456})
+        self.store1.set_hash_ids({b"ha\x00sh1": 123, b"ha\x00sh2": 456})
         self.store1.clear_hash_ids()
-        self.assertEqual(self.store2.get_hash_id("ha\x00sh1"), None)
-        self.assertEqual(self.store2.get_hash_id("ha\x00sh2"), None)
+        self.assertEqual(self.store2.get_hash_id(b"ha\x00sh1"), None)
+        self.assertEqual(self.store2.get_hash_id(b"ha\x00sh2"), None)
 
     def test_get_unexistent_hash(self):
-        self.assertEqual(self.store1.get_hash_id("hash1"), None)
+        self.assertEqual(self.store1.get_hash_id(b"hash1"), None)
 
     def test_get_unexistent_id(self):
         self.assertEqual(self.store1.get_id_hash(123), None)
 
     def test_overwrite_id_hash(self):
-        self.store1.set_hash_ids({"hash1": 123})
-        self.store2.set_hash_ids({"hash2": 123})
-        self.assertEqual(self.store1.get_hash_id("hash1"), None)
-        self.assertEqual(self.store1.get_hash_id("hash2"), 123)
+        self.store1.set_hash_ids({b"hash1": 123})
+        self.store2.set_hash_ids({b"hash2": 123})
+        self.assertEqual(self.store1.get_hash_id(b"hash1"), None)
+        self.assertEqual(self.store1.get_hash_id(b"hash2"), 123)
 
     def test_overwrite_hash_id(self):
-        self.store1.set_hash_ids({"hash1": 123})
-        self.store2.set_hash_ids({"hash1": 456})
+        self.store1.set_hash_ids({b"hash1": 123})
+        self.store2.set_hash_ids({b"hash1": 456})
         self.assertEqual(self.store1.get_id_hash(123), None)
-        self.assertEqual(self.store1.get_id_hash(456), "hash1")
+        self.assertEqual(self.store1.get_id_hash(456), b"hash1")
 
     def test_check_sanity(self):
 
@@ -184,22 +184,22 @@
 
     def test_get_hash_id_using_hash_id_dbs(self):
         # Without hash=>id dbs
-        self.assertEqual(self.store1.get_hash_id("hash1"), None)
-        self.assertEqual(self.store1.get_hash_id("hash2"), None)
+        self.assertEqual(self.store1.get_hash_id(b"hash1"), None)
+        self.assertEqual(self.store1.get_hash_id(b"hash2"), None)
 
         # This hash=>id will be overriden
-        self.store1.set_hash_ids({"hash1": 1})
+        self.store1.set_hash_ids({b"hash1": 1})
 
         # Add a couple of hash=>id dbs
-        self.store1.add_hash_id_db(self.hash_id_db_factory({"hash1": 2,
-                                                            "hash2": 3}))
-        self.store1.add_hash_id_db(self.hash_id_db_factory({"hash2": 4,
-                                                            "ha\x00sh1": 5}))
+        self.store1.add_hash_id_db(self.hash_id_db_factory({b"hash1": 2,
+                                                            b"hash2": 3}))
+        self.store1.add_hash_id_db(self.hash_id_db_factory({b"hash2": 4,
+                                                            b"ha\x00sh1": 5}))
 
         # Check look-up priorities and binary hashes
-        self.assertEqual(self.store1.get_hash_id("hash1"), 2)
-        self.assertEqual(self.store1.get_hash_id("hash2"), 3)
-        self.assertEqual(self.store1.get_hash_id("ha\x00sh1"), 5)
+        self.assertEqual(self.store1.get_hash_id(b"hash1"), 2)
+        self.assertEqual(self.store1.get_hash_id(b"hash2"), 3)
+        self.assertEqual(self.store1.get_hash_id(b"ha\x00sh1"), 5)
 
     def test_get_id_hash_using_hash_id_db(self):
         """
@@ -207,13 +207,13 @@
         to query them first, falling back to the regular db in case
         the desired mapping is not found.
         """
-        self.store1.add_hash_id_db(self.hash_id_db_factory({"hash1": 123}))
-        self.store1.add_hash_id_db(self.hash_id_db_factory({"hash1": 999,
-                                                            "hash2": 456}))
-        self.store1.set_hash_ids({"hash3": 789})
-        self.assertEqual(self.store1.get_id_hash(123), "hash1")
-        self.assertEqual(self.store1.get_id_hash(456), "hash2")
-        self.assertEqual(self.store1.get_id_hash(789), "hash3")
+        self.store1.add_hash_id_db(self.hash_id_db_factory({b"hash1": 123}))
+        self.store1.add_hash_id_db(self.hash_id_db_factory({b"hash1": 999,
+                                                            b"hash2": 456}))
+        self.store1.set_hash_ids({b"hash3": 789})
+        self.assertEqual(self.store1.get_id_hash(123), b"hash1")
+        self.assertEqual(self.store1.get_id_hash(456), b"hash2")
+        self.assertEqual(self.store1.get_id_hash(789), b"hash3")
 
     def test_add_and_get_available_packages(self):
         self.store1.add_available([1, 2])

=== modified file 'landscape/schema.py'
--- landscape/schema.py	2017-03-10 12:40:17 +0000
+++ landscape/schema.py	2017-03-21 17:01:15 +0000
@@ -67,8 +67,8 @@
 class Bytes(object):
     """A binary string."""
     def coerce(self, value):
-        if not isinstance(value, str):
-            raise InvalidError("%r isn't a str" % (value,))
+        if not isinstance(value, bytes):
+            raise InvalidError("%r isn't a bytestring" % (value,))
         return value
 
 

=== modified file 'py3_ready_tests'
--- py3_ready_tests	2017-03-15 08:40:11 +0000
+++ py3_ready_tests	2017-03-21 17:01:15 +0000
@@ -1,2 +1,5 @@
 landscape.lib.tests
 landscape.sysinfo.tests
+landscape.package.tests.test_store
+landscape.package.tests.test_reporter
+