launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad-buildd:py3-fix-buildrecipe into launchpad-buildd:master.

Commit message:
Fix bytes/text handling in RecipeBuilder.buildTree

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/+git/launchpad-buildd/+merge/391378
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad-buildd:py3-fix-buildrecipe into launchpad-buildd:master.

diff --git a/bin/buildrecipe b/bin/buildrecipe
index f3034e7..b447f01 100755
--- a/bin/buildrecipe
+++ b/bin/buildrecipe
@@ -14,13 +14,7 @@ import os
 import pwd
 import socket
 import stat
-from subprocess import (
-    PIPE,
-    Popen,
-    call,
-    check_call,
-    check_output,
-    )
+import subprocess
 import sys
 import tempfile
 from textwrap import dedent
@@ -35,18 +29,15 @@ RETCODE_FAILURE_INSTALL_BUILD_DEPS = 202
 RETCODE_FAILURE_BUILD_SOURCE_PACKAGE = 203
 
 
-def call_report_rusage(args, env):
+def call_report(args, env):
     """Run a subprocess.
 
-    Report that it was run, and the resources used, and complain if it fails.
+    Report that it was run and complain if it fails.
 
-    :return: The process wait status.
+    :return: The process exit status.
     """
     print('RUN %r' % args)
-    proc = Popen(args, env=env)
-    pid, status, rusage = os.wait4(proc.pid, 0)
-    print(rusage)
-    return status
+    return subprocess.call(args, env=env)
 
 
 class RecipeBuilder:
@@ -102,31 +93,28 @@ class RecipeBuilder:
         assert not os.path.exists(self.tree_path)
         recipe_path = os.path.join(self.work_dir, 'recipe')
         manifest_path = os.path.join(self.tree_path, 'manifest')
-        recipe_file = open(recipe_path, 'rb')
-        try:
+        with open(recipe_path) as recipe_file:
             recipe = recipe_file.read()
-        finally:
-            recipe_file.close()
         # As of bzr 2.2, a defined identity is needed.  In this case, we're
         # using buildd@<hostname>.
         hostname = socket.gethostname()
         email = 'buildd@%s' % hostname
-        lsb_release = Popen(['/usr/bin/sudo',
+        lsb_release = subprocess.Popen(['sudo',
             '/usr/sbin/chroot', self.chroot_path, 'lsb_release',
-            '-r', '-s'], stdout=PIPE)
+            '-r', '-s'], stdout=subprocess.PIPE, universal_newlines=True)
         distroseries_version = lsb_release.communicate()[0].rstrip()
         assert lsb_release.returncode == 0
 
         if self.git:
             print('Git version:')
-            check_call(['git', '--version'])
-            print(check_output(
-                ['dpkg-query', '-W', 'git-build-recipe']).rstrip(
-                    '\n').replace('\t', ' '))
+            subprocess.check_call(['git', '--version'])
+            print(subprocess.check_output(
+                ['dpkg-query', '-W', 'git-build-recipe'],
+                universal_newlines=True).rstrip('\n').replace('\t', ' '))
         else:
             print('Bazaar versions:')
-            check_call(['bzr', 'version'])
-            check_call(['bzr', 'plugins'])
+            subprocess.check_call(['bzr', 'version'])
+            subprocess.check_call(['bzr', 'plugins'])
 
         print('Building recipe:')
         print(recipe)
@@ -149,7 +137,7 @@ class RecipeBuilder:
             '--append-version', '~ubuntu%s.1' % distroseries_version,
             recipe_path, self.tree_path,
             ])
-        retcode = call_report_rusage(cmd, env=env)
+        retcode = call_report(cmd, env=env)
         if retcode != 0:
             return retcode
         (source,) = [name for name in os.listdir(self.tree_path)
@@ -217,14 +205,14 @@ class RecipeBuilder:
                 file=conf)
         ftparchive_env = dict(os.environ)
         ftparchive_env.pop("APT_CONFIG", None)
-        ret = call(
+        ret = subprocess.call(
             ["apt-ftparchive", "-q=2", "generate", conf_path],
             env=ftparchive_env)
         if ret != 0:
             return ret
 
         with open(os.path.join(self.apt_dir, "Release"), "w") as release:
-            return call(
+            return subprocess.call(
                 ["apt-ftparchive", "-q=2", "-c", conf_path, "release",
                  self.apt_dir],
                 stdout=release, env=ftparchive_env)
@@ -252,7 +240,7 @@ class RecipeBuilder:
         if ret == 0:
             list_path = os.path.join(
                 self.apt_sources_list_dir, "buildrecipe-archive.list")
-            return call(['sudo', 'mv', tmp_list_path, list_path])
+            return subprocess.call(['sudo', 'mv', tmp_list_path, list_path])
         return ret
 
     def setUpAptArchive(self, package):
@@ -296,7 +284,8 @@ class RecipeBuilder:
             print("Running in chroot: %s" %
                   ' '.join("'%s'" % arg for arg in args))
             sys.stdout.flush()
-        return call(['sudo', '/usr/sbin/chroot', self.chroot_path] + args)
+        return subprocess.call(
+            ['sudo', '/usr/sbin/chroot', self.chroot_path] + args)
 
     def copy_in(self, source_path, target_path):
         """Copy a file into the target environment.
@@ -316,7 +305,7 @@ class RecipeBuilder:
         mode = stat.S_IMODE(os.stat(source_path).st_mode)
         full_target_path = os.path.join(
             self.chroot_path, target_path.lstrip("/"))
-        check_call(
+        subprocess.check_call(
             ["sudo", "install", "-o", "root", "-g", "root", "-m", "%o" % mode,
              source_path, full_target_path])
 
diff --git a/debian/changelog b/debian/changelog
index e46111d..547c167 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,6 +1,7 @@
 launchpad-buildd (193) UNRELEASED; urgency=medium
 
   * Fix handling of bytes arguments passed to BuildManager.runSubProcess.
+  * Fix bytes/text handling in RecipeBuilder.buildTree.
 
  -- Colin Watson <cjwatson@xxxxxxxxxx>  Thu, 24 Sep 2020 16:45:27 +0100
 
diff --git a/lpbuildd/tests/test_buildrecipe.py b/lpbuildd/tests/test_buildrecipe.py
index ba93dfe..a4da180 100644
--- a/lpbuildd/tests/test_buildrecipe.py
+++ b/lpbuildd/tests/test_buildrecipe.py
@@ -7,6 +7,7 @@ __metaclass__ = type
 
 from contextlib import contextmanager
 import imp
+import io
 import os
 import shutil
 import stat
@@ -14,7 +15,10 @@ import sys
 import tempfile
 from textwrap import dedent
 
-from fixtures import MockPatchObject
+from fixtures import (
+    MockPatch,
+    MockPatchObject,
+    )
 import six
 from systemfixtures import FakeProcesses
 from testtools import TestCase
@@ -82,6 +86,107 @@ class TestRecipeBuilder(TestCase):
         self.resetEnvironment()
         super(TestRecipeBuilder, self).tearDown()
 
+    def test_buildTree_git(self):
+        def fake_git(args):
+            if args["args"][1] == "--version":
+                print("git version x.y.z")
+                return {}
+            else:
+                return {"returncode": 1}
+
+        def fake_git_build_recipe(args):
+            print("dummy recipe build")
+            os.makedirs(os.path.join(self.builder.tree_path, "foo"))
+            return {}
+
+        processes_fixture = self.useFixture(FakeProcesses())
+        processes_fixture.add(
+            lambda _: {"stdout": io.StringIO(u"5.10\n")}, name="sudo")
+        processes_fixture.add(fake_git, name="git")
+        processes_fixture.add(
+            lambda _: {"stdout": io.StringIO(u"git-build-recipe\tx.y.z\n")},
+            name="dpkg-query")
+        processes_fixture.add(fake_git_build_recipe, name="git-build-recipe")
+        self.builder = RecipeBuilder(
+            self.build_id, "Recipe Builder", "builder@xxxxxxxxxxx>", "grumpy",
+            "grumpy", "main", "PPA", git=True)
+        with open(os.path.join(self.builder.work_dir, "recipe"), "w") as f:
+            f.write("dummy recipe contents\n")
+        mock_stdout = six.StringIO()
+        self.useFixture(MockPatch("sys.stdout", mock_stdout))
+        self.assertEqual(0, self.builder.buildTree())
+        self.assertEqual(
+            os.path.join(self.builder.work_dir_relative, "tree", "foo"),
+            self.builder.source_dir_relative)
+        expected_recipe_command = [
+            "git-build-recipe", "--safe", "--no-build",
+            "--manifest", os.path.join(self.builder.tree_path, "manifest"),
+            "--distribution", "grumpy", "--allow-fallback-to-native",
+            "--append-version", u"~ubuntu5.10.1",
+            os.path.join(self.builder.work_dir, "recipe"),
+            self.builder.tree_path,
+            ]
+        self.assertEqual(
+            dedent("""\
+                Git version:
+                git version x.y.z
+                git-build-recipe x.y.z
+                Building recipe:
+                dummy recipe contents
+
+                RUN %s
+                dummy recipe build
+                """) % repr(expected_recipe_command),
+            mock_stdout.getvalue())
+
+    def test_buildTree_bzr(self):
+        def fake_bzr(args):
+            if args["args"][1] == "version":
+                print("bzr version x.y.z")
+                return {}
+            elif args["args"][1] == "plugins":
+                print("bzr-plugin x.y.z")
+                return {}
+            elif "dailydeb" in args["args"][1:]:
+                print("dummy recipe build")
+                os.makedirs(os.path.join(self.builder.tree_path, "foo"))
+                return {}
+            else:
+                return {"returncode": 1}
+
+        processes_fixture = self.useFixture(FakeProcesses())
+        processes_fixture.add(
+            lambda _: {"stdout": io.StringIO(u"5.10\n")}, name="sudo")
+        processes_fixture.add(fake_bzr, name="bzr")
+        with open(os.path.join(self.builder.work_dir, "recipe"), "w") as f:
+            f.write("dummy recipe contents\n")
+        mock_stdout = six.StringIO()
+        self.useFixture(MockPatch("sys.stdout", mock_stdout))
+        self.assertEqual(0, self.builder.buildTree())
+        self.assertEqual(
+            os.path.join(self.builder.work_dir_relative, "tree", "foo"),
+            self.builder.source_dir_relative)
+        expected_recipe_command = [
+            "bzr", "-Derror", "dailydeb", "--safe", "--no-build",
+            "--manifest", os.path.join(self.builder.tree_path, "manifest"),
+            "--distribution", "grumpy", "--allow-fallback-to-native",
+            "--append-version", u"~ubuntu5.10.1",
+            os.path.join(self.builder.work_dir, "recipe"),
+            self.builder.tree_path,
+            ]
+        self.assertEqual(
+            dedent("""\
+                Bazaar versions:
+                bzr version x.y.z
+                bzr-plugin x.y.z
+                Building recipe:
+                dummy recipe contents
+
+                RUN %s
+                dummy recipe build
+                """) % repr(expected_recipe_command),
+            mock_stdout.getvalue())
+
     def test_makeDummyDsc(self):
         self.builder.source_dir_relative = os.path.join(
             self.builder.work_dir_relative, "tree", "foo")