Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug184: Blocker #77

Merged
merged 43 commits into from Mar 27, 2011
Merged

Bug184: Blocker #77

merged 43 commits into from Mar 27, 2011

Conversation

gward
Copy link
Contributor

@gward gward commented Mar 14, 2011

As discussed. This is relative to 0.7.12 I think, since that's what we're running at work. But I only added new files, so there should be no merge conflicts. The conflicts will be at the API level. ;-)

Greg Ward added 30 commits February 5, 2009 11:05
  - add BadStepError
  - modify _getStepStatus() to raise it
  - modify start() to handle BadStepError and accumulate them
    so multiple errors are reported together
  - add 'idlePolicy'
  - implement policies "error" (current behaviour) and "ignore"
  - still need to implement "block" policy
Simplify code that finds the upstream BuildStepStatus by jumping from
Builder to BuilderStatus and BuildStatus.
  - split _getStepStatus() up into _getBuildStatus() and _getStepStatus()
  - add _blocking_builders instance attribute and populate it in
    _getBuildStatus() (when idlePolicy == "block")
  - replace use of DeferredList with _blocking_steps instance attr
  - add BuildStatusReceiver and Blocker._upstreamBuildStarted() to
    listen for upstream builder starting a build
  - replace simple finished() method (called by the DeferredList) with
    _upstreamStepFinished()
So far I only test trivial stuff, ie. anything that can be tested
without the whole Buildbot object graph in place.
…ack.

Specifically, if a step has already finished, its "step finished"
callback is called immediately, often before we had a chance to add the
step to _blocking_steps.  So add it to _blocking_steps before
subscribing to the "finish" event.
(Pass only _overall_code to finished(); _overall_text is passed to
step_status.setText2().)
This implementation only consider upstream build steps from builds whose
build number equals the build number of the build where the Blocker is
running.
…rridden.

(I.e. rename _buildsMatch() to buildsMatch().)
(It just confused the heck out of me getting an error because
the other builder was idle; it'll be even harder for anyone else
to understand!)
…testing.

Eventually this should become automated unit tests, but first I
at least have to figure out to systematically test Blocker
(semi-)manually.
This should enough for any clueful Buildbot hacker on a Unix box
to systematically (not automatically!) test Blocker.
override this method to implement custom logic (e.g. by
comparing source stamps or some build property).
"""
return buildStatus1.getNumber() == buildStatus2.getNumber()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should raise NotImplemented as discussed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 19fcc1f (coming in my next pull request).

@djmitche
Copy link
Member

As discussed, this should be documented as "beta" (or whatever you'd like to call it), meaning that its behavior may change significantly over subsequent releases. And it should be documented :)

# Hmmm: this step has failed. But it is still subscribed to
# various upstream events, so if they happen despite this
# timeout, various callbacks in this object will still be
# called. This could be confusing and is definitely a bit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this set a flag that's checked by _upstreamStepFinished, which will then ignore the callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly necessary: _upstreamStepFinished() just does unnecessary work after timeout, but everything works out OK. But you're right; things are clearer with a _timed_out flag. Added in dc818b4.

- set flunkOnFailure true in my custom NoOp step (looks like the
  upstream default changed, breaking my implicit assumption)
- update test README to match current Blocker output
…ildsMatch().

The existing implementation, matching on build number, is only
appropriate in artificial examples like Blocker's tests.  So move it
there.  In real life, users are going to have to come up with their
own way of matching builds.  Force them to do this by making
buildsMatch() raise NotImplementedError.  Until BuildBot has build
flocks (or something similar), anyone using Blocker will have to
subclass it and provide their own buildsMatch() implementation.
Add a flag so we know when the Blocker timed out, and return early
from _upstreamStepFinished() in that case. If we timeout, we don't
care about the fate of any upstream steps; we just need to clean them
out of Blocker's data structures and get out of the way.

Also add a test case for timeouts.
@djmitche
Copy link
Member

If you push your changes to your bug184 branch, github will smartly update this pull request - no need for a fresh one

@djmitche djmitche merged commit 60aaf1e into buildbot:master Mar 27, 2011
@djmitche
Copy link
Member

Hoorj!

mzdaniel pushed a commit to mzdaniel/buildbot that referenced this pull request Aug 2, 2013
Issue buildbot#77, Now mountpoints are always deleted even when not currently mounted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants