Code review goodness with ReviewBoard

The City of Ottawa’s Web group required a code inspection tool to comply with the Payment Card Industry Data Security Standard (PCI DSS) section 6.6 “Code Review”. I investigated a couple of solutions, both open source and commercial, that fit our need for a fast, Web 2.0, side-by-side diff, inline comments, LDAP supported, and feature rich tool. It seems that Atlassian’s Crucible (purchase), Smartbear’s CodeCollaborator (purchase), and ReviewBoard (open source MIT license) were the pack leaders. I had used Crucible at Nortel and I really liked that product, but it was costly. Since I ❤ open source alternatives rather to paying for license fees, I opted to try ReviewBoard first. There was one problem though, the City is a Microsoft shop and we use Team Foundation Server (TFS) <grumble> as our source control repository but ReviewBoard didn’t list TFS in its supported repositories. Hrm…

Server Setup

The ReviewBoard v1.0.9 environment was installed on Windows XP, Apache 2.2, PHP 5.3.2, MySQL 5.1, Python 2.6 (2.7 was the most recent however all of the required plugins for ReviewBoard were not Python 2.7 compatible yet so I moved down to 2.6). After the installation my environment variable for PATH is:

C:GnuWin32bin;C:PHP;%SystemRoot%system32;%SystemRoot%;%SystemRoot%System32Wbem;c:progra~1oracleora10bin;C:MySQLMySQL Server 5.1bin;C:Python26;C:Python26Scripts;C:Program FilesMicrosoft Visual Studio 9.0Common7IDE

Installation

I followed all of the steps in the 1.0 stable and also the 1.5 development documentation. Here are some notes on that install:

1. Installing Python Support: you can use Python 2.6 safely. Also install the setuptools for 2.6.
2. Installing patch: I had trouble getting this into the correct path for ReviewBoard to see it. I had to put it in C:GnuWin32bin and C:Windowssystem32 and set my PATH environment variable as seen above.
3. Installing memcached: follow the steps listed
4. Installing Python Imaging Library (PIL): follow the steps
5. Installing PyCrypto: follow the steps
6. Installing Review Board: follow the steps
7. Installing Database Bindings: try using the command but I had trouble using easy_install to get mysql-python. I had to go out and download the binary from a website and install it manually. I found MySQL-python-1.2.3c1.win32-py2.6.exe from a website (sorry cant remember which but let me know if you find it).
8. Installing Source Control Components: it sounds a little odd, but we install the Subversion component to connect to TFS. Later we will install an adapter that will interpret SVN commands and forward them to the TFS server. Install the PySVN component as instructed.
9. Syntax Highlighting: ReviewBoard can take advantage of syntax highlighting through a program call Pygments. To install Pygments run the following command from the command prompt:

   easy_install pygments

   Re-start Apache, login to ReviewBoard as the admin user, go to Admin -> Settings -> Diff Viewer -> Enable Syntax Highlighting. Click “Save”. Each existing user will need to enable syntax highlighting in their preferences. New users will be created with syntax highlighting enabled by default.

Create Site

The installation of RB is pretty much complete so now we need to set up a “site” and hook it into Apache to be served.

1. Beginning Installation: run the rb-site command to create a new instance of the site. I used the command:

   rb-site install C:wwwrb

   but you have to be sure that the rb-site command’s directory C:Python26Scripts is in the PATH var (see above).

2. It is also recommended to install mod_python for Apache. Find mod_python for Python 2.6 and install it. I found a package at http://www.hydra1983.com/?p=1508. I installed mod_python-3.3.2-dev-20080819.win32-py2.6.exe and it is working well. Note: Sorin Popa mentioned in the comments that you can also get the file from http://sinihong.springnote.com/pages/5249175/attachments/3036637
3. After the site is created, copy to contents of the C:wwwrbconfapache-modpython.conf file and append it to the Apache virtual host file C:Apache2.2confextrahttpd-vhosts.conf. I had to change the DocumentRoot and I also added some logging. See below my httpd-vhosts.conf:

   NameVirtualHost *:80
   
   <VirtualHost *:80>
     ServerName myHostName
     DocumentRoot "C:/www"
   
     # Error handlers
     ErrorDocument 500 /errordocs/500.html
   
     # Serve django pages
     <Location "/rb/">
       PythonPath "['C:/www/rb/conf'] + sys.path"
       SetEnv DJANGO_SETTINGS_MODULE reviewboard.settings
       SetEnv PYTHON_EGG_CACHE "C:/www/rb/tmp/egg_cache"
       SetHandler mod_python
       PythonHandler django.core.handlers.modpython
       PythonAutoReload Off
       PythonDebug Off
       # Used to run multiple mod_python sites in the same apache
       PythonInterpreter reviewboard_rb
     </Location>
   
     # Serve static media without running it through mod_python
     # (overrides the above)
     <Location "/rb/media">
       SetHandler None
     </Location>
     <Location "/rb/errordocs">
       SetHandler None
     </Location>
   
     <Directory "C:/www/rb/htdocs">
       AllowOverride All
     </Directory>
   
     # Alias static media requests to filesystem
     Alias /rb/media "C:/www/rb/htdocs/media"
     Alias /rb/errordocs "C:/www/rb/htdocs/errordocs"
   
     ErrorLog "C:/logs/apache/reviewboard-error.log"
     CustomLog "C:/logs/apache/reviewboard-access.log" common
   </VirtualHost>

4. After a restart of Apache you should be able to access RB in a browser (http://myHostName/rb)

Email/Logging

I set up email support so that reviewers can be notified of new reviews or comments. Visit http://myHostName/rb/admin/settings/email/ and set:

• Mail Server: mailhost
• Port: 25

Logging can also be set in http://myHostName/rb/admin/settings/logging/:

• Log directory: C:logsreviewboard

Active Directory

I also installed the optional LDAP module python-ldap-2.3.11.win32-py2.6.msi so that I could enable AD user connections. In the RB Admin console (http://myHostName/rb/admin/settings/general/) select:

• Advanced Authentication
  • Authentication Method: Active Directory
• Active Directory Authentication Settings
  • Domain name: your domain
  • Domain controller: host name of AD and domain above (FQDN)
  • OU name: OU of user accounts
  • Recursion Depth: -1

At this point we were able to log in using our City network accounts. If the Administrator set up groups then the user can select their own groups when the sign in for the first time.

SVNBridge

There is a nifty tool from Codeplex (hosted by Microsoft) called SVNBridge that allows you set a service that will accept SVN commands but forwards them as TFS commands to the TFS server. This tool is useful to us as ReviewBoard does not support TFS out of the box but does support SVN. For the SVNBridge Server installation, I had to deviate a bit from their instructions as WinXp uses IIS 5.1 and not the recommended 6 or 7 version:

1. Ensure that IIS 5.1 and ASP 2.0.50727 are installed.
2. Get the Server version of SVNBridge and extract it to C:SvnBridge.
3. For the most part we can follow the instructions for SvnBridge Server as if installing on IIS 6.
4. Change the Web.config file to something similar to:

   <?xml version="1.0" encoding="UTF-8"?>
   <configuration>
     <appSettings>
       <add key="TfsUrl" value="http://cmap041:8080" />
       <add key="LogPath" value="C:SvnBridgeLogs" />
     </appSettings>
     <system.web>
       <httpRuntime maxRequestLength="500000" /> <!-- 250+ MB -->
       <customErrors mode="Off" />
       <compilation debug="true" />
       <authentication mode="Windows" />
       <identity impersonate="true" />
       <!-- Uncomment this httpHandlers element if running under IIS 6.0 / 5.1 -->
       <httpHandlers>
         <clear/>
         <add verb="*" path="*" type="SvnBridgeServer.SvnBridgeHttpHandler, SvnBridgeServer" />
       </httpHandlers>
     </system.web>
     <system.net>
       <defaultProxy enabled="true" />
     </system.net>
   
     <!-- IIS 7.0 -->
     <system.webServer>
       <validation validateIntegratedModeConfiguration="false" />
       <modules>
         <remove name="ServiceModel" />
       </modules>
       <handlers>
         <clear/>
         <add name="SvnBridgeHandler"
           path="*"
           verb="*"
           responseBufferLimit="0"
           type="SvnBridgeServer.SvnBridgeHttpHandler"
           resourceType="Unspecified"
           preCondition="integratedMode" />
       </handlers>
       <security>
         <requestFiltering>
           <requestLimits maxAllowedContentLength="262144000" /> <!-- 250 MB -->
           <fileExtensions>
             <clear/>
           </fileExtensions>
           <hiddenSegments>
             <clear/>
           </hiddenSegments>
         </requestFiltering>
       </security>
     </system.webServer>
   </configuration>

5. Using IIS Manager, create a new website (or rename the default as I did) called “SvnBridge”:
   1. Web Site tab: Change the TCP port to 8081.
   2. Home Directoy tab: Local path is the folder where you unzipped the contents of SvnBridge-Server.zip (ex C:SvnBridge). Also, click the Configuration button, and click the Add button to add a new wildcard application map. Specify “c:Windowsmicrosoft.netframeworkv2.0.50727aspnet_isapi.dll” as the executable, set the extension to “.*”, set All Verbs, and UNCHECK “Check that file exists” checkbox.
   3. ASP.NET tab: make sure the version is set to 2.0.50727
   4. Directory Security tab: click the Edit button for “Anonymous access and authentication control”, and make sure only Basic authentication is checked using your domain as both the Default domain and Realm. (Note: this deviates from the instructions. I couldn’t get it to work with Digest).
6. Restart the IIS server.
7. If you have SVN installed you can run the command to list the projects:

   svn ls http://myHostName:8081/projA

   This will connect to the TFS server and list everything in the projA project. If you see output then the connector is working correctly.

Link to TFS

1. To set up a repository, visit http://myHostName/rb/admin/db/scmtools/repository/ and select “Add Repository”.
2. Set the following:
   1. General Information
      1. Name: TFS
      2. Path: http://myHostName:8081
      3. Repository Type: Subversion
      4. Bug tracker URL: http://myBugzillaServer/cgi-bin/bugzilla/show_bug.cgi?id=%s
   2. Authentication
      1. Username: choose a user that has full access TFS
      2. Password: password
3. This will connect into the SVNBridge and return the TFS content even though ReviewBoard thinks it is SVN. Woohoo 🙂

Post-Review tool to create reviews automatically

Visit the post-review site or the dev doc version. I created a php script to auto-create reviews to make it easy for our developers. The root of the web server (http://myHostName) will display a script which allows a user to simply enter their name and the TFS changeset. The script then calls post-review on the server to retrieve the diff and create the review for the user.

1. Using the admin console (http://myHostName/rb/admin/db/auth/user/), create a user “Poster” and give them the permissions:
   1. reviews | review request | Can submit as user
   2. reviews | review request | Can edit review request

   This will allow the Poster user to be used in the script to create requests on behalf of another user.

2. Create default.css on the root of the webserver:

   div.row {
     clear: both;
     padding-top: 10px;
   }
   
   div.row span.label {
     float: left;
     width: 75px;
     text-align: right;
   }
   
   div.row span.formw {
     float: right;
     width: 510px;
     text-align: left;
   }
   
   div.borderBox{
     width: 600px;
     background-color: #D3D3D3;
     border: 1px dotted #333;
     padding: 5px;
     margin: 5px
   }
   
   div.spacer {
     clear: both;
   }

3. Create index.html on the root of the webserver:

   <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
   <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
   <head>
     <title>Create Review | Review Board</title>
     <link rel="icon" type="image/png" href="/rb/media/rb/images/favicon.png" />
     <link rel="stylesheet" type="text/css" href="/rb/media/rb/css/common.css" />
     <link rel="stylesheet" type="text/css" href="default.css" />
   
     <script type="text/javascript">
       var MEDIA_URL = "/rb/media/";
     </script>
   
     <!--[if lt IE 7.]>
     <style type="text/css">
       body {
         behavior: url("/rb/media/rb/js/csshover2.htc");
       }
   
       img {
         behavior: url("/rb/media/rb/js/pngfix.htc");
       }
     </style>
     <![endif]-->
   
     <!--[if lt IE 8.]>
     <link rel="stylesheet" type="text/css" href="/rb/media/rb/css/ie_hacks.css"></script>
     <![endif]-->
   </head>
   <body>
     <div id="container">
       <br />
       <div id="rbinfo">
         <img id="logo" src="/rb/media/rb/images/logo.png" />
         <h1 id="title">Review Board</h1>
       </div>
       <br />
       <form action="createReview.php" method="post">
         <h2>Required Fields</h2>
         <div class="borderBox">
           <div class="row">
             <span class="label">Name:</span>
             <span class="formw">
               <select name="yourname">
                 <option value=""></option>
                 <option value="dev1">dev1</option>
                 <option value="dev2">dev2</option>
                 <option value="dev3">dev3</option>
               </select><br />
               <i>(Your Windows network login without the domain; eg: pinetti, zhuma, patchmi, etc)</i>
             </span>
           </div>
           <div class="row">
             <span class="label">Changeset:</span>
             <span class="formw">
               <div id="divChangeSetLow" style="display:none">
                 <input type="text" name="changesetLow" /> to
               </div>
               <input type="text" name="changesetHi" />
               <input type="checkbox" name="modtype" value="Range" onclick="showMe('divChangeSetLow', 'inline', this)" />Range<br />
               <i>(The changeset submitted to TFS that you want to be reviewed. Number only.)</i>
             </span>
           </div>
           <div class="row">
             <span class="label">TFS Path:</span>
             <span class="formw">
               <input type="text" name="tfsPath" /><br />
               <i>(Sets the TFS path for the project. For example, if work was done in TFS on project $/projA/trunk/ then this value should be "projA/trunk/".)</i>
             </span>
           </div>
           <div class="spacer">
             &nbsp;
           </div>
         </div>
   
         <h2><input type="checkbox" name="modtype" value="Optional" onclick="showMe('divOptional', 'block', this)" />Optional Fields</h2>
         <div class="borderBox" id="divOptional" style="display:none">
           <div class="row">
             <span class="label">Summary:</span>
             <span class="formw">
               <input type="text" name="summary" /><br />
               <i>(Sets the review request's summary field to the specified text.)</i>
             </span>
           </div>
           <div class="row">
             <span class="label">Branch:</span>
             <span class="formw">
               <input type="text" name="branch" /><br />
               <i>(Sets the review request's branch field to the specified text.)</i>
             </span>
           </div>
           <div class="row">
             <span class="label">Bugs:</span>
             <span class="formw">
               <input type="text" name="bugs" /><br />
               <i>(Specifies a list of bug numbers to use for the Bugs Closed section in the review request. This should be a comma-separated list. Check our <a href="http://dc01app02ps/cgi-bin/bugzilla/index.cgi">Bugzilla page</a> for the list of bugs.)</i>
             </span>
           </div>
           <div class="row">
             <span class="label">Groups:</span>
             <span class="formw">
               <input type="text" name="groups" /><br />
               <i>(Provides a list of groups that should be on the reviewer list. This should be a comma-separated list.)</i>
             </span>
           </div>
           <div class="row">
             <span class="label">People:</span>
             <span class="formw">
               <input type="text" name="people" /><br />
               <i>(Provides a list of usernames that should be on the reviewer list. This should be a comma-separated list.)</i>
             </span>
           </div>
           <div class="row">
             <span class="label">Description:</span>
             <span class="formw">
               <textarea cols="40" rows="4" name="description" ></textarea><br />
               <i>(Sets the description for the review request to the specified text.)</i>
             </span>
           </div>
           <div class="row">
             <span class="label">Testing:</span>
             <span class="formw">
               <textarea cols="40" rows="4" name="testing" ></textarea><br />
               <i>(Sets the testing done text for the review request to the specified text.)</i>
             </span>
           </div>
           <div class="spacer">
             &nbsp;
           </div>
         </div>
         <p><input type="submit" value="Create review"></p>
       </form>
     </div>
   
     <script type="text/javascript">
       function showMe (it, displayStyle, box) {
         var vis = (box.checked) ? displayStyle : "none";
         document.getElementById(it).style.display = vis;
       }
     </script>
   </body>
   </html>

4. Create createReview.php on the root of the webserver:

   <?php
   
   set_time_limit(200); //200 seconds
   
   $isDEBUG=true;
   $TFS_SERVER_URL = "http://myHostName:8081/";
   $RB_SERVER_URL = "http://myHostName/rb";
   $POSTER_USER = "poster";
   $POSTER_PASS = "p@ssw0rd";
   
   print("<h1>Debug ReviewBoard post-review command</h1>");
   print("<h2>Parameters</h2>");
   
   $yourname    = check_input($_POST['yourname'], "***Required field missing: You must enter your name.***");
   print("yourname=".$yourname."<br>");
   
   $changesetHi    = check_input($_POST['changesetHi'], "***Required field missing: You must enter a changest number.***");
   check_number($changesetHi, "***Invalid parameter: The changest value must be a valid number.***");
   print("changesetHi=".$changesetHi."<br>");
   
   $changesetLow = check_input($_POST['changesetLow']);
   if (empty($changesetLow)){
     $changesetLow = $changesetHi - 1;
   } else {
     check_number($changesetLow, "***Invalid parameter: The lower bound of the changest range value must be a valid number.***");
     if ($changesetLow >= $changesetHi){
       show_error("***Invalid parameter: The lower bound of the changest range value must be lower than the higher bound.***");
     }
   }
   print("changesetLow=".$changesetLow."<br>");
   
   $tfsPath    = check_input($_POST['tfsPath'], "***Required field missing: You must enter the TFS Project Path.***");
   print("tfsPath=".$tfsPath."<br>");
   
   $summary    = check_input("TFS ".$changesetLow.":".$changesetHi." -- ".$_POST['summary']);
   print("summary=".$summary."<br>");
   
   $branch        = check_input($_POST['branch']);
   print("branch=".$branch."<br>");
   
   $bugs         = check_input($_POST['bugs']);
   print("bugs=".$bugs."<br>");
   
   $groups        = check_input($_POST['groups']);
   print("groups=".$groups."<br>");
   
   $people        = check_input($_POST['people']);
   print("people=".$people."<br>");
   
   $description= check_input($_POST['description']);
   print("description=".$description."<br>");
   
   $testing    = check_input($_POST['testing']);
   print("testing=".$testing);
   
   $command = "post-review --repository-url="$TFS_SERVER_URL$tfsPath" --server="$RB_SERVER_URL" --username="$POSTER_USER" --password="$POSTER_PASS"";
   $command .= " --submit-as="$yourname" --revision-range="$changesetLow:$changesetHi"";
   $command .= " --summary="$summary" --branch="$branch" --bugs-closed="$bugs" --target-groups="$groups"";
   $command .= " --target-people="$people" --description="$description" --testing-done="$testing"";
   if (!$isDEBUG) {
     $command .= " --debug";
   }
   print("<h2>Compiled command</h2>".$command);
   
   $output = shell_exec($command);
   print("<h2>Command Output</h2>".$output);
   
   $pos = strripos($output, $RB_SERVER_URL."/r/");
   if ($pos === false) {
     show_error("The review was not created correctly.");
   } else {
     $newReviewURL = substr($output, $pos);
     print("<h2><span style="color:green;">Success</span></h2>");
     print("New review created: <a href="".$newReviewURL."">$newReviewURL</a>");
     if (!$isDEBUG) {
      header("Location: $newReviewURL") ; //redirect to new review page
     }
   }
   
   function check_input($data, $problem='') {
     $data = trim($data);
     $data = stripslashes($data);
     $data = htmlspecialchars($data);
     if ($problem && strlen($data) == 0) {
       show_error($problem);
     }
     return $data;
   }
   
   function check_number($data, $problem='') {
     if (preg_match("/D/",$data)) {
       show_error($problem);
     }
   }
   
   function show_error($myError) {
     print("<h2><span style="color:red;">Failure</span></h2>");
     print($myError);
     print("<p>Press the back button and correct the error</p>");
     exit();
   }
   ?>

Change the “diff” behaviour

Due to the way that we have TFS running through SVNBridge and ReviewBoard thinks it is calling a SVN repo, we have encountered an error with binary files. Reviewboard has coded into post-review to use:

svn diff --diff-cmd=diff

which tells SVN diff to use the external tool “diff” which is assumed to be GNU diffutils. SVN diff will only accept exit codes of 0/same and 1/different (Ref: http://svnbook.red-bean.com/nightly/en/svn.advanced.externaldifftools.html). However, GNU diffutils will return exit codes 0/same, 1/different, and 2/trouble (Ref: http://www.gnu.org/software/hello/manual/diff.html#Binary). An error occurs when a binary file (ex: image) is passed to the diff tool. The ReviewBoard log will spit out:

'Index: Reports/toolbar/upd.gifn','===================================================================n',
'Files Reports/toolbar/upd.gift(revision 0) and Reports/toolbar/upd.gift(revision 17489) differn',
"svn: 'diff' returned 2n",'svn:Error reading spooled REPORT request responsen']

indicating that GNU diff returned 2 meaning “trouble”. The trouble occurs because SVN should not be trying to diff binary files. SVN determines if a file is binary by checking the SVN property svn:mime-type to see if the file is binary or not. According to http://help.collab.net/index.jsp?topic=/faq/svnbinary.html, when a file is checked into a SVN repo it is checked for mime and then the svn:mime-type is assigned. When a subsequent SVN diff is called, it will check the svn:mime-type property to see if it is text or binary. If no property is set, then SVN diff assumes that the file is text. Since our files are checked into TFS, there are no SVN properties assigned and so when a binary file is retrieved through SVNBridge, there is also no property. This is why SVN treats a binary as text and this is why we are getting errors.

To fix this I had to modify post-review. Since C:Python26Scriptspost-review.exe is a compiled executable, I downloaded rbtools 0.2 stable from
http://github.com/reviewboard/rbtools, made my changes to postreview.py, and reinstalled the package using the command:

python setup.py install

at the rbtool root. In postreview.py I changed the code snippet:

["svn", "diff", "--diff-cmd=diff"]

to

["svn", "diff", "--diff-cmd=C:\GnuWin32\bin\diffForceText.bat"]

in 3 places in the file and reinstalled (using above python command). I created the wrapper script (DOS batch) diffForceText.bat and put it in to C:GnuWin32bin and it simply includes:

@echo off
diff %* --text

This wrapper essentially passes all parameters onto GNU diff but also adds the –text parameter which will force diff to read a binary file as text (http://www.gnu.org/software/diffutils/manual/#Binary). This will appear a little garbled in ReviewBoard but it does avoid the above error, allow the review to be created and let the reviewer know that a binary file has changed. This is more than good enough for us.

I opened a couple bugs/discussions on the topic of the diff issue:

• SVNBridge Codeplex bug: http://svnbridge.codeplex.com/workitem/15309
• ReviewBoard forum: http://groups.google.com/group/reviewboard/browse_thread/thread/32f88fa7061cf549#

Summary

This whole solution would have been so much easier if we were using Git or Subversion but I was only able to play with the environment that we already had in place. I am extremely happy with the outcome and feature set that ReviewBoard provides. I have demoed this solution to the other developers and management and they are all surprised by how well the solution performs.

If you want an open source product that surpasses Crucible/CodeCollaborator and has an active community, then try ReviewBoard and, for the love of god, help to get it working natively with TFS!