[Buildbot-devel] Patches index

Tue Mar 2 10:54:46 UTC 2004

> It makes sense and it fits in well with some -D changes that are in the
> works, but I'd like to know what prompted you to change it: is there a usage
> model that gets broken when -r HEAD is passed on each command?
> 
> My concern is that somebody will do something foolish like check out a tree
> on their slave machine by hand, with -r BRANCH, and then expect the
> buildslave to always do an update (with no -r tag, to avoid un-selecting the
> branch). This strikes me as fragile.
> 
> > * cvs-clobber.patch
> > Don't do a clobber if clobber=0, just do an update each time instead
> 
> [not yet]

> Let's talk about this one: I think it shouldn't be necessary. The only case
> where it changes the slave's behavior is (clobber=False, copydir=something).
> I'm writing up some docs to understand/explain the possibilities better, but
> basically I think that (clobber=False, copydir=Something) should have the
> same effective behavior as (clobber=False, copydir=None).

For GStreamer, we're doing some pretty nasty stuff, eg checking out a
separate module, cvs checkout and checking out with -r breaks this, but
it works fine with cvs update. I agree that the patch is fragile, some
more tests are needed before we can assume that a cvs update is okay,
and even so, if it fails (in some way), a full checkout should be
considered.

The normal usage of cvs, as I can understand is the following:

  [cvs login (only for non ssh)]
  cvs checkout module(s)

And from then on, when the module(s) are fully checked out:
  cvs update 

> > * change-filter.patch 
[...]
> [probably, but not yet]
> 
> Sounds useful.. I need to think about it a bit more to make sure it's
> sufficiently generalized.
> 

Agree with that, I recently found out that filter on revisions/tags is
quite necessary, to avoid rebuilding on checkins on separate branches.
Filtering on username and other properties could also be useful.

Maybe something like this:

from buildbot import Filter
c['filters'] = [Filter(name='...', module='...', rev='HEAD')]

All of them should allow regexps.

I can work on this, providing we can agree upon a interface.

> > * event-fix.patch 
[...]
> [not yet]
> 
> Hmm. I'd like to see a copy of those exceptions: they indicate a different
> problem. Once the Event is .finish()'ed, nothing more is supposed to be added
> to it, and self.entries=None is an indication that entries have been pushed
> out to disk (using Swappable), so if someone is doing an .append on that None
> then it means they're adding entries when the Event is supposed to be closed.
> 
> That said, the code is ugly and needs to be cleaned up (I think the
> self.entries=None should probably be inside that if self.doSwap clause
> anyway), so I will revisit that class and probably rewrite it a bit.

Okay, I'll see if I can trigger the bugs again.
Something related to the render functions IIRC.

> > * change-links.patch 
> > Add an argument to the Change class, a list of links
> > * change-html.patch 
> > Adds a asHtml method to Change, it's turned on in our local build
> > instead of the asText method (in the html status output), but I did not
> > include a patch for it
> 
> [will apply]
> 
> I'll apply this, with some changes (to let the html-status code provide
> headers and footers, and HTML-escape the body of the <pre> block). I need to
> work on it a bit more before I commit it. Could you provide an example of
> what is meant to be passed in the links= argument? The HTML-izing code looks
> like it might get confused if it encounters two similar filenames in the same
> Change, and it might be better to change the API to pass (file, link) pairs
> instead of using separate list arguments for each.

Sure, but in the mails from syncmail I receive (for example
http://sourceforge.net/mailarchive/forum.php?thread_id=3940200&forum_id=7791)
 there are one diff per file.

This turns up like this in buildbot: 
http://build.fluendo.com:8080/commits/68

> I retained the "usePTY=0 on non-posix" code. I believe usePTY needs to be a
> slave-local decision, not a master-side one (I think I'll turn it into a
> mktap argument anyway, to make life easier for the not-quite-Posix-enough
> Solaris buildslaves that fail when usePTY=1).
> 
> I'd like to know what your use case is for this. I'm working on replacing the
> ugly split-on-spaces command string with a sensible argv list. We can make
> useSH=1 something that the buildmaster can control, and then have the command
> take a string (like it does now) instead of a list (like it will in the
> future), but if you're doing stuff like command += ">out.txt &" or command =
> "make foo | grep ERROR" then we need to make sure the new scheme can
> accomplish whatever you're doing with the old one.

We have a couple of builds running in chroots, they're failing very
strangely when using PTY, haven't really been able to fully debug it.
We still have some other strange issues that only turns up the chroots
(weird segfauls), so it might just be an ill configured chroot.

I'd really like to have something like useSH to be controlled by the
buildmaster.

ATM we're only doing foo || (bar && baz), but ideally the whole shell
syntax should be supported.
Or, rather some nice dependency features, eg conditional builds, might
be doable currently, but I couldn't figure out how.

> Finally, I want to make sure we don't do anything to make the w32 buildslave
> harder to get working, and I suspect /bin/sh is not an option there.

Good point. Some abstraction is clearly needed.

> > * syncmail-rewrite.patch 
> > Rewrote the syncmail parsing totally, since the last one seemed to be
> > quite broken. 
> 
> [not yet]
> 
> I'm willing to believe that, and apply the patch, but the current test cases
> pass with the old code and fail with the patch. Could you provide a test case
> which shows the bug this fixes? It may be that we've got messages from
> different versions of syncmail, in which case we need to fix the parser to
> handle either kind.

I couldn't really get anything working with the old syncmail code. I
didn't seem like it was tested at all, or just for a different version
of syncmail.

> > * slave-basedir.patch 
> > Checks for $BASEDIR in the command and environment. We're using it like
> > this:
> > # GStreamer plugins
> > configure = './autogen.sh --prefix=$BASEDIR/prefix'
> > configureEnv = {'PKG_CONFIG_PATH': '$BASEDIR/prefix/lib/pkgconfig'}

I hope this is useful for other projects.

> > * install-steps.patch 
> > Adds two new steps, for Install and Uninstall

Can also be archived with ShellCommand and name, so maybe not necessary
at all.

I'm still learning how to use buildbot :-)

Regards, Johan

-- 
|#| Johan Dahlin < johan (at) gnome (dot) org> | Gombau 10       |#|
|#| Web:   http://www.async.com.br/~jdahlin/   | 08003 Barcelona |#|
|#| Phone: +46 (0)735-570778                   | Spain           |#|