launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad-buildd/sbuild-schroot into lp:launchpad-buildd.

Commit message:
Configure sbuild to use schroot sessions rather than sudo.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/sbuild-schroot/+merge/327634

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.

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.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad-buildd/sbuild-schroot into lp:launchpad-buildd.

=== modified file 'MANIFEST.in'
--- MANIFEST.in	2017-05-11 11:45:22 +0000
+++ MANIFEST.in	2017-07-18 15:57:24 +0000
@@ -16,7 +16,6 @@
 include scan-for-processes
 include slave-prep
 include snap-git-proxy
-include sudo-wrapper
 include template-buildd-slave.conf
 include test_buildd_generatetranslationtemplates
 include test_buildd_recipe

=== modified file 'debian/changelog'
--- debian/changelog	2017-07-18 00:28:23 +0000
+++ debian/changelog	2017-07-18 15:57:24 +0000
@@ -3,6 +3,7 @@
   * buildsnap: Initialise git submodules (LP: #1694413).
   * Run snapcraft as non-root with passwordless sudo, since we run into
     buggy corner cases in some plugins when running as root (LP: #1702656).
+  * Configure sbuild to use schroot sessions rather than sudo.
 
  -- Colin Watson <cjwatson@xxxxxxxxxx>  Wed, 31 May 2017 11:19:36 +0100
 

=== modified file 'debian/control'
--- debian/control	2015-10-20 09:41:07 +0000
+++ debian/control	2017-07-18 15:57:24 +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}
 Recommends: qemu-user-static
 Description: Launchpad buildd slave
  This is the launchpad buildd slave package. It contains everything needed to

=== modified file 'debian/launchpad-buildd.install'
--- debian/launchpad-buildd.install	2017-05-11 11:45:22 +0000
+++ debian/launchpad-buildd.install	2017-07-18 15:57:24 +0000
@@ -15,7 +15,6 @@
 scan-for-processes		usr/share/launchpad-buildd/slavebin
 slave-prep			usr/share/launchpad-buildd/slavebin
 snap-git-proxy			usr/share/launchpad-buildd/slavebin
-sudo-wrapper			usr/share/launchpad-buildd/slavebin
 umount-chroot			usr/share/launchpad-buildd/slavebin
 unpack-chroot			usr/share/launchpad-buildd/slavebin
 update-debian-chroot		usr/share/launchpad-buildd/slavebin

=== modified file 'lpbuildd/binarypackage.py'
--- lpbuildd/binarypackage.py	2016-12-09 18:04:00 +0000
+++ lpbuildd/binarypackage.py	2017-07-18 15:57:24 +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,29 @@
 
     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', 'cp', '-a', schroot_file.name,
+                 self.schroot_config_path])
+            subprocess.check_call(
+                ['sudo', 'chown', 'root:root', self.schroot_config_path])
+            subprocess.check_call(
+                ['sudo', 'chmod', '0644', self.schroot_config_path])
+
         currently_building_path = os.path.join(
             self.chroot_path, 'CurrentlyBuilding')
         currently_building_contents = (
@@ -137,10 +166,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")
@@ -397,5 +425,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	2016-12-02 14:20:28 +0000
+++ lpbuildd/tests/test_binarypackage.py	2017-07-18 15:57:24 +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 'sbuild-package'
--- sbuild-package	2016-12-02 14:20:28 +0000
+++ sbuild-package	2017-07-18 15:57:24 +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

=== modified file 'sbuildrc'
--- sbuildrc	2017-04-26 12:24:32 +0000
+++ sbuildrc	2017-07-18 15:57:24 +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";

=== removed file 'sudo-wrapper'
--- sudo-wrapper	2015-07-05 12:43:54 +0000
+++ sudo-wrapper	1970-01-01 00:00:00 +0000
@@ -1,4 +0,0 @@
-#! /bin/sh
-set -e
-
-exec sudo -E "$@"