launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad-buildd/translation-merge-states into lp:launchpad-buildd with lp:~cjwatson/launchpad-buildd/translation-refactor-chdir as a prerequisite.

Commit message:
Merge TranslationTemplatesBuildState.{INSTALL,GENERATE} into a single state.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/translation-merge-states/+merge/330434

This will make it easier to turn generate-translation-templates into a subclass of Operation, and brings it more into line with buildlivefs and buildsnap.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad-buildd/translation-merge-states into lp:launchpad-buildd.

=== modified file 'bin/generate-translation-templates'
--- bin/generate-translation-templates	2017-07-25 22:11:19 +0000
+++ bin/generate-translation-templates	2017-09-08 14:41:25 +0000
@@ -1,6 +1,6 @@
 #!/bin/sh
 #
-# Copyright 2010,2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # Buildd Slave tool to generate translation templates. Boiler plate code
@@ -53,6 +53,8 @@
    $1 || echo "Got error $? from '$1'."
 }
 
+$SUDO $CHROOT $BUILD_CHROOT apt-get install -y bzr intltool || exit 200
+
 # Copy pottery files to chroot.
 debug_exec "$SUDO $MKDIR -vp $BUILD_CHROOT$PYMODULES/$BUILDD_PACKAGE"
 debug_exec "$SUDO $TOUCH $BUILD_CHROOT$PYMODULES/$BUILDD_PACKAGE/__init__.py"
@@ -63,4 +65,5 @@
 # Enter chroot, switch back to unprivileged user, execute the generate script.
 $SUDO $CHROOT $BUILD_CHROOT \
   $SU - $USER \
-    -c "PYTHONPATH=$PYMODULES $GENERATE_SCRIPT $BRANCH_URL $RESULT_NAME"
+    -c "PYTHONPATH=$PYMODULES $GENERATE_SCRIPT $BRANCH_URL $RESULT_NAME" \
+  || exit 201

=== modified file 'debian/changelog'
--- debian/changelog	2017-09-08 14:41:25 +0000
+++ debian/changelog	2017-09-08 14:41:25 +0000
@@ -1,6 +1,8 @@
 launchpad-buildd (152) UNRELEASED; urgency=medium
 
   * Refactor lpbuildd.pottery.intltool to avoid calling chdir.
+  * Merge TranslationTemplatesBuildState.{INSTALL,GENERATE} into a single
+    state.
 
  -- Colin Watson <cjwatson@xxxxxxxxxx>  Fri, 08 Sep 2017 13:42:17 +0100
 

=== modified file 'lpbuildd/tests/test_translationtemplatesbuildmanager.py'
--- lpbuildd/tests/test_translationtemplatesbuildmanager.py	2017-08-22 15:55:44 +0000
+++ lpbuildd/tests/test_translationtemplatesbuildmanager.py	2017-09-08 14:41:25 +0000
@@ -14,6 +14,8 @@
 from lpbuildd.tests.fakeslave import FakeSlave
 from lpbuildd.tests.matchers import HasWaitingFiles
 from lpbuildd.translationtemplates import (
+    RETCODE_FAILURE_BUILD,
+    RETCODE_FAILURE_INSTALL,
     TranslationTemplatesBuildManager,
     TranslationTemplatesBuildState,
     )
@@ -65,23 +67,9 @@
         self.buildmanager.backend_name = original_backend_name
 
         # Skip states that are done in DebianBuildManager to the state
-        # directly before INSTALL.
+        # directly before GENERATE.
         self.buildmanager._state = TranslationTemplatesBuildState.UPDATE
 
-        # INSTALL: Install additional packages needed for this job into
-        # the chroot.
-        self.buildmanager.iterate(0)
-        self.assertEqual(
-            TranslationTemplatesBuildState.INSTALL, self.getState())
-        expected_command = [
-            '/usr/bin/sudo',
-            'sudo', 'chroot', self.chrootdir,
-            'apt-get',
-            ]
-        self.assertEqual(expected_command, self.buildmanager.commands[-1][:5])
-        self.assertEqual(
-            self.buildmanager.iterate, self.buildmanager.iterators[-1])
-
         # GENERATE: Run the slave's payload, the script that generates
         # templates.
         self.buildmanager.iterate(0)
@@ -136,34 +124,50 @@
             self.buildmanager.iterate, self.buildmanager.iterators[-1])
         self.assertFalse(self.slave.wasCalled('buildFail'))
 
-    def test_iterate_fail_INSTALL(self):
-        # See that a failing INSTALL is handled properly.
+    def test_iterate_fail_GENERATE_install(self):
+        # See that a GENERATE that fails at the install step is handled
+        # properly.
         url = 'lp:~my/branch'
         # The build manager's iterate() kicks off the consecutive states
         # after INIT.
         self.buildmanager.initiate(
             {}, 'chroot.tar.gz', {'series': 'xenial', 'branch_url': url})
 
-        # Skip states to the INSTALL state.
-        self.buildmanager._state = TranslationTemplatesBuildState.INSTALL
+        # Skip states to the GENERATE state.
+        self.buildmanager._state = TranslationTemplatesBuildState.GENERATE
 
-        # The buildmanager fails and iterates to the UMOUNT state.
-        self.buildmanager.iterate(-1)
+        # The buildmanager fails and reaps processes.
+        self.buildmanager.iterate(RETCODE_FAILURE_INSTALL)
         self.assertEqual(
-            TranslationTemplatesBuildState.UMOUNT, self.getState())
+            TranslationTemplatesBuildState.GENERATE, self.getState())
         expected_command = [
             'sharepath/slavebin/in-target', 'in-target',
-            'umount-chroot',
+            'scan-for-processes',
             '--backend=chroot', '--series=xenial', '--arch=i386',
             self.buildid,
             ]
         self.assertEqual(expected_command, self.buildmanager.commands[-1])
-        self.assertEqual(
+        self.assertNotEqual(
             self.buildmanager.iterate, self.buildmanager.iterators[-1])
         self.assertTrue(self.slave.wasCalled('chrootFail'))
 
-    def test_iterate_fail_GENERATE(self):
-        # See that a failing GENERATE is handled properly.
+        # The buildmanager iterates to the UMOUNT state.
+        self.buildmanager.iterateReap(self.getState(), 0)
+        self.assertEqual(
+            TranslationTemplatesBuildState.UMOUNT, self.getState())
+        expected_command = [
+            'sharepath/slavebin/in-target', 'in-target',
+            'umount-chroot',
+            '--backend=chroot', '--series=xenial', '--arch=i386',
+            self.buildid,
+            ]
+        self.assertEqual(expected_command, self.buildmanager.commands[-1])
+        self.assertEqual(
+            self.buildmanager.iterate, self.buildmanager.iterators[-1])
+
+    def test_iterate_fail_GENERATE_build(self):
+        # See that a GENERATE that fails at the build step is handled
+        # properly.
         url = 'lp:~my/branch'
         # The build manager's iterate() kicks off the consecutive states
         # after INIT.
@@ -174,7 +178,7 @@
         self.buildmanager._state = TranslationTemplatesBuildState.GENERATE
 
         # The buildmanager fails and reaps processes.
-        self.buildmanager.iterate(-1)
+        self.buildmanager.iterate(RETCODE_FAILURE_BUILD)
         expected_command = [
             'sharepath/slavebin/in-target', 'in-target',
             'scan-for-processes',

=== modified file 'lpbuildd/translationtemplates.py'
--- lpbuildd/translationtemplates.py	2017-08-05 09:43:43 +0000
+++ lpbuildd/translationtemplates.py	2017-09-08 14:41:25 +0000
@@ -1,15 +1,21 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
 
 import os
 
-from lpbuildd.debian import DebianBuildManager, DebianBuildState
+from lpbuildd.debian import (
+    DebianBuildManager,
+    DebianBuildState,
+    )
+
+
+RETCODE_FAILURE_INSTALL = 200
+RETCODE_FAILURE_BUILD = 201
 
 
 class TranslationTemplatesBuildState(DebianBuildState):
-    INSTALL = "INSTALL"
     GENERATE = "GENERATE"
 
 
@@ -21,7 +27,7 @@
     runs on the build slave.
     """
 
-    initial_build_state = TranslationTemplatesBuildState.INSTALL
+    initial_build_state = TranslationTemplatesBuildState.GENERATE
 
     def __init__(self, slave, buildid):
         super(TranslationTemplatesBuildManager, self).__init__(slave, buildid)
@@ -33,26 +39,10 @@
     def initiate(self, files, chroot, extra_args):
         """See `BuildManager`."""
         self._branch_url = extra_args['branch_url']
-        self._chroot_path = os.path.join(
-            self.home, 'build-' + self._buildid, 'chroot-autobuild')
 
         super(TranslationTemplatesBuildManager, self).initiate(
             files, chroot, extra_args)
 
-    def doInstall(self):
-        """Install packages required."""
-        required_packages = [
-            'bzr',
-            'intltool',
-            ]
-        command = ['apt-get', 'install', '-y'] + required_packages
-        chroot = ['sudo', 'chroot', self._chroot_path]
-        self.runSubProcess('/usr/bin/sudo', chroot + command)
-
-    # To satisfy DebianPackageManagers needs without having a misleading
-    # method name here.
-    doRunBuild = doInstall
-
     def doGenerate(self):
         """Generate templates."""
         command = [
@@ -60,37 +50,33 @@
             self._buildid, self._branch_url, self._resultname]
         self.runSubProcess(self._generatepath, command)
 
+    # Satisfy DebianPackageManager's needs without having a misleading
+    # method name here.
+    doRunBuild = doGenerate
+
     def gatherResults(self):
         """Gather the results of the build and add them to the file cache."""
-        # The file is inside the chroot, in the home directory of the buildd
+        # The file is inside the target, in the home directory of the buildd
         # user. Should be safe to assume the home dirs are named identically.
         path = os.path.join(self.home, self._resultname)
         if self.backend.path_exists(path):
             self.addWaitingFileFromBackend(path)
 
-    def iterate_INSTALL(self, success):
-        """Installation was done."""
-        if success == 0:
-            self._state = TranslationTemplatesBuildState.GENERATE
-            self.doGenerate()
-        else:
-            if not self.alreadyfailed:
-                self._slave.chrootFail()
-                self.alreadyfailed = True
-            self._state = TranslationTemplatesBuildState.UMOUNT
-            self.doUnmounting()
-
-    def iterate_GENERATE(self, success):
+    def iterate_GENERATE(self, retcode):
         """Template generation finished."""
-        if success == 0:
+        if retcode == 0:
             # It worked! Now let's bring in the harvest.
             self.gatherResults()
-            self.doReapProcesses(self._state)
         else:
             if not self.alreadyfailed:
-                self._slave.buildFail()
+                if retcode == RETCODE_FAILURE_INSTALL:
+                    self._slave.chrootFail()
+                elif retcode == RETCODE_FAILURE_BUILD:
+                    self._slave.buildFail()
+                else:
+                    self._slave.builderFail()
                 self.alreadyfailed = True
-            self.doReapProcesses(self._state)
+        self.doReapProcesses(self._state)
 
     def iterateReap_GENERATE(self, success):
         """Finished reaping after template generation."""