[Buildbot-devel] got_revision Build Property (w/ PATCH!)

[Oz]

>> This function parses the output from "svnversion ." on a working copy.
>> However, it assume that the stdout can always be converted to an int.
>> svnversion can legally return all sorts of stuff. In addition to the plain
>> revision number (for unmodified working copies), it can also return stuff
>> like (this is from the --help):
>>
>> 4123:4168     mixed revision working copy
>> 4168M         modified working copy
>> 4123S         switched working copy
>> 4123:4168MS   mixed revision, modified, switched working copy
> 
> Ooh, that's a good one.
> 
> I'm not really sure what the best fix would be. The buildbot is generally
> focussed on a VC model in which there is a checkout of a single specific
> revision, and then maybe a patch is applied. So we can probably ignore the
> 'switched' part and the 4123:4168 mixed-revision part (by which I mean it is
> ok if trees of this sort cause got_revision to be None, as long as a warning
> message gets emitted somewhere).
> 
> The M suffix can happen either when the build process modifies some files (a
> practice I would discourage, incidentally, but obviously there are times when
> that is not worth the effort it would take to fix), and when you do a 'try'
> build that applies a patch immediately after checkout. Since we already have
> information on the buildmaster side as to whether or not a patch was applied,
> we probably don't need to be aware of the M suffix, so it should be safe and
> useful to just strip it.

Absolutely. Also, doing incremental builds will often touch intermediate 
files that may be checked into SVN. We frown on doing that here, but 
there are some cases where it must be done.

> Would it seem reasonable to apply a variant of your patch that only strips
> the M and not the S, so that 'S' would cause a warning? And/or, maybe we
> should provide a more obvious warning message if we see these sorts of trees.

Yeah, the switched case doesn't bug me as much. So a warning sounds fine.

> FYI, the got_version=None default is there make sure that the build still
> completes even if svn is in a weird state (maybe an older version of svn),
> since I decided that the 'got_revision' build property is less important than
> the checkout step itself. Was there a "unable to parse output of svnversion"
> warning message in the logged output of the SVN step?

Yes, there was a unable to parse output of svnversion error message, but 
I did not even notice it until I had gone through the buildbot code and 
knew what to look for. Somehow the error needs to be migrated to where 
the build property is actually used (i.e. any call to WithProperties) so 
the user can find the cause. The "unable to parse" warning was way back 
in the beginning of my build, while my call to WithProperties was at the 
very end, so I had no idea where to look. Perhaps we could cache a throw 
a better exception on WithProperties if the property is None? Cause just 
moving None through the pipe ends up (for my steps at least) with an 
exception trying to format '%d' % None. So, that told me got_revision 
wasn't working, but I had no idea why.

Thanks.
=oz