← Back to team overview

launchpad-reviewers team mailing list archive

Re: [Merge] lp:~wgrant/launchpad-buildd/cut-down-config into lp:launchpad-buildd

 

Review: Needs Fixing



Diff comments:

> === modified file 'debian/changelog'
> --- debian/changelog	2015-04-17 20:49:23 +0000
> +++ debian/changelog	2015-05-11 05:58:41 +0000
> @@ -1,6 +1,7 @@
>  launchpad-buildd (127) UNRELEASED; urgency=low
>  
>    * Refactor lpbuildd.binarypackage tests to be readable.
> +  * Drop duplicated paths from the config file.
>  
>   -- William Grant <wgrant@xxxxxxxxxx>  Sat, 18 Apr 2015 06:41:52 +1000
>  
> 
> === modified file 'debian/upgrade-config'
> --- debian/upgrade-config	2014-08-13 00:41:22 +0000
> +++ debian/upgrade-config	2015-05-11 05:58:41 +0000
> @@ -5,10 +5,13 @@
>  
>  """Upgrade a launchpad-buildd configuration file."""
>  
> +import os
>  import re
>  import sys
>  import subprocess
>  
> +from ConfigParser import SafeConfigParser
> +
>  import apt_pkg
>  
>  
> @@ -167,6 +170,28 @@
>      in_file.close()
>      out_file.close()
>  
> +def upgrade_to_127():
> +    print "Upgrading %s to version 127" % conf_file
> +    os.rename(conf_file, conf_file + "-prev127~")
> +
> +    config = SafeConfigParser()
> +    config.read(conf_file + "-prev127~")
> +
> +    # Remove all of the pointlessly configurable binary paths which are
> +    # all inside slavebin.
> +    config.set('slave', 'sharepath', '/usr/share/launchpad-buildd')
> +    deprecated_sections = [
> +        'allmanagers', 'debianmanager', 'sourcepackagerecipemanager',
> +        'livefilesystemmanager']
> +    for section in deprecated_sections:
> +        config.remove_section(section)
> +    config.remove_option('binarypackagemanager', 'sbuildpath')
> +    config.remove_option('translationtemplatesmanager', 'generatepath')
> +
> +    out_file = open(conf_file, "w")
> +    config.write(out_file)
> +    out_file.close()
> +

This procedure doesn't round-trip template-buildd-slave.conf correctly: it loses the comments at the top, and adds an extra newline at the end.  Could we just do like other upgrade functions do and parse the file by hand?

>  if __name__ == "__main__":
>      old_version = re.sub(r'[~-].*', '', old_version)
>      if version_compare(old_version, "12") < 0:
> @@ -191,3 +216,5 @@
>          upgrade_to_120()
>      if version_compare(old_version, "126") < 0:
>          upgrade_to_126()
> +    if version_compare(old_version, "127") < 0:
> +        upgrade_to_127()
> 
> === modified file 'lpbuildd/binarypackage.py'
> --- lpbuildd/binarypackage.py	2014-08-06 07:54:20 +0000
> +++ lpbuildd/binarypackage.py	2015-05-11 05:58:41 +0000
> @@ -2,6 +2,7 @@
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  
> +import os
>  import re
>  
>  from lpbuildd.debian import DebianBuildManager, DebianBuildState
> @@ -41,7 +42,7 @@
>  
>      def __init__(self, slave, buildid, **kwargs):
>          DebianBuildManager.__init__(self, slave, buildid, **kwargs)
> -        self._sbuildpath = slave._config.get("binarypackagemanager", "sbuildpath")
> +        self._sbuildpath = os.path.join(self._slavebin, "sbuild-package")
>          self._sbuildargs = slave._config.get("binarypackagemanager",
>                                               "sbuildargs").split(" ")
>  
> 
> === modified file 'lpbuildd/debian.py'
> --- lpbuildd/debian.py	2014-05-04 21:35:37 +0000
> +++ lpbuildd/debian.py	2015-05-11 05:58:41 +0000
> @@ -33,9 +33,10 @@
>  
>      def __init__(self, slave, buildid, **kwargs):
>          BuildManager.__init__(self, slave, buildid, **kwargs)
> -        self._updatepath = slave._config.get("debianmanager", "updatepath")
> -        self._sourcespath = slave._config.get("debianmanager", "sourcespath")
> -        self._cachepath = slave._config.get("slave","filecache")
> +        self._updatepath = os.path.join(self._slavebin, "update-debian-chroot")
> +        self._sourcespath = os.path.join(
> +            self._slavebin, "override-sources-list")
> +        self._cachepath = slave._config.get("slave", "filecache")
>          self._state = DebianBuildState.INIT
>          slave.emptyLog()
>          self.alreadyfailed = False
> 
> === modified file 'lpbuildd/livefs.py'
> --- lpbuildd/livefs.py	2014-06-27 12:19:31 +0000
> +++ lpbuildd/livefs.py	2015-05-11 05:58:41 +0000
> @@ -29,8 +29,7 @@
>  
>      def __init__(self, slave, buildid, **kwargs):
>          DebianBuildManager.__init__(self, slave, buildid, **kwargs)
> -        self.build_livefs_path = slave._config.get(
> -            "livefilesystemmanager", "buildlivefspath")
> +        self.build_livefs_path = os.path.join(self._slavebin, "buildlivefs")
>  
>      def initiate(self, files, chroot, extra_args):
>          """Initiate a build with a given set of files and chroot."""
> 
> === modified file 'lpbuildd/slave.py'
> --- lpbuildd/slave.py	2014-05-10 17:40:29 +0000
> +++ lpbuildd/slave.py	2015-05-11 05:58:41 +0000
> @@ -108,12 +108,14 @@
>          if reactor is None:
>              reactor = default_reactor
>          self._reactor = reactor
> -        self._preppath = slave._config.get("allmanagers", "preppath")
> -        self._unpackpath = slave._config.get("allmanagers", "unpackpath")
> -        self._cleanpath = slave._config.get("allmanagers", "cleanpath")
> -        self._mountpath = slave._config.get("allmanagers", "mountpath")
> -        self._umountpath = slave._config.get("allmanagers", "umountpath")
> -        self._scanpath = slave._config.get("allmanagers", "processscanpath")
> +        self._sharepath = slave._config.get("slave", "sharepath")
> +        self._slavebin = os.path.join(self._sharepath, "slavebin")
> +        self._preppath = os.path.join(self._slavebin, "slave-prep")
> +        self._unpackpath = os.path.join(self._slavebin, "unpack-chroot")
> +        self._cleanpath = os.path.join(self._slavebin, "remove-build")
> +        self._mountpath = os.path.join(self._slavebin, "mount-chroot")
> +        self._umountpath = os.path.join(self._slavebin, "umount-chroot")
> +        self._scanpath = os.path.join(self._slavebin, "scan-for-processes")
>          self._subprocess = None
>          self._reaped_states = set()
>          self.is_archive_private = False
> 
> === modified file 'lpbuildd/sourcepackagerecipe.py'
> --- lpbuildd/sourcepackagerecipe.py	2013-10-03 10:39:23 +0000
> +++ lpbuildd/sourcepackagerecipe.py	2015-05-11 05:58:41 +0000
> @@ -60,8 +60,7 @@
>          :param buildid: The id of the build (a str).
>          """
>          DebianBuildManager.__init__(self, slave, buildid)
> -        self.build_recipe_path = slave._config.get(
> -            "sourcepackagerecipemanager", "buildrecipepath")
> +        self.build_recipe_path = os.path.join(self._slavebin, "buildrecipe")
>  
>      def initiate(self, files, chroot, extra_args):
>          """Initiate a build with a given set of files and chroot.
> 
> === modified file 'lpbuildd/tests/buildd-slave-test.conf'
> --- lpbuildd/tests/buildd-slave-test.conf	2013-07-24 10:42:05 +0000
> +++ lpbuildd/tests/buildd-slave-test.conf	2015-05-11 05:58:41 +0000
> @@ -5,23 +5,7 @@
>  filecache = /var/tmp/buildd/filecache
>  bindhost = localhost
>  bindport = 8221
> -
> -[allmanagers]
> -unpackpath = /var/tmp/buildd/slavebin/unpack-chroot
> -cleanpath = /var/tmp/buildd/slavebin/remove-build
> -mountpath = /var/tmp/buildd/slavebin/mount-chroot
> -umountpath = /var/tmp/buildd/slavebin/umount-chroot
> -preppath = /var/tmp/buildd/slavebin/slave-prep
> -processscanpath = /var/tmp/buildd/slavebin/scan-for-processes
> -
> -[debianmanager]
> -sbuildpath = /var/tmp/buildd/slavebin/sbuild-package
> -sbuildargs = -dautobuild --nolog --batch
> -updatepath = /var/tmp/buildd/slavebin/update-debian-chroot
> -sourcespath = /usr/share/launchpad-buildd/slavebin/override-sources-list
> +sharepath = /var/tmp/buildd
>  
>  [binarypackagemanager]
> -sbuildpath = /var/tmp/buildd/slavebin/sbuild-package
>  sbuildargs = -dautobuild --nolog --batch
> -updatepath = /var/tmp/buildd/slavebin/update-debian-chroot
> -sourcespath = /usr/share/launchpad-buildd/slavebin/override-sources-list
> 
> === modified file 'lpbuildd/tests/test_binarypackage.py'
> --- lpbuildd/tests/test_binarypackage.py	2015-04-17 19:16:17 +0000
> +++ lpbuildd/tests/test_binarypackage.py	2015-05-11 05:58:41 +0000
> @@ -94,9 +94,9 @@
>          self.assertState(
>              BinaryPackageBuildState.SBUILD,
>              [
> -            'sbuildpath', 'sbuild-package', self.buildid, 'i386', 'warty',
> -            'sbuildargs', '--archive=ubuntu', '--dist=warty',
> -            '--architecture=i386', '--comp=main', 'foo_1.dsc',
> +            'sharepath/slavebin/sbuild-package', 'sbuild-package',
> +            self.buildid, 'i386', 'warty', 'sbuildargs', '--archive=ubuntu',
> +            '--dist=warty', '--architecture=i386', '--comp=main', 'foo_1.dsc',
>              ], True)
>          self.assertFalse(self.slave.wasCalled('chrootFail'))
>  
> @@ -115,13 +115,15 @@
>          self.buildmanager.iterate(exit_code)
>          self.assertState(
>              BinaryPackageBuildState.SBUILD,
> -            ['processscanpath', 'scan-for-processes', self.buildid], False)
> +            ['sharepath/slavebin/scan-for-processes', 'scan-for-processes',
> +             self.buildid], False)
>  
>      def assertUnmountsSanely(self):
>          self.buildmanager.iterateReap(self.getState(), 0)
>          self.assertState(
>              BinaryPackageBuildState.UMOUNT,
> -            ['umountpath', 'umount-chroot', self.buildid], True)
> +            ['sharepath/slavebin/umount-chroot', 'umount-chroot',
> +             self.buildid], True)
>  
>      def test_iterate(self):
>          # The build manager iterates a normal build from start to finish.
> @@ -153,7 +155,8 @@
>          self.buildmanager.abort()
>          self.assertState(
>              BinaryPackageBuildState.SBUILD,
> -            ['processscanpath', 'scan-for-processes', self.buildid], False)
> +            ['sharepath/slavebin/scan-for-processes', 'scan-for-processes',
> +             self.buildid], False)
>          self.assertFalse(self.slave.wasCalled('buildFail'))
>  
>          # If reaping completes successfully, the build manager returns
> @@ -171,7 +174,8 @@
>          self.buildmanager.abort()
>          self.assertState(
>              BinaryPackageBuildState.SBUILD,
> -            ['processscanpath', 'scan-for-processes', self.buildid], False)
> +            ['sharepath/slavebin/scan-for-processes', 'scan-for-processes',
> +             self.buildid], False)
>          self.assertFalse(self.slave.wasCalled('builderFail'))
>          reap_subprocess = self.buildmanager._subprocess
>  
> @@ -194,7 +198,8 @@
>          self.buildmanager.iterate(128 + 9)  # SIGKILL
>          self.assertState(
>              BinaryPackageBuildState.UMOUNT,
> -            ['umountpath', 'umount-chroot', self.buildid], True)
> +            ['sharepath/slavebin/umount-chroot', 'umount-chroot',
> +             self.buildid], True)
>  
>      def test_abort_between_subprocesses(self):
>          # If a build is aborted between subprocesses, the build manager
> @@ -207,12 +212,14 @@
>          self.buildmanager.abort()
>          self.assertState(
>              BinaryPackageBuildState.INIT,
> -            ['processscanpath', 'scan-for-processes', self.buildid], False)
> +            ['sharepath/slavebin/scan-for-processes', 'scan-for-processes',
> +             self.buildid], False)
>  
>          self.buildmanager.iterate(0)
>          self.assertState(
>              BinaryPackageBuildState.CLEANUP,
> -            ['cleanpath', 'remove-build', self.buildid], True)
> +            ['sharepath/slavebin/remove-build', 'remove-build', self.buildid],
> +            True)
>          self.assertFalse(self.slave.wasCalled('builderFail'))
>  
>      def test_missing_changes(self):
> 
> === modified file 'lpbuildd/tests/test_livefs.py'
> --- lpbuildd/tests/test_livefs.py	2014-07-24 11:44:04 +0000
> +++ lpbuildd/tests/test_livefs.py	2015-05-11 05:58:41 +0000
> @@ -72,8 +72,9 @@
>          self.assertEqual(
>              LiveFilesystemBuildState.BUILD_LIVEFS, self.getState())
>          expected_command = [
> -            "buildlivefspath", "buildlivefs", "--build-id", self.buildid,
> -            "--arch", "i386", "--project", "ubuntu", "--series", "saucy",
> +            "sharepath/slavebin/buildlivefs", "buildlivefs", "--build-id",
> +            self.buildid, "--arch", "i386", "--project", "ubuntu",
> +            "--series", "saucy",
>              ]
>          self.assertEqual(expected_command, self.buildmanager.commands[-1])
>          self.assertEqual(
> @@ -98,7 +99,8 @@
>          # After building the package, reap processes.
>          self.buildmanager.iterate(0)
>          expected_command = [
> -            "processscanpath", "scan-for-processes", self.buildid,
> +            "sharepath/slavebin/scan-for-processes", "scan-for-processes",
> +            self.buildid,
>              ]
>          self.assertEqual(
>              LiveFilesystemBuildState.BUILD_LIVEFS, self.getState())
> @@ -111,7 +113,8 @@
>  
>          # Control returns to the DebianBuildManager in the UMOUNT state.
>          self.buildmanager.iterateReap(self.getState(), 0)
> -        expected_command = ["umountpath", "umount-chroot", self.buildid]
> +        expected_command = [
> +            "sharepath/slavebin/umount-chroot", "umount-chroot", self.buildid]
>          self.assertEqual(LiveFilesystemBuildState.UMOUNT, self.getState())
>          self.assertEqual(expected_command, self.buildmanager.commands[-1])
>          self.assertEqual(
> 
> === modified file 'lpbuildd/tests/test_sourcepackagerecipe.py'
> --- lpbuildd/tests/test_sourcepackagerecipe.py	2013-10-03 10:39:23 +0000
> +++ lpbuildd/tests/test_sourcepackagerecipe.py	2015-05-11 05:58:41 +0000
> @@ -84,7 +84,7 @@
>          self.assertEqual(
>              SourcePackageRecipeBuildState.BUILD_RECIPE, self.getState())
>          expected_command = [
> -            'buildrecipepath', 'buildrecipe', self.buildid,
> +            'sharepath/slavebin/buildrecipe', 'buildrecipe', self.buildid,
>              'Steve\u1234'.encode('utf-8'), 'stevea@xxxxxxxxxxx',
>              'maverick', 'maverick', 'universe', 'puppies',
>              ]
> @@ -118,7 +118,8 @@
>          # After building the package, reap processes.
>          self.buildmanager.iterate(0)
>          expected_command = [
> -            'processscanpath', 'scan-for-processes', self.buildid,
> +            'sharepath/slavebin/scan-for-processes', 'scan-for-processes',
> +            self.buildid,
>              ]
>          self.assertEqual(
>              SourcePackageRecipeBuildState.BUILD_RECIPE, self.getState())
> @@ -133,7 +134,7 @@
>          # Control returns to the DebianBuildManager in the UMOUNT state.
>          self.buildmanager.iterateReap(self.getState(), 0)
>          expected_command = [
> -            'umountpath', 'umount-chroot', self.buildid
> +            'sharepath/slavebin/umount-chroot', 'umount-chroot', self.buildid
>              ]
>          self.assertEqual(SourcePackageRecipeBuildState.UMOUNT, self.getState())
>          self.assertEqual(expected_command, self.buildmanager.commands[-1])
> @@ -156,7 +157,8 @@
>          # The buildmanager calls depFail correctly and reaps processes.
>          self.buildmanager.iterate(RETCODE_FAILURE_INSTALL_BUILD_DEPS)
>          expected_command = [
> -            'processscanpath', 'scan-for-processes', self.buildid
> +            'sharepath/slavebin/scan-for-processes', 'scan-for-processes',
> +            self.buildid,
>              ]
>          self.assertEqual(
>              SourcePackageRecipeBuildState.BUILD_RECIPE, self.getState())
> @@ -170,7 +172,7 @@
>          # Control returns to the DebianBuildManager in the UMOUNT state.
>          self.buildmanager.iterateReap(self.getState(), 0)
>          expected_command = [
> -            'umountpath', 'umount-chroot', self.buildid
> +            'sharepath/slavebin/umount-chroot', 'umount-chroot', self.buildid,
>              ]
>          self.assertEqual(SourcePackageRecipeBuildState.UMOUNT, self.getState())
>          self.assertEqual(expected_command, self.buildmanager.commands[-1])
> @@ -191,7 +193,8 @@
>          # The buildmanager calls buildFail correctly and reaps processes.
>          self.buildmanager.iterate(RETCODE_FAILURE_INSTALL_BUILD_DEPS)
>          expected_command = [
> -            'processscanpath', 'scan-for-processes', self.buildid
> +            'sharepath/slavebin/scan-for-processes', 'scan-for-processes',
> +            self.buildid,
>              ]
>          self.assertEqual(
>              SourcePackageRecipeBuildState.BUILD_RECIPE, self.getState())
> @@ -204,7 +207,7 @@
>          # Control returns to the DebianBuildManager in the UMOUNT state.
>          self.buildmanager.iterateReap(self.getState(), 0)
>          expected_command = [
> -            'umountpath', 'umount-chroot', self.buildid
> +            'sharepath/slavebin/umount-chroot', 'umount-chroot', self.buildid,
>              ]
>          self.assertEqual(SourcePackageRecipeBuildState.UMOUNT, self.getState())
>          self.assertEqual(expected_command, self.buildmanager.commands[-1])
> 
> === modified file 'lpbuildd/tests/test_translationtemplatesbuildmanager.py'
> --- lpbuildd/tests/test_translationtemplatesbuildmanager.py	2013-09-26 09:03:14 +0000
> +++ lpbuildd/tests/test_translationtemplatesbuildmanager.py	2015-05-11 05:58:41 +0000
> @@ -82,7 +82,9 @@
>          self.assertEqual(
>              TranslationTemplatesBuildState.GENERATE, self.getState())
>          expected_command = [
> -            'generatepath', 'generatepath', self.buildid, url, 'resultarchive'
> +            'sharepath/slavebin/generate-translation-templates',
> +            'sharepath/slavebin/generate-translation-templates',
> +            self.buildid, url, 'resultarchive'
>              ]
>          self.assertEqual(expected_command, self.buildmanager.commands[-1])
>          self.assertEqual(
> @@ -101,7 +103,8 @@
>          # After generating templates, reap processes.
>          self.buildmanager.iterate(0)
>          expected_command = [
> -            'processscanpath', 'scan-for-processes', self.buildid
> +            'sharepath/slavebin/scan-for-processes', 'scan-for-processes',
> +            self.buildid,
>              ]
>          self.assertEqual(
>              TranslationTemplatesBuildState.GENERATE, self.getState())
> @@ -115,7 +118,7 @@
>          # The control returns to the DebianBuildManager in the UMOUNT state.
>          self.buildmanager.iterateReap(self.getState(), 0)
>          expected_command = [
> -            'umountpath', 'umount-chroot', self.buildid
> +            'sharepath/slavebin/umount-chroot', 'umount-chroot', self.buildid,
>              ]
>          self.assertEqual(
>              TranslationTemplatesBuildState.UMOUNT, self.getState())
> @@ -139,7 +142,7 @@
>          self.assertEqual(
>              TranslationTemplatesBuildState.UMOUNT, self.getState())
>          expected_command = [
> -            'umountpath', 'umount-chroot', self.buildid
> +            'sharepath/slavebin/umount-chroot', 'umount-chroot', self.buildid,
>              ]
>          self.assertEqual(expected_command, self.buildmanager.commands[-1])
>          self.assertEqual(
> @@ -159,7 +162,8 @@
>          # The buildmanager fails and reaps processes.
>          self.buildmanager.iterate(-1)
>          expected_command = [
> -            'processscanpath', 'scan-for-processes', self.buildid
> +            'sharepath/slavebin/scan-for-processes', 'scan-for-processes',
> +            self.buildid,
>              ]
>          self.assertEqual(
>              TranslationTemplatesBuildState.GENERATE, self.getState())
> @@ -173,7 +177,7 @@
>          self.assertEqual(
>              TranslationTemplatesBuildState.UMOUNT, self.getState())
>          expected_command = [
> -            'umountpath', 'umount-chroot', self.buildid
> +            'sharepath/slavebin/umount-chroot', 'umount-chroot', self.buildid
>              ]
>          self.assertEqual(expected_command, self.buildmanager.commands[-1])
>          self.assertEqual(
> 
> === modified file 'lpbuildd/translationtemplates.py'
> --- lpbuildd/translationtemplates.py	2013-07-25 17:26:10 +0000
> +++ lpbuildd/translationtemplates.py	2015-05-11 05:58:41 +0000
> @@ -25,8 +25,8 @@
>  
>      def __init__(self, slave, buildid):
>          super(TranslationTemplatesBuildManager, self).__init__(slave, buildid)
> -        self._generatepath = slave._config.get(
> -            "translationtemplatesmanager", "generatepath")
> +        self._generatepath = os.path.join(
> +            self._slavebin, "generate-translation-templates")
>          self._resultname = slave._config.get(
>              "translationtemplatesmanager", "resultarchive")
>  
> 
> === modified file 'template-buildd-slave.conf'
> --- template-buildd-slave.conf	2014-08-06 08:01:35 +0000
> +++ template-buildd-slave.conf	2015-05-11 05:58:41 +0000
> @@ -8,29 +8,10 @@
>  bindhost = @BINDHOST@
>  bindport = @BINDPORT@
>  ntphost = ntp.buildd
> -
> -[allmanagers]
> -preppath = /usr/share/launchpad-buildd/slavebin/slave-prep
> -unpackpath = /usr/share/launchpad-buildd/slavebin/unpack-chroot
> -cleanpath = /usr/share/launchpad-buildd/slavebin/remove-build
> -mountpath = /usr/share/launchpad-buildd/slavebin/mount-chroot
> -umountpath = /usr/share/launchpad-buildd/slavebin/umount-chroot
> -processscanpath = /usr/share/launchpad-buildd/slavebin/scan-for-processes
> -
> -[debianmanager]
> -updatepath = /usr/share/launchpad-buildd/slavebin/update-debian-chroot
> -sourcespath = /usr/share/launchpad-buildd/slavebin/override-sources-list
> +sharepath = /usr/share/launchpad-buildd
>  
>  [binarypackagemanager]
> -sbuildpath = /usr/share/launchpad-buildd/slavebin/sbuild-package
>  sbuildargs = --nolog --batch
>  
> -[sourcepackagerecipemanager]
> -buildrecipepath = /usr/share/launchpad-buildd/slavebin/buildrecipe
> -
>  [translationtemplatesmanager]
> -generatepath = /usr/share/launchpad-buildd/slavebin/generate-translation-templates
>  resultarchive = translation-templates.tar.gz
> -
> -[livefilesystemmanager]
> -buildlivefspath = /usr/share/launchpad-buildd/slavebin/buildlivefs
> 


-- 
https://code.launchpad.net/~wgrant/launchpad-buildd/cut-down-config/+merge/258734
Your team Launchpad code reviewers is subscribed to branch lp:launchpad-buildd.


References