launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad-buildd/fix-recipe-depfail into lp:launchpad-buildd.

Commit message:
Fix dep-wait detection when recipes fail to install build-dependencies.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1234621 in launchpad-buildd: "lp-buildd crashes on recipe failures: too many values to unpack"
  https://bugs.launchpad.net/launchpad-buildd/+bug/1234621

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/fix-recipe-depfail/+merge/189041

Fix a crash in the recipe build manager, introduced in launchpad-buildd 116.  This slipped through because the recipe build manager was untested, so I wrote a load of test code first.

QA: Try a recipe build with a deliberately unsatisfiable dependency, and make sure it lands in dep-wait.
-- 
https://code.launchpad.net/~cjwatson/launchpad-buildd/fix-recipe-depfail/+merge/189041
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad-buildd/fix-recipe-depfail into lp:launchpad-buildd.

=== modified file 'Makefile'
--- Makefile	2013-07-25 17:26:10 +0000
+++ Makefile	2013-10-03 10:41:22 +0000
@@ -31,4 +31,5 @@
 		   lpbuildd.tests.test_buildd_slave \
 		   lpbuildd.tests.test_check_implicit_pointer_functions \
 		   lpbuildd.tests.test_harness \
-		   lpbuildd.tests.test_translationtemplatesbuildmanager
+		   lpbuildd.tests.test_translationtemplatesbuildmanager \
+		   lpbuildd.tests.test_sourcepackagerecipe

=== modified file 'debian/changelog'
--- debian/changelog	2013-09-27 12:12:29 +0000
+++ debian/changelog	2013-10-03 10:41:22 +0000
@@ -1,3 +1,10 @@
+launchpad-buildd (117) UNRELEASED; urgency=low
+
+  * Fix dep-wait detection when recipes fail to install build-dependencies
+    (LP: #1234621).
+
+ -- Colin Watson <cjwatson@xxxxxxxxxx>  Thu, 03 Oct 2013 11:38:46 +0100
+
 launchpad-buildd (116) hardy; urgency=low
 
   [ Colin Watson ]

=== modified file 'lpbuildd/sourcepackagerecipe.py'
--- lpbuildd/sourcepackagerecipe.py	2013-09-19 11:01:10 +0000
+++ lpbuildd/sourcepackagerecipe.py	2013-10-03 10:41:22 +0000
@@ -102,7 +102,7 @@
                 rx = (
                     'The following packages have unmet dependencies:\n'
                     '.*: Depends: ([^ ]*( \([^)]*\))?)')
-                _, mo = self.searchLogContents([rx, re.M])
+                _, mo = self.searchLogContents([[rx, re.M]])
                 if mo:
                     self._slave.depFail(mo.group(1))
                     print("Returning build status: DEPFAIL")

=== added file 'lpbuildd/tests/test_sourcepackagerecipe.py'
--- lpbuildd/tests/test_sourcepackagerecipe.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/tests/test_sourcepackagerecipe.py	2013-10-03 10:41:22 +0000
@@ -0,0 +1,212 @@
+# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import os
+import shutil
+import tempfile
+from textwrap import dedent
+
+from testtools import TestCase
+
+from lpbuildd.tests.fakeslave import FakeSlave
+from lpbuildd.sourcepackagerecipe import (
+    RETCODE_FAILURE_INSTALL_BUILD_DEPS,
+    SourcePackageRecipeBuildManager,
+    SourcePackageRecipeBuildState,
+    )
+
+
+class MockBuildManager(SourcePackageRecipeBuildManager):
+    def __init__(self, *args, **kwargs):
+        super(MockBuildManager, self).__init__(*args, **kwargs)
+        self.commands = []
+        self.iterators = []
+
+    def runSubProcess(self, path, command, iterate=None):
+        self.commands.append([path]+command)
+        if iterate is None:
+            iterate = self.iterate
+        self.iterators.append(iterate)
+        return 0
+
+
+class TestSourcePackageRecipeBuildManagerIteration(TestCase):
+    """Run SourcePackageRecipeBuildManager through its iteration steps."""
+    def setUp(self):
+        super(TestSourcePackageRecipeBuildManagerIteration, self).setUp()
+        self.working_dir = tempfile.mkdtemp()
+        self.addCleanup(lambda: shutil.rmtree(self.working_dir))
+        slave_dir = os.path.join(self.working_dir, 'slave')
+        home_dir = os.path.join(self.working_dir, 'home')
+        for dir in (slave_dir, home_dir):
+            os.mkdir(dir)
+        self.slave = FakeSlave(slave_dir)
+        self.buildid = '123'
+        self.buildmanager = MockBuildManager(self.slave, self.buildid)
+        self.buildmanager.home = home_dir
+        self.buildmanager._cachepath = self.slave._cachepath
+        self.chrootdir = os.path.join(
+            home_dir, 'build-%s' % self.buildid, 'chroot-autobuild')
+
+    def getState(self):
+        """Retrieve build manager's state."""
+        return self.buildmanager._state
+
+    def startBuild(self):
+        # The build manager's iterate() kicks off the consecutive states
+        # after INIT.
+        extra_args = {
+            'recipe_text': dedent("""\
+                # bzr-builder format 0.2 deb-version {debupstream}-0~{revno}
+                http://bazaar.launchpad.dev/~ppa-user/+junk/wakeonlan""";),
+            'suite': 'maverick',
+            'ogrecomponent': 'universe',
+            'author_name': 'Steve\u1234',
+            'author_email': 'stevea@xxxxxxxxxxx',
+            'archive_purpose': 'puppies',
+            'distroseries_name': 'maverick',
+            'archives': [
+                'deb http://archive.ubuntu.com/ubuntu maverick main universe',
+                'deb http://ppa.launchpad.net/launchpad/bzr-builder-dev/'
+                    'ubuntu main',
+                ],
+            }
+        self.buildmanager.initiate({}, 'chroot.tar.gz', extra_args)
+
+        # Skip states that are done in DebianBuildManager to the state
+        # directly before BUILD_RECIPE.
+        self.buildmanager._state = SourcePackageRecipeBuildState.UPDATE
+
+        # BUILD_RECIPE: Run the slave's payload to build the source package.
+        self.buildmanager.iterate(0)
+        self.assertEqual(
+            SourcePackageRecipeBuildState.BUILD_RECIPE, self.getState())
+        expected_command = [
+            'buildrecipepath', 'buildrecipe', self.buildid,
+            'Steve\u1234'.encode('utf-8'), 'stevea@xxxxxxxxxxx',
+            'maverick', 'maverick', 'universe', 'puppies',
+            ]
+        self.assertEqual(expected_command, self.buildmanager.commands[-1])
+        self.assertEqual(
+            self.buildmanager.iterate, self.buildmanager.iterators[-1])
+        self.assertFalse(self.slave.wasCalled('chrootFail'))
+
+    def test_iterate(self):
+        # The build manager iterates a normal build from start to finish.
+        self.startBuild()
+
+        log_path = os.path.join(self.buildmanager._cachepath, 'buildlog')
+        log = open(log_path, 'w')
+        log.write("I am a build log.")
+        log.close()
+
+        changes_path = os.path.join(
+            self.buildmanager.home, 'build-%s' % self.buildid,
+            'foo_1_source.changes')
+        changes = open(changes_path, 'w')
+        changes.write("I am a changes file.")
+        changes.close()
+
+        manifest_path = os.path.join(
+            self.buildmanager.home, 'build-%s' % self.buildid, 'manifest')
+        manifest = open(manifest_path, 'w')
+        manifest.write("I am a manifest file.")
+        manifest.close()
+
+        # After building the package, reap processes.
+        self.buildmanager.iterate(0)
+        expected_command = [
+            'processscanpath', 'scan-for-processes', self.buildid,
+            ]
+        self.assertEqual(
+            SourcePackageRecipeBuildState.BUILD_RECIPE, self.getState())
+        self.assertEqual(expected_command, self.buildmanager.commands[-1])
+        self.assertNotEqual(
+            self.buildmanager.iterate, self.buildmanager.iterators[-1])
+        self.assertFalse(self.slave.wasCalled('buildFail'))
+        self.assertEqual(
+            [((changes_path,), {}), ((manifest_path,), {})],
+            self.slave.addWaitingFile.calls)
+
+        # Control returns to the DebianBuildManager in the UMOUNT state.
+        self.buildmanager.iterateReap(self.getState(), 0)
+        expected_command = [
+            'umountpath', 'umount-chroot', self.buildid
+            ]
+        self.assertEqual(SourcePackageRecipeBuildState.UMOUNT, self.getState())
+        self.assertEqual(expected_command, self.buildmanager.commands[-1])
+        self.assertEqual(
+            self.buildmanager.iterate, self.buildmanager.iterators[-1])
+        self.assertFalse(self.slave.wasCalled('buildFail'))
+
+    def test_iterate_BUILD_RECIPE_install_build_deps_depfail(self):
+        # The build manager can detect dependency wait states.
+        self.startBuild()
+
+        log_path = os.path.join(self.buildmanager._cachepath, 'buildlog')
+        log = open(log_path, 'w')
+        log.write(
+            "The following packages have unmet dependencies:\n"
+            " pbuilder-satisfydepends-dummy : Depends: base-files (>= 1000)"
+            " but it is not going to be installed.\n")
+        log.close()
+
+        # The buildmanager calls depFail correctly and reaps processes.
+        self.buildmanager.iterate(RETCODE_FAILURE_INSTALL_BUILD_DEPS)
+        expected_command = [
+            'processscanpath', 'scan-for-processes', self.buildid
+            ]
+        self.assertEqual(
+            SourcePackageRecipeBuildState.BUILD_RECIPE, self.getState())
+        self.assertEqual(expected_command, self.buildmanager.commands[-1])
+        self.assertNotEqual(
+            self.buildmanager.iterate, self.buildmanager.iterators[-1])
+        self.assertFalse(self.slave.wasCalled('buildFail'))
+        self.assertEqual(
+            [(("base-files (>= 1000)",), {})], self.slave.depFail.calls)
+
+        # Control returns to the DebianBuildManager in the UMOUNT state.
+        self.buildmanager.iterateReap(self.getState(), 0)
+        expected_command = [
+            'umountpath', 'umount-chroot', self.buildid
+            ]
+        self.assertEqual(SourcePackageRecipeBuildState.UMOUNT, self.getState())
+        self.assertEqual(expected_command, self.buildmanager.commands[-1])
+        self.assertEqual(
+            self.buildmanager.iterate, self.buildmanager.iterators[-1])
+        self.assertFalse(self.slave.wasCalled('buildFail'))
+
+    def test_iterate_BUILD_RECIPE_install_build_deps_buildfail(self):
+        # If the build manager cannot detect a dependency wait from a
+        # build-dependency installation failure, it fails the build.
+        self.startBuild()
+
+        log_path = os.path.join(self.buildmanager._cachepath, 'buildlog')
+        log = open(log_path, 'w')
+        log.write("I am a failing build log.")
+        log.close()
+
+        # The buildmanager calls buildFail correctly and reaps processes.
+        self.buildmanager.iterate(RETCODE_FAILURE_INSTALL_BUILD_DEPS)
+        expected_command = [
+            'processscanpath', 'scan-for-processes', self.buildid
+            ]
+        self.assertEqual(
+            SourcePackageRecipeBuildState.BUILD_RECIPE, self.getState())
+        self.assertEqual(expected_command, self.buildmanager.commands[-1])
+        self.assertNotEqual(
+            self.buildmanager.iterate, self.buildmanager.iterators[-1])
+        self.assertTrue(self.slave.wasCalled('buildFail'))
+        self.assertFalse(self.slave.wasCalled('depFail'))
+
+        # Control returns to the DebianBuildManager in the UMOUNT state.
+        self.buildmanager.iterateReap(self.getState(), 0)
+        expected_command = [
+            'umountpath', 'umount-chroot', self.buildid
+            ]
+        self.assertEqual(SourcePackageRecipeBuildState.UMOUNT, self.getState())
+        self.assertEqual(expected_command, self.buildmanager.commands[-1])
+        self.assertEqual(
+            self.buildmanager.iterate, self.buildmanager.iterators[-1])