launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #24690
[Merge] ~cjwatson/launchpad-buildd:py3-fix-lxd-run into launchpad-buildd:master
Colin Watson has proposed merging ~cjwatson/launchpad-buildd:py3-fix-lxd-run into launchpad-buildd:master.
Commit message:
Fix LXD.run to not default to universal_newlines=True
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/+git/launchpad-buildd/+merge/383594
This was confusingly different from subprocess.Popen's default, and it broke Backend.find in a way that wasn't detected by the test suite because FakeProcess doesn't check type-safety here. I've arranged for some more stringent testing so that we don't run into this category of bug in future.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad-buildd:py3-fix-lxd-run into launchpad-buildd:master.
diff --git a/debian/changelog b/debian/changelog
index 867b35e..fd35575 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -21,6 +21,7 @@ launchpad-buildd (190) UNRELEASED; urgency=medium
* Rename [slave] configuration section to [builder].
* Convert translation templates builds to the VCS mixin, thereby adding
git support.
+ * Fix LXD.run to not default to universal_newlines=True.
-- Colin Watson <cjwatson@xxxxxxxxxx> Tue, 28 Apr 2020 10:19:27 +0100
diff --git a/lpbuildd/oci.py b/lpbuildd/oci.py
index 0b56623..27d34f1 100644
--- a/lpbuildd/oci.py
+++ b/lpbuildd/oci.py
@@ -152,7 +152,7 @@ class OCIBuildManager(SnapBuildProxyMixin, DebianBuildManager):
extract_path = tempfile.mkdtemp(prefix=self.name)
proc = self.backend.run(
['docker', 'save', self.name],
- get_output=True, universal_newlines=False, return_process=True)
+ get_output=True, return_process=True)
try:
tar = tarfile.open(fileobj=proc.stdout, mode="r|")
except Exception as e:
diff --git a/lpbuildd/target/build_snap.py b/lpbuildd/target/build_snap.py
index 9bc1dad..50fab7a 100644
--- a/lpbuildd/target/build_snap.py
+++ b/lpbuildd/target/build_snap.py
@@ -178,7 +178,7 @@ class BuildSnap(SnapBuildProxyOperationMixin, VCSOperationMixin,
status["revision_id"] = self.run_build_command(
["bzr", "revno"],
cwd=os.path.join("/build", self.args.name),
- get_output=True).rstrip("\n")
+ get_output=True, universal_newlines=True).rstrip("\n")
else:
rev = (
self.args.git_path
@@ -188,7 +188,7 @@ class BuildSnap(SnapBuildProxyOperationMixin, VCSOperationMixin,
# recursively until we get an actual commit.
["git", "rev-parse", rev + "^{}"],
cwd=os.path.join("/build", self.args.name),
- get_output=True).rstrip("\n")
+ get_output=True, universal_newlines=True).rstrip("\n")
self.save_status(status)
@property
diff --git a/lpbuildd/target/chroot.py b/lpbuildd/target/chroot.py
index 54d48eb..203142c 100644
--- a/lpbuildd/target/chroot.py
+++ b/lpbuildd/target/chroot.py
@@ -96,7 +96,10 @@ class Chroot(Backend):
if get_output:
if echo:
print("Output:")
- print(output)
+ output_text = output
+ if isinstance(output_text, bytes):
+ output_text = output_text.decode("UTF-8", "replace")
+ print(output_text)
return output
def copy_in(self, source_path, target_path):
diff --git a/lpbuildd/target/lxd.py b/lpbuildd/target/lxd.py
index ed1002b..fcd6706 100644
--- a/lpbuildd/target/lxd.py
+++ b/lpbuildd/target/lxd.py
@@ -504,8 +504,7 @@ class LXD(Backend):
"/etc/systemd/system/snapd.refresh.timer"])
def run(self, args, cwd=None, env=None, input_text=None, get_output=False,
- echo=False, return_process=False, universal_newlines=True,
- **kwargs):
+ echo=False, return_process=False, **kwargs):
"""See `Backend`."""
env_params = []
if env:
@@ -538,9 +537,7 @@ class LXD(Backend):
else:
if get_output:
kwargs["stdout"] = subprocess.PIPE
- proc = subprocess.Popen(
- cmd, stdin=subprocess.PIPE,
- universal_newlines=universal_newlines, **kwargs)
+ proc = subprocess.Popen(cmd, stdin=subprocess.PIPE, **kwargs)
if return_process:
return proc
output, _ = proc.communicate(input_text)
@@ -549,7 +546,10 @@ class LXD(Backend):
if get_output:
if echo:
print("Output:")
- print(output)
+ output_text = output
+ if isinstance(output_text, bytes):
+ output_text = output_text.decode("UTF-8", "replace")
+ print(output_text)
return output
def copy_in(self, source_path, target_path):
diff --git a/lpbuildd/target/tests/test_build_snap.py b/lpbuildd/target/tests/test_build_snap.py
index 5f8952f..3e9f320 100644
--- a/lpbuildd/target/tests/test_build_snap.py
+++ b/lpbuildd/target/tests/test_build_snap.py
@@ -36,7 +36,7 @@ from lpbuildd.tests.fakebuilder import FakeMethod
class RanCommand(MatchesListwise):
def __init__(self, args, echo=None, cwd=None, input_text=None,
- get_output=None, **env):
+ get_output=None, universal_newlines=None, **env):
kwargs_matcher = {}
if echo is not None:
kwargs_matcher["echo"] = Is(echo)
@@ -46,6 +46,8 @@ class RanCommand(MatchesListwise):
kwargs_matcher["input_text"] = Equals(input_text)
if get_output is not None:
kwargs_matcher["get_output"] = Is(get_output)
+ if universal_newlines is not None:
+ kwargs_matcher["universal_newlines"] = Is(universal_newlines)
if env:
kwargs_matcher["env"] = MatchesDict(
{key: Equals(value) for key, value in env.items()})
@@ -230,7 +232,9 @@ class TestBuildSnap(TestCase):
RanBuildCommand(
["bzr", "branch", "lp:foo", "test-snap"], cwd="/build"),
RanBuildCommand(
- ["bzr", "revno"], cwd="/build/test-snap", get_output=True),
+ ["bzr", "revno"],
+ cwd="/build/test-snap", get_output=True,
+ universal_newlines=True),
]))
status_path = os.path.join(build_snap.backend.build_path, "status")
with open(status_path) as status:
@@ -254,7 +258,8 @@ class TestBuildSnap(TestCase):
cwd="/build/test-snap"),
RanBuildCommand(
["git", "rev-parse", "HEAD^{}"],
- cwd="/build/test-snap", get_output=True),
+ cwd="/build/test-snap",
+ get_output=True, universal_newlines=True),
]))
status_path = os.path.join(build_snap.backend.build_path, "status")
with open(status_path) as status:
@@ -279,7 +284,8 @@ class TestBuildSnap(TestCase):
cwd="/build/test-snap"),
RanBuildCommand(
["git", "rev-parse", "next^{}"],
- cwd="/build/test-snap", get_output=True),
+ cwd="/build/test-snap", get_output=True,
+ universal_newlines=True),
]))
status_path = os.path.join(build_snap.backend.build_path, "status")
with open(status_path) as status:
@@ -305,7 +311,8 @@ class TestBuildSnap(TestCase):
cwd="/build/test-snap"),
RanBuildCommand(
["git", "rev-parse", "refs/tags/1.0^{}"],
- cwd="/build/test-snap", get_output=True),
+ cwd="/build/test-snap", get_output=True,
+ universal_newlines=True),
]))
status_path = os.path.join(build_snap.backend.build_path, "status")
with open(status_path) as status:
@@ -336,7 +343,8 @@ class TestBuildSnap(TestCase):
cwd="/build/test-snap", **env),
RanBuildCommand(
["git", "rev-parse", "HEAD^{}"],
- cwd="/build/test-snap", get_output=True),
+ cwd="/build/test-snap", get_output=True,
+ universal_newlines=True),
]))
status_path = os.path.join(build_snap.backend.build_path, "status")
with open(status_path) as status:
diff --git a/lpbuildd/target/tests/test_chroot.py b/lpbuildd/target/tests/test_chroot.py
index 861a893..88e3137 100644
--- a/lpbuildd/target/tests/test_chroot.py
+++ b/lpbuildd/target/tests/test_chroot.py
@@ -24,6 +24,7 @@ from testtools.matchers import DirContains
from lpbuildd.target.backend import BackendException
from lpbuildd.target.chroot import Chroot
from lpbuildd.target.tests.testfixtures import (
+ CarefulFakeProcessFixture,
FakeFilesystem,
KillFixture,
SudoUmount,
@@ -32,6 +33,10 @@ from lpbuildd.target.tests.testfixtures import (
class TestChroot(TestCase):
+ def setUp(self):
+ super(TestChroot, self).setUp()
+ self.useFixture(CarefulFakeProcessFixture())
+
def test_create(self):
self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
processes_fixture = self.useFixture(FakeProcesses())
diff --git a/lpbuildd/target/tests/test_lxd.py b/lpbuildd/target/tests/test_lxd.py
index 77e7f77..145630b 100644
--- a/lpbuildd/target/tests/test_lxd.py
+++ b/lpbuildd/target/tests/test_lxd.py
@@ -49,6 +49,7 @@ from lpbuildd.target.lxd import (
fallback_hosts,
policy_rc_d,
)
+from lpbuildd.target.tests.testfixtures import CarefulFakeProcessFixture
LXD_RUNNING = 103
@@ -118,6 +119,10 @@ class FakeFilesystem(_FakeFilesystem):
class TestLXD(TestCase):
+ def setUp(self):
+ super(TestLXD, self).setUp()
+ self.useFixture(CarefulFakeProcessFixture())
+
def make_chroot_tarball(self, output_path):
source = self.useFixture(TempDir()).path
hello = os.path.join(source, "bin", "hello")
diff --git a/lpbuildd/target/tests/testfixtures.py b/lpbuildd/target/tests/testfixtures.py
index 41116ff..02cd995 100644
--- a/lpbuildd/target/tests/testfixtures.py
+++ b/lpbuildd/target/tests/testfixtures.py
@@ -4,10 +4,13 @@
__metaclass__ = type
import argparse
+import io
import os
import shutil
from fixtures import MonkeyPatch
+from fixtures._fixtures import popen
+import six
from systemfixtures import FakeFilesystem as _FakeFilesystem
@@ -101,3 +104,37 @@ class FakeFilesystem(_FakeFilesystem):
if path.startswith(prefix):
return False
return super(FakeFilesystem, self)._is_fake_path(path, *args, **kwargs)
+
+
+class CarefulFakeProcess(popen.FakeProcess):
+ """A version of FakeProcess that is more careful about text mode."""
+
+ def __init__(self, *args, **kwargs):
+ super(CarefulFakeProcess, self).__init__(*args, **kwargs)
+ text_mode = bool(self._args.get("universal_newlines"))
+ if not self.stdout:
+ self.stdout = io.StringIO() if text_mode else io.BytesIO()
+ if not self.stderr:
+ self.stderr = io.StringIO() if text_mode else io.BytesIO()
+
+ def communicate(self, *args, **kwargs):
+ out, err = super(CarefulFakeProcess, self).communicate(*args, **kwargs)
+ if self._args.get("universal_newlines"):
+ if isinstance(out, bytes):
+ raise TypeError("Process stdout is bytes, expecting text")
+ if isinstance(err, bytes):
+ raise TypeError("Process stderr is bytes, expecting text")
+ else:
+ if isinstance(out, six.text_type):
+ raise TypeError("Process stdout is text, expecting bytes")
+ if isinstance(err, six.text_type):
+ raise TypeError("Process stderr is text, expecting bytes")
+ return out, err
+
+
+class CarefulFakeProcessFixture(MonkeyPatch):
+ """Patch the Popen fixture to be more careful about text mode."""
+
+ def __init__(self):
+ super(CarefulFakeProcessFixture, self).__init__(
+ "fixtures._fixtures.popen.FakeProcess", CarefulFakeProcess)
