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

[Steffen Allner]

Support python 2 and 3 in 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.

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

[🤖 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)

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[Steffen Allner (sallner)]

I updated the tests of landscape.lib.twisted_util to work with the optional encoding of landscape.lib.fs.

review: Approve

[🤖 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/3606/

review: Approve (test results)

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

+1

review: Approve

[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().

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

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

review: Approve (test results)

[Daniel Havlik (nilo)]

+1

review: Approve

=== modified file 'landscape/lib/tests/test_twisted_util.py'
--- landscape/lib/tests/test_twisted_util.py	2017-03-14 09:54:21 +0000
+++ landscape/lib/tests/test_twisted_util.py	2017-03-14 09:54:21 +0000
@@ -1,8 +1,5 @@
 import os
 
-from twisted.python.compat import _PY3
-from unittest import skipIf
-
 from landscape.lib.fs import create_text_file
 from landscape.lib.twisted_util import spawn_process
 from landscape.tests.helpers import LandscapeTest
@@ -23,8 +20,8 @@
 
         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"))
@@ -53,8 +50,8 @@
 
         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"))
@@ -68,7 +65,7 @@
         """
         create_text_file(self.command, "#!/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):
@@ -89,7 +86,7 @@
         """
         create_text_file(self.command, "#!/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):
@@ -111,7 +108,7 @@
         """
         create_text_file(self.command, "#!/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,7 +123,6 @@
         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.
@@ -135,7 +131,7 @@
 
         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-14 09:54:21 +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