launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad-buildd/create-remove-python into lp:launchpad-buildd with lp:~cjwatson/launchpad-buildd/target-abstractions as a prerequisite.

Commit message:
Rewrite unpack-chroot and remove-build in Python, allowing them to have
unit tests.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/create-remove-python/+merge/328556
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad-buildd/create-remove-python into lp:launchpad-buildd.

=== modified file 'bin/remove-build'
--- bin/remove-build	2017-07-25 22:11:19 +0000
+++ bin/remove-build	2017-08-04 02:29:49 +0000
@@ -1,25 +1,24 @@
-#!/bin/sh
+#! /usr/bin/python -u
 #
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# Buildd Slave tool to remove an unmounted chroot
-
-# Expects build id as arg 1, makes build-id to contain the build
-
-# Needs RM to be set to a gnu rm instance
-# Needs SUDO to be set to a sudo instance for passwordless access
-
-RM=/bin/rm
-SUDO=/usr/bin/sudo
-BUILDID="$1"
-
-set -e
-
-exec 2>&1
-
-echo "Removing build $BUILDID"
-
-cd $HOME
-
-$SUDO $RM -rf "build-$BUILDID"
+"""Remove an unmounted chroot."""
+
+from __future__ import print_function
+
+__metaclass__ = type
+
+import sys
+
+from lpbuildd.target.operation import configure_logging
+from lpbuildd.target.remove import Remove
+
+
+def main():
+    configure_logging()
+    return Remove().run()
+
+
+if __name__ == "__main__":
+    sys.exit(main())

=== modified file 'bin/unpack-chroot'
--- bin/unpack-chroot	2017-07-25 22:11:19 +0000
+++ bin/unpack-chroot	2017-08-04 02:29:49 +0000
@@ -1,43 +1,24 @@
-#!/bin/sh
+#! /usr/bin/python -u
 #
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# Buildd Slave tool to unpack a chroot tarball
-
-# Expects build id as arg 1, makes build-id to contain the build
-# Expects bzip2 compressed tarball as arg 2
-
-# Needs TAR to be set to a gnu tar instance, that needs bzip2
-# Needs SUDO to be set to a sudo instance for passwordless access
-# BUNZIP2 must un-bzip2
-# FILE must implement the -b and -i arguments (so a Debianish file)
-
-export PATH=/usr/bin:/bin:/usr/sbin:/sbin:${PATH}
-
-TAR=tar
-SUDO=sudo
-BUNZIP2=bunzip2
-FILE=file
-
-BUILDID="$1"
-TARBALL="$2"
-
-set -e
-
-exec 2>&1
-
-MIMETYPE=$($FILE -bi "$TARBALL")
-
-if [ x"$MIMETYPE" = "xapplication/x-bzip2" ]; then
-  echo "Uncompressing the tarball..."
-  $BUNZIP2 -c < "$TARBALL" > "$TARBALL".tmp
-  mv "$TARBALL".tmp "$TARBALL"
-  exec $0 "$@"
-fi
-
-cd $HOME
-cd "build-$BUILDID"
-
-echo "Unpacking chroot for build $BUILDID"
-$SUDO $TAR -xf "$TARBALL"
+"""Unpack a chroot tarball."""
+
+from __future__ import print_function
+
+__metaclass__ = type
+
+import sys
+
+from lpbuildd.target.create import Create
+from lpbuildd.target.operation import configure_logging
+
+
+def main():
+    configure_logging()
+    return Create().run()
+
+
+if __name__ == "__main__":
+    sys.exit(main())

=== modified file 'debian/changelog'
--- debian/changelog	2017-08-03 13:13:08 +0000
+++ debian/changelog	2017-08-04 02:29:49 +0000
@@ -15,6 +15,8 @@
   * Rewrite override-sources-list in Python, allowing it to have unit tests.
   * Rewrite add-trusted-keys in Python, allowing it to have unit tests.
   * Configure sbuild to use schroot sessions rather than sudo.
+  * Rewrite unpack-chroot and remove-build in Python, allowing them to have
+    unit tests.
 
  -- Colin Watson <cjwatson@xxxxxxxxxx>  Tue, 25 Jul 2017 23:07:58 +0100
 

=== modified file 'lpbuildd/slave.py'
--- lpbuildd/slave.py	2017-08-04 02:29:49 +0000
+++ lpbuildd/slave.py	2017-08-04 02:29:49 +0000
@@ -163,9 +163,8 @@
 
     def doUnpack(self):
         """Unpack the build chroot."""
-        self.runSubProcess(
-            self._unpackpath,
-            ["unpack-chroot", self._buildid, self._chroottarfile])
+        self.runTargetSubProcess(
+            self._unpackpath, ["unpack-chroot", self._chroottarfile])
 
     def doReapProcesses(self, state, notify=True):
         """Reap any processes left lying around in the chroot."""
@@ -188,7 +187,7 @@
 
     def doCleanup(self):
         """Remove the build tree etc."""
-        self.runSubProcess(self._cleanpath, ["remove-build", self._buildid])
+        self.runTargetSubProcess(self._cleanpath, ["remove-build"])
 
         # Sanitize the URLs in the buildlog file if this is a build
         # in a private archive.

=== modified file 'lpbuildd/target/backend.py'
--- lpbuildd/target/backend.py	2017-08-04 02:29:49 +0000
+++ lpbuildd/target/backend.py	2017-08-04 02:29:49 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 import os.path
+import subprocess
 
 
 class BackendException(Exception):
@@ -30,6 +31,13 @@
             raise KeyError("Unknown backend: %s" % name)
         return backend_factory(build_id, series=series, arch=arch)
 
+    def create(self, tarball_path):
+        """Create the backend based on a chroot tarball.
+
+        This puts the backend into a state where it is ready to be started.
+        """
+        raise NotImplementedError
+
     def run(self, args, env=None, input_text=None, **kwargs):
         """Run a command in the target environment.
 
@@ -53,3 +61,7 @@
             environment's root.
         """
         raise NotImplementedError
+
+    def remove(self):
+        """Remove the backend."""
+        subprocess.check_call(["sudo", "rm", "-rf", self.build_path])

=== modified file 'lpbuildd/target/chroot.py'
--- lpbuildd/target/chroot.py	2017-08-04 02:29:49 +0000
+++ lpbuildd/target/chroot.py	2017-08-04 02:29:49 +0000
@@ -23,6 +23,11 @@
         super(Chroot, self).__init__(build_id, series=series, arch=arch)
         self.chroot_path = os.path.join(self.build_path, "chroot-autobuild")
 
+    def create(self, tarball_path):
+        """See `Backend`."""
+        subprocess.check_call(
+            ["sudo", "tar", "-C", self.build_path, "-xf", tarball_path])
+
     def run(self, args, env=None, input_text=None, **kwargs):
         """See `Backend`."""
         if env:

=== added file 'lpbuildd/target/create.py'
--- lpbuildd/target/create.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/target/create.py	2017-08-04 02:29:49 +0000
@@ -0,0 +1,28 @@
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import print_function
+
+__metaclass__ = type
+
+import logging
+
+from lpbuildd.target.operation import Operation
+
+
+logger = logging.getLogger(__name__)
+
+
+class Create(Operation):
+
+    description = "Create the target environment."
+
+    def make_parser(self):
+        parser = super(Create, self).make_parser()
+        parser.add_argument("tarball_path", help="path to chroot tarball")
+        return parser
+
+    def run(self):
+        logger.info("Creating target for build %s", self.args.build_id)
+        self.backend.create(self.args.tarball_path)
+        return 0

=== added file 'lpbuildd/target/remove.py'
--- lpbuildd/target/remove.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/target/remove.py	2017-08-04 02:29:49 +0000
@@ -0,0 +1,23 @@
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import print_function
+
+__metaclass__ = type
+
+import logging
+
+from lpbuildd.target.operation import Operation
+
+
+logger = logging.getLogger(__name__)
+
+
+class Remove(Operation):
+
+    description = "Remove the target environment."
+
+    def run(self):
+        logger.info("Removing build %s", self.args.build_id)
+        self.backend.remove()
+        return 0

=== modified file 'lpbuildd/target/tests/test_chroot.py'
--- lpbuildd/target/tests/test_chroot.py	2017-08-04 02:29:49 +0000
+++ lpbuildd/target/tests/test_chroot.py	2017-08-04 02:29:49 +0000
@@ -17,6 +17,20 @@
 
 class TestChroot(TestCase):
 
+    def test_create(self):
+        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
+        processes_fixture = self.useFixture(FakeProcesses())
+        processes_fixture.add(lambda _: {}, name="sudo")
+        Chroot("1", "xenial", "amd64").create("/path/to/tarball")
+
+        expected_args = [
+            ["sudo", "tar", "-C", "/expected/home/build-1",
+             "-xf", "/path/to/tarball"],
+            ]
+        self.assertEqual(
+            expected_args,
+            [proc._args["args"] for proc in processes_fixture.procs])
+
     def test_run(self):
         self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
         processes_fixture = self.useFixture(FakeProcesses())
@@ -54,3 +68,14 @@
         self.assertEqual(
             expected_args,
             [proc._args["args"] for proc in processes_fixture.procs])
+
+    def test_remove(self):
+        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
+        processes_fixture = self.useFixture(FakeProcesses())
+        processes_fixture.add(lambda _: {}, name="sudo")
+        Chroot("1", "xenial", "amd64").remove()
+
+        expected_args = [["sudo", "rm", "-rf", "/expected/home/build-1"]]
+        self.assertEqual(
+            expected_args,
+            [proc._args["args"] for proc in processes_fixture.procs])

=== added file 'lpbuildd/target/tests/test_create.py'
--- lpbuildd/target/tests/test_create.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/target/tests/test_create.py	2017-08-04 02:29:49 +0000
@@ -0,0 +1,30 @@
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from fixtures import EnvironmentVariable
+from systemfixtures import FakeProcesses
+from testtools import TestCase
+
+from lpbuildd.target.create import Create
+
+
+class TestCreate(TestCase):
+
+    def test_succeeds(self):
+        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
+        processes_fixture = self.useFixture(FakeProcesses())
+        processes_fixture.add(lambda _: {}, name="sudo")
+        args = [
+            "--backend=chroot", "--series=xenial", "--arch=amd64", "1",
+            "/path/to/tarball"]
+        Create(args=args).run()
+
+        expected_args = [
+            ["sudo", "tar", "-C", "/expected/home/build-1",
+             "-xf", "/path/to/tarball"],
+            ]
+        self.assertEqual(
+            expected_args,
+            [proc._args["args"] for proc in processes_fixture.procs])

=== added file 'lpbuildd/target/tests/test_remove.py'
--- lpbuildd/target/tests/test_remove.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/target/tests/test_remove.py	2017-08-04 02:29:49 +0000
@@ -0,0 +1,25 @@
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from fixtures import EnvironmentVariable
+from systemfixtures import FakeProcesses
+from testtools import TestCase
+
+from lpbuildd.target.remove import Remove
+
+
+class TestRemove(TestCase):
+
+    def test_succeeds(self):
+        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
+        processes_fixture = self.useFixture(FakeProcesses())
+        processes_fixture.add(lambda _: {}, name="sudo")
+        args = ["--backend=chroot", "--series=xenial", "--arch=amd64", "1"]
+        Remove(args=args).run()
+
+        expected_args = [["sudo", "rm", "-rf", "/expected/home/build-1"]]
+        self.assertEqual(
+            expected_args,
+            [proc._args["args"] for proc in processes_fixture.procs])

=== modified file 'lpbuildd/tests/test_binarypackage.py'
--- lpbuildd/tests/test_binarypackage.py	2017-08-03 13:13:08 +0000
+++ lpbuildd/tests/test_binarypackage.py	2017-08-04 02:29:49 +0000
@@ -313,7 +313,9 @@
         self.buildmanager.iterate(0)
         self.assertState(
             BinaryPackageBuildState.CLEANUP,
-            ['sharepath/slavebin/remove-build', 'remove-build', self.buildid],
+            ['sharepath/slavebin/remove-build', 'remove-build',
+             '--backend=chroot', '--series=warty', '--arch=i386',
+             self.buildid],
             final=True)
         self.assertFalse(self.slave.wasCalled('builderFail'))
 

=== modified file 'lpbuildd/tests/test_debian.py'
--- lpbuildd/tests/test_debian.py	2017-08-04 02:29:49 +0000
+++ lpbuildd/tests/test_debian.py	2017-08-04 02:29:49 +0000
@@ -106,6 +106,7 @@
         self.assertEqual(DebianBuildState.UNPACK, self.getState())
         self.assertEqual(
             (['sharepath/slavebin/unpack-chroot', 'unpack-chroot',
+              '--backend=chroot', '--series=xenial', '--arch=amd64',
               self.buildid,
               os.path.join(self.buildmanager._cachepath, 'chroot.tar.gz')],
              None),
@@ -177,7 +178,9 @@
         self.buildmanager.iterate(0)
         self.assertEqual(DebianBuildState.CLEANUP, self.getState())
         self.assertEqual(
-            (['sharepath/slavebin/remove-build', 'remove-build', self.buildid],
+            (['sharepath/slavebin/remove-build', 'remove-build',
+              '--backend=chroot', '--series=xenial', '--arch=amd64',
+              self.buildid],
              None),
             self.buildmanager.commands[-1])
         self.assertEqual(
@@ -208,6 +211,7 @@
         self.assertEqual(DebianBuildState.UNPACK, self.getState())
         self.assertEqual(
             (['sharepath/slavebin/unpack-chroot', 'unpack-chroot',
+              '--backend=chroot', '--series=xenial', '--arch=amd64',
               self.buildid,
               os.path.join(self.buildmanager._cachepath, 'chroot.tar.gz')],
              None),
@@ -290,7 +294,9 @@
         self.buildmanager.iterate(0)
         self.assertEqual(DebianBuildState.CLEANUP, self.getState())
         self.assertEqual(
-            (['sharepath/slavebin/remove-build', 'remove-build', self.buildid],
+            (['sharepath/slavebin/remove-build', 'remove-build',
+              '--backend=chroot', '--series=xenial', '--arch=amd64',
+              self.buildid],
              None),
             self.buildmanager.commands[-1])
         self.assertEqual(