Introduce a 'run' method for steps, which simply returns the result

This does away with the complex need to call self.finished or
self.failed

master/buildbot/process/buildstep.py

@@ -39,12 +39,16 @@
 from buildbot.status.results import SUCCESS
 from buildbot.status.results import WARNINGS
 from buildbot.status.results import worst_status
-from buildbot.util.eventual import eventually
 
 
 class BuildStepFailed(Exception):
     pass
 
+
+class BuildStepCancelled(Exception):
+    # used internally for signalling
+    pass
+
 # old import paths for these classes
 RemoteCommand = remotecommand.RemoteCommand
 LoggedRemoteCommand = remotecommand.LoggedRemoteCommand
@@ -210,7 +214,6 @@ def setProgress(self, metric, value):
     @defer.inlineCallbacks
     def startStep(self, remote):
         self.remote = remote
-        self.deferred = defer.Deferred()
         # convert all locks into their real form
         self.locks = [(self.build.builder.botmaster.getLockByID(access.lockid), access)
                       for access in self.locks]
@@ -235,8 +238,7 @@ def startStep(self, remote):
             yield self.acquireLocks()
 
             if self.stopped:
-                self.finished(CANCELLED)
-                defer.returnValue((yield self.deferred))
+                raise BuildStepCancelled
 
             # ste up progress
             if self.progress:
@@ -262,29 +264,71 @@ def setRenderable(res, attr):
                 dl.append(d)
             yield defer.gatherResults(dl)
 
-            try:
-                if doStep:
-                    result = yield defer.maybeDeferred(self.start)
-                    if result == SKIPPED:
-                        doStep = False
-            except:
-                log.msg("BuildStep.startStep exception in .start")
-                self.failed(Failure())
-
-            if not doStep:
+            # run -- or skip -- the step
+            if doStep:
+                results = yield self.run()
+            else:
                 self.step_status.setText(self.describe(True) + ['skipped'])
                 self.step_status.setSkipped(True)
-                # this return value from self.start is a shortcut to finishing
-                # the step immediately; we skip calling finished() as
-                # subclasses may have overridden that an expect it to be called
-                # after start() (bug #837)
-                eventually(self._finishFinished, SKIPPED)
+                results = SKIPPED
+
+        except BuildStepCancelled:
+            results = CANCELLED
+
+        except BuildStepFailed:
+            results = FAILURE
+            # fall through to the end
+
         except Exception:
-            self.failed(Failure())
+            why = Failure()
+            log.err(why, "BuildStep.failed; traceback follows")
+            # log the exception to the user, too
+            try:
+                self.addCompleteLog("err.text", why.getTraceback())
+                self.addHTMLLog("err.html", formatFailure(why))
+            except Exception:
+                log.err(Failure(), "error while formatting exceptions")
 
-        # and finally, wait for self.deferred to get triggered and return its
-        # value
-        defer.returnValue((yield self.deferred))
+            self.step_status.setText([self.name, "exception"])
+            self.step_status.setText2([self.name])
+
+            results = EXCEPTION
+
+        if self.stopped and results != RETRY:
+            # We handle this specially because we don't care about
+            # the return code of an interrupted command; we know
+            # that this should just be exception due to interrupt
+            # At the same time we must respect RETRY status because it's used
+            # to retry interrupted build due to some other issues for example
+            # due to slave lost
+            if results == CANCELLED:
+                self.step_status.setText(self.describe(True) +
+                                         ["cancelled"])
+                self.step_status.setText2(["cancelled"])
+            else:
+                # leave RETRY as-is, but change anything else to EXCEPTION
+                if results != RETRY:
+                    results = EXCEPTION
+                self.step_status.setText(self.describe(True) +
+                                         ["interrupted"])
+                self.step_status.setText2(["interrupted"])
+
+        if self.progress:
+            self.progress.finish()
+
+        self.step_status.stepFinished(results)
+        hidden = self.hideStepIf
+        if callable(hidden):
+            try:
+                hidden = hidden(results, self)
+            except Exception:
+                results = EXCEPTION
+                hidden = False
+        self.step_status.setHidden(hidden)
+
+        self.releaseLocks()
+
+        defer.returnValue(results)
 
     def acquireLocks(self, res=None):
         self._acquiringLock = None
@@ -307,8 +351,36 @@ def acquireLocks(self, res=None):
         self.step_status.setWaitingForLocks(False)
         return defer.succeed(None)
 
+    @defer.inlineCallbacks
+    def run(self):
+        self._start_running = True
+        self._start_deferred = defer.Deferred()
+        try:
+            # start() can return a Deferred, but the step isn't finished
+            # at that point.  But if it returns SKIPPED, then finish will
+            # never be called.
+            results = yield self.start()
+            if results == SKIPPED:
+                self.step_status.setText(self.describe(True) + ['skipped'])
+                self.step_status.setSkipped(True)
+            else:
+                results = yield self._start_deferred
+        finally:
+            self._start_running = False
+        defer.returnValue(results)
+
+    def finished(self, results):
+        assert self._start_running, \
+            "finished() can only be called from old steps implementing start()"
+        self._start_deferred.callback(results)
+
+    def failed(self, why):
+        assert self._start_running, \
+            "failed() can only be called from old steps implementing start()"
+        self._start_deferred.errback(why)
+
     def start(self):
-        raise NotImplementedError("your subclass must implement this method")
+        raise NotImplementedError("your subclass must implement run()")
 
     def interrupt(self, reason):
         self.stopped = True
@@ -326,88 +398,6 @@ def releaseLocks(self):
                 # This should only happen if we've been interrupted
                 assert self.stopped
 
-    def finished(self, results):
-        if self.stopped and results != RETRY:
-            # We handle this specially because we don't care about
-            # the return code of an interrupted command; we know
-            # that this should just be exception due to interrupt
-            # At the same time we must respect RETRY status because it's used
-            # to retry interrupted build due to some other issues for example
-            # due to slave lost
-            if results == CANCELLED:
-                self.step_status.setText(self.describe(True) +
-                                         ["cancelled"])
-                self.step_status.setText2(["cancelled"])
-            else:
-                # leave RETRY as-is, but change anything else to EXCEPTION
-                if results != RETRY:
-                    results = EXCEPTION
-                self.step_status.setText(self.describe(True) +
-                                         ["interrupted"])
-                self.step_status.setText2(["interrupted"])
-
-        self._finishFinished(results)
-
-    def _finishFinished(self, results):
-        # internal function to indicate that this step is done; this is separated
-        # from finished() so that subclasses can override finished()
-        if self.progress:
-            self.progress.finish()
-
-        try:
-            hidden = self._maybeEvaluate(self.hideStepIf, results, self)
-        except Exception:
-            why = Failure()
-            self.addHTMLLog("err.html", formatFailure(why))
-            self.addCompleteLog("err.text", why.getTraceback())
-            results = EXCEPTION
-            hidden = False
-
-        self.step_status.stepFinished(results)
-        self.step_status.setHidden(hidden)
-
-        self.releaseLocks()
-        self.deferred.callback(results)
-
-    def failed(self, why):
-        # This can either be a BuildStepFailed exception/failure, meaning we
-        # should call self.finished, or it can be a real exception, which should
-        # be recorded as such.
-        if why.check(BuildStepFailed):
-            self.finished(FAILURE)
-            return
-
-        log.err(why, "BuildStep.failed; traceback follows")
-        try:
-            if self.progress:
-                self.progress.finish()
-            try:
-                self.addCompleteLog("err.text", why.getTraceback())
-                self.addHTMLLog("err.html", formatFailure(why))
-            except Exception:
-                log.err(Failure(), "error while formatting exceptions")
-
-            # could use why.getDetailedTraceback() for more information
-            self.step_status.setText([self.name, "exception"])
-            self.step_status.setText2([self.name])
-            self.step_status.stepFinished(EXCEPTION)
-
-            hidden = self._maybeEvaluate(self.hideStepIf, EXCEPTION, self)
-            self.step_status.setHidden(hidden)
-        except Exception:
-            log.err(Failure(), "exception during failure processing")
-            # the progress stuff may still be whacked (the StepStatus may
-            # think that it is still running), but the build overall will now
-            # finish
-
-        try:
-            self.releaseLocks()
-        except Exception:
-            log.err(Failure(), "exception while releasing locks")
-
-        log.msg("BuildStep.failed now firing callback")
-        self.deferred.callback(EXCEPTION)
-
     # utility methods that BuildSteps may find useful
 
     def slaveVersion(self, command, oldversion=None):
@@ -477,12 +467,6 @@ def runCommand(self, c):
         d = c.run(self, self.remote, self.build.builder.name)
         return d
 
-    @staticmethod
-    def _maybeEvaluate(value, *args, **kwargs):
-        if callable(value):
-            value = value(*args, **kwargs)
-        return value
-
 components.registerAdapter(
     BuildStep._getStepFactory,
     BuildStep, interfaces.IBuildStepFactory)

master/buildbot/test/unit/test_process_buildstep.py

@@ -22,6 +22,7 @@
 from buildbot.process.buildstep import regex_log_evaluator
 from buildbot.status.results import EXCEPTION
 from buildbot.status.results import FAILURE
+from buildbot.status.results import SKIPPED
 from buildbot.status.results import SUCCESS
 from buildbot.status.results import WARNINGS
 from buildbot.test.fake import fakebuild
@@ -103,6 +104,11 @@ class FakeBuildStep(buildstep.BuildStep):
         def start(self):
             eventually(self.finished, 0)
 
+    class SkippingBuildStep(buildstep.BuildStep):
+
+        def start(self):
+            return SKIPPED
+
     def setUp(self):
         return self.setUpBuildStep()
 
@@ -163,6 +169,43 @@ def test_runCommand(self):
         bs.runCommand(cmd)
         self.assertEqual(bs.cmd, cmd)
 
+    @defer.inlineCallbacks
+    def test_start_returns_SKIPPED(self):
+        self.setupStep(self.SkippingBuildStep())
+        self.step.finished = mock.Mock()
+        self.expectOutcome(result=SKIPPED, status_text=['generic', 'skipped'])
+        yield self.runStep()
+        # 837: we want to specifically avoid calling finished() if skipping
+        self.step.finished.assert_not_called()
+
+    @defer.inlineCallbacks
+    def test_doStepIf_false(self):
+        self.setupStep(self.FakeBuildStep(doStepIf=False))
+        self.step.finished = mock.Mock()
+        self.expectOutcome(result=SKIPPED, status_text=['generic', 'skipped'])
+        yield self.runStep()
+        # 837: we want to specifically avoid calling finished() if skipping
+        self.step.finished.assert_not_called()
+
+    @defer.inlineCallbacks
+    def test_doStepIf_returns_false(self):
+        self.setupStep(self.FakeBuildStep(doStepIf=lambda step: False))
+        self.step.finished = mock.Mock()
+        self.expectOutcome(result=SKIPPED, status_text=['generic', 'skipped'])
+        yield self.runStep()
+        # 837: we want to specifically avoid calling finished() if skipping
+        self.step.finished.assert_not_called()
+
+    @defer.inlineCallbacks
+    def test_doStepIf_returns_deferred_false(self):
+        self.setupStep(self.FakeBuildStep(
+            doStepIf=lambda step: defer.succeed(False)))
+        self.step.finished = mock.Mock()
+        self.expectOutcome(result=SKIPPED, status_text=['generic', 'skipped'])
+        yield self.runStep()
+        # 837: we want to specifically avoid calling finished() if skipping
+        self.step.finished.assert_not_called()
+
     def test_hideStepIf_False(self):
         self._setupWaterfallTest(False, False)
         return self.runStep()