[Buildbot-devel] Running Pylint on Buildbot's source code

[Vladimir Rutsky]

On 26.06.2013 17:34, Dustin J. Mitchell wrote:
> On Wed, Jun 26, 2013 at 6:52 AM, Vladimir Rutsky
> <rutsky.vladimir at gmail.com> wrote:
>> Buildbot have target to run Pylint on it's own source code in Makefile and
>> have Pylint configuration file, so looks like Pylint used on Buildbot
>> sources before.
>>
>> Right now running `make pylint` in Buildbot sources show a lot of style
>> errors and as I see Pylint is not run periodically on
>> http://buildbot.buildbot.net/
>>
>> Why Pylint usage was abandoned? It is very useful tool to keep source code
>> visually clean.
> We use pyflakes instead, which is a little less stringent, and focuses
> on errors that may hide real bugs.  I know pylint is quite
> configurable, so perhaps we could limit its scope rather severely?
> For example, I don't care how many spaces appear before and after a {,
> or between code and an end-of-line comment.  But I would like
> something to check line length.

I think it would be possible to configure Pylint to skip uninteresting 
style errors.

I'm using Eclipse with Pydev plugin with enabled Pylint highlighting and 
see quite a lot of tiny style mistakes, such as inconsistent indentation 
(tabs or 3 spaces), missed spaces after commas, unused variables, etc.  
Rather than fixing such things lazily when I see them, it would be 
better to configure Pylint, fix them all at once and setup automatic 
checks to prevent such mistakes in the future.

>
> Most folks don't run pyflakes before submitting a pull request, which
> leaves reviewers running pyflakes and deciding whether to fix the
> results (usually something trivial like an unused import) or request
> an update from the request submitter.  It would be wonderful to run
> pylint or pyflakes (or both!) on every pull request, and tag the
> result.  This would make it easier for those of us doing reviews to
> see whether the code passes or not.  That gets back to the
> oft-discussed but as-yet-not-implemented GitHub pull request status
> plugin / change source.

I agree, running pylint/pyflakes/tests for pull request before merging 
it is very useful feature. But automated run of such things on pull 
request may be insecure: run of tests that are defined in source code is 
definitely insecure. Security question of running pylint/pyflakes on 
untrusted code requires investigation, quick search shows, that there 
may be issues [1].

As I see, Github's pull requests can be fetched from repository from 
refs/pull/<pull request number>/head and refs/pull/<pull request 
number>/merge references [2].

How about configuring additional git source/scheduler on 
buildbot.buildbot.net that will accept pull request identifier, and 
allowing to run tests/pylint/pyflakes builders to authorized users?

This can be easily configured IMO (not sure is current Git source step 
may handle adding references and checking them out, but don't see any 
problems in implementing that).
On new pull request reviewer will look at patch for obvious security 
issues, if patch is ok he will force tests/pylint/pyflakes build on 
buildbot.buildbot.net to see if it passes all tests.

>
> One more point: we've still got two long-lived branches in play -
> master and nine.  Applying a wide-ranging "cleanup" to just one of
> those branches will make merging a nightmare.  So, when this does come
> to pass, the cleanup will need to be applied to master, merged to nine
> immediately, and then re-run on nine (to catch all of the new code in
> nine).
>
> Dustin

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=591676
[2] https://gist.github.com/harlow/3960799

--
Vladimir Rutsky