From 483eab5946aa783d3bd5805c4935d72d5b5cbb27 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 8 Jul 2012 11:19:15 -0500 Subject: [PATCH 01/47] vstudio: default to config='release', as documented --- master/buildbot/steps/vstudio.py | 2 +- master/buildbot/test/unit/test_steps_vstudio.py | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/master/buildbot/steps/vstudio.py b/master/buildbot/steps/vstudio.py index 592a5574195..2fb7a9605d0 100644 --- a/master/buildbot/steps/vstudio.py +++ b/master/buildbot/steps/vstudio.py @@ -108,7 +108,7 @@ def __init__(self, installdir = None, mode = "rebuild", projectfile = None, - config = None, + config = 'release', useenv = False, project = None, INCLUDE = [], diff --git a/master/buildbot/test/unit/test_steps_vstudio.py b/master/buildbot/test/unit/test_steps_vstudio.py index 62c1e49ada2..b7d106105d8 100644 --- a/master/buildbot/test/unit/test_steps_vstudio.py +++ b/master/buildbot/test/unit/test_steps_vstudio.py @@ -193,6 +193,10 @@ def setUp(self): def tearDown(self): return self.tearDownBuildStep() + def test_default_config(self): + vs = vstudio.VisualStudio() + self.assertEqual(vs.config, 'release') + def test_simple(self): self.setupStep(VCx()) self.expectCommands( From 276b67b4122930c0ff031216fe13a9e0f6335235 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 8 Jul 2012 17:01:04 -0500 Subject: [PATCH 02/47] fix 'don't know how to test this' skips One was correct (and only needed a change from ValueError to ConfigErorr). The other is but one in a large class of string-substitution syntax errors. We can't hope to catch them all, and the techniques required to catch a few would probably give us headaches. --- master/buildbot/process/properties.py | 5 +++-- .../test/unit/test_process_properties.py | 19 +++---------------- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/master/buildbot/process/properties.py b/master/buildbot/process/properties.py index 9152f150706..0fd6251daaa 100644 --- a/master/buildbot/process/properties.py +++ b/master/buildbot/process/properties.py @@ -424,11 +424,12 @@ def __init__(self, fmtstring, *args, **kwargs): self.fmtstring = fmtstring self.args = args self.kwargs = kwargs + if self.args and self.kwargs: + config.error("Interpolate takes either positional or keyword " + "substitutions, not both.") if not self.args: self.interpolations = {} self._parse(fmtstring) - if self.args and self.kwargs: - raise ValueError('Interpolate takes either positional or keyword substitutions, not both.') @staticmethod def _parse_prop(arg): diff --git a/master/buildbot/test/unit/test_process_properties.py b/master/buildbot/test/unit/test_process_properties.py index eb14af567b0..d64a1c93ccf 100644 --- a/master/buildbot/test/unit/test_process_properties.py +++ b/master/buildbot/test/unit/test_process_properties.py @@ -350,22 +350,9 @@ class TestInterpolateConfigure(unittest.TestCase, ConfigErrorsMixin): at configure time. """ - def test_invalid_params(self): - """ - Test that Interpolate rejects strings with both positional and keyword - substitutionss. - """ - self.assertRaises(ValueError, lambda : - Interpolate("%s %(foo)s", 1, foo=2)) - test_invalid_params.skip = "Don't know how to test this." - - def test_positional_string_keyword_args(self): - """ - """ - self.assertRaisesConfigError("keyword arguments passed to Interpolate " - "but uses postional substitutions", - lambda: Interpolate("%s", kwarg="test")) - test_positional_string_keyword_args.skip = "Don't know how to test this." + def test_invalid_args_and_kwargs(self): + self.assertRaisesConfigError("Interpolate takes either positional", + lambda : Interpolate("%s %(foo)s", 1, foo=2)) def test_invalid_selector(self): self.assertRaisesConfigError("invalid Interpolate selector 'garbage'", From 39146231710d33dc67fef1cfd81efb26d2945b95 Mon Sep 17 00:00:00 2001 From: Tom Prince Date: Sun, 8 Jul 2012 20:18:47 -0700 Subject: [PATCH 03/47] Extend make_master to optionally create the db object, as well. --- master/buildbot/test/fake/fakemaster.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/master/buildbot/test/fake/fakemaster.py b/master/buildbot/test/fake/fakemaster.py index 2c0a7be5c51..22267a59d8d 100644 --- a/master/buildbot/test/fake/fakemaster.py +++ b/master/buildbot/test/fake/fakemaster.py @@ -96,4 +96,9 @@ def _get_child_mock(self, **kw): return mock.Mock(**kw) # Leave this alias, in case we want to add more behavior later -make_master = FakeMaster +def make_master(wantDb=False, testcase=None, **kwargs): + master = FakeMaster(**kwargs) + if wantDb: + assert testcase is not None, "need testcase for wantDb" + master.db = fakedb.FakeDBConnector(testcase) + return master From 62f2a8de1c8a4ac194c06ecaba771734cf42fbde Mon Sep 17 00:00:00 2001 From: Tom Prince Date: Sun, 8 Jul 2012 18:30:41 -0700 Subject: [PATCH 04/47] Add StateMixin helper. This class factors out the helpers used for accessing object state in the database. --- master/buildbot/schedulers/base.py | 44 +---------- .../test/unit/test_schedulers_base.py | 50 ------------ master/buildbot/test/unit/test_util_state.py | 77 +++++++++++++++++++ master/buildbot/util/state.py | 26 +++++++ master/docs/developer/utils.rst | 38 +++++++++ master/docs/release-notes.rst | 3 + 6 files changed, 146 insertions(+), 92 deletions(-) create mode 100644 master/buildbot/test/unit/test_util_state.py create mode 100644 master/buildbot/util/state.py diff --git a/master/buildbot/schedulers/base.py b/master/buildbot/schedulers/base.py index a33799c0bf6..16c306e2bb5 100644 --- a/master/buildbot/schedulers/base.py +++ b/master/buildbot/schedulers/base.py @@ -20,8 +20,9 @@ from buildbot.process.properties import Properties from buildbot.util import ComparableMixin from buildbot import config, interfaces +from buildbot.util.state import StateMixin -class BaseScheduler(service.MultiService, ComparableMixin): +class BaseScheduler(service.MultiService, ComparableMixin, StateMixin): """ Base class for all schedulers; this provides the equipment to manage reconfigurations and to handle basic scheduler state. It also provides @@ -110,7 +111,6 @@ def __init__(self, name, builderNames, properties, # internal variables self._change_subscription = None self._change_consumption_lock = defer.DeferredLock() - self._objectid = None ## service handling @@ -125,46 +125,6 @@ def stopService(self): d.addCallback(lambda _ : service.MultiService.stopService(self)) return d - ## state management - - @defer.inlineCallbacks - def getState(self, *args, **kwargs): - """ - For use by subclasses; get a named state value from the scheduler's - state, defaulting to DEFAULT. - - @param name: name of the value to retrieve - @param default: (optional) value to return if C{name} is not present - @returns: state value via a Deferred - @raises KeyError: if C{name} is not present and no default is given - @raises TypeError: if JSON parsing fails - """ - # get the objectid, if not known - if self._objectid is None: - self._objectid = yield self.master.db.state.getObjectId(self.name, - self.__class__.__name__) - - rv = yield self.master.db.state.getState(self._objectid, *args, - **kwargs) - defer.returnValue(rv) - - @defer.inlineCallbacks - def setState(self, key, value): - """ - For use by subclasses; set a named state value in the scheduler's - persistent state. Note that value must be json-able. - - @param name: the name of the value to change - @param value: the value to set - must be a JSONable object - @param returns: Deferred - @raises TypeError: if JSONification fails - """ - # get the objectid, if not known - if self._objectid is None: - self._objectid = yield self.master.db.state.getObjectId(self.name, - self.__class__.__name__) - - yield self.master.db.state.setState(self._objectid, key, value) ## status queries diff --git a/master/buildbot/test/unit/test_schedulers_base.py b/master/buildbot/test/unit/test_schedulers_base.py index e87eca6f3b3..7eaa003d44c 100644 --- a/master/buildbot/test/unit/test_schedulers_base.py +++ b/master/buildbot/test/unit/test_schedulers_base.py @@ -62,56 +62,6 @@ def test_constructor_codebases_invalid(self): self.assertRaises(config.ConfigErrors, lambda : self.makeScheduler(codebases = codebases)) - def test_getState(self): - sched = self.makeScheduler() - self.db.state.fakeState('testsched', 'BaseScheduler', - fav_color=['red','purple']) - d = sched.getState('fav_color') - def check(res): - self.assertEqual(res, ['red', 'purple']) - d.addCallback(check) - return d - - def test_getState_default(self): - sched = self.makeScheduler() - d = sched.getState('fav_color', 'black') - def check(res): - self.assertEqual(res, 'black') - d.addCallback(check) - return d - - def test_getState_KeyError(self): - sched = self.makeScheduler() - self.db.state.fakeState('testsched', 'BaseScheduler', - fav_color=['red','purple']) - d = sched.getState('fav_book') - def cb(_): - self.fail("should not succeed") - def check_exc(f): - f.trap(KeyError) - pass - d.addCallbacks(cb, check_exc) - return d - - def test_setState(self): - sched = self.makeScheduler() - d = sched.setState('y', 14) - def check(_): - self.db.state.assertStateByClass('testsched', 'BaseScheduler', - y=14) - d.addCallback(check) - return d - - def test_setState_existing(self): - sched = self.makeScheduler() - self.db.state.fakeState('testsched', 'BaseScheduler', x=13) - d = sched.setState('x', 14) - def check(_): - self.db.state.assertStateByClass('testsched', 'BaseScheduler', - x=14) - d.addCallback(check) - return d - def test_listBuilderNames(self): sched = self.makeScheduler(builderNames=['x', 'y']) self.assertEqual(sched.listBuilderNames(), ['x', 'y']) diff --git a/master/buildbot/test/unit/test_util_state.py b/master/buildbot/test/unit/test_util_state.py new file mode 100644 index 00000000000..8e7adca2882 --- /dev/null +++ b/master/buildbot/test/unit/test_util_state.py @@ -0,0 +1,77 @@ +# This file is part of Buildbot. Buildbot is free software: you can +# redistribute it and/or modify it under the terms of the GNU General Public +# License as published by the Free Software Foundation, version 2. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS +# FOR A PARTICULAR PURPOSE. See the GNU General Public License for more +# details. +# +# You should have received a copy of the GNU General Public License along with +# this program; if not, write to the Free Software Foundation, Inc., 51 +# Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +# +# Copyright Buildbot Team Members + +from twisted.trial import unittest +from buildbot.util import state +from buildbot.test.fake.fakemaster import make_master + +class FakeObject(state.StateMixin): + name = "fake-name" + + def __init__(self, master): + self.master = master + +class TestStateMixin(unittest.TestCase): + + OBJECTID = 19 + + def setUp(self): + self.master = make_master(wantDb=True, testcase=self) + self.object = FakeObject(self.master) + + def test_getState(self): + self.master.db.state.fakeState('fake-name', 'FakeObject', + fav_color=['red','purple']) + d = self.object.getState('fav_color') + def check(res): + self.assertEqual(res, ['red', 'purple']) + d.addCallback(check) + return d + + def test_getState_default(self): + d = self.object.getState('fav_color', 'black') + def check(res): + self.assertEqual(res, 'black') + d.addCallback(check) + return d + + def test_getState_KeyError(self): + self.master.db.state.fakeState('fake-name', 'FakeObject', + fav_color=['red','purple']) + d = self.object.getState('fav_book') + def cb(_): + self.fail("should not succeed") + def check_exc(f): + f.trap(KeyError) + pass + d.addCallbacks(cb, check_exc) + return d + + def test_setState(self): + d = self.object.setState('y', 14) + def check(_): + self.master.db.state.assertStateByClass('fake-name', 'FakeObject', + y=14) + d.addCallback(check) + return d + + def test_setState_existing(self): + self.master.db.state.fakeState('fake-name', 'FakeObject', x=13) + d = self.object.setState('x', 14) + def check(_): + self.master.db.state.assertStateByClass('fake-name', 'FakeObject', + x=14) + d.addCallback(check) + return d diff --git a/master/buildbot/util/state.py b/master/buildbot/util/state.py new file mode 100644 index 00000000000..509f652764c --- /dev/null +++ b/master/buildbot/util/state.py @@ -0,0 +1,26 @@ +from twisted.internet import defer + +class StateMixin(object): + ## state management + + _objectid = None + + @defer.inlineCallbacks + def getState(self, *args, **kwargs): + # get the objectid, if not known + if self._objectid is None: + self._objectid = yield self.master.db.state.getObjectId(self.name, + self.__class__.__name__) + + rv = yield self.master.db.state.getState(self._objectid, *args, + **kwargs) + defer.returnValue(rv) + + @defer.inlineCallbacks + def setState(self, key, value): + # get the objectid, if not known + if self._objectid is None: + self._objectid = yield self.master.db.state.getObjectId(self.name, + self.__class__.__name__) + + yield self.master.db.state.setState(self._objectid, key, value) diff --git a/master/docs/developer/utils.rst b/master/docs/developer/utils.rst index 5b556819231..edcd0f4465f 100644 --- a/master/docs/developer/utils.rst +++ b/master/docs/developer/utils.rst @@ -524,3 +524,41 @@ buildbot.util.croniter This module is a copy of https://github.com/taichino/croniter, and provides suport for converting cron-like time specifications into actual times. + +buildbot.util.state +~~~~~~~~~~~~~~~~~~~ +.. py:module:: buildbot.util.state + +The classes in the :py:mod:`buildbot.util.subscription` module are used for dealing with object state stored in the database. + +.. py:class:: StateMixin + + This class provides helper methods for accessing the object state stored in the database. + + .. py:attribute:: name + + This must be set to the name to be used to identifiy this object in the database. + + .. py:attribute:: master + + This must point to the :py:class:`BuildMaster` object. + + .. py:method:: getState(name, default) + + :param name: name of the value to retrieve + :param default: (optional) value to return if `name` is not present + :returns: state value via a Deferred + :raises KeyError: if `name` is not present and no default is given + :raises TypeError: if JSON parsing fails + + Get a named state value from the object's state. + + .. py:method:: getState(name, value) + + :param name: the name of the value to change + :param value: the value to set - must be a JSONable object + :param returns: Deferred + :raises TypeError: if JSONification fails + + Set a named state value in the object's persistent state. + Note that value must be json-able. diff --git a/master/docs/release-notes.rst b/master/docs/release-notes.rst index 0d7fca3c475..112942ade29 100644 --- a/master/docs/release-notes.rst +++ b/master/docs/release-notes.rst @@ -76,6 +76,9 @@ Changes for Developers exceptions and failures raised will be captured and logged and the build shut down normally. +* The helper methods ``getState`` and ``setState`` from ``BaseScheduler`` have + been factored into ``buildbot.util.state.StateMixin`` for use elsewhere. + Features ~~~~~~~~ From 633926c9fd991701bfbb0be7a32e8b63745a1b46 Mon Sep 17 00:00:00 2001 From: Tom Prince Date: Sun, 8 Jul 2012 18:32:30 -0700 Subject: [PATCH 05/47] Add a nice __repr__ to GetProcessOutputMixin. This makes the test failure output more useful. --- master/buildbot/test/util/gpo.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/master/buildbot/test/util/gpo.py b/master/buildbot/test/util/gpo.py index 77c7458ef2b..e2a7f97c491 100644 --- a/master/buildbot/test/util/gpo.py +++ b/master/buildbot/test/util/gpo.py @@ -41,6 +41,10 @@ def check(self, test, bin, *args): test.assertEqual(args, self._args, "wrong args passed") return (self._stdout, self._stderr, self._exit) + def __repr__(self): + return "" % (self._bin, self._args) + + class GetProcessOutputMixin: def setUpGetProcessOutput(self): From b6e92b8abb0930cbe00c6737dc1854ff8e77c511 Mon Sep 17 00:00:00 2001 From: Tom Prince Date: Sun, 8 Jul 2012 18:33:34 -0700 Subject: [PATCH 06/47] Fix typo in GetProccessOutputMixin failure. --- master/buildbot/test/util/gpo.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/master/buildbot/test/util/gpo.py b/master/buildbot/test/util/gpo.py index e2a7f97c491..952e78ab89d 100644 --- a/master/buildbot/test/util/gpo.py +++ b/master/buildbot/test/util/gpo.py @@ -80,7 +80,7 @@ def patched_getProcessOutputAndValue(self, bin, args, env=None, **kwargs): self._check_env(env) if not self._expected_commands: - self.fail("got command %s %s when no further commands where expected" + self.fail("got command %s %s when no further commands were expected" % (bin, args)) expect = self._expected_commands.pop(0) From ef15fedc5dd5426677284265f8de3214fb9501f8 Mon Sep 17 00:00:00 2001 From: Tom Prince Date: Sun, 8 Jul 2012 21:34:21 -0700 Subject: [PATCH 07/47] Rewrite gitpoller. The original gitpoller had a number of issues: - it supported a single branch - it needed a seperate workdir for each poller. - it was easily confused by changes to the underlying workdir. The new gitpoller solves the above issues. - state is stored in the db, rather than implicitly in the repo. - the workdir is treated as a stateless cache. - it was easily confused by changes to the underlying workdir. Notes: - Only a single gitpoller can usefully be pointed at a given repo with a single db. Otherwise, the db state will get confused. - There is currently no support for migration from the old to new poller state. - `git init` supports an argument for a directory to create starting with git 1.6.5. Since we depend on git 1.7 this isn't an issue. --- master/buildbot/changes/gitpoller.py | 287 +++++-------- .../test/unit/test_changes_gitpoller.py | 382 +++++++++++++++++- master/buildbot/test/util/changesource.py | 3 +- master/docs/manual/cfg-changesources.rst | 40 +- master/docs/release-notes.rst | 7 + 5 files changed, 489 insertions(+), 230 deletions(-) diff --git a/master/buildbot/changes/gitpoller.py b/master/buildbot/changes/gitpoller.py index 45da7d13e68..4de0b24177e 100644 --- a/master/buildbot/changes/gitpoller.py +++ b/master/buildbot/changes/gitpoller.py @@ -13,58 +13,64 @@ # # Copyright Buildbot Team Members -import time -import tempfile import os +import urllib from twisted.python import log from twisted.internet import defer, utils -from buildbot.util import deferredLocked from buildbot.changes import base from buildbot.util import epoch2datetime +from buildbot.util.state import StateMixin +from buildbot import config -class GitPoller(base.PollingChangeSource): +class GitPoller(base.PollingChangeSource, StateMixin): """This source will poll a remote git repo for changes and submit them to the change master.""" - compare_attrs = ["repourl", "branch", "workdir", + compare_attrs = ["repourl", "branches", "workdir", "pollInterval", "gitbin", "usetimestamps", "category", "project"] - - def __init__(self, repourl, branch='master', + + def __init__(self, repourl, branches=None, branch=None, workdir=None, pollInterval=10*60, gitbin='git', usetimestamps=True, category=None, project=None, pollinterval=-2, fetch_refspec=None, - encoding='utf-8', name=None): + encoding='utf-8'): # for backward compatibility; the parameter used to be spelled with 'i' if pollinterval != -2: pollInterval = pollinterval - base.PollingChangeSource.__init__(self, name=name, pollInterval=pollInterval) + base.PollingChangeSource.__init__(self, name=repourl, + pollInterval=pollInterval) if project is None: project = '' + if branch and branches: + config.error("GitPoller: can't specify both branch and branches") + elif branch: + branches = [branch] + elif not branches: + branches = ['master'] + self.repourl = repourl - self.branch = branch - self.fetch_refspec = fetch_refspec + self.branches = branches self.encoding = encoding - self.lastChange = time.time() - self.lastPoll = time.time() self.gitbin = gitbin self.workdir = workdir self.usetimestamps = usetimestamps self.category = category self.project = project self.changeCount = 0 - self.commitInfo = {} - self.initLock = defer.DeferredLock() + self.lastRev = {} + + if fetch_refspec is not None: + config.error("GitPoller: fetch_refspec is no longer supported. " + "Instead, only the given branches are downloaded.") if self.workdir == None: - self.workdir = tempfile.gettempdir() + '/gitpoller_work' - log.msg("WARNING: gitpoller using deprecated temporary workdir " + - "'%s'; consider setting workdir=" % self.workdir) + self.workdir = 'gitpoller-work' def startService(self): # make our workdir absolute, relative to the master's basedir @@ -72,124 +78,70 @@ def startService(self): self.workdir = os.path.join(self.master.basedir, self.workdir) log.msg("gitpoller: using workdir '%s'" % self.workdir) - # initialize the repository we'll use to get changes; note that - # startService is not an event-driven method, so this method will - # instead acquire self.initLock immediately when it is called. - if not os.path.exists(self.workdir + r'/.git'): - d = self.initRepository() - d.addErrback(log.err, 'while initializing GitPoller repository') - else: - log.msg("GitPoller repository already exists") - - # call this *after* initRepository, so that the initLock is locked first - base.PollingChangeSource.startService(self) - - @deferredLocked('initLock') - def initRepository(self): - d = defer.succeed(None) - def make_dir(_): - dirpath = os.path.dirname(self.workdir.rstrip(os.sep)) - if not os.path.exists(dirpath): - log.msg('gitpoller: creating parent directories for workdir') - os.makedirs(dirpath) - d.addCallback(make_dir) - - def git_init(_): - log.msg('gitpoller: initializing working dir from %s' % self.repourl) - d = utils.getProcessOutputAndValue(self.gitbin, - ['init', '--bare', self.workdir], env=os.environ) - d.addCallback(self._convert_nonzero_to_failure) - d.addErrback(self._stop_on_failure) - return d - d.addCallback(git_init) - - def git_remote_add(_): - d = utils.getProcessOutputAndValue(self.gitbin, - ['remote', 'add', 'origin', self.repourl], - path=self.workdir, env=os.environ) - d.addCallback(self._convert_nonzero_to_failure) - d.addErrback(self._stop_on_failure) - return d - d.addCallback(git_remote_add) - - def git_fetch_origin(_): - args = ['fetch', 'origin'] - self._extend_with_fetch_refspec(args) - d = utils.getProcessOutputAndValue(self.gitbin, args, - path=self.workdir, env=os.environ) - d.addCallback(self._convert_nonzero_to_failure) - d.addErrback(self._stop_on_failure) - return d - d.addCallback(git_fetch_origin) - - def set_master(_): - log.msg('gitpoller: checking out %s' % self.branch) - if self.branch == 'master': # repo is already on branch 'master', so reset - d = utils.getProcessOutputAndValue(self.gitbin, - ['reset', '--hard', 'origin/%s' % self.branch], - path=self.workdir, env=os.environ) - else: - d = utils.getProcessOutputAndValue(self.gitbin, - ['checkout', '-b', self.branch, 'origin/%s' % self.branch], - path=self.workdir, env=os.environ) - d.addCallback(self._convert_nonzero_to_failure) - d.addErrback(self._stop_on_failure) - return d - d.addCallback(set_master) - def get_rev(_): - d = utils.getProcessOutputAndValue(self.gitbin, - ['rev-parse', self.branch], - path=self.workdir, env=os.environ) - d.addCallback(self._convert_nonzero_to_failure) - d.addErrback(self._stop_on_failure) - d.addCallback(lambda (out, err, code) : out.strip()) - return d - d.addCallback(get_rev) - def print_rev(rev): - log.msg("gitpoller: finished initializing working dir from %s at rev %s" - % (self.repourl, rev)) - d.addCallback(print_rev) + d = self.getState('lastRev', {}) + def setLastRev(lastRev): + self.lastRev = lastRev + d.addCallback(setLastRev) + + d.addCallback(lambda _: + base.PollingChangeSource.startService(self)) + d.addErrback(log.err, 'while initializing GitPoller repository') + return d def describe(self): status = "" if not self.master: status = "[STOPPED - check log]" - str = 'GitPoller watching the remote git repository %s, branch: %s %s' \ - % (self.repourl, self.branch, status) + str = ('GitPoller watching the remote git repository %s, branches: %s %s' + % (self.repourl, ', '.join(self.branches), status)) return str - @deferredLocked('initLock') + @defer.inlineCallbacks def poll(self): - d = self._get_changes() - d.addCallback(self._process_changes) - d.addErrback(self._process_changes_failure) - d.addCallback(self._catch_up) - d.addErrback(self._catch_up_failure) - return d + yield self._dovccmd('init', ['--bare', self.workdir]) + + refspecs = [ + '+%s:%s'% (branch, self._localBranch(branch)) + for branch in self.branches + ] + yield self._dovccmd('fetch', + [self.repourl] + refspecs, path=self.workdir) + + revs = {} + for branch in self.branches: + try: + revs[branch] = rev = yield self._dovccmd('rev-parse', + [self._localBranch(branch)], path=self.workdir) + yield self._process_changes(rev, branch) + except: + log.err(_why="trying to poll branch %s of %s" + % (branch, self.repourl)) + + self.lastRev.update(revs) + yield self.setState('lastRev', self.lastRev) def _get_commit_comments(self, rev): - args = ['log', rev, '--no-walk', r'--format=%s%n%b'] - d = utils.getProcessOutput(self.gitbin, args, path=self.workdir, env=os.environ, errortoo=False ) + args = [rev, '--no-walk', r'--format=%s%n%b'] + d = self._dovccmd('log', args, path=self.workdir) def process(git_output): - stripped_output = git_output.strip().decode(self.encoding) - if len(stripped_output) == 0: + git_output = git_output.decode(self.encoding) + if len(git_output) == 0: raise EnvironmentError('could not get commit comment for rev') - return stripped_output + return git_output d.addCallback(process) return d def _get_commit_timestamp(self, rev): # unix timestamp - args = ['log', rev, '--no-walk', r'--format=%ct'] - d = utils.getProcessOutput(self.gitbin, args, path=self.workdir, env=os.environ, errortoo=False ) + args = [rev, '--no-walk', r'--format=%ct'] + d = self._dovccmd('log', args, path=self.workdir) def process(git_output): - stripped_output = git_output.strip() if self.usetimestamps: try: - stamp = float(stripped_output) + stamp = float(git_output) except Exception, e: - log.msg('gitpoller: caught exception converting output \'%s\' to timestamp' % stripped_output) + log.msg('gitpoller: caught exception converting output \'%s\' to timestamp' % git_output) raise e return stamp else: @@ -198,8 +150,8 @@ def process(git_output): return d def _get_commit_files(self, rev): - args = ['log', rev, '--name-only', '--no-walk', r'--format=%n'] - d = utils.getProcessOutput(self.gitbin, args, path=self.workdir, env=os.environ, errortoo=False ) + args = [rev, '--name-only', '--no-walk', r'--format=%n'] + d = self._dovccmd('log', args, path=self.workdir) def process(git_output): fileList = git_output.split() return fileList @@ -207,38 +159,18 @@ def process(git_output): return d def _get_commit_author(self, rev): - args = ['log', rev, '--no-walk', r'--format=%aN <%aE>'] - d = utils.getProcessOutput(self.gitbin, args, path=self.workdir, env=os.environ, errortoo=False ) + args = [rev, '--no-walk', r'--format=%aN <%aE>'] + d = self._dovccmd('log', args, path=self.workdir) def process(git_output): - stripped_output = git_output.strip().decode(self.encoding) - if len(stripped_output) == 0: + git_output = git_output.decode(self.encoding) + if len(git_output) == 0: raise EnvironmentError('could not get commit author for rev') - return stripped_output + return git_output d.addCallback(process) return d - def _get_changes(self): - """Fetch changes from remote repository.""" - log.msg('gitpoller: polling git repo at %s' % self.repourl) - - self.lastPoll = time.time() - - # get a deferred object that performs the fetch - args = ['fetch', 'origin'] - self._extend_with_fetch_refspec(args) - - # This command always produces data on stderr, but we actually do not care - # about the stderr or stdout from this command. We set errortoo=True to - # avoid an errback from the deferred. The callback which will be added to this - # deferred will not use the response. - d = utils.getProcessOutput(self.gitbin, args, - path=self.workdir, - env=os.environ, errortoo=True ) - - return d - @defer.inlineCallbacks - def _process_changes(self, unused_output): + def _process_changes(self, newRev, branch): """ Read changes since last change. @@ -246,17 +178,20 @@ def _process_changes(self, unused_output): - Extract details from each commit. - Add changes to database. """ + + lastRev = self.lastRev.get(branch) + self.lastRev[branch] = newRev + if not lastRev: + return + # get the change list - revListArgs = ['log', '%s..origin/%s' % (self.branch, self.branch), r'--format=%H'] + revListArgs = ['%s..%s' % (lastRev, newRev), r'--format=%H'] self.changeCount = 0 - results = yield utils.getProcessOutput(self.gitbin, revListArgs, - path=self.workdir, env=os.environ, errortoo=False ) + results = yield self._dovccmd('log', revListArgs, path=self.workdir) + # process oldest change first revList = results.split() - if not revList: - return - revList.reverse() self.changeCount = len(revList) @@ -286,52 +221,24 @@ def _process_changes(self, unused_output): files=files, comments=comments, when_timestamp=epoch2datetime(timestamp), - branch=self.branch, + branch=branch, category=self.category, project=self.project, repository=self.repourl, src='git') - def _process_changes_failure(self, f): - log.msg('gitpoller: repo poll failed') - log.err(f) - # eat the failure to continue along the defered chain - we still want to catch up - return None - - def _catch_up(self, res): - """Update repository to record last seen change.""" - if self.changeCount == 0: - log.msg('gitpoller: no changes, no catch_up') - return - log.msg('gitpoller: catching up tracking branch') - args = ['reset', '--hard', 'origin/%s' % (self.branch,)] - d = utils.getProcessOutputAndValue(self.gitbin, args, path=self.workdir, env=os.environ) - d.addCallback(self._convert_nonzero_to_failure) + def _dovccmd(self, command, args, path=None): + d = utils.getProcessOutputAndValue(self.gitbin, + [command] + args, path=path, env=os.environ) + def _convert_nonzero_to_failure(res): + "utility method to handle the result of getProcessOutputAndValue" + (stdout, stderr, code) = res + if code != 0: + raise EnvironmentError('command failed with exit code %d: %s' + % (code, stderr)) + return stdout.strip() + d.addCallback(_convert_nonzero_to_failure) return d - def _catch_up_failure(self, f): - log.err(f) - log.msg('gitpoller: please resolve issues in local repo: %s' % self.workdir) - # this used to stop the service, but this is (a) unfriendly to tests and (b) - # likely to leave the error message lost in a sea of other log messages - - def _convert_nonzero_to_failure(self, res): - "utility method to handle the result of getProcessOutputAndValue" - (stdout, stderr, code) = res - if code != 0: - raise EnvironmentError('command failed with exit code %d: %s' % (code, stderr)) - return (stdout, stderr, code) - - def _stop_on_failure(self, f): - "utility method to stop the service when a failure occurs" - if self.running: - d = defer.maybeDeferred(lambda : self.stopService()) - d.addErrback(log.err, 'while stopping broken GitPoller service') - return f - - def _extend_with_fetch_refspec(self, args): - if self.fetch_refspec: - if type(self.fetch_refspec) in (list,set): - args.extend(self.fetch_refspec) - else: - args.append(self.fetch_refspec) + def _localBranch(self, branch): + return "refs/buildbot/%s/%s" % (urllib.quote(self.repourl, ''), branch) diff --git a/master/buildbot/test/unit/test_changes_gitpoller.py b/master/buildbot/test/unit/test_changes_gitpoller.py index 1db6e545cf8..97df95818c6 100644 --- a/master/buildbot/test/unit/test_changes_gitpoller.py +++ b/master/buildbot/test/unit/test_changes_gitpoller.py @@ -17,7 +17,7 @@ from twisted.trial import unittest from twisted.internet import defer from buildbot.changes import gitpoller -from buildbot.test.util import changesource, gpo +from buildbot.test.util import changesource, config, gpo from buildbot.util import epoch2datetime # Test that environment variables get propagated to subprocesses (See #2116) @@ -54,7 +54,7 @@ def eb_empty(f): d.addCallback(lambda _: self.assertAllCommandsRan()) # and the method shouldn't supress any exceptions - self.expectCommands(gpo.Expect('git', *args).stderr("some error")) + self.expectCommands(gpo.Expect('git', *args).exit(1)) def call_exception(_): return methodToTest(self.dummyRevStr) d.addCallback(call_exception) @@ -93,8 +93,7 @@ def test_get_commit_files(self): filesStr = 'file1\nfile2' return self._perform_git_output_test(self.poller._get_commit_files, ['log', self.dummyRevStr, '--name-only', '--no-walk', '--format=%n'], - filesStr, - filesStr.split(), emptyRaisesException=False) + filesStr, filesStr.split(), emptyRaisesException=False) def test_get_commit_timestamp(self): stampStr = '1273258009' @@ -108,11 +107,15 @@ class TestGitPoller(gpo.GetProcessOutputMixin, changesource.ChangeSourceMixin, unittest.TestCase): + POLLERID = 100 + REPOURL = 'git@example.com:foo/baz.git' + REPOURL_QUOTED = 'git%40example.com%3Afoo%2Fbaz.git' + def setUp(self): self.setUpGetProcessOutput() d = self.setUpChangeSource() def create_poller(_): - self.poller = gitpoller.GitPoller('git@example.com:foo/baz.git') + self.poller = gitpoller.GitPoller(self.REPOURL) self.poller.master = self.master d.addCallback(create_poller) return d @@ -123,23 +126,301 @@ def tearDown(self): def test_describe(self): self.assertSubstring("GitPoller", self.poller.describe()) - def test_gitbin_default(self): - self.assertEqual(self.poller.gitbin, "git") + def test_poll_initial(self): + self.expectCommands( + gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), + gpo.Expect('git', 'fetch', self.REPOURL, + '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED), + gpo.Expect('git', 'rev-parse', + 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .stdout('bf0b01df6d00ae8d1ffa0b2e2acbe642a6cd35d5\n'), + ) + + d = self.poller.poll() + @d.addCallback + def cb(_): + self.assertAllCommandsRan() + self.assertEqual(self.poller.lastRev, { + 'master': 'bf0b01df6d00ae8d1ffa0b2e2acbe642a6cd35d5' + }) + self.master.db.state.assertStateByClass( + name=self.REPOURL, class_name='GitPoller', + lastRev={ + 'master': 'bf0b01df6d00ae8d1ffa0b2e2acbe642a6cd35d5' + }) + return d + + def test_poll_failInit(self): + self.expectCommands( + gpo.Expect('git', 'init', '--bare', 'gitpoller-work') + .exit(1), + ) + + d = self.assertFailure(self.poller.poll(), EnvironmentError) + + d.addCallback(lambda _: self.assertAllCommandsRan) + return d + + def test_poll_failFetch(self): + self.expectCommands( + gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), + gpo.Expect('git', 'fetch', self.REPOURL, + '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .exit(1), + ) + + d = self.assertFailure(self.poller.poll(), EnvironmentError) + d.addCallback(lambda _: self.assertAllCommandsRan) + return d + + def test_poll_failRevParse(self): + self.expectCommands( + gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), + gpo.Expect('git', 'fetch', self.REPOURL, + '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED), + gpo.Expect('git', 'rev-parse', + 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .exit(1), + ) + + d = self.poller.poll() + @d.addCallback + def cb(_): + self.assertAllCommandsRan() + self.assertEqual(len(self.flushLoggedErrors()), 1) + self.assertEqual(self.poller.lastRev, {}) + + def test_poll_failLog(self): + self.expectCommands( + gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), + gpo.Expect('git', 'fetch', self.REPOURL, + '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED), + gpo.Expect('git', 'rev-parse', + 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .stdout('4423cdbcbb89c14e50dd5f4152415afd686c5241\n'), + gpo.Expect('git', 'log', + 'fa3ae8ed68e664d4db24798611b352e3c6509930..4423cdbcbb89c14e50dd5f4152415afd686c5241', + '--format=%H') + .exit(1), + ) + + # do the poll + self.poller.lastRev = { + 'master': 'fa3ae8ed68e664d4db24798611b352e3c6509930' + } + d = self.poller.poll() + + @d.addCallback + def cb(_): + self.assertAllCommandsRan() + self.assertEqual(len(self.flushLoggedErrors()), 1) + self.assertEqual(self.poller.lastRev, { + 'master': '4423cdbcbb89c14e50dd5f4152415afd686c5241' + }) + + def test_poll_nothingNew(self): + # Test that environment variables get propagated to subprocesses + # (See #2116) + self.patch(os, 'environ', {'ENVVAR': 'TRUE'}) + self.addGetProcessOutputExpectEnv({'ENVVAR': 'TRUE'}) + + + self.expectCommands( + gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), + gpo.Expect('git', 'fetch', self.REPOURL, + '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .stdout('no interesting output'), + gpo.Expect('git', 'rev-parse', + 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .stdout('4423cdbcbb89c14e50dd5f4152415afd686c5241\n'), + gpo.Expect('git', 'log', + '4423cdbcbb89c14e50dd5f4152415afd686c5241..4423cdbcbb89c14e50dd5f4152415afd686c5241', + '--format=%H') + .stdout(''), + ) + + self.poller.lastRev = { + 'master': '4423cdbcbb89c14e50dd5f4152415afd686c5241' + } + d = self.poller.poll() + @d.addCallback + def cb(_): + self.assertAllCommandsRan() + self.master.db.state.assertStateByClass( + name=self.REPOURL, class_name='GitPoller', + lastRev={ + 'master': '4423cdbcbb89c14e50dd5f4152415afd686c5241' + }) + return d + + def test_poll_multipleBranches_initial(self): + self.expectCommands( + gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), + gpo.Expect('git', 'fetch', self.REPOURL, + '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED, + '+release:refs/buildbot/%s/release' % self.REPOURL_QUOTED), + gpo.Expect('git', 'rev-parse', + 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .stdout('4423cdbcbb89c14e50dd5f4152415afd686c5241\n'), + gpo.Expect('git', 'rev-parse', + 'refs/buildbot/%s/release' % self.REPOURL_QUOTED) + .stdout('9118f4ab71963d23d02d4bdc54876ac8bf05acf2'), + ) + + # do the poll + self.poller.branches = ['master', 'release'] + d = self.poller.poll() + @d.addCallback + def cb(_): + self.assertAllCommandsRan() + self.assertEqual(self.poller.lastRev, { + 'master': '4423cdbcbb89c14e50dd5f4152415afd686c5241', + 'release': '9118f4ab71963d23d02d4bdc54876ac8bf05acf2' + }) + + return d + + + def test_poll_multipleBranches(self): + self.expectCommands( + gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), + gpo.Expect('git', 'fetch', self.REPOURL, + '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED, + '+release:refs/buildbot/%s/release' % self.REPOURL_QUOTED), + gpo.Expect('git', 'rev-parse', + 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .stdout('4423cdbcbb89c14e50dd5f4152415afd686c5241\n'), + gpo.Expect('git', 'log', + 'fa3ae8ed68e664d4db24798611b352e3c6509930..4423cdbcbb89c14e50dd5f4152415afd686c5241', + '--format=%H') + .stdout('\n'.join([ + '64a5dc2a4bd4f558b5dd193d47c83c7d7abc9a1a', + '4423cdbcbb89c14e50dd5f4152415afd686c5241'])), + gpo.Expect('git', 'rev-parse', + 'refs/buildbot/%s/release' % self.REPOURL_QUOTED) + .stdout('9118f4ab71963d23d02d4bdc54876ac8bf05acf2'), + gpo.Expect('git', 'log', + 'bf0b01df6d00ae8d1ffa0b2e2acbe642a6cd35d5..9118f4ab71963d23d02d4bdc54876ac8bf05acf2', + '--format=%H') + .stdout( '\n'.join([ + '9118f4ab71963d23d02d4bdc54876ac8bf05acf2' + ])), + ) + + # and patch out the _get_commit_foo methods which were already tested + # above + def timestamp(rev): + return defer.succeed(1273258009.0) + self.patch(self.poller, '_get_commit_timestamp', timestamp) + def author(rev): + return defer.succeed('by:' + rev[:8]) + self.patch(self.poller, '_get_commit_author', author) + def files(rev): + return defer.succeed(['/etc/' + rev[:3]]) + self.patch(self.poller, '_get_commit_files', files) + def comments(rev): + return defer.succeed('hello!') + self.patch(self.poller, '_get_commit_comments', comments) - def test_poll(self): - # Test that environment variables get propagated to subprocesses (See #2116) - self.patch(os, 'environ', {'TEST_THAT_ENVIRONMENT_GETS_PASSED_TO_SUBPROCESSES': 'TRUE'}) - self.addGetProcessOutputExpectEnv({'TEST_THAT_ENVIRONMENT_GETS_PASSED_TO_SUBPROCESSES': 'TRUE'}) + # do the poll + self.poller.branches = ['master', 'release'] + self.poller.lastRev = { + 'master': 'fa3ae8ed68e664d4db24798611b352e3c6509930', + 'release': 'bf0b01df6d00ae8d1ffa0b2e2acbe642a6cd35d5' + } + d = self.poller.poll() + @d.addCallback + def cb(_): + self.assertAllCommandsRan() + self.assertEqual(self.poller.lastRev, { + 'master': '4423cdbcbb89c14e50dd5f4152415afd686c5241', + 'release': '9118f4ab71963d23d02d4bdc54876ac8bf05acf2' + }) + + self.assertEqual(len(self.changes_added), 3) + + self.assertEqual(self.changes_added[0]['author'], 'by:4423cdbc') + self.assertEqual(self.changes_added[0]['when_timestamp'], + epoch2datetime(1273258009)) + self.assertEqual(self.changes_added[0]['comments'], 'hello!') + self.assertEqual(self.changes_added[0]['branch'], 'master') + self.assertEqual(self.changes_added[0]['files'], [ '/etc/442' ]) + self.assertEqual(self.changes_added[0]['src'], 'git') + + self.assertEqual(self.changes_added[1]['author'], 'by:64a5dc2a') + self.assertEqual(self.changes_added[1]['when_timestamp'], + epoch2datetime(1273258009)) + self.assertEqual(self.changes_added[1]['comments'], 'hello!') + self.assertEqual(self.changes_added[1]['files'], [ '/etc/64a' ]) + self.assertEqual(self.changes_added[1]['src'], 'git') + + self.assertEqual(self.changes_added[2]['author'], 'by:9118f4ab') + self.assertEqual(self.changes_added[2]['when_timestamp'], + epoch2datetime(1273258009)) + self.assertEqual(self.changes_added[2]['comments'], 'hello!') + self.assertEqual(self.changes_added[2]['files'], [ '/etc/911' ]) + self.assertEqual(self.changes_added[2]['src'], 'git') + + return d + + + def test_poll_noChanges(self): + # Test that environment variables get propagated to subprocesses + # (See #2116) + self.patch(os, 'environ', {'ENVVAR': 'TRUE'}) + self.addGetProcessOutputExpectEnv({'ENVVAR': 'TRUE'}) + + + self.expectCommands( + gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), + gpo.Expect('git', 'fetch', self.REPOURL, + '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .stdout('no interesting output'), + gpo.Expect('git', 'rev-parse', + 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .stdout('4423cdbcbb89c14e50dd5f4152415afd686c5241\n'), + gpo.Expect('git', 'log', + '4423cdbcbb89c14e50dd5f4152415afd686c5241..4423cdbcbb89c14e50dd5f4152415afd686c5241', + '--format=%H') + .stdout(''), + ) + + self.poller.lastRev = { + 'master': '4423cdbcbb89c14e50dd5f4152415afd686c5241' + } + d = self.poller.poll() + @d.addCallback + def cb(_): + self.assertAllCommandsRan() + self.assertEqual(self.poller.lastRev, { + 'master': '4423cdbcbb89c14e50dd5f4152415afd686c5241' + }) + return d + + def test_poll_old(self): + # Test that environment variables get propagated to subprocesses + # (See #2116) + self.patch(os, 'environ', {'ENVVAR': 'TRUE'}) + self.addGetProcessOutputExpectEnv({'ENVVAR': 'TRUE'}) # patch out getProcessOutput and getProcessOutputAndValue for the # benefit of the _get_changes method self.expectCommands( - gpo.Expect('git', 'fetch', 'origin').stdout('no interesting output'), - gpo.Expect('git', 'log', 'master..origin/master', '--format=%H').stdout( - '\n'.join([ + gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), + gpo.Expect('git', 'fetch', self.REPOURL, + '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .stdout('no interesting output'), + gpo.Expect('git', 'rev-parse', + 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .stdout('4423cdbcbb89c14e50dd5f4152415afd686c5241\n'), + gpo.Expect('git', 'log', + 'fa3ae8ed68e664d4db24798611b352e3c6509930..4423cdbcbb89c14e50dd5f4152415afd686c5241', + '--format=%H') + .stdout('\n'.join([ '64a5dc2a4bd4f558b5dd193d47c83c7d7abc9a1a', - '4423cdbcbb89c14e50dd5f4152415afd686c5241'])), - gpo.Expect('git', 'reset', '--hard', 'origin/master').stdout('done')) + '4423cdbcbb89c14e50dd5f4152415afd686c5241' + ])), + ) # and patch out the _get_commit_foo methods which were already tested # above @@ -157,10 +438,16 @@ def comments(rev): self.patch(self.poller, '_get_commit_comments', comments) # do the poll + self.poller.lastRev = { + 'master': 'fa3ae8ed68e664d4db24798611b352e3c6509930' + } d = self.poller.poll() # check the results def check_changes(_): + self.assertEqual(self.poller.lastRev, { + 'master': '4423cdbcbb89c14e50dd5f4152415afd686c5241' + }) self.assertEqual(len(self.changes_added), 2) self.assertEqual(self.changes_added[0]['author'], 'by:4423cdbc') self.assertEqual(self.changes_added[0]['when_timestamp'], @@ -176,6 +463,69 @@ def check_changes(_): self.assertEqual(self.changes_added[1]['files'], [ '/etc/64a' ]) self.assertEqual(self.changes_added[1]['src'], 'git') self.assertAllCommandsRan() + + self.master.db.state.assertStateByClass( + name=self.REPOURL, class_name='GitPoller', + lastRev={ + 'master': '4423cdbcbb89c14e50dd5f4152415afd686c5241' + }) d.addCallback(check_changes) return d + + def test_startService(self): + d = self.poller.startService() + def check(_): + self.assertEqual(self.poller.workdir, 'basedir/gitpoller-work') + self.assertEqual(self.poller.lastRev, {}) + self.assertTrue(self.poller.running) + d.addCallback(check) + return d + + def test_startService_loadLastRev(self): + self.master.db.state.fakeState( + name=self.REPOURL, class_name='GitPoller', + lastRev={"master": "fa3ae8ed68e664d4db24798611b352e3c6509930"}, + ) + + d = self.poller.startService() + def check(_): + self.assertEqual(self.poller.lastRev, { + "master": "fa3ae8ed68e664d4db24798611b352e3c6509930" + }) + d.addCallback(check) + return d + + + +class TestGitPollerConstructor(unittest.TestCase, config.ConfigErrorsMixin): + def test_deprecatedFetchRefspec(self): + self.assertRaisesConfigError("fetch_refspec is no longer supported", + lambda: gitpoller.GitPoller("/tmp/git.git", + fetch_refspec='not-supported')) + + def test_oldPollInterval(self): + poller = gitpoller.GitPoller("/tmp/git.git", pollinterval=10) + self.assertEqual(poller.pollInterval, 10) + + def test_branches_default(self): + poller = gitpoller.GitPoller("/tmp/git.git") + self.assertEqual(poller.branches, ["master"]) + + def test_branches_oldBranch(self): + poller = gitpoller.GitPoller("/tmp/git.git", branch='magic') + self.assertEqual(poller.branches, ["magic"]) + + def test_branches(self): + poller = gitpoller.GitPoller("/tmp/git.git", + branches=['magic', 'marker']) + self.assertEqual(poller.branches, ["magic", "marker"]) + + def test_branches_andBranch(self): + self.assertRaisesConfigError("can't specify both branch and branches", + lambda: gitpoller.GitPoller("/tmp/git.git", + branch='bad', branches=['listy'])) + + def test_gitbin_default(self): + poller = gitpoller.GitPoller("/tmp/git.git") + self.assertEqual(poller.gitbin, "git") diff --git a/master/buildbot/test/util/changesource.py b/master/buildbot/test/util/changesource.py index cb07f6edfbb..172f1584da0 100644 --- a/master/buildbot/test/util/changesource.py +++ b/master/buildbot/test/util/changesource.py @@ -16,6 +16,7 @@ import mock from twisted.internet import defer from twisted.trial import unittest +from buildbot.test.fake.fakemaster import make_master class ChangeSourceMixin(object): """ @@ -43,7 +44,7 @@ def addChange(**kwargs): "non-ascii string for key '%s': %r" % (k,v)) self.changes_added.append(kwargs) return defer.succeed(mock.Mock()) - self.master = mock.Mock() + self.master = make_master(testcase=self, wantDb=True) self.master.addChange = addChange return defer.succeed(None) diff --git a/master/docs/manual/cfg-changesources.rst b/master/docs/manual/cfg-changesources.rst index 836f80dad43..5d9f82a30f3 100644 --- a/master/docs/manual/cfg-changesources.rst +++ b/master/docs/manual/cfg-changesources.rst @@ -998,10 +998,11 @@ GitPoller If you cannot take advantage of post-receive hooks as provided by :file:`contrib/git_buildbot.py` for example, then you can use the :bb:chsrc:`GitPoller`. -The :bb:chsrc:`GitPoller` periodically fetches from a remote git repository and -processes any changes. It requires its own working directory for operation, which -can be specified via the ``workdir`` property. By default a temporary directory will -be used. +The :bb:chsrc:`GitPoller` periodically fetches from a remote git repository and processes any changes. +It requires its own working directory for operation. +The default should be adequate, but it can be overridden via the ``workdir`` property. + +.. note:: There can only be a single `GitPoller` pointed at any given repository. The :bb:chsrc:`GitPoller` requires git-1.7 and later. It accepts the following arguments: @@ -1011,14 +1012,12 @@ arguments: ``git@example.com:foobaz/myrepo.git`` (see the :command:`git fetch` help for more info on git-url formats) -``branch`` - the desired branch to fetch, will default to ``'master'`` +``branches`` + a list of the branches to fetch, will default to ``['master']`` -``workdir`` - the directory where the poller should keep its local repository. will - default to :samp:`{tempdir}/gitpoller_work`, which is probably not - what you want. If this is a relative path, it will be interpreted - relative to the master's basedir. +``branch`` + accepts a single branch name to fetch. + Exists for backwards compatability with old configurations. ``pollinterval`` interval in seconds between polls, default is 10 minutes @@ -1026,16 +1025,6 @@ arguments: ``gitbin`` path to the git binary, defaults to just ``'git'`` -``fetch_refspec`` - One or more refspecs to use when fetching updates for the - repository. By default, the :bb:chsrc:`GitPoller` will simply fetch - all refs. If your repository is large enough that this would be - unwise (or active enough on irrelevant branches that it'd be a - waste of time to fetch them all), you may wish to specify only a - certain refs to be updated. (A single refspec may be passed as a - string, or multiple refspecs may be passed as a list or set of - strings.) - ``category`` Set the category to be used for the changes produced by the :bb:chsrc:`GitPoller`. This will then be set in any changes generated @@ -1059,12 +1048,17 @@ arguments: applied to file names since git will translate non-ascii file names to unreadable escape sequences. +``workdir`` + the directory where the poller should keep its local repository. + The default is :samp:`gitpoller_work`. + If this is a relative path, it will be interpreted relative to the master's basedir. + Multiple git pollers can share the same directory. + An configuration for the git poller might look like this:: from buildbot.changes.gitpoller import GitPoller c['change_source'] = GitPoller('git@example.com:foobaz/myrepo.git', - branch='great_new_feature', - workdir='/home/buildbot/gitpoller_workdir') + branches=['master', 'great_new_feature']) .. bb:chsrc:: GerritChangeSource diff --git a/master/docs/release-notes.rst b/master/docs/release-notes.rst index 112942ade29..6dd992f0bbf 100644 --- a/master/docs/release-notes.rst +++ b/master/docs/release-notes.rst @@ -68,6 +68,9 @@ Deprecations, Removals, and Non-Compatible Changes * The ``P4Sync`` step, deprecated since 0.8.5, has been removed. The ``P4`` step remains. +* The ``fetch_spec`` argument to ``GitPoller`` is no longer supported. + ``GitPoller`` now only downloads branches that it is polling, so specifies a refspec itself. + Changes for Developers ~~~~~~~~~~~~~~~~~~~~~~ @@ -100,6 +103,10 @@ Features * The mercurial hook now supports multple masters. See :bb:pull:`436`. +* ``GitPoller`` has been rewritten. + It now supports multiple branches and can share a directory between multiple pollers. + It is also more resilient to changes in configuration, or in the underlying repository. + Slave ----- From 130d8607cfa5423e199be154cfe6281da944dfbf Mon Sep 17 00:00:00 2001 From: Tom Prince Date: Mon, 9 Jul 2012 00:23:54 -0700 Subject: [PATCH 08/47] gpo: Test that getProcessOutput{,andValue} are called with the correct path. --- .../test/unit/test_changes_gitpoller.py | 52 +++++++++++++--- .../test/unit/test_changes_hgpoller.py | 25 ++++---- .../buildbot/test/unit/test_test_util_gpo.py | 62 +++++++++++++++++-- master/buildbot/test/util/gpo.py | 24 ++++--- 4 files changed, 133 insertions(+), 30 deletions(-) diff --git a/master/buildbot/test/unit/test_changes_gitpoller.py b/master/buildbot/test/unit/test_changes_gitpoller.py index 97df95818c6..dd5b43c5649 100644 --- a/master/buildbot/test/unit/test_changes_gitpoller.py +++ b/master/buildbot/test/unit/test_changes_gitpoller.py @@ -36,7 +36,10 @@ def _perform_git_output_test(self, methodToTest, args, # make this call to self.patch here so that we raise a SkipTest if it # is not supported - self.expectCommands(gpo.Expect('git', *args)) + self.expectCommands( + gpo.Expect('git', *args) + .path('gitpoller-work'), + ) d = defer.succeed(None) def call_empty(_): @@ -54,7 +57,11 @@ def eb_empty(f): d.addCallback(lambda _: self.assertAllCommandsRan()) # and the method shouldn't supress any exceptions - self.expectCommands(gpo.Expect('git', *args).exit(1)) + self.expectCommands( + gpo.Expect('git', *args) + .path('gitpoller-work') + .exit(1), + ) def call_exception(_): return methodToTest(self.dummyRevStr) d.addCallback(call_exception) @@ -67,7 +74,11 @@ def eb_exception(f): d.addCallback(lambda _: self.assertAllCommandsRan()) # finally we should get what's expected from good output - self.expectCommands(gpo.Expect('git', *args).stdout(desiredGoodOutput)) + self.expectCommands( + gpo.Expect('git', *args) + .path('gitpoller-work') + .stdout(desiredGoodOutput) + ) def call_desired(_): return methodToTest(self.dummyRevStr) d.addCallback(call_desired) @@ -130,9 +141,11 @@ def test_poll_initial(self): self.expectCommands( gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), gpo.Expect('git', 'fetch', self.REPOURL, - '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED), + '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work'), gpo.Expect('git', 'rev-parse', 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work') .stdout('bf0b01df6d00ae8d1ffa0b2e2acbe642a6cd35d5\n'), ) @@ -166,6 +179,7 @@ def test_poll_failFetch(self): gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), gpo.Expect('git', 'fetch', self.REPOURL, '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work') .exit(1), ) @@ -177,9 +191,11 @@ def test_poll_failRevParse(self): self.expectCommands( gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), gpo.Expect('git', 'fetch', self.REPOURL, - '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED), + '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work'), gpo.Expect('git', 'rev-parse', 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work') .exit(1), ) @@ -194,13 +210,16 @@ def test_poll_failLog(self): self.expectCommands( gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), gpo.Expect('git', 'fetch', self.REPOURL, - '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED), + '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work'), gpo.Expect('git', 'rev-parse', 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work') .stdout('4423cdbcbb89c14e50dd5f4152415afd686c5241\n'), gpo.Expect('git', 'log', 'fa3ae8ed68e664d4db24798611b352e3c6509930..4423cdbcbb89c14e50dd5f4152415afd686c5241', '--format=%H') + .path('gitpoller-work') .exit(1), ) @@ -229,13 +248,16 @@ def test_poll_nothingNew(self): gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), gpo.Expect('git', 'fetch', self.REPOURL, '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work') .stdout('no interesting output'), gpo.Expect('git', 'rev-parse', 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work') .stdout('4423cdbcbb89c14e50dd5f4152415afd686c5241\n'), gpo.Expect('git', 'log', '4423cdbcbb89c14e50dd5f4152415afd686c5241..4423cdbcbb89c14e50dd5f4152415afd686c5241', '--format=%H') + .path('gitpoller-work') .stdout(''), ) @@ -258,12 +280,15 @@ def test_poll_multipleBranches_initial(self): gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), gpo.Expect('git', 'fetch', self.REPOURL, '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED, - '+release:refs/buildbot/%s/release' % self.REPOURL_QUOTED), + '+release:refs/buildbot/%s/release' % self.REPOURL_QUOTED) + .path('gitpoller-work'), gpo.Expect('git', 'rev-parse', 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work') .stdout('4423cdbcbb89c14e50dd5f4152415afd686c5241\n'), gpo.Expect('git', 'rev-parse', 'refs/buildbot/%s/release' % self.REPOURL_QUOTED) + .path('gitpoller-work') .stdout('9118f4ab71963d23d02d4bdc54876ac8bf05acf2'), ) @@ -286,22 +311,27 @@ def test_poll_multipleBranches(self): gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), gpo.Expect('git', 'fetch', self.REPOURL, '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED, - '+release:refs/buildbot/%s/release' % self.REPOURL_QUOTED), + '+release:refs/buildbot/%s/release' % self.REPOURL_QUOTED) + .path('gitpoller-work'), gpo.Expect('git', 'rev-parse', 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work') .stdout('4423cdbcbb89c14e50dd5f4152415afd686c5241\n'), gpo.Expect('git', 'log', 'fa3ae8ed68e664d4db24798611b352e3c6509930..4423cdbcbb89c14e50dd5f4152415afd686c5241', '--format=%H') + .path('gitpoller-work') .stdout('\n'.join([ '64a5dc2a4bd4f558b5dd193d47c83c7d7abc9a1a', '4423cdbcbb89c14e50dd5f4152415afd686c5241'])), gpo.Expect('git', 'rev-parse', 'refs/buildbot/%s/release' % self.REPOURL_QUOTED) + .path('gitpoller-work') .stdout('9118f4ab71963d23d02d4bdc54876ac8bf05acf2'), gpo.Expect('git', 'log', 'bf0b01df6d00ae8d1ffa0b2e2acbe642a6cd35d5..9118f4ab71963d23d02d4bdc54876ac8bf05acf2', '--format=%H') + .path('gitpoller-work') .stdout( '\n'.join([ '9118f4ab71963d23d02d4bdc54876ac8bf05acf2' ])), @@ -375,13 +405,16 @@ def test_poll_noChanges(self): gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), gpo.Expect('git', 'fetch', self.REPOURL, '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work') .stdout('no interesting output'), gpo.Expect('git', 'rev-parse', 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work') .stdout('4423cdbcbb89c14e50dd5f4152415afd686c5241\n'), gpo.Expect('git', 'log', '4423cdbcbb89c14e50dd5f4152415afd686c5241..4423cdbcbb89c14e50dd5f4152415afd686c5241', '--format=%H') + .path('gitpoller-work') .stdout(''), ) @@ -409,13 +442,16 @@ def test_poll_old(self): gpo.Expect('git', 'init', '--bare', 'gitpoller-work'), gpo.Expect('git', 'fetch', self.REPOURL, '+master:refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work') .stdout('no interesting output'), gpo.Expect('git', 'rev-parse', 'refs/buildbot/%s/master' % self.REPOURL_QUOTED) + .path('gitpoller-work') .stdout('4423cdbcbb89c14e50dd5f4152415afd686c5241\n'), gpo.Expect('git', 'log', 'fa3ae8ed68e664d4db24798611b352e3c6509930..4423cdbcbb89c14e50dd5f4152415afd686c5241', '--format=%H') + .path('gitpoller-work') .stdout('\n'.join([ '64a5dc2a4bd4f558b5dd193d47c83c7d7abc9a1a', '4423cdbcbb89c14e50dd5f4152415afd686c5241' diff --git a/master/buildbot/test/unit/test_changes_hgpoller.py b/master/buildbot/test/unit/test_changes_hgpoller.py index 0ca446dcaa2..4f9b0075e07 100644 --- a/master/buildbot/test/unit/test_changes_hgpoller.py +++ b/master/buildbot/test/unit/test_changes_hgpoller.py @@ -79,15 +79,16 @@ def test_poll_initial(self): self.expectCommands( gpo.Expect('hg', 'init', '/some/dir'), gpo.Expect('hg', 'pull', '-b', 'default', - 'ssh://example.com/foo/baz'), + 'ssh://example.com/foo/baz') + .path('/some/dir'), gpo.Expect('hg', 'heads', 'default', '--template={rev}\n') - .stdout("1"), + .path('/some/dir').stdout("1"), gpo.Expect('hg', 'log', '-b', 'default', '-r', '0:1', '--template={rev}:{node}\\n') - .stdout(os.linesep.join(['0:64a5dc2', '1:4423cdb'])), + .path('/some/dir').stdout(os.linesep.join(['0:64a5dc2', '1:4423cdb'])), gpo.Expect('hg', 'log', '-r', '64a5dc2', '--template={date|hgdate}\n{author}\n{files}\n{desc|strip}') - .stdout(os.linesep.join([ + .path('/some/dir').stdout(os.linesep.join([ '1273258009.0 -7200', 'Joe Test ', 'file1 file2', @@ -96,7 +97,7 @@ def test_poll_initial(self): ''])), gpo.Expect('hg', 'log', '-r', '4423cdb', '--template={date|hgdate}\n{author}\n{files}\n{desc|strip}') - .stdout(os.linesep.join([ + .path('/some/dir').stdout(os.linesep.join([ '1273258100.0 -7200', 'Bob Test ', 'file1 dir/file2', @@ -152,9 +153,10 @@ def test_poll_several_heads(self): # ancestor) self.expectCommands( gpo.Expect('hg', 'pull', '-b', 'default', - 'ssh://example.com/foo/baz'), + 'ssh://example.com/foo/baz') + .path('/some/dir'), gpo.Expect('hg', 'heads', 'default', '--template={rev}\n') - .stdout('5' + os.linesep + '6' + os.linesep), + .path('/some/dir').stdout('5' + os.linesep + '6' + os.linesep), ) yield self.poller._setCurrentRev(3) @@ -168,15 +170,16 @@ def test_poll_regular(self): # normal operation. There's a previous revision, we get a new one. self.expectCommands( gpo.Expect('hg', 'pull', '-b', 'default', - 'ssh://example.com/foo/baz'), + 'ssh://example.com/foo/baz') + .path('/some/dir'), gpo.Expect('hg', 'heads', 'default', '--template={rev}\n') - .stdout('5' + os.linesep), + .path('/some/dir').stdout('5' + os.linesep), gpo.Expect('hg', 'log', '-b', 'default', '-r', '5:5', '--template={rev}:{node}\\n') - .stdout('5:784bd' + os.linesep), + .path('/some/dir').stdout('5:784bd' + os.linesep), gpo.Expect('hg', 'log', '-r', '784bd', '--template={date|hgdate}\n{author}\n{files}\n{desc|strip}') - .stdout(os.linesep.join([ + .path('/some/dir').stdout(os.linesep.join([ '1273258009.0 -7200', 'Joe Test ', 'file1 file2', diff --git a/master/buildbot/test/unit/test_test_util_gpo.py b/master/buildbot/test/unit/test_test_util_gpo.py index 732102753af..8e7b8f043ae 100644 --- a/master/buildbot/test/unit/test_test_util_gpo.py +++ b/master/buildbot/test/unit/test_test_util_gpo.py @@ -106,7 +106,7 @@ def method(testcase): d = utils.getProcessOutput("command", ()) return d result = self.runTestMethod(method) - self.assertTestFailure(result, "wrong command run") + self.assertTestFailure(result, "unexpected command run") def test_gpo_wrongArgs(self): def method(testcase): @@ -115,7 +115,34 @@ def method(testcase): d.addCallback(lambda _: testcase.assertAllCommandsRan()) return d result = self.runTestMethod(method) - self.assertTestFailure(result, "wrong args passed") + self.assertTestFailure(result, "unexpected command run") + + def test_gpo_missingPath(self): + def method(testcase): + testcase.expectCommands(Expect("command", "arg").path("/home")) + d = utils.getProcessOutput("command", ("otherarg",)) + d.addCallback(lambda _: testcase.assertAllCommandsRan()) + return d + result = self.runTestMethod(method) + self.assertTestFailure(result, "unexpected command run") + + def test_gpo_wrongPath(self): + def method(testcase): + testcase.expectCommands(Expect("command", "arg").path("/home")) + d = utils.getProcessOutput("command", ("otherarg",), path="/work") + d.addCallback(lambda _: testcase.assertAllCommandsRan()) + return d + result = self.runTestMethod(method) + self.assertTestFailure(result, "unexpected command run") + + def test_gpo_notCurrentPath(self): + def method(testcase): + testcase.expectCommands(Expect("command", "arg")) + d = utils.getProcessOutput("command", ("otherarg",), path="/work") + d.addCallback(lambda _: testcase.assertAllCommandsRan()) + return d + result = self.runTestMethod(method) + self.assertTestFailure(result, "unexpected command run") def test_gpo_errorOutput(self): def method(testcase): @@ -221,7 +248,7 @@ def method(testcase): d.addCallback(lambda _: testcase.assertAllCommandsRan()) return d result = self.runTestMethod(method) - self.assertTestFailure(result, "wrong command run") + self.assertTestFailure(result, "unexpected command run") def test_gpoav_wrongArgs(self): def method(testcase): @@ -230,7 +257,34 @@ def method(testcase): d.addCallback(lambda _: testcase.assertAllCommandsRan()) return d result = self.runTestMethod(method) - self.assertTestFailure(result, "wrong args passed") + self.assertTestFailure(result, "unexpected command run") + + def test_gpoav_missingPath(self): + def method(testcase): + testcase.expectCommands(Expect("command", "arg").path("/home")) + d = utils.getProcessOutputAndValue("command", ("otherarg",)) + d.addCallback(lambda _: testcase.assertAllCommandsRan()) + return d + result = self.runTestMethod(method) + self.assertTestFailure(result, "unexpected command run") + + def test_gpoav_wrongPath(self): + def method(testcase): + testcase.expectCommands(Expect("command", "arg").path("/home")) + d = utils.getProcessOutputAndValue("command", ("otherarg",), path="/work") + d.addCallback(lambda _: testcase.assertAllCommandsRan()) + return d + result = self.runTestMethod(method) + self.assertTestFailure(result, "unexpected command run") + + def test_gpoav_notCurrentPath(self): + def method(testcase): + testcase.expectCommands(Expect("command", "arg")) + d = utils.getProcessOutputAndValue("command", ("otherarg",), path="/work") + d.addCallback(lambda _: testcase.assertAllCommandsRan()) + return d + result = self.runTestMethod(method) + self.assertTestFailure(result, "unexpected command run") def test_gpoav_errorOutput(self): def method(testcase): diff --git a/master/buildbot/test/util/gpo.py b/master/buildbot/test/util/gpo.py index 952e78ab89d..417eac9075e 100644 --- a/master/buildbot/test/util/gpo.py +++ b/master/buildbot/test/util/gpo.py @@ -19,6 +19,7 @@ class Expect(object): _stdout = "" _stderr = "" _exit = 0 + _path = None def __init__(self, bin, *args): self._bin = bin @@ -36,9 +37,15 @@ def exit(self, exit): self._exit = exit return self - def check(self, test, bin, *args): - test.assertEqual(bin, self._bin, "wrong command run") - test.assertEqual(args, self._args, "wrong args passed") + def path(self, path): + self._path = path + return self + + def check(self, test, bin, path, args): + test.assertEqual( + dict(bin=bin, path=path, args=tuple(args)), + dict(bin=self._bin, path=self._path, args=self._args), + "unexpected command run") return (self._stdout, self._stderr, self._exit) def __repr__(self): @@ -62,8 +69,10 @@ def _check_env(self, env): self.assertEqual(env.get(var), value, 'Expected environment to have %s = %r' % (var, value)) - def patched_getProcessOutput(self, bin, args, env=None, errortoo=False, **kwargs): - d = self.patched_getProcessOutputAndValue(bin, args, env=env, **kwargs) + def patched_getProcessOutput(self, bin, args, env=None, + errortoo=False, path=None, **kwargs): + d = self.patched_getProcessOutputAndValue(bin, args, env=env, + path=path, **kwargs) @d.addCallback def cb(res): stdout, stderr, exit = res @@ -76,7 +85,8 @@ def cb(res): return defer.succeed(stdout) return d - def patched_getProcessOutputAndValue(self, bin, args, env=None, **kwargs): + def patched_getProcessOutputAndValue(self, bin, args, env=None, + path=None, **kwargs): self._check_env(env) if not self._expected_commands: @@ -84,7 +94,7 @@ def patched_getProcessOutputAndValue(self, bin, args, env=None, **kwargs): % (bin, args)) expect = self._expected_commands.pop(0) - return defer.succeed(expect.check(self, bin, *args)) + return defer.succeed(expect.check(self, bin, path, args)) def _patch_gpo(self): if not self._gpo_patched: From 085edd69c69568ca4f01ebdb2e8751c1cb8fdeef Mon Sep 17 00:00:00 2001 From: Tom Prince Date: Mon, 9 Jul 2012 00:26:06 -0700 Subject: [PATCH 09/47] Don't allow arbitrary keywords in testing getProcessOuput. If we need to pass other arguments, then we should be testing that those arguments are passed correctly. --- master/buildbot/test/unit/test_test_util_gpo.py | 2 +- master/buildbot/test/util/gpo.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/master/buildbot/test/unit/test_test_util_gpo.py b/master/buildbot/test/unit/test_test_util_gpo.py index 8e7b8f043ae..3b6a90f9aa2 100644 --- a/master/buildbot/test/unit/test_test_util_gpo.py +++ b/master/buildbot/test/unit/test_test_util_gpo.py @@ -316,7 +316,7 @@ def method(testcase): def test_gpoav_outputAndError(self): def method(testcase): testcase.expectCommands(Expect("command").stdout("stdout").stderr("stderr")) - d = utils.getProcessOutputAndValue("command", (), errortoo=True) + d = utils.getProcessOutputAndValue("command", ()) d.addCallback(testcase.assertEqual, ("stdout",'stderr',0)) return d result = self.runTestMethod(method) diff --git a/master/buildbot/test/util/gpo.py b/master/buildbot/test/util/gpo.py index 417eac9075e..05bb5849cb2 100644 --- a/master/buildbot/test/util/gpo.py +++ b/master/buildbot/test/util/gpo.py @@ -70,9 +70,9 @@ def _check_env(self, env): 'Expected environment to have %s = %r' % (var, value)) def patched_getProcessOutput(self, bin, args, env=None, - errortoo=False, path=None, **kwargs): + errortoo=False, path=None): d = self.patched_getProcessOutputAndValue(bin, args, env=env, - path=path, **kwargs) + path=path) @d.addCallback def cb(res): stdout, stderr, exit = res @@ -86,7 +86,7 @@ def cb(res): return d def patched_getProcessOutputAndValue(self, bin, args, env=None, - path=None, **kwargs): + path=None): self._check_env(env) if not self._expected_commands: From f4b4897b6212f5a19f0e60d2337a6125c1d65b23 Mon Sep 17 00:00:00 2001 From: Benoit Allard Date: Tue, 10 Jul 2012 18:46:51 +0200 Subject: [PATCH 10/47] mercurial source step: don't update during pull, it will be done later on --- master/buildbot/steps/source/mercurial.py | 17 ++++++----------- .../test/unit/test_steps_source_mercurial.py | 14 +++++++------- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/master/buildbot/steps/source/mercurial.py b/master/buildbot/steps/source/mercurial.py index 5dce3598a4e..9ad397c3d07 100644 --- a/master/buildbot/steps/source/mercurial.py +++ b/master/buildbot/steps/source/mercurial.py @@ -142,7 +142,7 @@ def incremental(self): d = self._sourcedirIsUpdatable() def _cmd(updatable): if updatable: - command = ['pull', self.repourl, '--update'] + command = ['pull', self.repourl] else: command = ['clone', self.repourl, '.', '--noupdate'] return command @@ -200,19 +200,14 @@ def _checkBranchChange(self, _): current_branch = yield self._getCurrentBranch() msg = "Working dir is on in-repo branch '%s' and build needs '%s'." % \ (current_branch, self.update_branch) - if current_branch != self.update_branch: - if self.clobberOnBranchChange: + if current_branch != self.update_branch and self.clobberOnBranchChange: msg += ' Clobbering.' log.msg(msg) yield self.clobber(None) - else: - msg += ' Updating.' - log.msg(msg) - yield self._removeAddedFilesAndUpdate(None) - else: - msg += ' Updating.' - log.msg(msg) - yield self._removeAddedFilesAndUpdate(None) + return + msg += ' Updating.' + log.msg(msg) + yield self._removeAddedFilesAndUpdate(None) def _pullUpdate(self, res): command = ['pull' , self.repourl] diff --git a/master/buildbot/test/unit/test_steps_source_mercurial.py b/master/buildbot/test/unit/test_steps_source_mercurial.py index c8e7977f482..3672ac38f11 100644 --- a/master/buildbot/test/unit/test_steps_source_mercurial.py +++ b/master/buildbot/test/unit/test_steps_source_mercurial.py @@ -403,7 +403,7 @@ def test_mode_incremental_branch_change_dirname(self): + 0, ExpectShell(workdir='wkdir', command=['hg', '--verbose', 'pull', - 'http://hg.mozilla.org/stable', '--update']) + 'http://hg.mozilla.org/stable']) + 0, Expect('rmdir', dict(dir='wkdir', logEnviron=True)) @@ -476,7 +476,7 @@ def test_mode_incremental_existing_repo(self): + 0, # directory exists ExpectShell(workdir='wkdir', command=['hg', '--verbose', 'pull', - 'http://hg.mozilla.org', '--update']) + 'http://hg.mozilla.org']) + 0, ExpectShell(workdir='wkdir', command=['hg', '--verbose', 'identify', '--branch']) @@ -512,7 +512,7 @@ def test_mode_incremental_existing_repo_added_files(self): + 0, # directory exists ExpectShell(workdir='wkdir', command=['hg', '--verbose', 'pull', - 'http://hg.mozilla.org', '--update']) + 'http://hg.mozilla.org']) + 0, ExpectShell(workdir='wkdir', command=['hg', '--verbose', 'identify', '--branch']) @@ -553,7 +553,7 @@ def test_mode_incremental_existing_repo_added_files_old_rmdir(self): + 0, # directory exists ExpectShell(workdir='wkdir', command=['hg', '--verbose', 'pull', - 'http://hg.mozilla.org', '--update']) + 'http://hg.mozilla.org']) + 0, ExpectShell(workdir='wkdir', command=['hg', '--verbose', 'identify', '--branch']) @@ -599,7 +599,7 @@ def test_mode_incremental_given_revision(self): + 0, ExpectShell(workdir='wkdir', command=['hg', '--verbose', 'pull', - 'http://hg.mozilla.org', '--update']) + 'http://hg.mozilla.org']) + 0, ExpectShell(workdir='wkdir', command=['hg', '--verbose', 'identify', '--branch']) @@ -638,7 +638,7 @@ def test_mode_incremental_branch_change(self): + 0, ExpectShell(workdir='wkdir', command=['hg', '--verbose', 'pull', - 'http://hg.mozilla.org', '--update']) + 'http://hg.mozilla.org']) + 0, ExpectShell(workdir='wkdir', command=['hg', '--verbose', 'identify', '--branch']) @@ -682,7 +682,7 @@ def test_mode_incremental_branch_change_no_clobberOnBranchChange(self): + 0, ExpectShell(workdir='wkdir', command=['hg', '--verbose', 'pull', - 'http://hg.mozilla.org', '--update']) + 'http://hg.mozilla.org']) + 0, ExpectShell(workdir='wkdir', command=['hg', '--verbose', 'identify', '--branch']) From 4af717e72382fef29b71e9329226eb4c4c09d97e Mon Sep 17 00:00:00 2001 From: Benoit Allard Date: Tue, 10 Jul 2012 19:34:35 +0200 Subject: [PATCH 11/47] mercurial: blame vim, indentation was not correct --- master/buildbot/steps/source/mercurial.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/master/buildbot/steps/source/mercurial.py b/master/buildbot/steps/source/mercurial.py index 9ad397c3d07..c75a4412441 100644 --- a/master/buildbot/steps/source/mercurial.py +++ b/master/buildbot/steps/source/mercurial.py @@ -204,7 +204,7 @@ def _checkBranchChange(self, _): msg += ' Clobbering.' log.msg(msg) yield self.clobber(None) - return + return msg += ' Updating.' log.msg(msg) yield self._removeAddedFilesAndUpdate(None) From b29182ee590c71222608c031a21295741448d472 Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Date: Mon, 4 Jun 2012 19:03:43 +0530 Subject: [PATCH 12/47] Add an interface for Slave Protocol Signed-off-by: Saurabh Kumar --- slave/buildslave/interfaces.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/slave/buildslave/interfaces.py b/slave/buildslave/interfaces.py index fe2eaad077a..cda05a810d8 100644 --- a/slave/buildslave/interfaces.py +++ b/slave/buildslave/interfaces.py @@ -69,3 +69,20 @@ def interrupt(): Child shell processes should be killed. Simple ShellCommand classes can just insert a header line indicating that the process will be killed, then os.kill() the child.""" + +class ISlaveProtocol(Interface): + + def sendUpdates(self, builder, updates): + """ + send an update to the master + """ + + def sendComplete(self, builder, failure): + """ + send a completion signal to the master, for builds and commands + """ + + def gracefulShutdown(self): + """ + request shutdown from master + """ From 0d6778f10f003c8feeaceb811b1166b1366ddfd3 Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Date: Fri, 8 Jun 2012 21:52:00 +0530 Subject: [PATCH 13/47] Change slave-master calls to interface calls. Bot should implement ISlaveProtocol interface. BuildSlave and SlaveBuilder should call masters through ISlaveInterface. Signed-off-by: Saurabh Kumar --- slave/buildslave/bot.py | 47 ++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/slave/buildslave/bot.py b/slave/buildslave/bot.py index 368e547cd45..89569743b96 100644 --- a/slave/buildslave/bot.py +++ b/slave/buildslave/bot.py @@ -18,6 +18,8 @@ import sys import signal +from zope.interface import implements + from twisted.spread import pb from twisted.python import log from twisted.internet import error, reactor, task, defer @@ -28,6 +30,7 @@ from buildslave.pbutil import ReconnectingPBClientFactory from buildslave.commands import registry, base from buildslave import monkeypatches +from buildslave.interfaces import ISlaveProtocol class UnknownCommand(pb.Error): pass @@ -185,7 +188,7 @@ def sendUpdate(self, data): if self.remoteStep: update = [data, 0] updates = [update] - d = self.remoteStep.callRemote("update", updates) + d = self.parent.sendUpdates(self, updates) d.addCallback(self.ackUpdate) d.addErrback(self._ackFailed, "SlaveBuilder.sendUpdate") @@ -218,7 +221,7 @@ def commandComplete(self, failure): return if self.remoteStep: self.remoteStep.dontNotifyOnDisconnect(self.lostRemoteStep) - d = self.remoteStep.callRemote("complete", failure) + d = self.parent.sendComplete(self, failure) d.addCallback(self.ackComplete) d.addErrback(self._ackFailed, "sendComplete") self.remoteStep = None @@ -232,6 +235,7 @@ def remote_shutdown(self): class Bot(pb.Referenceable, service.MultiService): """I represent the slave-side bot.""" + implements (ISlaveProtocol) usePTY = None name = "bot" @@ -333,6 +337,29 @@ def remote_shutdown(self): # if this timeout is too short. reactor.callLater(0.2, reactor.stop) + def sendUpdates(self, builder, updates): + return builder.remoteStep.callRemote("update", updates) + + def sendComplete(self, builder, failure): + return builder.remoteStep.callRemote("complete", failure) + + def gracefulShutdown(self): + if not self.factory.perspective: + log.msg("No active connection, shutting down NOW") + reactor.stop() + return + + log.msg("Telling the master we want to shutdown after any running builds are finished") + d = self.factory.perspective.callRemote("shutdown") + def _shutdownfailed(err): + if err.check(AttributeError): + log.msg("Master does not support slave initiated shutdown. Upgrade master to 0.8.3 or later to use this feature.") + else: + log.msg('callRemote("shutdown") failed') + log.err(err) + d.addErrback(_shutdownfailed) + return d + class BotFactory(ReconnectingPBClientFactory): # 'keepaliveInterval' serves two purposes. The first is to keep the # connection alive: it guarantees that there will be at least some @@ -529,19 +556,5 @@ def _checkShutdownFile(self): def gracefulShutdown(self): """Start shutting down""" - if not self.bf.perspective: - log.msg("No active connection, shutting down NOW") - reactor.stop() - return - log.msg("Telling the master we want to shutdown after any running builds are finished") - d = self.bf.perspective.callRemote("shutdown") - def _shutdownfailed(err): - if err.check(AttributeError): - log.msg("Master does not support slave initiated shutdown. Upgrade master to 0.8.3 or later to use this feature.") - else: - log.msg('callRemote("shutdown") failed') - log.err(err) - - d.addErrback(_shutdownfailed) - return d + return self.bot.gracefulShutdown() From ad0d73d0dc3052501fd6cfae54a322b0fe916059 Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Date: Sun, 1 Jul 2012 20:40:54 +0530 Subject: [PATCH 14/47] Fix buildslave_gracefulShutdown test --- slave/buildslave/bot.py | 12 ++++----- slave/buildslave/test/fake/fakebot.py | 27 +++++++++++++++++++ .../test/unit/test_bot_BuildSlave.py | 16 +++++------ 3 files changed, 38 insertions(+), 17 deletions(-) create mode 100644 slave/buildslave/test/fake/fakebot.py diff --git a/slave/buildslave/bot.py b/slave/buildslave/bot.py index 89569743b96..0afaaea3544 100644 --- a/slave/buildslave/bot.py +++ b/slave/buildslave/bot.py @@ -344,13 +344,8 @@ def sendComplete(self, builder, failure): return builder.remoteStep.callRemote("complete", failure) def gracefulShutdown(self): - if not self.factory.perspective: - log.msg("No active connection, shutting down NOW") - reactor.stop() - return - log.msg("Telling the master we want to shutdown after any running builds are finished") - d = self.factory.perspective.callRemote("shutdown") + d = self.callRemote("shutdown") def _shutdownfailed(err): if err.check(AttributeError): log.msg("Master does not support slave initiated shutdown. Upgrade master to 0.8.3 or later to use this feature.") @@ -556,5 +551,8 @@ def _checkShutdownFile(self): def gracefulShutdown(self): """Start shutting down""" - log.msg("Telling the master we want to shutdown after any running builds are finished") + if not self.bot: + log.msg("No active connection, shutting down NOW") + reactor.stop() + return return self.bot.gracefulShutdown() diff --git a/slave/buildslave/test/fake/fakebot.py b/slave/buildslave/test/fake/fakebot.py new file mode 100644 index 00000000000..20bd2ffaaef --- /dev/null +++ b/slave/buildslave/test/fake/fakebot.py @@ -0,0 +1,27 @@ +# This file is part of Buildbot. Buildbot is free software: you can +# redistribute it and/or modify it under the terms of the GNU General Public +# License as published by the Free Software Foundation, version 2. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS +# FOR A PARTICULAR PURPOSE. See the GNU General Public License for more +# details. +# +# You should have received a copy of the GNU General Public License along with +# this program; if not, write to the Free Software Foundation, Inc., 51 +# Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +# +# Copyright Buildbot Team Members + +import sys + +class FakeBot(): + """Fake slave-side bot.""" + usePTY = None + name = "bot" + + def __init__(self, basedir, usePTY, unicode_encoding=None): + self.basedir = basedir + self.usePTY = usePTY + self.unicode_encoding = unicode_encoding or sys.getfilesystemencoding() or 'ascii' + self.builders = {} diff --git a/slave/buildslave/test/unit/test_bot_BuildSlave.py b/slave/buildslave/test/unit/test_bot_BuildSlave.py index 80d46cc781a..a1ec12a976b 100644 --- a/slave/buildslave/test/unit/test_bot_BuildSlave.py +++ b/slave/buildslave/test/unit/test_bot_BuildSlave.py @@ -25,6 +25,7 @@ from buildslave import bot from buildslave.test.util import misc +from buildslave.test.fake import FakeBot from mock import Mock @@ -158,18 +159,13 @@ def test_buildslave_graceful_shutdown(self): in a call to the master's shutdown method""" d = defer.Deferred() - fakepersp = Mock() - called = [] - def fakeCallRemote(*args): - called.append(args) - d1 = defer.succeed(None) - return d1 - fakepersp.callRemote = fakeCallRemote + fakebot = FakeBot(self.basedir, False, unicode_encoding=None) + + fakebot.gracefulShutdown = Mock() - # set up to call shutdown when we are attached, and chain the results onto # the deferred for the whole test def call_shutdown(mind): - self.buildslave.bf.perspective = fakepersp + self.buildslave.bot = fakebot shutdown_d = self.buildslave.gracefulShutdown() shutdown_d.addCallbacks(d.callback, d.errback) @@ -183,7 +179,7 @@ def call_shutdown(mind): self.buildslave.startService() def check(ign): - self.assertEquals(called, [('shutdown',)]) + fakebot.gracefulShutdown.assert_called_with() d.addCallback(check) return d From 64896037a18e7847d3238d7ff1ec4f3208c00a1f Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Date: Wed, 11 Jul 2012 19:44:40 +0530 Subject: [PATCH 15/47] Separate pb parts from SlaveBuilder --- slave/buildslave/bot.py | 129 ++++++++++++++++++++++++---------------- 1 file changed, 78 insertions(+), 51 deletions(-) diff --git a/slave/buildslave/bot.py b/slave/buildslave/bot.py index 0afaaea3544..8177d93325e 100644 --- a/slave/buildslave/bot.py +++ b/slave/buildslave/bot.py @@ -35,7 +35,7 @@ class UnknownCommand(pb.Error): pass -class SlaveBuilder(pb.Referenceable, service.Service): +class SlaveBuilder(service.Service): """This is the local representation of a single Builder: it handles a single kind of build (like an all-warnings build). It has a name and a @@ -44,19 +44,10 @@ class SlaveBuilder(pb.Referenceable, service.Service): stopCommandOnShutdown = True - # remote is a ref to the Builder object on the master side, and is set - # when they attach. We use it to detect when the connection to the master - # is severed. - remote = None - # .command points to a SlaveCommand instance, and is set while the step # is running. We use it to implement the stopBuild method. command = None - # .remoteStep is a ref to the master-side BuildStep object, and is set - # when the step is started - remoteStep = None - def __init__(self, name): #service.Service.__init__(self) # Service has no __init__ method self.setName(name) @@ -92,32 +83,18 @@ def activity(self): bf = bslave.bf bf.activity() - def remote_setMaster(self, remote): - self.remote = remote - self.remote.notifyOnDisconnect(self.lostRemote) - - def remote_print(self, message): + def printMessage(self, message): log.msg("SlaveBuilder.remote_print(%s): message from master: %s" % (self.name, message)) - def lostRemote(self, remote): - log.msg("lost remote") - self.remote = None - - def lostRemoteStep(self, remotestep): - log.msg("lost remote step") - self.remoteStep = None - if self.stopCommandOnShutdown: - self.stopCommand() - # the following are Commands that can be invoked by the master-side # Builder - def remote_startBuild(self): + def startBuild(self): """This is invoked before the first step of any new build is run. It doesn't do much, but masters call it so it's still here.""" pass - def remote_startCommand(self, stepref, stepId, command, args): + def startCommand(self, stepId, command, args): """ This gets invoked by L{buildbot.process.step.RemoteCommand.start}, as part of various master-side BuildSteps, to start various commands @@ -139,14 +116,12 @@ def remote_startCommand(self, stepref, stepId, command, args): self.command = factory(self, stepId, args) log.msg(" startCommand:%s [id %s]" % (command,stepId)) - self.remoteStep = stepref - self.remoteStep.notifyOnDisconnect(self.lostRemoteStep) d = self.command.doStart() d.addCallback(lambda res: None) d.addBoth(self.commandComplete) return None - def remote_interruptCommand(self, stepId, why): + def interruptCommand(self, stepId, why): """Halt the current step.""" log.msg("asked to interrupt current command: %s" % why) self.activity() @@ -157,7 +132,6 @@ def remote_interruptCommand(self, stepId, why): return self.command.doInterrupt() - def stopCommand(self): """Make any currently-running command die, with no further status output. This is used when the buildslave is shutting down or the @@ -182,15 +156,16 @@ def sendUpdate(self, data): # service is running or not. If we aren't running, don't send any # status messages. return + # the update[1]=0 comes from the leftover 'updateNum', which the # master still expects to receive. Provide it to avoid significant # interoperability issues between new slaves and old masters. - if self.remoteStep: - update = [data, 0] - updates = [update] - d = self.parent.sendUpdates(self, updates) - d.addCallback(self.ackUpdate) - d.addErrback(self._ackFailed, "SlaveBuilder.sendUpdate") + + update = [data, 0] + updates = [update] + d = self.parent.sendUpdates(self, updates) + d.addCallback(self.ackUpdate) + d.addErrback(self._ackFailed, "SlaveBuilder.sendUpdate") def ackUpdate(self, acknum): self.activity() # update the "last activity" timer @@ -202,16 +177,11 @@ def _ackFailed(self, why, where): log.msg("SlaveBuilder._ackFailed:", where) log.err(why) # we don't really care - # this is fired by the Deferred attached to each Command def commandComplete(self, failure): if failure: log.msg("SlaveBuilder.commandFailed", self.command) log.err(failure) - # failure, if present, is a failure.Failure. To send it across - # the wire, we must turn it into a pb.CopyableFailure. - failure = pb.CopyableFailure(failure) - failure.unsafeTracebacks = True else: # failure is None log.msg("SlaveBuilder.commandComplete", self.command) @@ -219,19 +189,71 @@ def commandComplete(self, failure): if not self.running: log.msg(" but we weren't running, quitting silently") return - if self.remoteStep: - self.remoteStep.dontNotifyOnDisconnect(self.lostRemoteStep) - d = self.parent.sendComplete(self, failure) - d.addCallback(self.ackComplete) - d.addErrback(self._ackFailed, "sendComplete") - self.remoteStep = None + d = self.parent.sendComplete(self, failure) + d.addCallback(self.ackComplete) + d.addErrback(self._ackFailed, "sendComplete") +class PBSlaveBuilder(pb.Referenceable): + + # remote is a ref to the Builder object on the master side, and is set + # when they attach. We use it to detect when the connection to the master + # is severed. + remote = None + + # .remoteStep is a ref to the master-side BuildStep object, and is set + # when the step is started + remoteStep = None + + def __init__(self, slavebuilder): + self.slavebuilder = slavebuilder def remote_shutdown(self): log.msg("slave shutting down on command from master") log.msg("NOTE: master is using deprecated slavebuilder.shutdown method") reactor.stop() + def remote_startBuild(self): + self.slavebuilder.startBuild() + + def remote_startCommand(self, stepref, stepId, command, args): + #TODO deal with stepref in this class. + self.remoteStep = stepref + self.remoteStep.notifyOnDisconnect(self.lostRemoteStep) + self.slavebuilder.startCommand(stepId, command, args) + + def remote_interruptCommand(self, stepId, why): + self.slavebuilder.interruptCommand(stepId, why) + + def remote_print(self, message): + self.slavebuilder.printMessage(message) + + def remote_setMaster(self, remote): + self.remote = remote + self.remote.notifyOnDisconnect(self.lostRemote) + + def lostRemote(self, remote): + log.msg("lost remote") + self.remote = None + + def lostRemoteStep(self, remotestep): + log.msg("lost remote step") + self.remoteStep = None + if self.stopCommandOnShutdown: + self.stopCommand() + + def sendUpdates(self, updates): + if self.remoteStep: + d = self.remoteStep.callRemote("update", updates) + return d + + def sendComplete(self, failure): + if failure: + failure = pb.CopyableFailure(failure) + failure.unsafeTracebacks = True + if self.remoteStep: + d = self.remoteStep.callRemote("complete", failure) + self.remoteStep = None + return d class Bot(pb.Referenceable, service.MultiService): """I represent the slave-side bot.""" @@ -245,6 +267,7 @@ def __init__(self, basedir, usePTY, unicode_encoding=None): self.usePTY = usePTY self.unicode_encoding = unicode_encoding or sys.getfilesystemencoding() or 'ascii' self.builders = {} + self.pbbuilders = {} def startService(self): assert os.path.isdir(self.basedir) @@ -276,8 +299,10 @@ def remote_setBuilderList(self, wanted): b.unicode_encoding = self.unicode_encoding b.setServiceParent(self) b.setBuilddir(builddir) + b.proto = PBSlaveBuilder(b) self.builders[name] = b - retval[name] = b + self.pbbuilders[name] = b.proto + retval[name] = b.proto # disown any builders no longer desired to_remove = list(set(self.builders.keys()) - wanted_names) @@ -291,6 +316,8 @@ def remote_setBuilderList(self, wanted): # and *then* remove them from the builder list for name in to_remove: del self.builders[name] + if self.pbbuilders: + del self.pbbuilders[name] # finally warn about any leftover dirs for dir in os.listdir(self.basedir): @@ -338,10 +365,10 @@ def remote_shutdown(self): reactor.callLater(0.2, reactor.stop) def sendUpdates(self, builder, updates): - return builder.remoteStep.callRemote("update", updates) + return builder.proto.sendUpdates(updates) def sendComplete(self, builder, failure): - return builder.remoteStep.callRemote("complete", failure) + return builder.proto.sendComplete(failure) def gracefulShutdown(self): log.msg("Telling the master we want to shutdown after any running builds are finished") From ee490482867f11c2a49e6cf258e70158caed961c Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Date: Thu, 12 Jul 2012 22:22:50 +0530 Subject: [PATCH 16/47] Fix for graceful_shutdown test Correct fix this time. --- slave/buildslave/test/unit/test_bot_BuildSlave.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/slave/buildslave/test/unit/test_bot_BuildSlave.py b/slave/buildslave/test/unit/test_bot_BuildSlave.py index a1ec12a976b..b9da691e3bc 100644 --- a/slave/buildslave/test/unit/test_bot_BuildSlave.py +++ b/slave/buildslave/test/unit/test_bot_BuildSlave.py @@ -25,7 +25,6 @@ from buildslave import bot from buildslave.test.util import misc -from buildslave.test.fake import FakeBot from mock import Mock @@ -33,6 +32,11 @@ # up a TCP connection. This just tests that the PB code will connect and can # execute a basic ping. The rest is done without TCP (or PB) in other test modules. +class FakeBot: + """Fake slave-side bot.""" + usePTY = None + name = "fakebot" + class MasterPerspective(pb.Avatar): def __init__(self, on_keepalive=None): self.on_keepalive = on_keepalive @@ -159,9 +163,9 @@ def test_buildslave_graceful_shutdown(self): in a call to the master's shutdown method""" d = defer.Deferred() - fakebot = FakeBot(self.basedir, False, unicode_encoding=None) + fakebot = FakeBot() - fakebot.gracefulShutdown = Mock() + fakebot.gracefulShutdown = Mock(return_value = defer.succeed(None)) # the deferred for the whole test def call_shutdown(mind): @@ -175,7 +179,6 @@ def call_shutdown(mind): self.buildslave = bot.BuildSlave("127.0.0.1", port, "testy", "westy", self.basedir, keepalive=0, usePTY=False, umask=022) - self.buildslave.startService() def check(ign): From a2920d0f78f4eaac9ee55d3babaa50b7b746b411 Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Date: Thu, 12 Jul 2012 23:23:49 +0530 Subject: [PATCH 17/47] Use buildername instead of builder --- slave/buildslave/bot.py | 19 ++++++++++--------- slave/buildslave/interfaces.py | 4 ++-- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/slave/buildslave/bot.py b/slave/buildslave/bot.py index 8177d93325e..affc08821b7 100644 --- a/slave/buildslave/bot.py +++ b/slave/buildslave/bot.py @@ -163,7 +163,7 @@ def sendUpdate(self, data): update = [data, 0] updates = [update] - d = self.parent.sendUpdates(self, updates) + d = self.parent.sendUpdates(self.name, updates) d.addCallback(self.ackUpdate) d.addErrback(self._ackFailed, "SlaveBuilder.sendUpdate") @@ -189,7 +189,7 @@ def commandComplete(self, failure): if not self.running: log.msg(" but we weren't running, quitting silently") return - d = self.parent.sendComplete(self, failure) + d = self.parent.sendComplete(self.name, failure) d.addCallback(self.ackComplete) d.addErrback(self._ackFailed, "sendComplete") @@ -299,10 +299,9 @@ def remote_setBuilderList(self, wanted): b.unicode_encoding = self.unicode_encoding b.setServiceParent(self) b.setBuilddir(builddir) - b.proto = PBSlaveBuilder(b) self.builders[name] = b - self.pbbuilders[name] = b.proto - retval[name] = b.proto + self.pbbuilders[name] = PBSlaveBuilder(b) + retval[name] = self.pbbuilders[name] # disown any builders no longer desired to_remove = list(set(self.builders.keys()) - wanted_names) @@ -364,11 +363,13 @@ def remote_shutdown(self): # if this timeout is too short. reactor.callLater(0.2, reactor.stop) - def sendUpdates(self, builder, updates): - return builder.proto.sendUpdates(updates) + def sendUpdates(self, buildername, updates): + pbbuilder = self.pbbuilders[buildername] + return pbbuilder.sendUpdates(updates) - def sendComplete(self, builder, failure): - return builder.proto.sendComplete(failure) + def sendComplete(self, buildername, failure): + pbbuilder = self.pbbuilders[buildername] + return pbbuilder.sendComplete(failure) def gracefulShutdown(self): log.msg("Telling the master we want to shutdown after any running builds are finished") diff --git a/slave/buildslave/interfaces.py b/slave/buildslave/interfaces.py index cda05a810d8..a5f1c968caf 100644 --- a/slave/buildslave/interfaces.py +++ b/slave/buildslave/interfaces.py @@ -72,12 +72,12 @@ def interrupt(): class ISlaveProtocol(Interface): - def sendUpdates(self, builder, updates): + def sendUpdates(self, buildername, updates): """ send an update to the master """ - def sendComplete(self, builder, failure): + def sendComplete(self, buildername, failure): """ send a completion signal to the master, for builds and commands """ From 9042f1000816d13679498b785110df3969b51c95 Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Date: Fri, 13 Jul 2012 03:43:36 +0530 Subject: [PATCH 18/47] Add tests for PBSlaveBuilder --- slave/buildslave/test/unit/test_bot.py | 93 +++++++++++++++++++++----- 1 file changed, 78 insertions(+), 15 deletions(-) diff --git a/slave/buildslave/test/unit/test_bot.py b/slave/buildslave/test/unit/test_bot.py index 24a97ea7fe3..6a1f79319f9 100644 --- a/slave/buildslave/test/unit/test_bot.py +++ b/slave/buildslave/test/unit/test_bot.py @@ -20,6 +20,7 @@ from twisted.trial import unittest from twisted.internet import defer, reactor, task from twisted.python import failure, log +from twisted.spread import pb from buildslave.test.util import command, compat from buildslave.test.fake.remote import FakeRemote @@ -27,6 +28,8 @@ import buildslave from buildslave import bot +from mock import Mock + class TestBot(unittest.TestCase): def setUp(self): @@ -219,21 +222,6 @@ def tearDown(self): def test_print(self): return self.sb.callRemote("print", "Hello, SlaveBuilder.") - def test_setMaster(self): - # not much to check here - what the SlaveBuilder does with the - # master is not part of the interface (and, in fact, it does very little) - return self.sb.callRemote("setMaster", mock.Mock()) - - def test_shutdown(self): - # don't *actually* shut down the reactor - that would be silly - stop = mock.Mock() - self.patch(reactor, "stop", stop) - d = self.sb.callRemote("shutdown") - def check(_): - self.assertTrue(stop.called) - d.addCallback(check) - return d - def test_startBuild(self): return self.sb.callRemote("startBuild") @@ -336,6 +324,81 @@ def check(_): d.addCallback(check) return d +class FakeSlaveBuilder: + """ Fake SlaveBuilder """ + +class TestPBSlaveBuilder(unittest.TestCase): + + def setUp(self): + self.slavebuilder = FakeSlaveBuilder() + self.pbbuilder = bot.PBSlaveBuilder(self.slavebuilder) + self.sb = FakeRemote(self.pbbuilder) + + def tearDown(self): + pass + + def test_startBuild(self): + self.slavebuilder.startBuild = Mock() + self.sb.callRemote("startBuild") + self.slavebuilder.startBuild.assert_called_with() + + def test_shutdown(self): + # don't *actually* shut down the reactor - that would be silly + stop = mock.Mock() + self.patch(reactor, "stop", stop) + d = self.sb.callRemote("shutdown") + def check(_): + self.assertTrue(stop.called) + d.addCallback(check) + return d + + def test_setMaster(self): + # not much to check here - what the SlaveBuilder does with the + # master is not part of the interface (and, in fact, it does very little) + return self.sb.callRemote("setMaster", mock.Mock()) + + def test_startCommand(self): + st = FakeStep() + self.slavebuilder.startCommand = Mock() + + self.sb.callRemote("startCommand", FakeRemote(st), + "13", "shell", dict( + command=[ 'echo', 'hello' ], + workdir='workdir', + )) + self.slavebuilder.startCommand.assert_called_with("13", "shell", dict( + command=[ 'echo', 'hello' ], + workdir='workdir', + ) ) + + def test_interruptCommand(self): + self.slavebuilder.interruptCommand = Mock() + + self.sb.callRemote("interruptCommand", "13", "tl/dr" ) + self.slavebuilder.interruptCommand.assert_called_with("13", "tl/dr") + + def test_print(self): + self.slavebuilder.printMessage = Mock() + + self.sb.callRemote("print", "Hello, SlaveBuilder.") + self.slavebuilder.printMessage.assert_called_with("Hello, SlaveBuilder.") + + def test_sendUpdates(self): + st = FakeStep() + self.pbbuilder.remoteStep = FakeRemote(st) + + self.pbbuilder.remoteStep.callRemote("update", [[{'hdr': 'headers'}, 0]]) + self.assertEqual(st.actions, [ + ['update', [[{'hdr': 'headers'}, 0]]] + ]) + + def test_sendComplete(self): + st = FakeStep() + self.pbbuilder.remoteStep = FakeRemote(st) + f = Mock() + self.pbbuilder.remoteStep.callRemote("complete", f) + self.assertEqual(st.actions, [['complete', f]]) + class TestBotFactory(unittest.TestCase): def setUp(self): From 87278b1bf6cdece99a1b625748d8c9ee525d459a Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Date: Fri, 13 Jul 2012 13:01:59 +0530 Subject: [PATCH 19/47] Fix lostRemoteStep stopCommandonShutdown is a slavebuilder attribute. --- slave/buildslave/bot.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/slave/buildslave/bot.py b/slave/buildslave/bot.py index affc08821b7..a658762e0ac 100644 --- a/slave/buildslave/bot.py +++ b/slave/buildslave/bot.py @@ -238,8 +238,8 @@ def lostRemote(self, remote): def lostRemoteStep(self, remotestep): log.msg("lost remote step") self.remoteStep = None - if self.stopCommandOnShutdown: - self.stopCommand() + if self.slavebuilder.stopCommandOnShutdown: + self.slavebuilder.stopCommand() def sendUpdates(self, updates): if self.remoteStep: From 76bc20f7280021e6c699daa4e7bce488ab724d9f Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Date: Fri, 13 Jul 2012 20:21:53 +0530 Subject: [PATCH 20/47] Delete fakebot --- slave/buildslave/test/fake/fakebot.py | 27 --------------------------- 1 file changed, 27 deletions(-) delete mode 100644 slave/buildslave/test/fake/fakebot.py diff --git a/slave/buildslave/test/fake/fakebot.py b/slave/buildslave/test/fake/fakebot.py deleted file mode 100644 index 20bd2ffaaef..00000000000 --- a/slave/buildslave/test/fake/fakebot.py +++ /dev/null @@ -1,27 +0,0 @@ -# This file is part of Buildbot. Buildbot is free software: you can -# redistribute it and/or modify it under the terms of the GNU General Public -# License as published by the Free Software Foundation, version 2. -# -# This program is distributed in the hope that it will be useful, but WITHOUT -# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS -# FOR A PARTICULAR PURPOSE. See the GNU General Public License for more -# details. -# -# You should have received a copy of the GNU General Public License along with -# this program; if not, write to the Free Software Foundation, Inc., 51 -# Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. -# -# Copyright Buildbot Team Members - -import sys - -class FakeBot(): - """Fake slave-side bot.""" - usePTY = None - name = "bot" - - def __init__(self, basedir, usePTY, unicode_encoding=None): - self.basedir = basedir - self.usePTY = usePTY - self.unicode_encoding = unicode_encoding or sys.getfilesystemencoding() or 'ascii' - self.builders = {} From 6e88b45e5aa6d156faa11b95a178eaf60b4339f1 Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Date: Mon, 16 Jul 2012 05:55:55 +0530 Subject: [PATCH 21/47] Add perspective attribute to Bot --- slave/buildslave/bot.py | 9 +++++++-- slave/buildslave/test/unit/test_bot_BuildSlave.py | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/slave/buildslave/bot.py b/slave/buildslave/bot.py index a658762e0ac..f4e7dfab936 100644 --- a/slave/buildslave/bot.py +++ b/slave/buildslave/bot.py @@ -268,6 +268,10 @@ def __init__(self, basedir, usePTY, unicode_encoding=None): self.unicode_encoding = unicode_encoding or sys.getfilesystemencoding() or 'ascii' self.builders = {} self.pbbuilders = {} + self.persp = None + + def setPersp(self, perspective): + self.persp = perspective def startService(self): assert os.path.isdir(self.basedir) @@ -373,7 +377,7 @@ def sendComplete(self, buildername, failure): def gracefulShutdown(self): log.msg("Telling the master we want to shutdown after any running builds are finished") - d = self.callRemote("shutdown") + d = self.persp.callRemote("shutdown") def _shutdownfailed(err): if err.check(AttributeError): log.msg("Master does not support slave initiated shutdown. Upgrade master to 0.8.3 or later to use this feature.") @@ -513,6 +517,7 @@ def __init__(self, buildmaster_host, port, name, passwd, basedir, bf.startLogin(credentials.UsernamePassword(name, passwd), client=bot) self.connection = c = internet.TCPClient(buildmaster_host, port, bf) c.setServiceParent(self) + self.bot.setPersp(bf.perspective) def startService(self): # first, apply all monkeypatches @@ -579,7 +584,7 @@ def _checkShutdownFile(self): def gracefulShutdown(self): """Start shutting down""" - if not self.bot: + if not self.bot.persp: log.msg("No active connection, shutting down NOW") reactor.stop() return diff --git a/slave/buildslave/test/unit/test_bot_BuildSlave.py b/slave/buildslave/test/unit/test_bot_BuildSlave.py index b9da691e3bc..43eb2c50e0c 100644 --- a/slave/buildslave/test/unit/test_bot_BuildSlave.py +++ b/slave/buildslave/test/unit/test_bot_BuildSlave.py @@ -170,6 +170,7 @@ def test_buildslave_graceful_shutdown(self): # the deferred for the whole test def call_shutdown(mind): self.buildslave.bot = fakebot + self.buildslave.bot.persp = mind shutdown_d = self.buildslave.gracefulShutdown() shutdown_d.addCallbacks(d.callback, d.errback) From 0d750c94fb9a05e0dabda96f11755121b5974e2b Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Date: Mon, 16 Jul 2012 05:55:55 +0530 Subject: [PATCH 22/47] Add perspective attribute to Bot --- slave/buildslave/bot.py | 12 ++++++++++-- slave/buildslave/test/unit/test_bot_BuildSlave.py | 1 + 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/slave/buildslave/bot.py b/slave/buildslave/bot.py index a658762e0ac..f0e1b64c395 100644 --- a/slave/buildslave/bot.py +++ b/slave/buildslave/bot.py @@ -268,6 +268,10 @@ def __init__(self, basedir, usePTY, unicode_encoding=None): self.unicode_encoding = unicode_encoding or sys.getfilesystemencoding() or 'ascii' self.builders = {} self.pbbuilders = {} + self.persp = None + + def setPersp(self, perspective): + self.persp = perspective def startService(self): assert os.path.isdir(self.basedir) @@ -373,7 +377,7 @@ def sendComplete(self, buildername, failure): def gracefulShutdown(self): log.msg("Telling the master we want to shutdown after any running builds are finished") - d = self.callRemote("shutdown") + d = self.persp.callRemote("shutdown") def _shutdownfailed(err): if err.check(AttributeError): log.msg("Master does not support slave initiated shutdown. Upgrade master to 0.8.3 or later to use this feature.") @@ -383,6 +387,9 @@ def _shutdownfailed(err): d.addErrback(_shutdownfailed) return d + def activeConnection(self): + return self.persp + class BotFactory(ReconnectingPBClientFactory): # 'keepaliveInterval' serves two purposes. The first is to keep the # connection alive: it guarantees that there will be at least some @@ -513,6 +520,7 @@ def __init__(self, buildmaster_host, port, name, passwd, basedir, bf.startLogin(credentials.UsernamePassword(name, passwd), client=bot) self.connection = c = internet.TCPClient(buildmaster_host, port, bf) c.setServiceParent(self) + self.bot.setPersp(bf.perspective) def startService(self): # first, apply all monkeypatches @@ -579,7 +587,7 @@ def _checkShutdownFile(self): def gracefulShutdown(self): """Start shutting down""" - if not self.bot: + if not self.bot.activeConnection(): log.msg("No active connection, shutting down NOW") reactor.stop() return diff --git a/slave/buildslave/test/unit/test_bot_BuildSlave.py b/slave/buildslave/test/unit/test_bot_BuildSlave.py index b9da691e3bc..c62accbb4f0 100644 --- a/slave/buildslave/test/unit/test_bot_BuildSlave.py +++ b/slave/buildslave/test/unit/test_bot_BuildSlave.py @@ -170,6 +170,7 @@ def test_buildslave_graceful_shutdown(self): # the deferred for the whole test def call_shutdown(mind): self.buildslave.bot = fakebot + self.buildslave.bot.activeConnection = Mock(return_value = mind) shutdown_d = self.buildslave.gracefulShutdown() shutdown_d.addCallbacks(d.callback, d.errback) From 0d338e6ae12d077aac8440a776f82191bc620204 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 15 Jul 2012 20:12:56 -0500 Subject: [PATCH 23/47] fix pyflake --- slave/buildslave/test/unit/test_bot.py | 1 - 1 file changed, 1 deletion(-) diff --git a/slave/buildslave/test/unit/test_bot.py b/slave/buildslave/test/unit/test_bot.py index 6a1f79319f9..4aca3bfd830 100644 --- a/slave/buildslave/test/unit/test_bot.py +++ b/slave/buildslave/test/unit/test_bot.py @@ -20,7 +20,6 @@ from twisted.trial import unittest from twisted.internet import defer, reactor, task from twisted.python import failure, log -from twisted.spread import pb from buildslave.test.util import command, compat from buildslave.test.fake.remote import FakeRemote From a7e8b9202d23d4607daa621cadb2552435e46c1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Allard?= Date: Mon, 16 Jul 2012 15:28:14 +0300 Subject: [PATCH 24/47] scheduler: Improve logging to say which scheduler refuses the codebase. --- master/buildbot/schedulers/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/master/buildbot/schedulers/base.py b/master/buildbot/schedulers/base.py index a33799c0bf6..00ba45187e7 100644 --- a/master/buildbot/schedulers/base.py +++ b/master/buildbot/schedulers/base.py @@ -213,7 +213,7 @@ def changeCallback(change): if change_filter and not change_filter.filter_change(change): return if change.codebase not in self.codebases: - log.msg('change contains codebase %s that is not processed by this scheduler' % change.codebase) + log.msg('change contains codebase %s that is not processed by scheduler %s' % (change.codebase, self.name)) return if fileIsImportant: try: From e3b351efacad5ae6708515032ddb805d3785ea9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fl=C3=A1vio=20J=2E=20Saraiva?= Date: Fri, 13 Jul 2012 13:21:34 +0100 Subject: [PATCH 25/47] Make split_file_branches reject files named "trunk" and "branch/"; Make sure directories end with a right slash when fed to the split_file function; Update example in the Customization section of the manual. --- master/buildbot/changes/svnpoller.py | 31 ++++++++++--- .../test/unit/test_changes_svnpoller.py | 46 ++++++++++++++++++- master/docs/manual/customization.rst | 4 +- 3 files changed, 70 insertions(+), 11 deletions(-) diff --git a/master/buildbot/changes/svnpoller.py b/master/buildbot/changes/svnpoller.py index fde04097856..f581dd4baf4 100644 --- a/master/buildbot/changes/svnpoller.py +++ b/master/buildbot/changes/svnpoller.py @@ -35,12 +35,16 @@ def split_file_alwaystrunk(path): return (None, path) def split_file_branches(path): - # turn trunk/subdir/file.c into (None, "subdir/file.c") - # and branches/1.5.x/subdir/file.c into ("branches/1.5.x", "subdir/file.c") + # turn "trunk/subdir/file.c" into (None, "subdir/file.c") + # and "trunk/subdir/" into (None, "subdir/") + # and "trunk/" into (None, "") + # and "branches/1.5.x/subdir/file.c" into ("branches/1.5.x", "subdir/file.c") + # and "branches/1.5.x/subdir/" into ("branches/1.5.x", "subdir/") + # and "branches/1.5.x/" into ("branches/1.5.x", "") pieces = path.split('/') - if pieces[0] == 'trunk': + if len(pieces) > 1 and pieces[0] == 'trunk': return (None, '/'.join(pieces[1:])) - elif pieces[0] == 'branches': + elif len(pieces) > 2 and pieces[0] == 'branches': return ('/'.join(pieces[0:2]), '/'.join(pieces[2:])) else: return None @@ -310,6 +314,7 @@ def create_changes(self, new_logentries): continue for p in pathlist.getElementsByTagName("path"): + kind = p.getAttribute("kind") action = p.getAttribute("action") path = "".join([t.data for t in p.childNodes]) # the rest of buildbot is certaily not yet ready to handle @@ -319,6 +324,8 @@ def create_changes(self, new_logentries): path = path.encode("ascii") if path.startswith("/"): path = path[1:] + if kind == "dir" and not path.endswith("/"): + path += "/" where = self._transform_path(path) # if 'where' is None, the file was outside any project that @@ -326,8 +333,17 @@ def create_changes(self, new_logentries): if where: branch, filename = where if not branch in branches: - branches[branch] = { 'files': []} - branches[branch]['files'].append(filename) + branches[branch] = { 'files': [], 'number_of_directories': 0} + if filename == "": + # root directory of branch + branches[branch]['files'].append(filename) + branches[branch]['number_of_directories'] += 1 + elif filename.endswith("/"): + # subdirectory of branch + branches[branch]['files'].append(filename[:-1]) + branches[branch]['number_of_directories'] += 1 + else: + branches[branch]['files'].append(filename) if not branches[branch].has_key('action'): branches[branch]['action'] = action @@ -335,9 +351,10 @@ def create_changes(self, new_logentries): for branch in branches.keys(): action = branches[branch]['action'] files = branches[branch]['files'] + number_of_directories_changed = branches[branch]['number_of_directories'] number_of_files_changed = len(files) - if action == u'D' and number_of_files_changed == 1 and files[0] == '': + if action == u'D' and number_of_directories_changed == 1 and number_of_files_changed == 1 and files[0] == '': log.msg("Ignoring deletion of branch '%s'" % branch) else: chdict = dict( diff --git a/master/buildbot/test/unit/test_changes_svnpoller.py b/master/buildbot/test/unit/test_changes_svnpoller.py index 69e9ae438b6..2a281ba4224 100644 --- a/master/buildbot/test/unit/test_changes_svnpoller.py +++ b/master/buildbot/test/unit/test_changes_svnpoller.py @@ -524,14 +524,56 @@ class TestSplitFile(unittest.TestCase): def test_split_file_alwaystrunk(self): self.assertEqual(svnpoller.split_file_alwaystrunk('foo'), (None, 'foo')) - def test_split_file_branches(self): + def test_split_file_branches_trunk(self): + self.assertEqual( + svnpoller.split_file_branches('trunk/'), + (None, '')) + + def test_split_file_branches_trunk_subdir(self): + self.assertEqual( + svnpoller.split_file_branches('trunk/subdir/'), + (None, 'subdir/')) + + def test_split_file_branches_trunk_subfile(self): self.assertEqual( svnpoller.split_file_branches('trunk/subdir/file.c'), (None, 'subdir/file.c')) + + def test_split_file_branches_trunk_invalid(self): + # file named trunk (not a directory): + self.assertEqual( + svnpoller.split_file_branches('trunk'), + None) + + def test_split_file_branches_branch(self): + self.assertEqual( + svnpoller.split_file_branches('branches/1.5.x/'), + ('branches/1.5.x', '')) + + def test_split_file_branches_branch_subdir(self): + self.assertEqual( + svnpoller.split_file_branches('branches/1.5.x/subdir/'), + ('branches/1.5.x', 'subdir/')) + + def test_split_file_branches_branch_subfile(self): self.assertEqual( svnpoller.split_file_branches('branches/1.5.x/subdir/file.c'), ('branches/1.5.x', 'subdir/file.c')) - # tags are ignored: + + def test_split_file_branches_branch_invalid(self): + # file named branches/1.5.x (not a directory): + self.assertEqual( + svnpoller.split_file_branches('branches/1.5.x'), + None) + + def test_split_file_branches_otherdir(self): + # other dirs are ignored: + self.assertEqual( + svnpoller.split_file_branches('tags/testthis/subdir/'), + None) + + def test_split_file_branches_otherfile(self): + # other files are ignored: self.assertEqual( svnpoller.split_file_branches('tags/testthis/subdir/file.c'), None) diff --git a/master/docs/manual/customization.rst b/master/docs/manual/customization.rst index ffc1022a512..0fe8ff5e81e 100644 --- a/master/docs/manual/customization.rst +++ b/master/docs/manual/customization.rst @@ -215,9 +215,9 @@ scheme, the following function will work:: def split_file_branches(path): pieces = path.split('/') - if pieces[0] == 'trunk': + if len(pieces) > 1 and pieces[0] == 'trunk': return (None, '/'.join(pieces[1:])) - elif pieces[0] == 'branches': + elif len(pieces) > 2 and pieces[0] == 'branches': return ('/'.join(pieces[0:2]), '/'.join(pieces[2:])) else: From 888008a0a4d72df11c67188b681071c4d3c4a57d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Allard?= Date: Tue, 17 Jul 2012 21:29:21 +0300 Subject: [PATCH 26/47] schedulers: turns out we can specify the logLevel of the logging messages. --- master/buildbot/schedulers/base.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/master/buildbot/schedulers/base.py b/master/buildbot/schedulers/base.py index 00ba45187e7..c79a971c146 100644 --- a/master/buildbot/schedulers/base.py +++ b/master/buildbot/schedulers/base.py @@ -20,6 +20,7 @@ from buildbot.process.properties import Properties from buildbot.util import ComparableMixin from buildbot import config, interfaces +import logging class BaseScheduler(service.MultiService, ComparableMixin): """ @@ -213,7 +214,9 @@ def changeCallback(change): if change_filter and not change_filter.filter_change(change): return if change.codebase not in self.codebases: - log.msg('change contains codebase %s that is not processed by scheduler %s' % (change.codebase, self.name)) + log.msg('change contains codebase %s that is not processed by' + ' scheduler %s' % (change.codebase, self.name), + logLevel=logging.DEBUG) return if fileIsImportant: try: From a8422112fb1c32b5579e729477993f06b0c35d1f Mon Sep 17 00:00:00 2001 From: Tom Prince Date: Wed, 18 Jul 2012 11:21:11 -0600 Subject: [PATCH 27/47] Revert "Merge branch 'slavesidechanges' of git://github.com/sa1/buildbot" This reverts commit 83f7d165ad64d2ff27ca7669221005af79a4f606 and d3505c2133e08c478e8fbdae9acb0de23e947445, reversing changes made to 28fa5718cdff57119bc89e4e3991636088dbe84d. --- slave/buildslave/bot.py | 171 +++++++----------- slave/buildslave/interfaces.py | 17 -- slave/buildslave/test/unit/test_bot.py | 92 ++-------- .../test/unit/test_bot_BuildSlave.py | 22 +-- 4 files changed, 88 insertions(+), 214 deletions(-) diff --git a/slave/buildslave/bot.py b/slave/buildslave/bot.py index f0e1b64c395..368e547cd45 100644 --- a/slave/buildslave/bot.py +++ b/slave/buildslave/bot.py @@ -18,8 +18,6 @@ import sys import signal -from zope.interface import implements - from twisted.spread import pb from twisted.python import log from twisted.internet import error, reactor, task, defer @@ -30,12 +28,11 @@ from buildslave.pbutil import ReconnectingPBClientFactory from buildslave.commands import registry, base from buildslave import monkeypatches -from buildslave.interfaces import ISlaveProtocol class UnknownCommand(pb.Error): pass -class SlaveBuilder(service.Service): +class SlaveBuilder(pb.Referenceable, service.Service): """This is the local representation of a single Builder: it handles a single kind of build (like an all-warnings build). It has a name and a @@ -44,10 +41,19 @@ class SlaveBuilder(service.Service): stopCommandOnShutdown = True + # remote is a ref to the Builder object on the master side, and is set + # when they attach. We use it to detect when the connection to the master + # is severed. + remote = None + # .command points to a SlaveCommand instance, and is set while the step # is running. We use it to implement the stopBuild method. command = None + # .remoteStep is a ref to the master-side BuildStep object, and is set + # when the step is started + remoteStep = None + def __init__(self, name): #service.Service.__init__(self) # Service has no __init__ method self.setName(name) @@ -83,18 +89,32 @@ def activity(self): bf = bslave.bf bf.activity() - def printMessage(self, message): + def remote_setMaster(self, remote): + self.remote = remote + self.remote.notifyOnDisconnect(self.lostRemote) + + def remote_print(self, message): log.msg("SlaveBuilder.remote_print(%s): message from master: %s" % (self.name, message)) + def lostRemote(self, remote): + log.msg("lost remote") + self.remote = None + + def lostRemoteStep(self, remotestep): + log.msg("lost remote step") + self.remoteStep = None + if self.stopCommandOnShutdown: + self.stopCommand() + # the following are Commands that can be invoked by the master-side # Builder - def startBuild(self): + def remote_startBuild(self): """This is invoked before the first step of any new build is run. It doesn't do much, but masters call it so it's still here.""" pass - def startCommand(self, stepId, command, args): + def remote_startCommand(self, stepref, stepId, command, args): """ This gets invoked by L{buildbot.process.step.RemoteCommand.start}, as part of various master-side BuildSteps, to start various commands @@ -116,12 +136,14 @@ def startCommand(self, stepId, command, args): self.command = factory(self, stepId, args) log.msg(" startCommand:%s [id %s]" % (command,stepId)) + self.remoteStep = stepref + self.remoteStep.notifyOnDisconnect(self.lostRemoteStep) d = self.command.doStart() d.addCallback(lambda res: None) d.addBoth(self.commandComplete) return None - def interruptCommand(self, stepId, why): + def remote_interruptCommand(self, stepId, why): """Halt the current step.""" log.msg("asked to interrupt current command: %s" % why) self.activity() @@ -132,6 +154,7 @@ def interruptCommand(self, stepId, why): return self.command.doInterrupt() + def stopCommand(self): """Make any currently-running command die, with no further status output. This is used when the buildslave is shutting down or the @@ -156,16 +179,15 @@ def sendUpdate(self, data): # service is running or not. If we aren't running, don't send any # status messages. return - # the update[1]=0 comes from the leftover 'updateNum', which the # master still expects to receive. Provide it to avoid significant # interoperability issues between new slaves and old masters. - - update = [data, 0] - updates = [update] - d = self.parent.sendUpdates(self.name, updates) - d.addCallback(self.ackUpdate) - d.addErrback(self._ackFailed, "SlaveBuilder.sendUpdate") + if self.remoteStep: + update = [data, 0] + updates = [update] + d = self.remoteStep.callRemote("update", updates) + d.addCallback(self.ackUpdate) + d.addErrback(self._ackFailed, "SlaveBuilder.sendUpdate") def ackUpdate(self, acknum): self.activity() # update the "last activity" timer @@ -177,11 +199,16 @@ def _ackFailed(self, why, where): log.msg("SlaveBuilder._ackFailed:", where) log.err(why) # we don't really care + # this is fired by the Deferred attached to each Command def commandComplete(self, failure): if failure: log.msg("SlaveBuilder.commandFailed", self.command) log.err(failure) + # failure, if present, is a failure.Failure. To send it across + # the wire, we must turn it into a pb.CopyableFailure. + failure = pb.CopyableFailure(failure) + failure.unsafeTracebacks = True else: # failure is None log.msg("SlaveBuilder.commandComplete", self.command) @@ -189,75 +216,22 @@ def commandComplete(self, failure): if not self.running: log.msg(" but we weren't running, quitting silently") return - d = self.parent.sendComplete(self.name, failure) - d.addCallback(self.ackComplete) - d.addErrback(self._ackFailed, "sendComplete") - -class PBSlaveBuilder(pb.Referenceable): - - # remote is a ref to the Builder object on the master side, and is set - # when they attach. We use it to detect when the connection to the master - # is severed. - remote = None - - # .remoteStep is a ref to the master-side BuildStep object, and is set - # when the step is started - remoteStep = None + if self.remoteStep: + self.remoteStep.dontNotifyOnDisconnect(self.lostRemoteStep) + d = self.remoteStep.callRemote("complete", failure) + d.addCallback(self.ackComplete) + d.addErrback(self._ackFailed, "sendComplete") + self.remoteStep = None - def __init__(self, slavebuilder): - self.slavebuilder = slavebuilder def remote_shutdown(self): log.msg("slave shutting down on command from master") log.msg("NOTE: master is using deprecated slavebuilder.shutdown method") reactor.stop() - def remote_startBuild(self): - self.slavebuilder.startBuild() - - def remote_startCommand(self, stepref, stepId, command, args): - #TODO deal with stepref in this class. - self.remoteStep = stepref - self.remoteStep.notifyOnDisconnect(self.lostRemoteStep) - self.slavebuilder.startCommand(stepId, command, args) - - def remote_interruptCommand(self, stepId, why): - self.slavebuilder.interruptCommand(stepId, why) - - def remote_print(self, message): - self.slavebuilder.printMessage(message) - - def remote_setMaster(self, remote): - self.remote = remote - self.remote.notifyOnDisconnect(self.lostRemote) - - def lostRemote(self, remote): - log.msg("lost remote") - self.remote = None - - def lostRemoteStep(self, remotestep): - log.msg("lost remote step") - self.remoteStep = None - if self.slavebuilder.stopCommandOnShutdown: - self.slavebuilder.stopCommand() - - def sendUpdates(self, updates): - if self.remoteStep: - d = self.remoteStep.callRemote("update", updates) - return d - - def sendComplete(self, failure): - if failure: - failure = pb.CopyableFailure(failure) - failure.unsafeTracebacks = True - if self.remoteStep: - d = self.remoteStep.callRemote("complete", failure) - self.remoteStep = None - return d class Bot(pb.Referenceable, service.MultiService): """I represent the slave-side bot.""" - implements (ISlaveProtocol) usePTY = None name = "bot" @@ -267,11 +241,6 @@ def __init__(self, basedir, usePTY, unicode_encoding=None): self.usePTY = usePTY self.unicode_encoding = unicode_encoding or sys.getfilesystemencoding() or 'ascii' self.builders = {} - self.pbbuilders = {} - self.persp = None - - def setPersp(self, perspective): - self.persp = perspective def startService(self): assert os.path.isdir(self.basedir) @@ -304,8 +273,7 @@ def remote_setBuilderList(self, wanted): b.setServiceParent(self) b.setBuilddir(builddir) self.builders[name] = b - self.pbbuilders[name] = PBSlaveBuilder(b) - retval[name] = self.pbbuilders[name] + retval[name] = b # disown any builders no longer desired to_remove = list(set(self.builders.keys()) - wanted_names) @@ -319,8 +287,6 @@ def remote_setBuilderList(self, wanted): # and *then* remove them from the builder list for name in to_remove: del self.builders[name] - if self.pbbuilders: - del self.pbbuilders[name] # finally warn about any leftover dirs for dir in os.listdir(self.basedir): @@ -367,29 +333,6 @@ def remote_shutdown(self): # if this timeout is too short. reactor.callLater(0.2, reactor.stop) - def sendUpdates(self, buildername, updates): - pbbuilder = self.pbbuilders[buildername] - return pbbuilder.sendUpdates(updates) - - def sendComplete(self, buildername, failure): - pbbuilder = self.pbbuilders[buildername] - return pbbuilder.sendComplete(failure) - - def gracefulShutdown(self): - log.msg("Telling the master we want to shutdown after any running builds are finished") - d = self.persp.callRemote("shutdown") - def _shutdownfailed(err): - if err.check(AttributeError): - log.msg("Master does not support slave initiated shutdown. Upgrade master to 0.8.3 or later to use this feature.") - else: - log.msg('callRemote("shutdown") failed') - log.err(err) - d.addErrback(_shutdownfailed) - return d - - def activeConnection(self): - return self.persp - class BotFactory(ReconnectingPBClientFactory): # 'keepaliveInterval' serves two purposes. The first is to keep the # connection alive: it guarantees that there will be at least some @@ -520,7 +463,6 @@ def __init__(self, buildmaster_host, port, name, passwd, basedir, bf.startLogin(credentials.UsernamePassword(name, passwd), client=bot) self.connection = c = internet.TCPClient(buildmaster_host, port, bf) c.setServiceParent(self) - self.bot.setPersp(bf.perspective) def startService(self): # first, apply all monkeypatches @@ -587,8 +529,19 @@ def _checkShutdownFile(self): def gracefulShutdown(self): """Start shutting down""" - if not self.bot.activeConnection(): + if not self.bf.perspective: log.msg("No active connection, shutting down NOW") reactor.stop() return - return self.bot.gracefulShutdown() + + log.msg("Telling the master we want to shutdown after any running builds are finished") + d = self.bf.perspective.callRemote("shutdown") + def _shutdownfailed(err): + if err.check(AttributeError): + log.msg("Master does not support slave initiated shutdown. Upgrade master to 0.8.3 or later to use this feature.") + else: + log.msg('callRemote("shutdown") failed') + log.err(err) + + d.addErrback(_shutdownfailed) + return d diff --git a/slave/buildslave/interfaces.py b/slave/buildslave/interfaces.py index a5f1c968caf..fe2eaad077a 100644 --- a/slave/buildslave/interfaces.py +++ b/slave/buildslave/interfaces.py @@ -69,20 +69,3 @@ def interrupt(): Child shell processes should be killed. Simple ShellCommand classes can just insert a header line indicating that the process will be killed, then os.kill() the child.""" - -class ISlaveProtocol(Interface): - - def sendUpdates(self, buildername, updates): - """ - send an update to the master - """ - - def sendComplete(self, buildername, failure): - """ - send a completion signal to the master, for builds and commands - """ - - def gracefulShutdown(self): - """ - request shutdown from master - """ diff --git a/slave/buildslave/test/unit/test_bot.py b/slave/buildslave/test/unit/test_bot.py index 4aca3bfd830..24a97ea7fe3 100644 --- a/slave/buildslave/test/unit/test_bot.py +++ b/slave/buildslave/test/unit/test_bot.py @@ -27,8 +27,6 @@ import buildslave from buildslave import bot -from mock import Mock - class TestBot(unittest.TestCase): def setUp(self): @@ -221,6 +219,21 @@ def tearDown(self): def test_print(self): return self.sb.callRemote("print", "Hello, SlaveBuilder.") + def test_setMaster(self): + # not much to check here - what the SlaveBuilder does with the + # master is not part of the interface (and, in fact, it does very little) + return self.sb.callRemote("setMaster", mock.Mock()) + + def test_shutdown(self): + # don't *actually* shut down the reactor - that would be silly + stop = mock.Mock() + self.patch(reactor, "stop", stop) + d = self.sb.callRemote("shutdown") + def check(_): + self.assertTrue(stop.called) + d.addCallback(check) + return d + def test_startBuild(self): return self.sb.callRemote("startBuild") @@ -323,81 +336,6 @@ def check(_): d.addCallback(check) return d -class FakeSlaveBuilder: - """ Fake SlaveBuilder """ - -class TestPBSlaveBuilder(unittest.TestCase): - - def setUp(self): - self.slavebuilder = FakeSlaveBuilder() - self.pbbuilder = bot.PBSlaveBuilder(self.slavebuilder) - self.sb = FakeRemote(self.pbbuilder) - - def tearDown(self): - pass - - def test_startBuild(self): - self.slavebuilder.startBuild = Mock() - self.sb.callRemote("startBuild") - self.slavebuilder.startBuild.assert_called_with() - - def test_shutdown(self): - # don't *actually* shut down the reactor - that would be silly - stop = mock.Mock() - self.patch(reactor, "stop", stop) - d = self.sb.callRemote("shutdown") - def check(_): - self.assertTrue(stop.called) - d.addCallback(check) - return d - - def test_setMaster(self): - # not much to check here - what the SlaveBuilder does with the - # master is not part of the interface (and, in fact, it does very little) - return self.sb.callRemote("setMaster", mock.Mock()) - - def test_startCommand(self): - st = FakeStep() - self.slavebuilder.startCommand = Mock() - - self.sb.callRemote("startCommand", FakeRemote(st), - "13", "shell", dict( - command=[ 'echo', 'hello' ], - workdir='workdir', - )) - self.slavebuilder.startCommand.assert_called_with("13", "shell", dict( - command=[ 'echo', 'hello' ], - workdir='workdir', - ) ) - - def test_interruptCommand(self): - self.slavebuilder.interruptCommand = Mock() - - self.sb.callRemote("interruptCommand", "13", "tl/dr" ) - self.slavebuilder.interruptCommand.assert_called_with("13", "tl/dr") - - def test_print(self): - self.slavebuilder.printMessage = Mock() - - self.sb.callRemote("print", "Hello, SlaveBuilder.") - self.slavebuilder.printMessage.assert_called_with("Hello, SlaveBuilder.") - - def test_sendUpdates(self): - st = FakeStep() - self.pbbuilder.remoteStep = FakeRemote(st) - - self.pbbuilder.remoteStep.callRemote("update", [[{'hdr': 'headers'}, 0]]) - self.assertEqual(st.actions, [ - ['update', [[{'hdr': 'headers'}, 0]]] - ]) - - def test_sendComplete(self): - st = FakeStep() - self.pbbuilder.remoteStep = FakeRemote(st) - f = Mock() - self.pbbuilder.remoteStep.callRemote("complete", f) - self.assertEqual(st.actions, [['complete', f]]) - class TestBotFactory(unittest.TestCase): def setUp(self): diff --git a/slave/buildslave/test/unit/test_bot_BuildSlave.py b/slave/buildslave/test/unit/test_bot_BuildSlave.py index c62accbb4f0..80d46cc781a 100644 --- a/slave/buildslave/test/unit/test_bot_BuildSlave.py +++ b/slave/buildslave/test/unit/test_bot_BuildSlave.py @@ -32,11 +32,6 @@ # up a TCP connection. This just tests that the PB code will connect and can # execute a basic ping. The rest is done without TCP (or PB) in other test modules. -class FakeBot: - """Fake slave-side bot.""" - usePTY = None - name = "fakebot" - class MasterPerspective(pb.Avatar): def __init__(self, on_keepalive=None): self.on_keepalive = on_keepalive @@ -163,14 +158,18 @@ def test_buildslave_graceful_shutdown(self): in a call to the master's shutdown method""" d = defer.Deferred() - fakebot = FakeBot() - - fakebot.gracefulShutdown = Mock(return_value = defer.succeed(None)) + fakepersp = Mock() + called = [] + def fakeCallRemote(*args): + called.append(args) + d1 = defer.succeed(None) + return d1 + fakepersp.callRemote = fakeCallRemote + # set up to call shutdown when we are attached, and chain the results onto # the deferred for the whole test def call_shutdown(mind): - self.buildslave.bot = fakebot - self.buildslave.bot.activeConnection = Mock(return_value = mind) + self.buildslave.bf.perspective = fakepersp shutdown_d = self.buildslave.gracefulShutdown() shutdown_d.addCallbacks(d.callback, d.errback) @@ -180,10 +179,11 @@ def call_shutdown(mind): self.buildslave = bot.BuildSlave("127.0.0.1", port, "testy", "westy", self.basedir, keepalive=0, usePTY=False, umask=022) + self.buildslave.startService() def check(ign): - fakebot.gracefulShutdown.assert_called_with() + self.assertEquals(called, [('shutdown',)]) d.addCallback(check) return d From b82f3876b01899cf6bfb8c5117ea54e245e605e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fl=C3=A1vio=20J=2E=20Saraiva?= Date: Thu, 19 Jul 2012 02:21:49 +0100 Subject: [PATCH 28/47] Update release notes about SVNPoller's split_file; Update documentation of SVNPoller's split_file; Fix grave accent mismatch in the Customizing SVNPoller section. --- master/docs/manual/cfg-changesources.rst | 12 +++++++++--- master/docs/manual/customization.rst | 2 +- master/docs/release-notes.rst | 3 +++ 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/master/docs/manual/cfg-changesources.rst b/master/docs/manual/cfg-changesources.rst index 5a60cc3b045..a2b32f5b2e7 100644 --- a/master/docs/manual/cfg-changesources.rst +++ b/master/docs/manual/cfg-changesources.rst @@ -856,9 +856,15 @@ multiple branches. A function to convert pathnames into ``(branch, relative_pathname)`` tuples. Use this to explain your repository's branch-naming policy to :bb:chsrc:`SVNPoller`. This function must accept a single string (the - pathname relative to the repository) and return a two-entry tuple. There - are a few utility functions in :mod:`buildbot.changes.svnpoller` that can - be used as a :meth:`split_file` function; see below for details. + pathname relative to the repository) and return a two-entry tuple. + Directory pathnames always end with a right slash to distinguish them from + files, like ``trunk/src/``, or ``src/``. There are a few utility functions + in :mod:`buildbot.changes.svnpoller` that can be used as a :meth:`split_file` + function; see below for details. + + For directories, the relative pathname returned by :meth:`split_file` should + end with a right slash but an empty string is also accepted for the root, + like ``("branches/1.5.x", "")`` being converted from ``"branches/1.5.x/"``. The default value always returns ``(None, path)``, which indicates that all files are on the trunk. diff --git a/master/docs/manual/customization.rst b/master/docs/manual/customization.rst index 0fe8ff5e81e..7182a023a54 100644 --- a/master/docs/manual/customization.rst +++ b/master/docs/manual/customization.rst @@ -155,7 +155,7 @@ fully-qualified SVN URL in the following form: ``({REPOURL})({PROJECT-plus-BRANCH})({FILEPATH})``. When you create the :bb:chsrc:`SVNPoller`, you give it a ``svnurl`` value that includes all of the ``{REPOURL}`` and possibly some portion of the -``{PROJECT-plus-BRANCH}` string. The :bb:chsrc:`SVNPoller`` is responsible +``{PROJECT-plus-BRANCH}`` string. The :bb:chsrc:`SVNPoller` is responsible for producing Changes that contain a branch name and a ``{FILEPATH}`` (which is relative to the top of a checked-out tree). The details of how these strings are split up depend upon how your repository names its branches. diff --git a/master/docs/release-notes.rst b/master/docs/release-notes.rst index 55541d4c859..36d84bced27 100644 --- a/master/docs/release-notes.rst +++ b/master/docs/release-notes.rst @@ -68,6 +68,9 @@ Deprecations, Removals, and Non-Compatible Changes * The ``P4Sync`` step, deprecated since 0.8.5, has been removed. The ``P4`` step remains. +* :bb:chsrc:`SVNPoller` now allows the ``split_file`` function to distinguish between + files and directories, by making sure directory pathnames end with a right slash. + Changes for Developers ~~~~~~~~~~~~~~~~~~~~~~ From 172620894a2b85eacf26c0e532c32a63c109ee83 Mon Sep 17 00:00:00 2001 From: Tom Prince Date: Sat, 21 Jul 2012 08:01:41 -0600 Subject: [PATCH 29/47] Fix `DirtyReactorError` caused by `PollingChangeSource` calling `callWhenRunning`. trial has an unusual interaction with the reactor. The reactor is only running when waiting for a deferred (and not a synchronous one). This avoids calling the real `PollingChangeSource.startService` to avoid this. --- master/buildbot/test/unit/test_changes_gitpoller.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/master/buildbot/test/unit/test_changes_gitpoller.py b/master/buildbot/test/unit/test_changes_gitpoller.py index dd5b43c5649..56628f8dcb5 100644 --- a/master/buildbot/test/unit/test_changes_gitpoller.py +++ b/master/buildbot/test/unit/test_changes_gitpoller.py @@ -14,9 +14,10 @@ # Copyright Buildbot Team Members import os +import mock from twisted.trial import unittest from twisted.internet import defer -from buildbot.changes import gitpoller +from buildbot.changes import base, gitpoller from buildbot.test.util import changesource, config, gpo from buildbot.util import epoch2datetime @@ -118,7 +119,6 @@ class TestGitPoller(gpo.GetProcessOutputMixin, changesource.ChangeSourceMixin, unittest.TestCase): - POLLERID = 100 REPOURL = 'git@example.com:foo/baz.git' REPOURL_QUOTED = 'git%40example.com%3Afoo%2Fbaz.git' @@ -510,15 +510,19 @@ def check_changes(_): return d def test_startService(self): + startService = mock.Mock() + self.patch(base.PollingChangeSource, "startService", startService) d = self.poller.startService() def check(_): self.assertEqual(self.poller.workdir, 'basedir/gitpoller-work') self.assertEqual(self.poller.lastRev, {}) - self.assertTrue(self.poller.running) + startService.assert_called_once_with(self.poller) d.addCallback(check) return d def test_startService_loadLastRev(self): + startService = mock.Mock() + self.patch(base.PollingChangeSource, "startService", startService) self.master.db.state.fakeState( name=self.REPOURL, class_name='GitPoller', lastRev={"master": "fa3ae8ed68e664d4db24798611b352e3c6509930"}, @@ -529,11 +533,11 @@ def check(_): self.assertEqual(self.poller.lastRev, { "master": "fa3ae8ed68e664d4db24798611b352e3c6509930" }) + startService.assert_called_once_with(self.poller) d.addCallback(check) return d - class TestGitPollerConstructor(unittest.TestCase, config.ConfigErrorsMixin): def test_deprecatedFetchRefspec(self): self.assertRaisesConfigError("fetch_refspec is no longer supported", From a315b0a6a349910c7a7287aa0a6a83c93edb4357 Mon Sep 17 00:00:00 2001 From: Tom Prince Date: Sat, 21 Jul 2012 08:07:42 -0600 Subject: [PATCH 30/47] Report repo not workdir when polling git. Since we now default to using the same workdir for every poller, it makes more sense to report the remote repository instead. --- master/buildbot/changes/gitpoller.py | 4 ++-- master/buildbot/test/unit/test_changes_gitpoller.py | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/master/buildbot/changes/gitpoller.py b/master/buildbot/changes/gitpoller.py index 4de0b24177e..68934703e83 100644 --- a/master/buildbot/changes/gitpoller.py +++ b/master/buildbot/changes/gitpoller.py @@ -195,8 +195,8 @@ def _process_changes(self, newRev, branch): revList.reverse() self.changeCount = len(revList) - log.msg('gitpoller: processing %d changes: %s in "%s"' - % (self.changeCount, revList, self.workdir) ) + log.msg('gitpoller: processing %d changes: %s from "%s"' + % (self.changeCount, revList, self.repourl) ) for rev in revList: dl = defer.DeferredList([ diff --git a/master/buildbot/test/unit/test_changes_gitpoller.py b/master/buildbot/test/unit/test_changes_gitpoller.py index 56628f8dcb5..765b99e21fa 100644 --- a/master/buildbot/test/unit/test_changes_gitpoller.py +++ b/master/buildbot/test/unit/test_changes_gitpoller.py @@ -509,6 +509,9 @@ def check_changes(_): return d + # We mock out base.PollingChangeSource.startService, since it calls + # reactor.callWhenRunning, which leaves a dirty reactor if a synchronous + # deferred is returned from a test method. def test_startService(self): startService = mock.Mock() self.patch(base.PollingChangeSource, "startService", startService) From a6a833647f2c3b12028c094f1bf9047a2d059535 Mon Sep 17 00:00:00 2001 From: Tom Prince Date: Sat, 21 Jul 2012 08:11:40 -0600 Subject: [PATCH 31/47] Fix comment in GitPoller. --- master/buildbot/changes/gitpoller.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/master/buildbot/changes/gitpoller.py b/master/buildbot/changes/gitpoller.py index 68934703e83..085f6d5a93b 100644 --- a/master/buildbot/changes/gitpoller.py +++ b/master/buildbot/changes/gitpoller.py @@ -231,7 +231,7 @@ def _dovccmd(self, command, args, path=None): d = utils.getProcessOutputAndValue(self.gitbin, [command] + args, path=path, env=os.environ) def _convert_nonzero_to_failure(res): - "utility method to handle the result of getProcessOutputAndValue" + "utility to handle the result of getProcessOutputAndValue" (stdout, stderr, code) = res if code != 0: raise EnvironmentError('command failed with exit code %d: %s' From 742650621d33dc61679efc63532b622f1cf86020 Mon Sep 17 00:00:00 2001 From: Marius Rieder Date: Sun, 22 Jul 2012 16:29:18 +0200 Subject: [PATCH 32/47] Replace check for not None with check fot not '' request.getUser() return the empty string if no user was supplied. http://twistedmatrix.com/documents/12.0.0/api/twisted.web.http.Request.html#getUser --- master/buildbot/status/web/authz.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/master/buildbot/status/web/authz.py b/master/buildbot/status/web/authz.py index 19a1d42ca8e..d11a0601e6c 100644 --- a/master/buildbot/status/web/authz.py +++ b/master/buildbot/status/web/authz.py @@ -65,7 +65,7 @@ def session(self, request): def authenticated(self, request): if self.useHttpHeader: - return request.getUser() != None + return request.getUser() != '' return self.session(request) != None def getUserInfo(self, user): From 47fded37e84ebe37e11902d7c8a7af1fde2aded2 Mon Sep 17 00:00:00 2001 From: Marius Rieder Date: Sun, 22 Jul 2012 16:36:24 +0200 Subject: [PATCH 33/47] Add httpLoginUrl argument to Authz. And login link to Template. Allow to specifie a login url to render a login link in case of useHttpHeader Authentication. --- master/buildbot/status/web/authz.py | 2 ++ master/buildbot/status/web/templates/layout.html | 2 ++ 2 files changed, 4 insertions(+) diff --git a/master/buildbot/status/web/authz.py b/master/buildbot/status/web/authz.py index d11a0601e6c..c31fbe27add 100644 --- a/master/buildbot/status/web/authz.py +++ b/master/buildbot/status/web/authz.py @@ -40,12 +40,14 @@ def __init__(self, default_action=False, auth=None, useHttpHeader=False, + httpLoginUrl=False, **kwargs): self.auth = auth if auth: assert IAuth.providedBy(auth) self.useHttpHeader = useHttpHeader + self.httpLoginUrl = httpLoginUrl self.config = dict( (a, default_action) for a in self.knownActions ) for act in self.knownActions: diff --git a/master/buildbot/status/web/templates/layout.html b/master/buildbot/status/web/templates/layout.html index 3c25a93aabc..15fd0d47c55 100644 --- a/master/buildbot/status/web/templates/layout.html +++ b/master/buildbot/status/web/templates/layout.html @@ -39,6 +39,8 @@ {% if authz.authenticated(request) %} {{ authz.getUsernameHTML(request) }} |Logout + {% elif authz.useHttpHeader and authz.httpLoginUrl %} + Login {% elif authz.auth %}
From 89dc69248bab8cfe0e4c9f18c99cbe880801c096 Mon Sep 17 00:00:00 2001 From: Marius Rieder Date: Tue, 24 Jul 2012 07:50:27 +0200 Subject: [PATCH 34/47] Last but not least some documentation. --- master/docs/manual/cfg-statustargets.rst | 34 ++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/master/docs/manual/cfg-statustargets.rst b/master/docs/manual/cfg-statustargets.rst index 95f01b6536e..31769791778 100644 --- a/master/docs/manual/cfg-statustargets.rst +++ b/master/docs/manual/cfg-statustargets.rst @@ -553,6 +553,40 @@ listen for incoming connections only on localhost (or on some firewall-protected port). Frontend must require HTTP authentication to access WebStatus pages (using any source for credentials, such as htpasswd, PAM, LDAP). +If you allow unauthenticated access through frontend as well, it's possible to +specify a ``httpLoginLink`` which will be rendered on the WebStatus for +unauthenticated users as a link named Login. :: + + authz = Authz(useHttpHeader=True, httpLoginLink='https://buildbot/login') + +A configuration example with Apache HTTPD as reverse proxy could look like the +following. :: + + authz = Authz( + useHttpHeader=True, + httpLoginLink='https://buildbot/login', + + auth = HTPasswdAprAuth('/var/www/htpasswd'), + + forceBuild = 'auth') + +Corresponding Apache configuration. + +.. code-block:: apache + + ProxyPass / http://127.0.0.1:8010/ + + + AuthType Basic + AuthName "Buildbot" + AuthUserFile /var/www/htpasswd + Require valid-user + + RewriteEngine on + RewriteCond %{HTTP_REFERER} ^https?://([^/]+)/(.*)$ + RewriteRule ^.*$ https://%1/%2 [R,L] + + Logging configuration ##################### From 7573f9896d694d66170975eb68f20a02c2a78df9 Mon Sep 17 00:00:00 2001 From: Tim Horton Date: Tue, 24 Jul 2012 18:15:22 -0700 Subject: [PATCH 35/47] Wrap console "Personalized for..." field in a form so that pressing enter submits it --- master/buildbot/status/web/templates/console.html | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/master/buildbot/status/web/templates/console.html b/master/buildbot/status/web/templates/console.html index d27a3e8a5a3..910bf782b13 100644 --- a/master/buildbot/status/web/templates/console.html +++ b/master/buildbot/status/web/templates/console.html @@ -124,11 +124,13 @@

Console View

} // ]]> - - + + + + From d7bc842c63372cb455a2914329b9210fd000b148 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Tue, 24 Jul 2012 23:08:18 -0500 Subject: [PATCH 36/47] Don't fail triggering if there's no got_revision. Refs #2328. --- master/buildbot/steps/trigger.py | 11 ++++++----- master/buildbot/test/unit/test_steps_trigger.py | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/master/buildbot/steps/trigger.py b/master/buildbot/steps/trigger.py index ee11424cd79..66e0b6fdfdb 100644 --- a/master/buildbot/steps/trigger.py +++ b/master/buildbot/steps/trigger.py @@ -122,11 +122,12 @@ def prepareSourcestampListForTrigger(self): properties = self.build.getProperties() got = properties.getProperty('got_revision') # be sure property is always a dictionary - if got and not isinstance(got, dict): - got = {'': got} - for codebase in ss_for_trigger: - if codebase in got: - ss_for_trigger[codebase]['revision'] = got[codebase] + if got: + if not isinstance(got, dict): + got = {'': got} + for codebase in ss_for_trigger: + if codebase in got: + ss_for_trigger[codebase]['revision'] = got[codebase] # update sourcestamps from build with passed set of fixed sourcestamps # or add fixed sourcestamp to the dictionary diff --git a/master/buildbot/test/unit/test_steps_trigger.py b/master/buildbot/test/unit/test_steps_trigger.py index 7578c6e3252..03c04608ada 100644 --- a/master/buildbot/test/unit/test_steps_trigger.py +++ b/master/buildbot/test/unit/test_steps_trigger.py @@ -257,6 +257,22 @@ def test_updateSourceStamp(self): }, {})) return self.runStep() + def test_updateSourceStamp_no_got_revision(self): + self.setupStep(trigger.Trigger(schedulerNames=['a'], + updateSourceStamp=True), + sourcestampsInBuild = [FakeSourceStamp(self.THIS_SSID, + self.THIS_SS_SETID, + codebase='', + repository='x', + revision=11111) + ]) + self.expectOutcome(result=SUCCESS, status_text=['triggered', 'a']) + self.expectTriggeredWith(a=({'':{'codebase':'', + 'repository': 'x', + 'revision': 11111} # uses old revision + }, {})) + return self.runStep() + def test_not_updateSourceStamp(self): self.setupStep(trigger.Trigger(schedulerNames=['a'], updateSourceStamp=False), From c84a4e908248cc98353c87470af113a42097a27b Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Tue, 24 Jul 2012 23:22:37 -0500 Subject: [PATCH 37/47] Remove bogus check At the very least, the check's condition is wrong. I think that the text is wrong, and the comment is confusing, too. Fixes #2328. --- master/buildbot/schedulers/base.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/master/buildbot/schedulers/base.py b/master/buildbot/schedulers/base.py index ef0788f6b55..0f945b13174 100644 --- a/master/buildbot/schedulers/base.py +++ b/master/buildbot/schedulers/base.py @@ -340,17 +340,10 @@ def addBuildsetForSourceStampSetDetails(self, reason, sourcestamps, properties): # sourcestamp attributes for this codebase. ss.update(sourcestamps.get(codebase,{})) - # at least repository must be set, this is normaly forced except when - # codebases is not explicitly set in configuration file. - ss_repository = ss.get('repository') - if not ss_repository: - config.error("The codebases argument is not set but still receiving " + - "non empty codebase values") - # add sourcestamp to the new setid yield self.master.db.sourcestamps.addSourceStamp( codebase=codebase, - repository=ss_repository, + repository=ss.get('repository'), branch=ss.get('branch', None), revision=ss.get('revision', None), project=ss.get('project', ''), From 128d546669153ccc5889504aea54ee794013bbca Mon Sep 17 00:00:00 2001 From: Marius Rieder Date: Wed, 25 Jul 2012 07:28:23 +0200 Subject: [PATCH 38/47] Remove newlines. --- master/docs/manual/cfg-statustargets.rst | 2 -- 1 file changed, 2 deletions(-) diff --git a/master/docs/manual/cfg-statustargets.rst b/master/docs/manual/cfg-statustargets.rst index 31769791778..ce93d413627 100644 --- a/master/docs/manual/cfg-statustargets.rst +++ b/master/docs/manual/cfg-statustargets.rst @@ -565,9 +565,7 @@ following. :: authz = Authz( useHttpHeader=True, httpLoginLink='https://buildbot/login', - auth = HTPasswdAprAuth('/var/www/htpasswd'), - forceBuild = 'auth') Corresponding Apache configuration. From c719474148e4c11df50d2fe2fe1fb0b4c9a8c00a Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Wed, 25 Jul 2012 09:40:42 -0500 Subject: [PATCH 39/47] Set the BuilderStatus's category on reconfig Fixes #2331. Tested manually. --- master/buildbot/process/builder.py | 1 + master/buildbot/status/builder.py | 4 ++++ master/buildbot/test/fake/fakemaster.py | 3 +++ 3 files changed, 8 insertions(+) diff --git a/master/buildbot/process/builder.py b/master/buildbot/process/builder.py index 5ce09c9b3df..88e3d04bc54 100644 --- a/master/buildbot/process/builder.py +++ b/master/buildbot/process/builder.py @@ -88,6 +88,7 @@ def reconfigService(self, new_config): self.config = builder_config + self.builder_status.setCategory(builder_config.category) self.builder_status.setSlavenames(self.config.slavenames) self.builder_status.setCacheSize(new_config.caches['Builds']) diff --git a/master/buildbot/status/builder.py b/master/buildbot/status/builder.py index 70322131c9d..7812ea467a7 100644 --- a/master/buildbot/status/builder.py +++ b/master/buildbot/status/builder.py @@ -295,6 +295,10 @@ def getLastFinishedBuild(self): b = self.getBuild(-2) return b + def setCategory(self, category): + # used during reconfig + self.category = category + def getCategory(self): return self.category diff --git a/master/buildbot/test/fake/fakemaster.py b/master/buildbot/test/fake/fakemaster.py index 22267a59d8d..9e0efb5f291 100644 --- a/master/buildbot/test/fake/fakemaster.py +++ b/master/buildbot/test/fake/fakemaster.py @@ -56,6 +56,9 @@ def builderAdded(self, name, basedir, category=None): class FakeBuilderStatus(object): + def setCategory(self, category): + pass + def setSlavenames(self, names): pass From 673145e83fefb001c9dcc6579e5e2f962b4b43be Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Wed, 25 Jul 2012 09:52:04 -0500 Subject: [PATCH 40/47] Don't test types too carefully (long is not an int) This code really just needs to know whether there was an error. Fixes #2329. --- master/buildbot/status/web/build.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/master/buildbot/status/web/build.py b/master/buildbot/status/web/build.py index 13f43eba7cb..18150eed7fc 100644 --- a/master/buildbot/status/web/build.py +++ b/master/buildbot/status/web/build.py @@ -67,10 +67,8 @@ def performAction(self, req): msg += "could not get builder control" else: tup = yield bc.rebuildBuild(b, reason, extraProperties) - # check that (bsid, brids) were properly stored - if not (isinstance(tup, tuple) and - isinstance(tup[0], int) and - isinstance(tup[1], dict)): + # rebuildBuild returns None on error (?!) + if not tup: msg = "rebuilding a build failed "+ str(tup) # we're at # http://localhost:8080/builders/NAME/builds/5/rebuild?[args] From 63d9e221890dd1748aba5b3ad02f05ea59e04a0c Mon Sep 17 00:00:00 2001 From: Marius Rieder Date: Wed, 25 Jul 2012 17:29:05 +0200 Subject: [PATCH 41/47] Add entry to release-notes.rst --- master/docs/release-notes.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/master/docs/release-notes.rst b/master/docs/release-notes.rst index 0d7fca3c475..d2fc42b792b 100644 --- a/master/docs/release-notes.rst +++ b/master/docs/release-notes.rst @@ -97,6 +97,10 @@ Features * The mercurial hook now supports multple masters. See :bb:pull:`436`. +* Added a new property ``httpLoginUrl`` to ``buildbot.status.web.authz.Authz`` + to render a nice Login link in WebStatus for unauthenticated users if + ``useHttpHeader`` and ``httpLoginUrl`` are set. + Slave ----- From 35ade7866fab159d600910f3c472a5830ae8d93e Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Wed, 25 Jul 2012 10:41:30 -0500 Subject: [PATCH 42/47] fix tests for getUser() returning '' as documented --- master/buildbot/test/unit/test_status_web_authz_Authz.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/master/buildbot/test/unit/test_status_web_authz_Authz.py b/master/buildbot/test/unit/test_status_web_authz_Authz.py index 4753226795a..01b3b528e28 100644 --- a/master/buildbot/test/unit/test_status_web_authz_Authz.py +++ b/master/buildbot/test/unit/test_status_web_authz_Authz.py @@ -30,7 +30,7 @@ def __init__(self, username=None, passwd=None): self.received_cookies = {} self.send_cookies = [] def getUser(self): - return None + return '' def getPassword(self): return None @@ -204,7 +204,7 @@ def test_getPassword_http(self): def test_getUsername_http_missing(self): z = Authz(useHttpHeader = True) - assert z.getUsername(StubRequest('foo', 'bar')) == None + assert z.getUsername(StubRequest('foo', 'bar')) == '' def test_getPassword_http_missing(self): z = Authz(useHttpHeader = True) From de0ed4240abf4be3c147de5016bbd8775a217d2b Mon Sep 17 00:00:00 2001 From: Marius Rieder Date: Sat, 28 Jul 2012 10:09:02 +0200 Subject: [PATCH 43/47] Set Cache-Controll header for log files in WebStatus. (Fix #2292) --- master/buildbot/status/web/logs.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/master/buildbot/status/web/logs.py b/master/buildbot/status/web/logs.py index 79b953ada5f..bc47d49f17b 100644 --- a/master/buildbot/status/web/logs.py +++ b/master/buildbot/status/web/logs.py @@ -103,6 +103,11 @@ def render_GET(self, req): self._setContentType(req) self.req = req + if (self.original.isFinished()): + req.setHeader("Cache-Control", "max-age=604800") + else: + req.setHeader("Cache-Control", "no-cache") + if not self.asText: self.template = req.site.buildbot_service.templates.get_template("logs.html") From a16c4ececcab7f304d369f742e6e3dff04b66a95 Mon Sep 17 00:00:00 2001 From: Jacob Stultz Date: Thu, 26 Jul 2012 17:53:36 -0700 Subject: [PATCH 44/47] Maintain locks in FIFO order Locks should be kept in a queue in FIFO order to prevent potential exclusive lock holders from being starved by a continuous stream of counting locks holders. This change maintains order of all waiters in a FIFO queue until they actually acquire the lock, instead of removing them from the queue on wake and adding them back to the end if they failed to acquire the lock. The Deferred instance associated with each waiter is cleared when it is woken. It is set when waitUntilMaybeAvailable is called. --- master/buildbot/buildslave.py | 2 +- master/buildbot/locks.py | 59 +++++++++++++++++----------- master/buildbot/process/build.py | 2 +- master/buildbot/process/buildstep.py | 2 +- 4 files changed, 40 insertions(+), 25 deletions(-) diff --git a/master/buildbot/buildslave.py b/master/buildbot/buildslave.py index 7c605713887..e7e1d654565 100644 --- a/master/buildbot/buildslave.py +++ b/master/buildbot/buildslave.py @@ -141,7 +141,7 @@ def locksAvailable(self): if not self.locks: return True for lock, access in self.locks: - if not lock.isAvailable(access): + if not lock.isAvailable(self, access): return False return True diff --git a/master/buildbot/locks.py b/master/buildbot/locks.py index 6fabdb1401d..d2f277181ca 100644 --- a/master/buildbot/locks.py +++ b/master/buildbot/locks.py @@ -29,17 +29,16 @@ class BaseLock: Class handling claiming and releasing of L{self}, and keeping track of current and waiting owners. - @note: Ideally, we'd like to maintain FIFO order. The place to do that - would be the L{isAvailable()} function. However, this function is - called by builds/steps both for the first time, and after waking - them up by L{self} from the L{self.waiting} queue. There is - currently no way of distinguishing between them. + We maintain the wait queue in FIFO order, and ensure that counting waiters + in the queue behind exclusive waiters cannot acquire the lock. This ensures + that exclusive waiters are not starved. """ description = "" def __init__(self, name, maxCount=1): self.name = name # Name of the lock - self.waiting = [] # Current queue, tuples (LockAccess, deferred) + self.waiting = [] # Current queue, tuples (waiter, LockAccess, + # deferred) self.owners = [] # Current owners, tuples (owner, LockAccess) self.maxCount = maxCount # maximal number of counting owners @@ -67,26 +66,34 @@ def _getOwnersCount(self): return num_excl, num_counting - def isAvailable(self, access): + def isAvailable(self, requester, access): """ Return a boolean whether the lock is available for claiming """ - debuglog("%s isAvailable(%s): self.owners=%r" - % (self, access, self.owners)) + debuglog("%s isAvailable(%s, %s): self.owners=%r" + % (self, requester, access, self.owners)) num_excl, num_counting = self._getOwnersCount() + + # Find all waiters ahead of the requester in the wait queue + w_index = next((idx for idx, waiter in enumerate(self.waiting) \ + if waiter[0] == requester), len(self.waiting)) + ahead = self.waiting[:w_index] + if access.mode == 'counting': # Wants counting access - return num_excl == 0 and num_counting < self.maxCount + return num_excl == 0 and num_counting + len(ahead) < self.maxCount \ + and all([w[1].mode == 'counting' for w in ahead]) else: # Wants exclusive access - return num_excl == 0 and num_counting == 0 + return num_excl == 0 and num_counting == 0 and len(ahead) == 0 def claim(self, owner, access): """ Claim the lock (lock must be available) """ debuglog("%s claim(%s, %s)" % (self, owner, access.mode)) assert owner is not None - assert self.isAvailable(access), "ask for isAvailable() first" + assert self.isAvailable(owner, access), "ask for isAvailable() first" assert isinstance(access, LockAccess) assert access.mode in ['counting', 'exclusive'] + self.waiting = [w for w in self.waiting if w[0] != owner] self.owners.append((owner, access)) debuglog(" %s is claimed '%s'" % (self, access.mode)) @@ -109,22 +116,24 @@ def release(self, owner, access): # After an exclusive access, we may need to wake up several waiting. # Break out of the loop when the first waiting client should not be awakened. num_excl, num_counting = self._getOwnersCount() - while len(self.waiting) > 0: - access, d = self.waiting[0] - if access.mode == 'counting': + for i, (w_owner, w_access, d) in enumerate(self.waiting): + if w_access.mode == 'counting': if num_excl > 0 or num_counting == self.maxCount: break else: num_counting = num_counting + 1 else: - # access.mode == 'exclusive' + # w_access.mode == 'exclusive' if num_excl > 0 or num_counting > 0: break else: num_excl = num_excl + 1 - del self.waiting[0] - reactor.callLater(0, d.callback, self) + # If the waiter has a deferred, wake it up and clear the deferred + # from the wait queue entry to indicate that it has been woken. + if d: + self.waiting[i] = (w_owner, w_access, None) + reactor.callLater(0, d.callback, self) # notify any listeners self.release_subs.deliver() @@ -138,17 +147,23 @@ def waitUntilMaybeAvailable(self, owner, access): """ debuglog("%s waitUntilAvailable(%s)" % (self, owner)) assert isinstance(access, LockAccess) - if self.isAvailable(access): + if self.isAvailable(owner, access): return defer.succeed(self) d = defer.Deferred() - self.waiting.append((access, d)) + + # Are we already in the wait queue? + w = [i for i, w in enumerate(self.waiting) if w[0] == owner] + if w: + self.waiting[w[0]] = (owner, access, d) + else: + self.waiting.append((owner, access, d)) return d def stopWaitingUntilAvailable(self, owner, access, d): debuglog("%s stopWaitingUntilAvailable(%s)" % (self, owner)) assert isinstance(access, LockAccess) - assert (access, d) in self.waiting - self.waiting.remove( (access, d) ) + assert (owner, access, d) in self.waiting + self.waiting = [w for w in self.waiting if w[0] != owner] def isOwner(self, owner, access): return (owner, access) in self.owners diff --git a/master/buildbot/process/build.py b/master/buildbot/process/build.py index 42fc4f18080..347744447bf 100644 --- a/master/buildbot/process/build.py +++ b/master/buildbot/process/build.py @@ -280,7 +280,7 @@ def acquireLocks(self, res=None): return defer.succeed(None) log.msg("acquireLocks(build %s, locks %s)" % (self, self.locks)) for lock, access in self.locks: - if not lock.isAvailable(access): + if not lock.isAvailable(self, access): log.msg("Build %s waiting for lock %s" % (self, lock)) d = lock.waitUntilMaybeAvailable(self, access) d.addCallback(self.acquireLocks) diff --git a/master/buildbot/process/buildstep.py b/master/buildbot/process/buildstep.py index 6512bf43de3..b400d74f957 100644 --- a/master/buildbot/process/buildstep.py +++ b/master/buildbot/process/buildstep.py @@ -545,7 +545,7 @@ def acquireLocks(self, res=None): return defer.succeed(None) log.msg("acquireLocks(step %s, locks %s)" % (self, self.locks)) for lock, access in self.locks: - if not lock.isAvailable(access): + if not lock.isAvailable(self, access): self.step_status.setWaitingForLocks(True) log.msg("step %s waiting for lock %s" % (self, lock)) d = lock.waitUntilMaybeAvailable(self, access) From d76c5d32340c123c72bf20069e879a7f10327a3c Mon Sep 17 00:00:00 2001 From: Jacob Stultz Date: Fri, 27 Jul 2012 16:01:42 -0700 Subject: [PATCH 45/47] Ensure that locks are acquired in FIFO order To prevent starvation, locks should be acquired in the order in which they were requested. Particularly, counting locks requested after exclusive locks must wait until after the exclusive lock is acquired and subsequently released, even if the owner(s) at the time the lock is requested are counting and below maxCount. The test acquires a counting lock, and then starts a build needing an exclusive lock followed by a build needing a counting lock. The first counting lock is then released, and the test checks that the exclusive lock was claimed before the second counting lock. --- .../buildbot/test/unit/test_process_build.py | 59 ++++++++++++++++++- 1 file changed, 57 insertions(+), 2 deletions(-) diff --git a/master/buildbot/test/unit/test_process_build.py b/master/buildbot/test/unit/test_process_build.py index 4edc1be0cc5..849e2a9d9b7 100644 --- a/master/buildbot/test/unit/test_process_build.py +++ b/master/buildbot/test/unit/test_process_build.py @@ -100,10 +100,13 @@ def setUp(self): r.sources = [FakeSource()] r.sources[0].changes = [FakeChange()] r.sources[0].revision = "12345" - + + self.request = r + self.master = FakeMaster() + self.build = Build([r]) self.builder = Mock() - self.builder.botmaster = FakeMaster() + self.builder.botmaster = self.master self.build.setBuilder(self.builder) def testRunSuccessfulBuild(self): @@ -215,6 +218,58 @@ def claim(owner, access): in step.method_calls) self.assertEquals(claimCount[0], 1) + def testBuildLocksOrder(self): + """Test that locks are acquired in FIFO order; specifically that + counting locks cannot jump ahead of exclusive locks""" + eBuild = self.build + + cBuild = Build([self.request]) + cBuilder = Mock() + cBuilder.botmaster = self.master + cBuild.setBuilder(cBuilder) + + eSlavebuilder = Mock() + cSlavebuilder = Mock() + + slave = eSlavebuilder.slave + cSlavebuilder.slave = slave + + l = SlaveLock('lock', 2) + claimLog = [] + realLock = self.master.getLockByID(l).getLock(slave) + def claim(owner, access): + claimLog.append(owner) + return realLock.oldClaim(owner, access) + realLock.oldClaim = realLock.claim + realLock.claim = claim + + eBuild.setLocks([l.access('exclusive')]) + cBuild.setLocks([l.access('counting')]) + + fakeBuild = Mock() + fakeBuildAccess = l.access('counting') + realLock.claim(fakeBuild, fakeBuildAccess) + + step = Mock() + step.return_value = step + step.startStep.return_value = SUCCESS + eBuild.setStepFactories([FakeStepFactory(step)]) + cBuild.setStepFactories([FakeStepFactory(step)]) + + e = eBuild.startBuild(FakeBuildStatus(), None, eSlavebuilder) + c = cBuild.startBuild(FakeBuildStatus(), None, cSlavebuilder) + d = defer.DeferredList([e, c]) + + realLock.release(fakeBuild, fakeBuildAccess) + + def check(ign): + self.assertEqual(eBuild.result, SUCCESS) + self.assertEqual(cBuild.result, SUCCESS) + self.assertEquals(claimLog, [fakeBuild, eBuild, cBuild]) + + d.addCallback(check) + return d + def testBuildWaitingForLocks(self): b = self.build From 9bd07dc6c6282dc3d57d405c0dc476429f0940c1 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Mon, 30 Jul 2012 22:03:46 -0400 Subject: [PATCH 46/47] fix for py25, where next() isn't defined --- master/buildbot/locks.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/master/buildbot/locks.py b/master/buildbot/locks.py index d2f277181ca..b70f358ea21 100644 --- a/master/buildbot/locks.py +++ b/master/buildbot/locks.py @@ -73,8 +73,12 @@ def isAvailable(self, requester, access): num_excl, num_counting = self._getOwnersCount() # Find all waiters ahead of the requester in the wait queue - w_index = next((idx for idx, waiter in enumerate(self.waiting) \ - if waiter[0] == requester), len(self.waiting)) + for idx, waiter in enumerate(self.waiting): + if waiter[0] == requester: + w_index = idx + break + else: + w_index = len(self.waiting) ahead = self.waiting[:w_index] if access.mode == 'counting': From 2e7f5e9b097725fb8223126170a66736ac3664c6 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Tue, 31 Jul 2012 10:25:46 -0400 Subject: [PATCH 47/47] Nicer directions for finding release notes --- master/docs/release-notes.rst | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/master/docs/release-notes.rst b/master/docs/release-notes.rst index 1502679244a..ce420c022d4 100644 --- a/master/docs/release-notes.rst +++ b/master/docs/release-notes.rst @@ -144,6 +144,5 @@ git log itself: Older Versions -------------- -Release notes for older versions of Buildbot are available in the -:bb:src:`master/docs/release-notes/` directory of the source tree, or in the archived -documentation for those versions at http://buildbot.net/buildbot/docs. +Release notes for older versions of Buildbot are available in the :bb:src:`master/docs/release-notes/` directory of the source tree. +Starting with version 0.8.6, they are also available under the appropriate version at http://buildbot.net/buildbot/docs.