[Buildbot-devel] Severe issues with buildbot 0.7.4

[Brian Warner]

Kenneth Lareau <Ken.Lareau at nominum.com> writes:

>> These line numbers don't match up.. is your version of commands.py patched?
>
> No, it's not patched; I just checked a system on which there's no
> problem and the file is precisely the same.

Thanks for the copy of your commands.py . The difference between your
commands.py and the version[1] that was shipped with 0.7.4 is as follows:

--- buildbot/slave/commands.py	2006-08-23 00:11:16.000000000 -0700
+++ /tmp/commands-patched.txt	2007-04-19 15:30:45.000000000 -0700
@@ -230,6 +230,8 @@
         self.sendRC = sendRC
         self.logfiles = logfiles
         self.workdir = workdir
+        if not os.path.exists(workdir):
+            os.makedirs(workdir)
         self.environ = os.environ.copy()
         if environ:
             if environ.has_key('PYTHONPATH'):
@@ -312,12 +314,12 @@
             else:
                 # for posix, use /bin/sh. for other non-posix, well, doesn't
                 # hurt to try
-                argv = ['/bin/sh', '-c', self.command]
+                argv = ['/bin/sh', '-c', self.command % os.environ]
         else:
             if runtime.platformType  == 'win32':
                 argv = [os.environ['COMSPEC'], '/c'] + list(self.command)
             else:
-                argv = self.command
+                argv = (" ".join(self.command) % os.environ).split()
 
         # self.stdin is handled in ShellCommandPP.connectionMade

Those are some interesting changes.. I wonder how your copy wound up with
them :). The first is useful and harmless, the second adds a new feature by
which your ShellCommand string will automatically interpolate environment
variables when you use %(varname)s (at the expense of failing when other '%x'
format-string specifications are used), and the third adds that same feature
but also introduces a bug. Any arguments with spaces in them will get split
up, so a ShellCommand with command=["touch", "filename has spaces.txt"] will
turn into ["touch", "filename", "has", "spaces.txt"], bypassing the sort of
direct-control-over-argv that using a list (instead of a single string) gets
you.

A safer way to add this feature while avoiding the space-splitting problem
would be:

  argv = [c % os.environ for c in self.command]


>>>            File "/usr/local/lib/python2.4/UserDict.py", line 17, in __getitem__
>>>              def __getitem__(self, key): return self.data[key]
>>>          exceptions.KeyError: 'workdir'
>>

>> Hrm, where did that UserDict come from? os.environ is a regular built-in
>> dict type. And why would 'workdir' be present in os.environ? In
>> ShellCommand that usually comes from self.workdir .
>
> UserDict.py is a standard file for Python 2.4.x, AFAIK, or are you
> questioning why it's being called?  An 'env' on the system shows
> no setting of 'workdir' that I see, so I'm not sure why os.environ
> is being used here.  I will include a copy of the commands.py file
> here.

Yeah, my curiosity stems from the fact that UserDicts normally only appear
when someone is subclassing it, to add new features. The Buildbot doesn't do
this anywhere, and most python stdlib code doesn't either.

Somebody must have patched your buildbot to allow a ShellCommand like:

    %(workdir)s/tare/head/nightly/copy_package %(workdir)s dhcptest-package %(platform)s

to work, since there's no way that would do what you expect it to on an
unmodified buildbot-0.7.4 (or any other version). I think that either your
modified buildbot is supposed to copy the workdir into os.environ (and might
replace os.environ with a UserDict in the process), or it expects the
buildslave to be run in an environment that sets the 'workdir' environment
variable first.

Could you do two things to check this out? The first is to look at the
buildslave that *is* working and see if it has any extra environment
variables set. Under linux you can look in /proc/PID/env, there might be a
way to do this under BSD but I'm not sure. Another approach is to look at the
way the buildslave is started: look for 'workdir=SOMETHING buildbot start .'
-type lines in init.d files or crontab @reboot entries. Also look in .profile
and .bashrc -type files. If 'workdir' is being set on the working
buildslaves, then try setting it to an appropriate value on the broken ones.

The second thing is to download a fresh copy[2] of buildbot-0.7.4, unpack it,
and compare it to the version that you have installed, to find out what other
modifications have been made to your copy. Something like:

 tar xf buildbot-0.7.4.tar.gz   # creates buildbot-0.7.4/
 diff -ur buildbot-0.7.4/buildbot /usr/lib/python2.4/site-packages/buildbot

Perhaps if we can track down who modified your local copy, we can discover
where they thought that 'workdir' environment variable was supposed to come
from.

I still don't understand why logging would die, but without knowing what
other changes have been made to your code, it will be tricky to narrow down
the possibilities.

Also, since ShellCommands are normally run from within the normal workdir for
that buildslave+builder combination, you don't normally need to insert it
into your commands unless you need an absolute pathname for some reason. So a
command like the following might happen to work:

    ./tare/head/nightly/copy_package . dhcptest-package %(platform)s


hope that helps,
 -Brian

[1]: http://buildbot.net/trac/browser/buildbot/slave/commands.py?rev=181
[2]: https://sourceforge.net/project/showfiles.php?group_id=73177