[Buildbot-devel] Severe issues with buildbot 0.7.4

Fri Apr 20 02:53:16 UTC 2007

Brian Warner wrote:
> 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.

I found out how my copy ended up with them: the person who initially did
the setup made the changes.  You were right, on the other system the
variables were being set in the environment; the issue on the FreeBSD
6.2 system was that someone had changed the user's shell and the proper
startup scripts weren't getting run.  While the change had been made
some time ago, the restart of the buildbot process running there didn't
occur until yesterday, which caused the environmental variables to be
"reset".  The user's shell has been corrected, though this still leaves
the logging issue to be dealt with in case this happens again.

> A safer way to add this feature while avoiding the space-splitting problem
> would be:
> 
>   argv = [c % os.environ for c in self.command]

I will look into implementing this, though thankfully at the moment we
have no such filenames.

> 
>>>>            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.

As mentioned above, it was being set in the environment and because of a
shell change and a buildbot restart on the buildslave, the variables
were lost.

> 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 grabbed all the diffs (we have the whole thing store in Subversion,
it was merely a running of 'svn diff -r<import rev>:HEAD' at the top
level of the buildbot-0.7.4 tree to get the information).  It's small,
thankfully but I've included it here and will explain some of the other
changes briefly as well (that I know why they were made at least).

> 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.

To explain the changes in the diff file I've sent:

   contrib/svn_buildbot.py: I'm uncertain exactly what this change does,
      but I believe it was for the way we handle the information output
      during a Subversion update

   buildbot/status/mail.py: The changes here were for customization of
      the output sent in a failed email notification; I'm planning on
      eventually replacing this with a custom module/method as it cur-
      rently doesn't do what I'd like it to do.

   buildbot/status/html.py: The changes here were for customization of
      the output given in the html pages (though we have a custom
      html.py file that overrides much of what's in this file); I be-
      lieve the main reason for the changes was to get an accurate
      time for a given builder (excluding the time it's in lock-wait)
      as we run only one builder at a time on a given buildslave.

   buildbot/scripts/runner.py: The change here was to make 'buildbot
      stop .' wait 15 seconds instead of 5; it's actually still not
      enough, though I'm not sure how useful increasing it would be.

   buildbot/slave/commands.py: This file has already been discussed,
      and the reason for it was to have additional variables set to
      use within various build steps.

These are the only files we have that have been modified from the
buildbot source, though we have several additional files (step.py,
helpers.py and html.py) that are imported via our master.cfg file
to modify various things; I don't know if these need to be analyzed
as well, but they're far larger and more complex than the changes
above (and to be honest I haven't completely figured out what they
do at this point, still digging through this code left behind).

> 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
> 
> 

If you need any further information, please let me know.

Ken Lareau
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: buildbot-diffs.txt
URL: <http://buildbot.net/pipermail/devel/attachments/20070419/f414492a/attachment.txt>