Merge lp:~cjwatson/launchpad-buildd/sbuild-schroot into lp:launchpad-buildd

[Colin Watson]

Configure sbuild to use schroot sessions rather than sudo.

This is closer to how Debian buildds behave and to how developers typically run sbuild interactively, so should further reduce the number of Launchpad-specific failures; and although we have to spend about the same number of lines of code on setup, I think less of it is about working around peculiar idiosyncrasies. It also has the substantial benefit of the inactivity timeout actually working properly, so a build that leaves processes hanging around will eventually be reaped automatically rather than having to be cancelled manually.

The diff of the "User Environment" reported by sbuild is as follows:

 APT_CONFIG=/var/lib/sbuild/apt.conf
 DEB_BUILD_OPTIONS=noautodbgsym parallel=4
-HOME=/home/buildd
+HOME=/sbuild-nonexistent
 LANG=C.UTF-8
 LC_ALL=C.UTF-8
 LOGNAME=buildd
-MAIL=/var/mail/buildd
-OLDPWD=/
-PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
-PWD=/<<PKGBUILDDIR>>
+PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games
+SCHROOT_ALIAS_NAME=build-PACKAGEBUILD-37
+SCHROOT_CHROOT_NAME=build-PACKAGEBUILD-37
+SCHROOT_COMMAND=env
+SCHROOT_GID=2501
+SCHROOT_GROUP=buildd
+SCHROOT_SESSION_ID=build-PACKAGEBUILD-37
+SCHROOT_UID=2001
+SCHROOT_USER=buildd
 SHELL=/bin/sh
-SUDO_COMMAND=/usr/sbin/chroot /<<CHROOT>> su buildd -s /bin/sh -c cd '/<<PKGBUILDDIR>>' && 'env'
-SUDO_GID=2501
-SUDO_UID=2001
-SUDO_USER=buildd
 TERM=unknown
 USER=buildd
-USERNAME=root
+V=1

Of these:

 * The SUDO_* and SCHROOT_* changes are natural.
 * Dropping MAIL, OLDPWD, and PWD seems correct (shells will set PWD later anyway).
 * Dropping /usr/local/games from PATH should be harmless.
 * sudo sets USERNAME, but it's rather non-standard and there's no obvious reason why it bothers, and in any case its value was arguably wrong here.
 * I'd intended to set V=1 way back in launchpad-buildd 125 but it was eaten by the environment filter and I didn't notice.
 * The change in HOME might possibly cause some problems, since it's now a nonexistent directory. However, this matches Debian so it might just as easily cause some progressions, and I think it's more correct this way.

[William Grant (wgrant)]

review: Approve (code)

[lp:~cjwatson/launchpad-buildd/sbuild-schroot]

229. By Colin Watson on 2017-08-03

Simplify using install(1).

230. By Colin Watson on 2017-08-03

Merge trunk.

=== modified file 'MANIFEST.in'
--- MANIFEST.in	2017-07-25 22:11:19 +0000
+++ MANIFEST.in	2017-08-03 13:13:28 +0000
@@ -12,7 +12,6 @@
 include bin/scan-for-processes
 include bin/slave-prep
 include bin/snap-git-proxy
-include bin/sudo-wrapper
 include bin/test_buildd_generatetranslationtemplates
 include bin/test_buildd_recipe
 include bin/umount-chroot

=== modified file 'bin/sbuild-package'
--- bin/sbuild-package	2017-07-25 22:11:19 +0000
+++ bin/sbuild-package	2017-08-03 13:13:28 +0000
@@ -19,33 +19,6 @@
 
 exec 2>&1
 
-# Keep this in sync with sbuild/lib/Buildd.pm.
-unset LANG
-unset LANGUAGE
-unset LC_CTYPE
-unset LC_NUMERIC
-unset LC_TIME
-unset LC_COLLATE
-unset LC_MONETARY
-unset LC_MESSAGES
-unset LC_PAPER
-unset LC_NAME
-unset LC_ADDRESS
-unset LC_TELEPHONE
-unset LC_MEASUREMENT
-unset LC_IDENTIFICATION
-unset LC_ALL
-unset DISPLAY
-unset TERM
-
-# Force values for these, since otherwise sudo/pam_env may fill in unwanted
-# values from /etc/default/locale.
-export LANG=C.UTF-8 LC_ALL=C.UTF-8 LANGUAGE=
-
-# A number of build systems (e.g. automake, Linux) use this as an indication
-# that they should be more verbose.
-export V=1
-
 # On multi-guest PPA hosts, the per-guest overlay sometimes gets out of
 # sync, and we notice this by way of a corrupted .sbuildrc.  We aren't going
 # to be able to build anything in this situation, so immediately return

=== removed file 'bin/sudo-wrapper'
--- bin/sudo-wrapper	2017-07-25 22:11:19 +0000
+++ bin/sudo-wrapper	1970-01-01 00:00:00 +0000
@@ -1,4 +0,0 @@
-#! /bin/sh
-set -e
-
-exec sudo -E "$@"

=== modified file 'debian/changelog'
--- debian/changelog	2017-08-03 12:14:34 +0000
+++ debian/changelog	2017-08-03 13:13:28 +0000
@@ -14,6 +14,7 @@
     and to have unit tests.
   * 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.
 
  -- Colin Watson <[e-mail]>  Tue, 25 Jul 2017 23:07:58 +0100
 

=== modified file 'debian/control'
--- debian/control	2017-07-28 14:08:52 +0000
+++ debian/control	2017-08-03 13:13:28 +0000
@@ -10,7 +10,7 @@
 Architecture: all
 Depends: python-lpbuildd (=${source:Version}), python, debootstrap, dpkg-dev,
  file, bzip2, sudo, ntpdate, adduser, apt-transport-https, lsb-release,
- pristine-tar, python-apt, sbuild, ${misc:Depends}
+ pristine-tar, python-apt, sbuild, schroot, ${misc:Depends}
 Description: Launchpad buildd slave
  This is the launchpad buildd slave package. It contains everything needed to
  get a launchpad buildd going apart from the database manipulation required to

=== modified file 'debian/launchpad-buildd.install'
--- debian/launchpad-buildd.install	2017-07-25 22:11:19 +0000
+++ debian/launchpad-buildd.install	2017-08-03 13:13:28 +0000
@@ -15,7 +15,6 @@
 bin/scan-for-processes			usr/share/launchpad-buildd/slavebin
 bin/slave-prep				usr/share/launchpad-buildd/slavebin
 bin/snap-git-proxy			usr/share/launchpad-buildd/slavebin
-bin/sudo-wrapper			usr/share/launchpad-buildd/slavebin
 bin/umount-chroot			usr/share/launchpad-buildd/slavebin
 bin/unpack-chroot			usr/share/launchpad-buildd/slavebin
 bin/update-debian-chroot		usr/share/launchpad-buildd/slavebin

=== modified file 'lpbuildd/binarypackage.py'
--- lpbuildd/binarypackage.py	2017-07-28 13:25:55 +0000
+++ lpbuildd/binarypackage.py	2017-08-03 13:13:28 +0000
@@ -1,12 +1,14 @@
 # Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-from __future__ import absolute_import
+from __future__ import absolute_import, print_function
 
 from collections import defaultdict
 import os
 import re
 import subprocess
+import tempfile
+from textwrap import dedent
 import traceback
 
 import apt_pkg
@@ -99,6 +101,10 @@
         return os.path.join(
             self.home, "build-" + self._buildid, 'chroot-autobuild')
 
+    @property
+    def schroot_config_path(self):
+        return os.path.join('/etc/schroot/chroot.d', 'build-' + self._buildid)
+
     def initiate(self, files, chroot, extra_args):
         """Initiate a build with a given set of files and chroot."""
 
@@ -121,6 +127,25 @@
 
     def doRunBuild(self):
         """Run the sbuild process to build the package."""
+        with tempfile.NamedTemporaryFile(mode="w") as schroot_file:
+            # Use the "plain" chroot type because we do the necessary setup
+            # and teardown ourselves: it's easier to do this the same way
+            # for all build types.
+            print(dedent('''\
+                [build-{buildid}]
+                description=build-{buildid}
+                groups=sbuild,root
+                root-groups=sbuild,root
+                type=plain
+                directory={chroot_path}
+                ''').format(
+                    buildid=self._buildid, chroot_path=self.chroot_path),
+                file=schroot_file, end='')
+            schroot_file.flush()
+            subprocess.check_call(
+                ['sudo', 'install', '-o', 'root', '-g', 'root', '-m', '0644',
+                 schroot_file.name, self.schroot_config_path])
+
         currently_building_path = os.path.join(
             self.chroot_path, 'CurrentlyBuilding')
         currently_building_contents = (
@@ -137,10 +162,9 @@
 
         args = ["sbuild-package", self._buildid, self.arch_tag]
         args.append(self.suite)
-        args.extend(["-c", "chroot:autobuild"])
+        args.extend(["-c", "chroot:build-%s" % self._buildid])
         args.append("--arch=" + self.arch_tag)
         args.append("--dist=" + self.suite)
-        args.append("--purge=never")
         args.append("--nolog")
         if self.arch_indep:
             args.append("-A")
@@ -399,5 +423,8 @@
 
     def iterateReap_SBUILD(self, success):
         """Finished reaping after sbuild run."""
+        # Ignore errors from tearing down schroot configuration.
+        subprocess.call(['sudo', 'rm', '-f', self.schroot_config_path])
+
         self._state = DebianBuildState.UMOUNT
         self.doUnmounting()

=== modified file 'lpbuildd/tests/test_binarypackage.py'
--- lpbuildd/tests/test_binarypackage.py	2017-07-28 13:57:47 +0000
+++ lpbuildd/tests/test_binarypackage.py	2017-08-03 13:13:28 +0000
@@ -3,12 +3,15 @@
 
 __metaclass__ = type
 
+from functools import partial
 import os
 import shutil
+import subprocess
 import tempfile
 from textwrap import dedent
 
 from debian.deb822 import PkgRelation
+from fixtures import MonkeyPatch
 from testtools import TestCase
 from testtools.matchers import (
     Contains,
@@ -59,6 +62,19 @@
         return 0
 
 
+class DisableSudo(MonkeyPatch):
+
+    def __init__(self):
+        super(DisableSudo, self).__init__(
+            'subprocess.call', partial(self.call_patch, subprocess.call))
+
+    def call_patch(self, old_call, cmd, *args, **kwargs):
+        if cmd[0] == 'sudo':
+            return 0
+        else:
+            return old_call(cmd, *args, **kwargs)
+
+
 def write_file(path, content):
     if not os.path.isdir(os.path.dirname(path)):
         os.makedirs(os.path.dirname(path))
@@ -70,6 +86,7 @@
     """Run BinaryPackageBuildManager through its iteration steps."""
     def setUp(self):
         super(TestBinaryPackageBuildManagerIteration, self).setUp()
+        self.useFixture(DisableSudo())
         self.working_dir = tempfile.mkdtemp()
         self.addCleanup(lambda: shutil.rmtree(self.working_dir))
         slave_dir = os.path.join(self.working_dir, 'slave')
@@ -110,8 +127,9 @@
             BinaryPackageBuildState.SBUILD,
             [
             'sharepath/slavebin/sbuild-package', 'sbuild-package',
-            self.buildid, 'i386', 'warty', '-c', 'chroot:autobuild',
-            '--arch=i386', '--dist=warty', '--purge=never', '--nolog',
+            self.buildid, 'i386', 'warty',
+            '-c', 'chroot:build-' + self.buildid,
+            '--arch=i386', '--dist=warty', '--nolog',
             'foo_1.dsc',
             ], final=True)
         self.assertFalse(self.slave.wasCalled('chrootFail'))
@@ -179,8 +197,9 @@
         self.assertState(
             BinaryPackageBuildState.SBUILD,
             ['sharepath/slavebin/sbuild-package', 'sbuild-package',
-             self.buildid, 'i386', 'warty', '-c', 'chroot:autobuild',
-             '--arch=i386', '--dist=warty', '--purge=never', '--nolog',
+             self.buildid, 'i386', 'warty',
+             '-c', 'chroot:build-' + self.buildid,
+             '--arch=i386', '--dist=warty', '--nolog',
              'foo_1.dsc'],
             env_matcher=Not(Contains('DEB_BUILD_OPTIONS')), final=True)
         self.assertFalse(self.slave.wasCalled('chrootFail'))
@@ -207,8 +226,9 @@
         self.assertState(
             BinaryPackageBuildState.SBUILD,
             ['sharepath/slavebin/sbuild-package', 'sbuild-package',
-             self.buildid, 'i386', 'warty', '-c', 'chroot:autobuild',
-             '--arch=i386', '--dist=warty', '--purge=never', '--nolog',
+             self.buildid, 'i386', 'warty',
+             '-c', 'chroot:build-' + self.buildid,
+             '--arch=i386', '--dist=warty', '--nolog',
              'foo_1.dsc'],
             env_matcher=ContainsDict(
                 {'DEB_BUILD_OPTIONS': Equals('noautodbgsym')}),

=== modified file 'sbuildrc'
--- sbuildrc	2017-04-26 12:24:32 +0000
+++ sbuildrc	2017-08-03 13:13:28 +0000
@@ -16,29 +16,30 @@
 
 $resolve_alternatives = 1;
 
-# Also let LANG through to subprocesses, or sudo will set it to
-# /etc/default/locale on the host, which probably doesn't exist in the
-# chroot. sbuild-package invokes sbuild with LANG=C, which is what we
-# want.
+$build_environment = {
+    # sbuild sets LC_ALL=C.UTF-8 by default, so setting LANG as well should
+    # be redundant, but do so anyway for compatibility.
+    'LANG' => 'C.UTF-8',
+    # It's not clear how much sense this makes, but sudo set this as a
+    # fallback default, so keep it for compatibility.
+    'TERM' => 'unknown',
+    # A number of build systems (e.g. automake, Linux) use this as an
+    # indication that they should be more verbose.
+    'V' => '1',
+};
+
+# We want to expose almost nothing from the buildd environment.
+# DEB_BUILD_OPTIONS is set by sbuild-package.
 $environment_filter = [
-    '^PATH$',
-    '^DEB(IAN|SIGN)?_[A-Z_]+$',
-    '^(C(PP|XX)?|LD|F)FLAGS(_APPEND)?$',
-    '^USER(NAME)?$',
-    '^LOGNAME$',
-    '^HOME$',
-    '^TERM$',
-    '^SHELL$',
-    '^LANG$'];
+    '^DEB_BUILD_OPTIONS$',
+    ];
 
-# We need to use "sudo -E" so that the above environment variables are
-# allowed through.
-$sudo = "/usr/share/launchpad-buildd/slavebin/sudo-wrapper";
+# We're just going to throw the chroot away anyway.
+$purge_build_deps = 'never';
 
 # After that time (in minutes) of inactivity a build is terminated.
 # Activity
 # is measured by output to the log file.
 $stalled_pkg_timeout = 150;
 
-$chroot_mode="sudo";
 $sbuild_mode="buildd";