Merge lp:~gocept/landscape-client/py3-twisted-util into lp:~landscape/landscape-client/trunk

[Steffen Allner]

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

This MP considers the part of code which receives data from the stdout and stderr and therefore handles with Bytes and not with Stings in Python 3. To reduce the amount of compat code, the code has been changed to use the same `io.BytesIO()` in Python 2 and 3.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Fail
Revno: 959
Branch: lp:~gocept/landscape-client/py3-twisted-util
Jenkins: https://ci.lscape.net/job/latch-test-precise/887/

review: Needs Fixing (test results)

[Adam Collard (adam-collard)]

review: Abstain

[David Britton (dpb)]

Hi Daniel -- Please add 'landscape' on these in the future. Otherwise only Adam will be able to review. :)

I've taken care of this one.

Thanks!

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

I've just noticed that the dependency branch is still missing the second Landscape dev reviewer: I've added myself there and will be reviewing that first. We also require Adam to approve the changes there too.

I'll get back to this after I am done with that branch.

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

FWIW, you should fix the test failure as well: http://paste.ubuntu.com/24136946/

(test builder is still running on Ubuntu 12.04 [precise], so maybe that's to blame, but I haven't looked deeper: if it's easy, it's nice to get a fix for that too, but if not, we can probably move to supporting only 14.04 and later since 12.04 is EOLin this April)

[Steffen Allner (sallner)]

I am currently developing on an Ubuntu 16.04.1 and cannot reproduce this test failure. But I agree, we should at least test it on 14.04.

[Steffen Allner (sallner)]

I am actually surprised that there should be only one test failure. I tried to test it on an Ubuntu 12.04. and 14.04. and failed both times. For 12.04 I already failed to install bzr as python-bzrlib depends on python-configobj, which in turn is a virtual-package now and did not install anything.

On 14.04. executing the tests led to some ImportErrors out of `twisted.python.compat` as some of the compat code we took is not available in twisted 13.2 (which is in the repos in 14.04.). For 12.04. twisted 11.1 is in the repos with even less compat code. So this is a bit unclear to me. Or is the landscape-builder using any kind of virtualenv to do the testing?

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

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

Yeah, we are definitely missing some development setup bits for landscape-client: I've done an "apt-get build-dep landscape-client", and as you noted, bzr was still missing, but also python-mock and xvfb. xvfb is probably not really needed anymore with our GUI component retired, but I'll need to double check that. However, the problem with python-mock on 12.04 is that it seems to be very old (0.7.2) and the tests already use features from a newer version (1.0.1, eg. by adding "deb http://ubuntu-cloud.archive.canonical.com/ubuntu precise-updates/cloud-tools main" to your /etc/apt/sources.list). Updating python-twisted-core to the version from that archive (13.02) also made trial accept -j4 parameter so "make check" fully passed for me. Perhaps the failed test is failing intermittently: if so, I'll file a separate bug about that (retrying the test run right now).

I'll now look if 14.04 as-is has all the deps needed at sufficiently new versions and update our infrastructure to run the tests on that instead.

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 959
Branch: lp:~gocept/landscape-client/py3-twisted-util
Jenkins: https://ci.lscape.net/job/latch-test-precise/893/

review: Approve (test results)

[Steffen Allner (sallner)]

At least that one worked. Thanks, Danilo! Apart from that, I recall that we agreed on compatibility with 16.04. for the new version of landscape client. So if those problems persist or occur more frequently, we have to find a different mode.

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

Righto: please do focus on what the agreement specifies, we can deal with compatibility in that case (sorry if I overstepped).

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 959
Branch: lp:~gocept/landscape-client/py3-twisted-util
Jenkins: https://ci.lscape.net/job/latch-test-xenial/1458/

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: 959
Branch: lp:~gocept/landscape-client/py3-twisted-util
Jenkins: https://ci.lscape.net/job/latch-test-xenial/1459/

review: Approve (test results)

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

Looks good, +1 (we do need to wait for the prereq to land, of course)

review: Approve

[Daniel Havlik (nilo)]

+1

review: Approve

[Steffen Allner (sallner)]

If we merge the py3-fs branch first, we will get some failing tests. So I will merge py3-fs into this branch and update the proposal to have it as prerequisite.

review: Needs Fixing

[lp:~gocept/landscape-client/py3-twisted-util]

960. By Steffen Allner on 2017-03-13

Backmerge from trunk.

961. By Steffen Allner on 2017-03-13

Merge landscape.lib.fs with optional encoding.

962. By Steffen Allner on 2017-03-13

Use bytes here as we do not have non-ascii characters in the scripts.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 962
Branch: lp:~gocept/landscape-client/py3-twisted-util
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3605/

review: Approve (test results)

[lp:~gocept/landscape-client/py3-twisted-util]

963. By Steffen Allner on 2017-03-14

Merge in current l.l.fs and use create_text_file().

Unmerged revisions

=== modified file 'landscape/broker/tests/test_store.py'
--- landscape/broker/tests/test_store.py	2016-08-17 20:31:48 +0000
+++ landscape/broker/tests/test_store.py	2017-03-13 10:45:51 +0000
@@ -280,7 +280,7 @@
         self.store.add({"type": "data", "data": 1})
         # We simulate it by creating a fake file which raises halfway through
         # writing a file.
-        with mock.patch("__builtin__.open") as mock_open:
+        with mock.patch("landscape.lib.fs.open") as mock_open:
             mocked_file = mock_open.return_value
             mocked_file.write.side_effect = IOError("Sorry, pal!")
             # This kind of ensures that raising an exception is somewhat
@@ -288,7 +288,7 @@
             # on special exception-handling in the file-writing code.
             self.assertRaises(
                 IOError, self.store.add, {"type": "data", "data": 2})
-            mock_open.assert_called_with(mock.ANY, "w")
+            mock_open.assert_called_with(mock.ANY, "wb")
             mocked_file.write.assert_called_once_with(mock.ANY)
         self.assertEqual(self.store.get_pending_messages(),
                          [{"type": "data", "data": 1, "api": "3.2"}])

=== modified file 'landscape/lib/fs.py'
--- landscape/lib/fs.py	2013-05-28 08:48:31 +0000
+++ landscape/lib/fs.py	2017-03-13 10:45:51 +0000
@@ -1,18 +1,31 @@
 """File-system utils"""
 
+import codecs
 import os
 import time
 
 
-def create_file(path, content):
+from twisted.python.compat import long
+
+
+def create_file(path, content, encoding=None):
     """Create a file with the given content.
 
     @param path: The path to the file.
     @param content: The content to be written in the file.
+    @param encoding: An optional encoding. If set, the content will be encoded
+        with C{encoding} before writing to the file.
     """
-    fd = open(path, "w")
-    fd.write(content)
-    fd.close()
+    if encoding:
+        content = codecs.encode(content, encoding)
+    # XXX: Due to a very specific mock of `open()` in landscape.broker.tests.\
+    # test_store.MessageStoreTest.test_atomic_message_writing it is hard to
+    # write this file opening as context manager.
+    fd = open(path, "wb")
+    try:
+        fd.write(content)
+    finally:
+        fd.close()
 
 
 def append_file(path, content):
@@ -23,28 +36,33 @@
     @param path: The path to the file.
     @param content: The content to be written in the file at the end.
     """
-    fd = open(path, "a")
-    fd.write(content)
-    fd.close()
-
-
-def read_file(path, limit=None):
+    with open(path, "a") as fd:
+        fd.write(content)
+
+
+def read_file(path, limit=None, encoding=None):
     """Return the content of the given file.
 
     @param path: The path to the file.
     @param limit: An optional read limit. If positive, read up to that number
         of bytes from the beginning of the file. If negative, read up to that
         number of bytes from the end of the file.
-    @return content: The content of the file, possibly trimmed to C{limit}.
+    @param encoding: An optional encoding. If set, the content will be returned
+        decoded with C{encoding}.
+    @return content: The content of the file as bytes or unicode string
+        (depending on C{encoding}), possibly trimmed to C{limit}.
     """
-    fd = open(path, "r")
-    if limit and os.path.getsize(path) > abs(limit):
-        whence = 0
-        if limit < 0:
-            whence = 2
-        fd.seek(limit, whence)
-    content = fd.read()
-    fd.close()
+    # Use binary mode since opening a file in text mode in Python 3 does not
+    # allow non-zero offset seek from the end of the file.
+    with open(path, "rb") as fd:
+        if limit and os.path.getsize(path) > abs(limit):
+            whence = 0
+            if limit < 0:
+                whence = 2
+            fd.seek(limit, whence)
+        content = fd.read()
+    if encoding:
+        content = codecs.decode(content, encoding)
     return content
 
 

=== modified file 'landscape/lib/juju.py'
--- landscape/lib/juju.py	2014-09-24 08:09:55 +0000
+++ landscape/lib/juju.py	2017-03-13 10:45:51 +0000
@@ -13,7 +13,7 @@
     if not os.path.exists(config.juju_filename):
         return
 
-    json_contents = read_file(config.juju_filename)
+    json_contents = read_file(config.juju_filename, encoding="utf-8")
     try:
         juju_info = json.loads(json_contents)
     # Catch any error the json lib could throw, because we don't know or

=== modified file 'landscape/lib/tests/test_fs.py'
--- landscape/lib/tests/test_fs.py	2016-06-15 16:42:17 +0000
+++ landscape/lib/tests/test_fs.py	2017-03-13 10:45:51 +0000
@@ -1,7 +1,11 @@
+# -*- coding: utf-8 -*-
+import codecs
 import os
 from mock import patch
 import time
 
+from twisted.python.compat import long
+
 from landscape.tests.helpers import LandscapeTest
 
 from landscape.lib.fs import append_file, read_file, touch_file
@@ -14,7 +18,16 @@
         With no options L{read_file} reads the whole file passed as argument.
         """
         path = self.makeFile("foo")
-        self.assertEqual(read_file(path), "foo")
+        self.assertEqual(read_file(path), b"foo")
+
+    def test_read_file_encoding(self):
+        """
+        With encoding L{read_file} reads the whole file passed as argument and
+        decodes it.
+        """
+        utf8_content = codecs.encode(u"foo \N{SNOWMAN}", "utf-8")
+        path = self.makeFile(utf8_content, mode="wb")
+        self.assertEqual(read_file(path, encoding="utf-8"), u"foo ☃")
 
     def test_read_file_with_limit(self):
         """
@@ -22,14 +35,23 @@
         given limit.
         """
         path = self.makeFile("foo bar")
-        self.assertEqual(read_file(path, limit=3), " bar")
+        self.assertEqual(read_file(path, limit=3), b" bar")
+
+    def test_read_file_with_limit_encoding(self):
+        """
+        With a positive limit L{read_file} reads only the bytes after the
+        given limit.
+        """
+        utf8_content = codecs.encode(u"foo \N{SNOWMAN}", "utf-8")
+        path = self.makeFile(utf8_content, mode="wb")
+        self.assertEqual(read_file(path, limit=3, encoding="utf-8"), u" ☃")
 
     def test_read_file_with_negative_limit(self):
         """
         With a negative limit L{read_file} reads only the tail of the file.
         """
         path = self.makeFile("foo bar from end")
-        self.assertEqual(read_file(path, limit=-3), "end")
+        self.assertEqual(read_file(path, limit=-3), b"end")
 
     def test_read_file_with_limit_bigger_than_file(self):
         """
@@ -37,8 +59,8 @@
         file.
         """
         path = self.makeFile("foo bar from end")
-        self.assertEqual(read_file(path, limit=100), "foo bar from end")
-        self.assertEqual(read_file(path, limit=-100), "foo bar from end")
+        self.assertEqual(read_file(path, limit=100), b"foo bar from end")
+        self.assertEqual(read_file(path, limit=-100), b"foo bar from end")
 
 
 class TouchFileTest(LandscapeTest):

=== modified file 'landscape/lib/tests/test_process.py'
--- landscape/lib/tests/test_process.py	2017-03-09 10:13:30 +0000
+++ landscape/lib/tests/test_process.py	2017-03-13 10:45:51 +0000
@@ -25,7 +25,8 @@
         os.mkdir(process_dir)
 
         cmd_line = "/usr/bin/foo"
-        create_file(os.path.join(process_dir, "cmdline"), cmd_line)
+        create_file(
+            os.path.join(process_dir, "cmdline"), cmd_line, encoding="utf-8")
 
         status = "\n".join([
             "Name: foo",
@@ -34,11 +35,12 @@
             "Gid: 2000",
             "VmSize: 3000",
             "Ignored: value"])
-        create_file(os.path.join(process_dir, "status"), status)
+        create_file(
+            os.path.join(process_dir, "status"), status, encoding="utf-8")
 
         stat_array = [str(index) for index in range(44)]
         stat = " ".join(stat_array)
-        create_file(os.path.join(process_dir, "stat"), stat)
+        create_file(os.path.join(process_dir, "stat"), stat, encoding="utf-8")
 
     @mock.patch("landscape.lib.process.detect_jiffies", return_value=1)
     @mock.patch("os.listdir")

=== modified file 'landscape/lib/tests/test_twisted_util.py'
--- landscape/lib/tests/test_twisted_util.py	2017-01-23 12:14:33 +0000
+++ landscape/lib/tests/test_twisted_util.py	2017-03-13 10:45:51 +0000
@@ -1,8 +1,5 @@
 import os
 
-from twisted.python.compat import _PY3
-from unittest import skipIf
-
 from landscape.lib.fs import create_file
 from landscape.lib.twisted_util import spawn_process
 from landscape.tests.helpers import LandscapeTest
@@ -19,12 +16,12 @@
         """
         The process is executed and returns the expected exit code.
         """
-        create_file(self.command, "#!/bin/sh\nexit 2")
+        create_file(self.command, b"#!/bin/sh\nexit 2")
 
         def callback(args):
             out, err, code = args
-            self.assertEqual(out, "")
-            self.assertEqual(err, "")
+            self.assertEqual(out, b"")
+            self.assertEqual(err, b"")
             self.assertEqual(code, 2)
 
         result = spawn_process(self.command)
@@ -37,8 +34,8 @@
         """
         def callback(args):
             out, err, code = args
-            self.assertEqual(out, "a b")
-            self.assertEqual(err, "")
+            self.assertEqual(out, b"a b")
+            self.assertEqual(err, b"")
             self.assertEqual(code, 0)
 
         result = spawn_process(self.command, args=("a", "b"))
@@ -49,12 +46,12 @@
         """
         The process returns the expected standard error.
         """
-        create_file(self.command, "#!/bin/sh\necho -n $@ >&2")
+        create_file(self.command, b"#!/bin/sh\necho -n $@ >&2")
 
         def callback(args):
             out, err, code = args
-            self.assertEqual(out, "")
-            self.assertEqual(err, "a b")
+            self.assertEqual(out, b"")
+            self.assertEqual(err, b"a b")
             self.assertEqual(code, 0)
 
         result = spawn_process(self.command, args=("a", "b"))
@@ -66,9 +63,9 @@
         If a callback for process output is provieded, it is called for every
         line of output.
         """
-        create_file(self.command, "#!/bin/sh\n/bin/echo -ne $@")
+        create_file(self.command, b"#!/bin/sh\n/bin/echo -ne $@")
         param = r"some text\nanother line\nok, last one\n"
-        expected = ["some text", "another line", "ok, last one"]
+        expected = [b"some text", b"another line", b"ok, last one"]
         lines = []
 
         def line_received(line):
@@ -87,9 +84,9 @@
         """
         If output ends with more than one newline, empty lines are preserved.
         """
-        create_file(self.command, "#!/bin/sh\n/bin/echo -ne $@")
+        create_file(self.command, b"#!/bin/sh\n/bin/echo -ne $@")
         param = r"some text\nanother line\n\n\n"
-        expected = ["some text", "another line", "", ""]
+        expected = [b"some text", b"another line", b"", b""]
         lines = []
 
         def line_received(line):
@@ -109,9 +106,9 @@
         If output ends without a newline, the line is still passed to the
         callback.
         """
-        create_file(self.command, "#!/bin/sh\n/bin/echo -ne $@")
+        create_file(self.command, b"#!/bin/sh\n/bin/echo -ne $@")
         param = r"some text\nanother line\nok, last one"
-        expected = ["some text", "another line", "ok, last one"]
+        expected = [b"some text", b"another line", b"ok, last one"]
         lines = []
 
         def line_received(line):
@@ -126,16 +123,15 @@
         result.addCallback(callback)
         return result
 
-    @skipIf(_PY3, 'Takes long with Python3, probably unclean Reactor')
     def test_spawn_process_with_stdin(self):
         """
         Optionally C{spawn_process} accepts a C{stdin} argument.
         """
-        create_file(self.command, "#!/bin/sh\n/bin/cat")
+        create_file(self.command, b"#!/bin/sh\n/bin/cat")
 
         def callback(args):
             out, err, code = args
-            self.assertEqual("hello", out)
+            self.assertEqual(b"hello", out)
 
         result = spawn_process(self.command, stdin="hello")
         result.addCallback(callback)

=== modified file 'landscape/lib/twisted_util.py'
--- landscape/lib/twisted_util.py	2017-01-10 16:12:53 +0000
+++ landscape/lib/twisted_util.py	2017-03-13 10:45:51 +0000
@@ -1,10 +1,10 @@
+import io
+
 from twisted.internet.defer import DeferredList, Deferred
 from twisted.internet.protocol import ProcessProtocol
 from twisted.internet.process import Process, ProcessReader
 from twisted.internet import reactor
-from twisted.python.compat import itervalues
-
-from landscape.compat import StringIO
+from twisted.python.compat import itervalues, networkString
 
 
 def gather_results(deferreds, consume_errors=False):
@@ -20,16 +20,16 @@
 
     def __init__(self, deferred, stdin=None, line_received=None):
         self.deferred = deferred
-        self.outBuf = StringIO()
-        self.errBuf = StringIO()
+        self.outBuf = io.BytesIO()
+        self.errBuf = io.BytesIO()
         self.errReceived = self.errBuf.write
         self.stdin = stdin
         self.line_received = line_received
-        self._partial_line = ""
+        self._partial_line = b""
 
     def connectionMade(self):
         if self.stdin is not None:
-            self.transport.write(self.stdin)
+            self.transport.write(networkString(self.stdin))
             self.transport.closeStdin()
 
     def outReceived(self, data):
@@ -41,7 +41,7 @@
         # data may contain more than one line, so we split the output and save
         # the last line. If it's an empty string nothing happens, otherwise it
         # will be returned once complete
-        lines = data.split("\n")
+        lines = data.split(b"\n")
         lines[0] = self._partial_line + lines[0]
         self._partial_line = lines.pop()
 
@@ -51,7 +51,7 @@
     def processEnded(self, reason):
         if self._partial_line:
             self.line_received(self._partial_line)
-            self._partial_line = ""
+            self._partial_line = b""
         out = self.outBuf.getvalue()
         err = self.errBuf.getvalue()
         e = reason.value

=== modified file 'landscape/lib/vm_info.py'
--- landscape/lib/vm_info.py	2016-09-23 15:43:36 +0000
+++ landscape/lib/vm_info.py	2017-03-13 10:45:51 +0000
@@ -34,7 +34,7 @@
     for filename in ("container_type", "systemd/container"):
         path = os.path.join(run_path, filename)
         if os.path.exists(path):
-            return read_file(path).strip()
+            return read_file(path, encoding="utf-8").strip()
     return ""
 
 
@@ -52,7 +52,7 @@
 
 def _get_vm_by_vendor(sys_vendor_path):
     """Return the VM type string (possibly empty) based on the vendor."""
-    vendor = read_file(sys_vendor_path).lower()
+    vendor = read_file(sys_vendor_path, encoding="utf-8").lower()
     # Use lower-key string for vendors, since we do case-insentive match.
     content_vendors_map = (
         ("bochs", "kvm"),
@@ -72,7 +72,8 @@
 def _get_vm_legacy(root_path):
     """Check if the host is virtualized looking at /proc/cpuinfo content."""
     try:
-        cpuinfo = read_file(os.path.join(root_path, "proc/cpuinfo"))
+        cpuinfo = read_file(
+            os.path.join(root_path, "proc/cpuinfo"), encoding="utf-8")
     except (IOError, OSError):
         return ""