Add 2 new worker types to Reviewbot. CPPlint and CPPCheck.

Review Request #3579 — Created Nov. 29, 2012 and submitted

Information

ReviewBot

Reviewers

This updates adds support for 2 new workers.
Cpplint and cppcheck.
This allows code style to be checked according to the cpplint rules (various configurable options available - not yet in the options).
It then checks the code using the static analysis tool cppcheck.
This will raise an Issue if it discovers and 'Error' otherwise just a comment.

Whilst developing this I added a few patches to the core functionality.
So the temp file now has a file extension (where applicable) - otherwise cpplint and cppcheck refuse to operate on the file (they want .c/.cpp/.cc etc)

I also found that multiple submits was getting multiple reviews - so I now stop this.

Other than that just a check in __init__.py to check these programs are installed.

Have noticed %ORGANIZATION% still in the BSD headers - this needs to be changed to Ericsson Television Ltd
We have it running and seems to work ok.
Could so with some of the options being broken out and made easier to configure
Description From Last Updated

This should be rewritten to say something like: "... and comes with the following plugins by default: ..."

SM smacleod

Once cpplint is added to the install_requires, we shouldn't have to worry about these instructions. You can probably go ahead …

SM smacleod

Remove trailing whitespace

SM smacleod

This should be removed.

SM smacleod

After removing the call to logger, this should be removed.

SM smacleod

This should be removed as well.

SM smacleod

I feel like this is going to be dumping way too much information to the log. Could you remove this …

SM smacleod

Lets get rid of this change. I've updated Review Bot to only send a task when a new diff is …

SM smacleod

Since we're no longer going to kill the review when processed_files is empty, lets remove all these changes. The base …

SM smacleod

Trailing whitespace should be removed

SM smacleod

This can be removed.

SM smacleod

blank line between these.

SM smacleod

The order should be flipped here (alphabetical)

SM smacleod

2 blank lines between these.

SM smacleod

This should be 'CPPCheckTool'

SM smacleod

How about something like: "Checks code for errors using Cppcheck - A tool for static C/C++ code analysis"

SM smacleod

Most of the calls to logger in here should probably call 'logger.debug' instead of 'logger.info'. Could you update them please.

SM smacleod

It might be a good idea to handle the cases where the file extensions are upper or mixed case etc.

SM smacleod

Remove trailing whitespace.

SM smacleod

Remove trailing whitespace.

SM smacleod

Let's remove this.

SM smacleod

This may not do what is expected. Review Bot will open an issue depending on global settings configured in the …

SM smacleod

Remove trailing whitespace.

SM smacleod

this can be removed.

SM smacleod

blank line between these

SM smacleod

flip the order of these (alphabetical)

SM smacleod

This should be 'CPPLintTool'

SM smacleod

Why the '(Modified by E///)' part?

SM smacleod

let's add 'max_value', and 'min_value' of 5 and 1 respectively.

SM smacleod

Most of these should be changed to logger.debug as well.

SM smacleod

might want to handle upper or mixed case as well.

SM smacleod

I've added the ability for tools to specify a 'check_dependencies' method which can carry out checks like this. If the …

SM smacleod

I've added an AUTHORS file to the repo where you may add your name, this should remain unchanged.

SM smacleod

Can you put these in alphabetical order.

SM smacleod

You should be able to add cpplint to this list, and it will be automatically installed with Review Bot.

SM smacleod

Lets axe the dependency on distutils2. I've added a 'is_exe_in_path' function which you can use to check for the tools …

SM smacleod

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (97 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (137 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (107 > 79 characters)

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (86 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 42 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 43 E262 inline comment should start with '# '

reviewbotreviewbot

Col: 80 E501 line too long (137 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (131 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (132 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (92 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (127 > 79 characters)

reviewbotreviewbot

Col: 14 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 17 E121 continuation line indentation is not a multiple of four

reviewbotreviewbot

Col: 17 E121 continuation line indentation is not a multiple of four

reviewbotreviewbot

Col: 17 E121 continuation line indentation is not a multiple of four

reviewbotreviewbot

Col: 80 E501 line too long (100 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (101 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (100 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (122 > 79 characters)

reviewbotreviewbot

Col: 43 E262 inline comment should start with '# '

reviewbotreviewbot

Col: 80 E501 line too long (88 > 79 characters)

reviewbotreviewbot

Col: 50 E262 inline comment should start with '# '

reviewbotreviewbot

Col: 80 E501 line too long (88 > 79 characters)

reviewbotreviewbot

Col: 51 E262 inline comment should start with '# '

reviewbotreviewbot

Col: 80 E501 line too long (88 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot
SM
  1. Thanks for submitting this patch.
    
    Before I do a throrough review I'd like to clear up an issue with licensing.
    ReviewBot is MIT licensed, and I will only be able to include contributions
    if they are under the same license. The license you appear to have chosen is
    the BSD license, which is very similar to the MIT license.
    
    What does concern me though is the addition of a clause to the BSD license:
    
        #  * All advertising materials mentioning features or use of this software must
        # display the following acknowledgement: This product includes software developed
        # by %ORGANIZATION% and its contributors.
    
    I will not be able to accept code with this licensing clause.
    
    If you are willing to relicense the code, I would still love to include it
    in the main ReviewBot package. Otherwise, you would have to release these
    tools yourself.
    1. Done - just had to ask for permission (default was BSD).
      Let me know what else I need to fix - bit of a Git Newbie but hopefully i will manage.
  2. 
      
djl197
SM
  1. Hey Dan, these look good, I'm excited to get them in. Sorry
    this review has taken me so long.
    
    There are mainly just some style issues, and some changes that
    should be made due to recent improvements to Review Bot (which
    will hopefully make things nicer for these tools anyway).
  2. README.md (Diff revision 2)
     
     
     
     
     
     
    Show all issues
    This should be rewritten to say something like:
    
    "... and comes with the following plugins by default: ..."
  3. README.md (Diff revision 2)
     
     
     
     
     
    Show all issues
    Once cpplint is added to the install_requires, we shouldn't have to worry about these instructions. You can probably go ahead and remove this.
    1. I cant find CPPLint (the google one) in either pip or easy_install which is why I have had to add this comment.
    2. You should be able to install it. See http://pypi.python.org/pypi/cpplint/
  4. README.md (Diff revision 2)
     
     
    Show all issues
    Remove trailing whitespace
  5. bot/reviewbot/processing/filesystem.py (Diff revision 2)
     
     
    Good idea, I didn't consider some tools would refuse to work on files without an extension.
  6. bot/reviewbot/processing/review.py (Diff revision 2)
     
     
    Show all issues
    This should be removed.
  7. bot/reviewbot/processing/review.py (Diff revision 2)
     
     
    Show all issues
    After removing the call to logger, this should be removed.
  8. bot/reviewbot/processing/review.py (Diff revision 2)
     
     
    Show all issues
    This should be removed as well.
  9. bot/reviewbot/processing/review.py (Diff revision 2)
     
     
    Show all issues
    I feel like this is going to be dumping way too much information to the log. Could you remove this please.
  10. bot/reviewbot/tasks.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues
    Lets get rid of this change. I've updated Review Bot to only send a task when a new diff is posted.
    
    Currently we need to show a review happened even if no files were processed, otherwise there is no way to tell if the review was actually triggered.
  11. bot/reviewbot/tools/__init__.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Since we're no longer going to kill the review when processed_files is empty, lets remove all these changes.
    
    The base class has changed slightly with the dependency checking as well.
  12. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Trailing whitespace should be removed
  13. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
    Show all issues
    This can be removed.
  14. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
     
    Show all issues
    blank line between these.
  15. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
     
    Show all issues
    The order should be flipped here (alphabetical)
  16. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
     
     
    Show all issues
    2 blank lines between these.
  17. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
    Show all issues
    This should be 'CPPCheckTool'
  18. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
    Show all issues
    How about something like: "Checks code for errors using Cppcheck - A tool for static C/C++ code analysis"
  19. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
    Show all issues
    Most of the calls to logger in here should probably call 'logger.debug' instead of 'logger.info'.
    
    Could you update them please.
  20. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
    Show all issues
    It might be a good idea to handle the cases where the file extensions are upper or mixed case etc.
  21. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
    Show all issues
    Remove trailing whitespace.
  22. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
     
     
    I like that you included these example lines, it's a nice addition.
  23. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
    Show all issues
    Remove trailing whitespace.
  24. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
    Show all issues
    Let's remove this.
  25. bot/reviewbot/tools/cppcheck.py (Diff revision 2)
     
     
     
     
    Show all issues
    This may not do what is expected. Review Bot will open an issue depending on global settings configured in the admin panel. When you're passing 'issue=True' you are overriding this behaviour, and ensuring an issue is made.
    
    In the else case, you don't pass any value for issue, so you are letting Review Bot decide if an issue should be raised based on the admin configuration.
    
    If you really want to override Review Bots issue decisions, you should probably also pass 'issue=False'.
    1. I have added issue=False, the reason is I would like the raise issue and comment on unmodified code to be per checker rather than global.  
      I think some plugins you would want to always run on the entire file and others on just the changes made.
      Then some plugins you want to raise issues and others (like style check) you probably only want comments.
      For now this works - but I see an improvement opportunity here!
    2. Definitely a change that should be made. The global settings were something
      I designed before tools even had their own settings, and this is a result
      of that fact.
      
      I'll fix this up and get a change out.
  26. bot/reviewbot/tools/cpplint.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Remove trailing whitespace.
  27. bot/reviewbot/tools/cpplint.py (Diff revision 2)
     
     
    Show all issues
    this can be removed.
  28. bot/reviewbot/tools/cpplint.py (Diff revision 2)
     
     
     
    Show all issues
    blank line between these
  29. bot/reviewbot/tools/cpplint.py (Diff revision 2)
     
     
     
    Show all issues
    flip the order of these (alphabetical)
  30. bot/reviewbot/tools/cpplint.py (Diff revision 2)
     
     
    Show all issues
    This should be 'CPPLintTool'
  31. bot/reviewbot/tools/cpplint.py (Diff revision 2)
     
     
    Show all issues
    Why the '(Modified by E///)' part?
  32. bot/reviewbot/tools/cpplint.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues
    let's add 'max_value', and 'min_value' of 5 and 1 respectively.
  33. bot/reviewbot/tools/cpplint.py (Diff revision 2)
     
     
    Show all issues
    Most of these should be changed to logger.debug as well.
  34. bot/reviewbot/tools/cpplint.py (Diff revision 2)
     
     
    Show all issues
    might want to handle upper or mixed case as well.
  35. bot/setup.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    I've added the ability for tools to specify a 'check_dependencies' method which can carry out checks like this. If the method returns false, that tool won't be activated for the worker.
    
    Lets move these checks into methods on their respective tools.
  36. bot/setup.py (Diff revision 2)
     
     
    Show all issues
    I've added an AUTHORS file to the repo where you may add your name, this should remain unchanged.
  37. bot/setup.py (Diff revision 2)
     
     
     
     
    Show all issues
    Can you put these in alphabetical order.
  38. bot/setup.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues
    You should be able to add cpplint to this list, and it will be automatically installed with Review Bot.
  39. bot/setup.py (Diff revision 2)
     
     
    Show all issues
    Lets axe the dependency on distutils2. I've added a 'is_exe_in_path' function which you can use to check for the tools you call out to.
  40. 
      
djl197
djl197
SM
  1. Alright, so I took a look at your github fork of Review Bot, and it looks like
    the reason you're getting my changes in your review requests is because you've
    been committing to the master branch.
    
    I'm going to assume going forward that your local repository was cloned from
    the main Review Bot repository, so the origin remote refers to
    git://github.com/smacleod/ReviewBot.git. Also, <fork> should refer to the
    name you gave the remote for your github fork of Review Bot.
    
    Your development should take place on a separate branch, which I'll refer to
    as <feature> from this point forward. As you make changes, you should be
    committing to the <feature> branch. When you are ready to update your review
    request, you should merge the changes from master into your <feature> branch,
    and call post-review with <feature> checked out. ex:
    
        $ git checkout master
        $ git fetch origin
        $ git merge origin/master
        $ git checkout <feature>
        $ git merge master
        $ post-review -r 3579
    
    Before this will work though, we need to fix the master branch on your repo.
    So, what we need to do is move your changes into a <feature> branch, and reset
    your master to origin/master. We'll also force push these changes to your
    fork.
    
    First Create the <feature> branch:
        $ git checkout master
        $ git branch <feature>
    
    Then reset your master branch:
        $ git reset --hard origin/master
    
    Push the changes to your fork:
        $ git push --force <fork> master
    
    Now, you'll want to properly base your changes off master:
        $ git checkout <feature>
        $ git rebase master
    
    You might get conflicts when rebasing here, which you'll have to resolve.
    After this is finished, you should be in a good state to update the
    review request.
    
    Let me know if you have any problems.
  2. 
      
djl197
djl197
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        bot/reviewbot/tasks.py
        bot/reviewbot/tools/cpplint.py
        bot/setup.py
        bot/reviewbot/processing/filesystem.py
        bot/reviewbot/tools/cppcheck.py
        bot/reviewbot/processing/review.py
      Ignored Files:
        README.md
        AUTHORS
    
    
    WARNING: Number of comments exceeded maximum, showing 30 of 35.
  2. bot/reviewbot/processing/review.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
    1. Do you want all these PEP8 issues fixed? If so is line length 79 a bit small? 
      Is this holding up the changes being merged?
    2. Yeah, the PEP8 issues will need to be fixed before this goes in. 79
      characters is the PEP8 maximum line length.
      
      Are you able to make the modifications, or would you like me to take
      it from here and fix things up before committing?
  3. bot/reviewbot/tools/cppcheck.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (97 > 79 characters)
    
  4. bot/reviewbot/tools/cppcheck.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (137 > 79 characters)
    
  5. bot/reviewbot/tools/cppcheck.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (107 > 79 characters)
    
  6. bot/reviewbot/tools/cppcheck.py (Diff revision 6)
     
     
    Show all issues
    Col: 9
     E303 too many blank lines (2)
    
  7. bot/reviewbot/tools/cppcheck.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  8. bot/reviewbot/tools/cppcheck.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  9. bot/reviewbot/tools/cppcheck.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  10. bot/reviewbot/tools/cppcheck.py (Diff revision 6)
     
     
    Show all issues
    Col: 42
     E261 at least two spaces before inline comment
    
  11. bot/reviewbot/tools/cppcheck.py (Diff revision 6)
     
     
    Show all issues
    Col: 43
     E262 inline comment should start with '# '
    
  12. bot/reviewbot/tools/cppcheck.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (137 > 79 characters)
    
  13. bot/reviewbot/tools/cppcheck.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (131 > 79 characters)
    
  14. bot/reviewbot/tools/cppcheck.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (132 > 79 characters)
    
  15. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  16. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (92 > 79 characters)
    
  17. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (127 > 79 characters)
    
  18. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 14
     E111 indentation is not a multiple of four
    
  19. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 17
     E121 continuation line indentation is not a multiple of four
    
  20. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 17
     E121 continuation line indentation is not a multiple of four
    
  21. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 17
     E121 continuation line indentation is not a multiple of four
    
  22. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (100 > 79 characters)
    
  23. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (101 > 79 characters)
    
  24. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (100 > 79 characters)
    
  25. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (122 > 79 characters)
    
  26. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 43
     E262 inline comment should start with '# '
    
  27. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (88 > 79 characters)
    
  28. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 50
     E262 inline comment should start with '# '
    
  29. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (88 > 79 characters)
    
  30. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 51
     E262 inline comment should start with '# '
    
  31. bot/reviewbot/tools/cpplint.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (88 > 79 characters)
    
  32. 
      
djl197
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        bot/reviewbot/tasks.py
        bot/reviewbot/tools/cpplint.py
        bot/setup.py
        bot/reviewbot/processing/filesystem.py
        bot/reviewbot/tools/cppcheck.py
        bot/reviewbot/processing/review.py
      Ignored Files:
        README.md
        AUTHORS
    
    
  2. bot/reviewbot/processing/review.py (Diff revision 7)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  3. 
      
SM
  1. Thanks Dan!
  2. 
      
djl197
Review request changed
Status:
Completed
Change Summary:
Pushed to master (52d09ddbb3b6000bd4b55e8936a8dcc67908cf1d). Thanks!