[Buildbot-devel] Request for assistance: IRC bot

[Brian Warner]

> Any suggestions, or comments on the patch as a whole?

I've got a couple. The first is that the subscribe/unsubscribe should
probably be done in startService/stopService rather than
setServiceParent/disown. The start/stop methods will be called after/before
the set/disown pair, respectively (i.e. "inside" them), and the service
parent will be available in 'self.parent'. In general, you want to make sure
your own service is running before you give anyone else the ability to start
sending you events.

The second, which is probably the source of the problem you're seeing, looks
like a slight misunderstanding about the way IStatus.subscribe works. Status
event delivery in the buildbot code is hierarchical, designed to let
StatusReceivers get as little or as much as they want. It is also designed to
be very dynamic, to cope with configuration changes at runtime.

The highest level of this hierarchy is the builderAdded/builderRemoved pair.
A very coarse (and rather useless) StatusReceiver which only wanted to
announce changes to the set of Builders in the config file would only bother
implementing these two methods. IStatus.subscribe *only* sets you up to
receive these two events. To make sure it doesn't matter *when* you
subscribe, if there were any Builders configured before you called
subscribe(), you will immediately be hit with a builderAdded() event for each
one of them.

The next level of the hierarchy is the Build, with events like buildStarted
and buildFinished. To receive these, you must subscribe to each Builder
separately, by asking the IBuilderStatus. This allows a StatusReceiver to pay
attention to Builds of some Builders while ignoring those of others.
IBuilderStatus.subscribe sets you up to receive builderChangedState,
buildStarted, and buildFinished events for just that one Builder.

To avoid the race condition between the time your StatusReceiver finds out
about builderAdded and the time it manages to call IBuilderStatus.subscribe
(but mostly for convenience), you can return a suitable StatusReceiver
instance from builderAdded(), and it will automatically be subscribed to the
builder. In most cases, you'll just return 'self'. In general, this approach
allows the StatusReceiver to receive events for "dynamic" state (like
Builders coming and going) while being sure not to miss any "static" state
(like the Builders which are already present). You can also call subscribe()
separately, perhaps if you have a GUI status target that doesn't know whether
it wants to pay attention to the builder until it presents a dialog box to
the user and gets a response.

So, you'll need to add the following method to get your buildStarted() method
invoked:

    def builderAdded(self, builderName, builder):
        return self

(and, since I'm an idiot and haven't thought this whole subscription thing
through properly, you might need to add this:

    def builderRemoved(self, builderName):
        status.getBuilder(builderName).unsubscribe(self)

but I think if you don't bother then the subscription will just evaporate
when the IBuilderStatus goes away).


Other notes:

 build.getETA() returns the number of seconds from now that the build is
 expected to finish in.. you might want to reformat this as HH:MM:SS or maybe
 even absolute time

 build.getSourceStamp() might probably change in the future (to return an
 actual SourceStamp instance, or something). Use it for now, it is the only
 interface you've got, but keep an eye on the release notes for future
 versions in case it changes.

 0.7.0 features the new IStatus.getURLForThing() method, which will give
 you a URL for any I*Status instance you want. (it returns None if there is
 no buildbotURL, or if there is no HTML page for the thing you gave it). This
 was added specifically to provide the sort of per-Build URL that your "See
 %s%s/builds/%s" code wants to get. (post-0.7.0, this will probably turn
 into a separate getURL() method on each I*Status object, rather than just
 the one on the top-level IStatus object).


BTW, this is the sort of thing I'd love to have in the trunk, once it
acquires some knobs (like a way to enable/disable it from the config file,
and an IRC-side command like "buildbot: shut up").

cheers,
 -Brian, still plugging away at the 0.7.0 release


PS: the whole subscribe/return-self approach may be overkill here.. it seemed
inportant at the time, but now I'm not so sure. The race condition I was
trying to solve is mostly a problem for a remote status target (like the
client attached to a buildbot.status.client.PBListener), and in that case the
client just gives us an overall "what level of detail do you want" keyword
when it first connects. The resulting StatusReceiver then handles all the
'return self' stuff locally.