Merge lp:~cjwatson/launchpad-buildd/recipe-currentlybuilding-permissions into lp:launchpad-buildd

[Colin Watson]

Fix recipe building to not rely on /CurrentlyBuilding existing in base images.

This involves a bit of copy-and-paste from Chroot.copy_in, since buildrecipe doesn't use the Operation framework yet.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'bin/buildrecipe'
--- bin/buildrecipe	2017-07-25 22:11:19 +0000
+++ bin/buildrecipe	2019-08-22 16:24:05 +0000
@@ -1,5 +1,5 @@
 #!/usr/bin/python -u
-# Copyright 2010, 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """A script that builds a package from a recipe and a chroot."""
@@ -12,6 +12,7 @@
 import os
 import pwd
 import socket
+import stat
 from subprocess import (
     PIPE,
     Popen,
@@ -20,6 +21,7 @@
     check_output,
     )
 import sys
+import tempfile
 from textwrap import dedent
 
 from debian.deb822 import Deb822
@@ -260,8 +262,6 @@
     def installBuildDeps(self):
         """Install the build-depends of the source tree."""
         package = self.getPackageName()
-        currently_building_path = os.path.join(
-            self.chroot_path, 'CurrentlyBuilding')
         currently_building_contents = (
             'Package: %s\n'
             'Suite: %s\n'
@@ -269,9 +269,11 @@
             'Purpose: %s\n'
             'Build-Debug-Symbols: no\n' %
             (package, self.suite, self.component, self.archive_purpose))
-        currently_building = open(currently_building_path, 'w')
-        currently_building.write(currently_building_contents)
-        currently_building.close()
+        with tempfile.NamedTemporaryFile(mode='w+') as currently_building:
+            currently_building.write(currently_building_contents)
+            currently_building.flush()
+            os.fchmod(currently_building.fileno(), 0o644)
+            self.copy_in(currently_building.name, '/CurrentlyBuilding')
         self.setUpAptArchive(package)
         return self.chroot(
             ['apt-get', 'build-dep', '-y', '--only-source', package])
@@ -286,8 +288,29 @@
             print("Running in chroot: %s" %
                   ' '.join("'%s'" % arg for arg in args))
             sys.stdout.flush()
-        return call([
-            '/usr/bin/sudo', '/usr/sbin/chroot', self.chroot_path] + args)
+        return call(['sudo', '/usr/sbin/chroot', self.chroot_path] + args)
+
+    def copy_in(self, source_path, target_path):
+        """Copy a file into the target environment.
+
+        The target file will be owned by root/root and have the same
+        permission mode as the source file.
+
+        :param source_path: the path to the file that should be copied from
+            the host system.
+        :param target_path: the path where the file should be installed
+            inside the target environment, relative to the target
+            environment's root.
+        """
+        # Use install(1) so that we can end up with root/root ownership with
+        # a minimum of subprocess calls; the buildd user may not make sense
+        # in the target.
+        mode = stat.S_IMODE(os.stat(source_path).st_mode)
+        full_target_path = os.path.join(
+            self.chroot_path, target_path.lstrip("/"))
+        check_call(
+            ["sudo", "install", "-o", "root", "-g", "root", "-m", "%o" % mode,
+             source_path, full_target_path])
 
     def buildSourcePackage(self):
         """Build the source package.

=== modified file 'debian/changelog'
--- debian/changelog	2019-06-18 16:54:33 +0000
+++ debian/changelog	2019-08-22 16:24:05 +0000
@@ -1,3 +1,10 @@
+launchpad-buildd (177) UNRELEASED; urgency=medium
+
+  * Fix recipe building to not rely on /CurrentlyBuilding existing in base
+    images (LP: #1841075).
+
+ -- Colin Watson <[e-mail]>  Thu, 22 Aug 2019 17:16:24 +0100
+
 launchpad-buildd (176) xenial; urgency=medium
 
   * Don't rely on /CurrentlyBuilding existing in base images.

=== modified file 'lpbuildd/tests/test_buildrecipe.py'
--- lpbuildd/tests/test_buildrecipe.py	2017-10-27 07:42:35 +0000
+++ lpbuildd/tests/test_buildrecipe.py	2019-08-22 16:24:05 +0000
@@ -1,4 +1,4 @@
-# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from __future__ import print_function
@@ -9,11 +9,20 @@
 import imp
 import os
 import shutil
+import stat
 import sys
 import tempfile
 from textwrap import dedent
 
+from fixtures import MockPatchObject
+import six
+from systemfixtures import FakeProcesses
 from testtools import TestCase
+from testtools.matchers import (
+    Equals,
+    MatchesListwise,
+    StartsWith,
+    )
 
 
 @contextmanager
@@ -30,6 +39,23 @@
         "buildrecipe", "bin/buildrecipe").RecipeBuilder
 
 
+class RanCommand(MatchesListwise):
+
+    def __init__(self, *args):
+        args_matchers = [
+            Equals(arg) if isinstance(arg, six.string_types) else arg
+            for arg in args]
+        super(RanCommand, self).__init__(args_matchers)
+
+
+class RanInChroot(RanCommand):
+
+    def __init__(self, home_dir, *args):
+        super(RanInChroot, self).__init__(
+            "sudo", "/usr/sbin/chroot",
+            os.path.join(home_dir, "build-1", "chroot-autobuild"), *args)
+
+
 class TestRecipeBuilder(TestCase):
     def setUp(self):
         super(TestRecipeBuilder, self).setUp()
@@ -143,6 +169,71 @@
             self.assertIn(
                 "Build-Depends: debhelper (>= 9~), libfoo-dev\n", sources_text)
 
-    # XXX cjwatson 2015-06-15: We should unit-test enableAptArchive and
-    # installBuildDeps too, but it involves a lot of mocks.  For now,
-    # integration testing is probably more useful.
+    def test_installBuildDeps(self):
+        processes_fixture = self.useFixture(FakeProcesses())
+        processes_fixture.add(lambda _: {}, name="sudo")
+        copies = {}
+
+        def mock_copy_in(source_path, target_path):
+            with open(source_path, "rb") as source:
+                copies[target_path] = (
+                    source.read(), os.fstat(source.fileno()).st_mode)
+
+        self.useFixture(
+            MockPatchObject(self.builder, "copy_in", mock_copy_in))
+        self.builder.source_dir_relative = os.path.join(
+            self.builder.work_dir_relative, "tree", "foo")
+        changelog_path = os.path.join(
+            self.builder.work_dir, "tree", "foo", "debian", "changelog")
+        control_path = os.path.join(
+            self.builder.work_dir, "tree", "foo", "debian", "control")
+        os.makedirs(os.path.dirname(changelog_path))
+        with open(changelog_path, "w") as changelog:
+            # Not a valid changelog, but only the first line matters here.
+            print("foo (1.0-1) bionic; urgency=medium", file=changelog)
+        with open(control_path, "w") as control:
+            print(dedent("""\
+                Source: foo
+                Build-Depends: debhelper (>= 9~), libfoo-dev
+
+                Package: foo
+                Depends: ${shlibs:Depends}"""),
+                file=control)
+        self.assertEqual(0, self.builder.installBuildDeps())
+        self.assertThat(
+            [proc._args["args"] for proc in processes_fixture.procs],
+            MatchesListwise([
+                RanInChroot(
+                    self.home_dir, "apt-get",
+                    "-o", StartsWith("Dir::Etc::sourcelist="),
+                    "-o", "APT::Get::List-Cleanup=false",
+                    "update"),
+                RanCommand(
+                    "sudo", "mv",
+                    os.path.join(
+                        self.builder.apt_dir, "buildrecipe-archive.list"),
+                    os.path.join(
+                        self.builder.apt_sources_list_dir,
+                        "buildrecipe-archive.list")),
+                RanInChroot(
+                    self.home_dir, "apt-get",
+                    "build-dep", "-y", "--only-source", "foo"),
+                ]))
+        self.assertEqual(
+            (dedent("""\
+                Package: foo
+                Suite: grumpy
+                Component: main
+                Purpose: PPA
+                Build-Debug-Symbols: no
+                """).encode("UTF-8"), stat.S_IFREG | 0o644),
+            copies["/CurrentlyBuilding"])
+        # This is still in the temporary location, since we mocked the "sudo
+        # mv" command.
+        with open(os.path.join(
+                self.builder.apt_dir,
+                "buildrecipe-archive.list")) as tmp_list:
+            self.assertEqual(
+                "deb-src [trusted=yes] file://%s ./\n" %
+                self.builder.apt_dir_relative,
+                tmp_list.read())