ROSE Compiler Framework/Code Review

Please note: the URL of the internal github has changed! It is now https://rose-github.llnl.gov/, instead of https://github.llnl.gov/ .

Motivation

Without code review, developers have:

added unreadable contributions which do not conform to any consistent coding styles.
added undocumented contributions which cannot be understood by anybody else(essentially useless contributions).
added untested contributions (codes without accompanying tests) so the contributions do not work as expected or can be easily broken by other conflicting contributions (another essentially less useful contributions)
disabled tests to subvert our stringent Jenkins CI regression tests
added files into wrong directories, with improper names
committed hundreds of reformatted files
re-invented the wheel by implementing features that already exist
added 160MB MPI trace files into the git repository

See Phabricator's "Advantages of Review" document (a Facebook project).

Goals

Our primary goals for code reviewing ROSE are to:

share knowledge about the code: coder + reviewer will know the code, instead of just the coder
group-study: learn through studying other peoples' code
enforce policies for consistent usability and maintainability of ROSE code: documented and tested
avoid reinventing the wheel and eliminating unnecessary redundancy
safe-guarding the code: disallowing subversive attempts to disable or remove regression tests

Software

We are currently testing Github Enterprise and looking into the possibility of leveraging Redmine for internal code review.

In the past, we have looked at Google's Gerrit code review system.

Github

Releases: https://enterprise.github.com/releases

Support: https://support.enterprise.github.com

rosebot

(Under development)

An automated pull request analyzer to perform various tasks:

Automatically add reviewers to Pull Requests based on hierarchical configuration
"Pre-receive hook" analyses: file sizes, quantity of files, proprietary source, etc.
more...

Developer Checklist

Read these tips and guidelines before sending a request for code review.

Coding Standards

Please go to Coding Standard for the complete guideline. Here we only summary some key points.

Your code should be written in a way that makes it easily maintainable and reviewable:

write easy to understand code; avoid using exotic techniques which nobody can easily understand.
add sufficient documentation (source-code comments, README, etc.) to aid the understandability of your code, your documentation should cover
- why do you do this (motivation)
- how do you do it (design and/or algorithm)
- where are the associated tests (works as expected)
before submission of your code for review, make sure
- you have merged with the latest central repository's master branch without conflicts
- your working copy can pass local tests via: make, make check, and make distcheck
- you have fixed all compiler warnings of your code whenever possible
submit a logical unit of work (one or more commits); something coherent like a bug fix, an improvement of documentation, an intermediate stage for reaching a big new feature.
balance code submissions with a good ratio of [lines of code] and [complexity of code]. A good balance needs to be achieved to make the reviewer's life easier.
- the time needed to review your code should not exceed 1 hour. Please avoid pushing thousands of lines at a time.
- Please also avoid pushing any trivial (fixed a typo, commented out a single line etc.) to be reviewed.

One time setup

Steps for initializing code review:

1. Login to http://rose-github.llnl.gov using your OUN and PAC.

2. Fork your own clone of the ROSE repository from http://rose-github.llnl.gov/rose-compiler/rose.

Go to http://rose-github.llnl.gov/rose-compiler/rose
Click the Fork button at the upper right corner of the webpage

3. Add Collaborators:

Go to http://rose-github.llnl.gov/<your_account>/rose
- Click Admin
- Click Collaborators
  - Add candidate code reviewers: liao6, too1. These developers will review and merge your work.
  - Add admins: hudson-rose. This user will automatically synchronize your master branch with /nfs/casc/overture/ROSE/git/ROSE.git:master.

4. Create your public-private SSH key pair using ssh-keygen, and add the public key to your rose-github.llnl.gov account. Refer to Generating SSH Keys or use a public key tat you already have. (rose-github.llnl.gov only supports the SSH protocol for now; HTTPS is not yet supported.)

5. Configure Auto-syncs: Contact the Jenkins administrator (too1 and liao6) to have your repository added to a white-list of repositories to be synced whenever new commits are integrated into ROSE's official master branch.

6. Setup polling job: Contact the Jenkins administrator (too1 and liao6) to have your Github repository polled for new changes on the master branch. When new changes are detected, your master branch will be pushed to the central repository (and added to the Jenkins testing queue) as <oun>-reviewd-rc.

Daily work process

have a local git repo to do your work and submit local commits, you have two choices:
- clone it from /nfs/casc/overture/rose/rose.git as we usually do before
- clone your fork on rose-github.llnl.gov to a local repo (only HTTPS is supported via LC)

Note: You may encounter SSL certificate problems. If you do, simply disable SSL verification in cURL using either export GIT_SSL_NO_VERIFY=false or configuring git:

$ git config --global http.sslVerify false

- don't use branches, use separated git repositories for each of your tasks. So status/progress of one task won't interfere with other tasks.
When ready to push your commits, synchronize with the latest rose-compiler/master to resolve merge conflicts, etc.
- type: git pull origin master # this should always work since master branches on rose-github.llnl.gov are automatically kept up-to-date
- make sure your local changes can pass 1)make -j8, 2)make check -j8, and 3)make distcheck -j8
push your commits to your fork's non-master branch, (like bugfix-rc , featurex-rc, work-status, etc.) You have total freedom in creating any branches in your forked repo, with any names you like

  # If your local repository was cloned from /nfs/casc/overture/ROSE/rose.git. 
  # There is no need to discard it. You can just add the rose-github.llnl's repo as an additional remote repository and push things there:
  git remote add github-llnl-youraccount-rose http://rose-github.llnl.gov/youraccount/rose.git
  git push github-llnl-youraccount-rose HEAD:refs/heads/bugfix-rc

- It is encouraged to push your work to a remote branch with a -status suffix, which will trigger a pre-screening Jenkins Job: http://hudson-rose-30:8080/view/Status/job/S0-pre-screening-before-code-review/. This is often useful to make sure your pushes can pass a minimum make check rules, including your own, before reviewers spend time on reading your code. Reviewers can also see both your code and your code's actions.
add a pull(merge) request to merge bugfix-rc into your own fork's master,
- please note that the default pull request will use rose-compiler/rose's master as the base branch (destination of the merge). Please change it to be your own fork's master branch instead.
- Also make sure the source (head) branch of the pull (merge) request is the one your want (bugfix-rc in this example)
- Double check the diff tab of your pull request only shows the differences you made, without other things brought in from the central repo. Or your own repo's master is out-of-sync with the central repo's master. Notify system admin (too1) for the problem or manually fix it using the troubleshooting section of this page.
notify a reviewer that you have a pull request (requesting to merge your bugfix-rc into your master branch)
- You can assign the pull request to the reviewer so an email notification will be automatically sent to the reviewer
- Or you can add discussion within the pull request using @revieweraccount. NOTE: please only click "Comment on this issue" once and manually refresh the web page. Github Enterprise has a bug so it cannot automatically shown the newly added comment. bug79
- Or you can just email the reviewer
waiting for reviewer's feedback:

Review results

Completion and Submission To Jenkins
- If your code passes code review, the reviewer should have merged your bugfix-rc into your master. Jenkins will automatically poll your master and do the testing/merging
How To Make Changes
- To implement changes make local edits, local commits, push to your remote branch, and send a merge request again
Taking Code Review Seriously
- Remember code review is not an attack on you as a person. The purpose of code review is to allow a colleague to evaluate your code. This can take a reasonable amount of time, so respect their efforts and seriously look at your code anew.
- Look through the reviewer comments and address them or comment the purpose of the code as it stands and wait for response
- Some comments are mandatory changes, these must be addressed before you will pass code review
- Some comments are suggestions. You should think about their suggestions carefully. If the reviewer suggests something you should form a rationalization for the difference or consider the implications of changing your code. ROSE is a team effort, we must take our colleagues seriously.
- DO NOT CLOSE' the pull request. You can push your new commits to the same branch again and comment on the pull request to indicate there are new updates. Please review them again. This will avoid unnecessary repetition.

Benefits of Code Review

Avoiding coding a feature that is already present.
- Remember you are coding for a user, and we must try our best to write clear code
- Code coherency is extremely important in a large project. Coherent code allows the user to spent his or her time on their project rather than trying to find an answer in the doxygen page and finding seven or eight ways to do the same thing without knowing the consequences of the different approaches
Coding As A Team
- If every coder hid away and coded by himself without regard to the features ROSE already has, it not only can confuse users but developers as well.
- At a glance, the ROSE source directory weighs in at almost 1 GB. The compilation directory after make, make check, make install, make installcheck, make distcheck comes in at 19G. Lesson: ROSE is large and the chance of someone knowing everything about ROSE and its functionality is rather slim
- As a team the size is quite large but manageable so long as the work each person does on their particular section and asks about possible feature duplication and code readability

Reviewer Checklist

What to look out for as a code reviewer?

Be familiar with the current Coding Standard as a general guideline to perform the code review.
Allocate up to 1 hour at a time to review approximately 500-1000 lines of code: a longer time may not pay off due to the attention span limits of human brains

What to check

Six major things to check:

Documentation: What are the commits about? Is this reflected in README, source comments, or LaTex files?
Style: Does the coding style follow our standard? Is the code clean, robust, and maintainable?
Interface: Does the code has a clean and simple interface to be used by users?
Algorithm: Does the code have sufficient comments about what algorithm is used? Is the algorithm correct and efficient (space and time complexity)?
Implementation: Does the code correctly implement the documented algorithm(s)?
Testing: Does the code have the accompanying test translator and input test codes to ensure the contributions do what they are supposed to do?
- Is Jenkins being configured to trigger these tests (your work may require new pre-requisite software or configure options)? Local tests on developer's workstation do not count.

More details, quick summary from Coding Standard

Naming conventions: File and directory names follow our standards; clear and intuitive
- Directory structure: source code, test code, and documentation files are added into the correct locations
Maintainability: clarity of code; can somebody who did not write the code easily understand what the code does?
- No looong functions: a function with hundreds of lines of code is a no-no
- Architecture/design: the reasons and motivations for writing the code, and its design.
No duplication: similar code may already exist or can be extended
Re-use: can part of the code be refactored to be reusable by others?
Unit tests: make check rules are associated with each new feature to ensure the new feature will be tested and verified for expected behaviors
Sanity: no turning off, or relaxing, other tests to make the developer's commits pass Jenkins. In other words, no cheating.

Commenting

Reviewer comments should be clearly delimited into these three well-defined sections:

1. Mandatory: the details of the comment must be implemented in a new commit and added to the Pull Request before the code review can be completed.

2. Recommended: the details of the comment could represent a best-practice or, simply, it could be intended to provide some insight to the developer that they may have not thought about.

Both Mandatory and Recommended can be accompanied by the keyword Nitpick:

3. Nitpick: the details of the comment represent a fix that usually involves a spelling/grammatical or coding style correction. The main purpose of the nitpick indication is to let the developer know that you're not trying to be on their case and make their life difficult, but an error is an error, or there's a better way to do something.

Decisions

Make a clear and definitive decision for the code review:

Pass: The code does what it is supposed to do with clear documentation and test cases. Merge and close the pull request.
Pass but with future tasks. The commits are accepted. But some additional tasks are needed in the future to improve the code. They can be put into a separate set of commits and pushed later on.
Fail. Additional work is needed, such as better naming, better places to put files, more source comments, add regression tests, etc. Notify the developers of the issues and ask for a new set of commits to be pushed addressing the corrections or improvements.

Giving negative feedback

We directly quote from http://www.mediawiki.org/wiki/Code_review_guide#Giving_negative_feedback

" Here are a few guidelines in the event you need to reject someone's submission or ask them to clean up their work:

Focus your comments on the code and any objectively-observed behavior, not motivations; for example, don't state or imply assumptions about motivating factors like whether the developer was just too lazy or stupid to do things right.
Be empathetic and kind. Recognize that the developer has probably put a lot of work in their idea, and thank them for their contribution if you feel comfortable and sincere in doing so (and try to muster the comfort and sincerity). Most importantly, put yourself in their shoes, and say something that indicates you've done so.
Help them schedule their work. If their idea is a "not yet" kind of idea, try to recommend the best way you know of to get their idea on a backlog (i.e. the backlog most likely to eventually get revisited).
Let them know where they can appeal your decision. For example, if the contributor doesn't have a history of being disruptive or dense, invite them to discuss the issue on wikitech-l.
Be clear. Don't sugarcoat things so much that the central message is obscured.
Most importantly, give the feedback quickly. While tactful is better (and you should learn from past mistakes), you can always apologize for a poorly-delivered comment with a quick followup. Don't just leave negative feedback to someone else or hope they aren't persistent enough to make their contribution stick."

Who should review what

Ideally, every ROSE contributor should participate in code review as a reviewer at some point so the benefits of peer-review can fully be fulfilled.

However, due to the limited access to our internal github enterprise server, we currently have a centralized review process in which ROSE staff members (liao6, too1) serve as the default code reviewers. They are responsible for either reviewing the code themselves or delegate to other developers who either has better knowledge about the contributions or should be aware of the contributions.

We am actively looking at better options and will gradually expand the pool of reviewers so the reviewing step won't become a bottleneck.

TODO: use rosebot to automatically assign reviewers according to a hierarchical configuration of the source-tree.

What to avoid

Judging code by whether it's what the reviewer would have written
- Given a problem, there are usually a dozen different ways to solve it. And given a solution, there's a million ways to render it as code.
degenerating into nitpicks:
- perfectionism may hurt the progress. we should allow some non-critical improvements to be done in the next version/commits.
feel obligated to say something critical: it is perfectly fine to say "looks good, pass"
delay in review: we should not rush it but we should keep in mind that somebody is waiting for the review to be done to move forward

Criticism

Code reviews often degenerate into nitpicks. Brainstorming and design reviews to be more productive.

This makes sense, the early we catch the problems, the better. Design happens earlier. Design should be reviewed. The same idea applies to requirement analysis also.
To mitigate this risk, we now have rules for design document in our coding standard.

Troubleshooting

master is out-of-sync

The master branch of each developer's git repository (http://rose-github.llnl.gov) should be automatically synchronized with the central git repository's master branch (/nfs/casc/overture/ROSE/git/ROSE.git). In rare cases, it could be out-of-sync. Here is an example to perform a manual synchronization:

1. Clone your Github repository:

$ cd ~/Development/projects/rose
$ git clone git@github.com:<user_oun>/rose.git
Cloning into ROSE...
remote: Counting objects: 216579, done.
remote: Compressing objects: 100% (55675/55675), done.
remote: Total 216579 (delta 159850), reused 211131 (delta 155786)
Receiving objects: 100% (216579/216579), 296.41 MiB | 35.65 MiB/s, done.
Resolving deltas: 100% (159850/159850), done.

2. Add the central repository as a remote repository:

$ git remote add central /nfs/casc/overture/ROSE/git/ROSE.git
$ git fetch central
From /nfs/casc/overture/ROSE/git/ROSE.git
 * [new branch]      master     -> central/master
 ...

3. Push the central master branch to your Github's master branch:

-bash-3.2$ git push central central/master:refs/heads/master
Total 0 (delta 0), reused 0 (delta 0)
To git@rose-github.llnl.gov:<user_oun>/rose.git
   16101fd..563b510  central/master -> master

master cannot be synchronized

In rare cases, your repository's master branch cannot be automatically synchronized. This is most likely due to merge conflicts. You will receive an error message through an automated email, resembling the following (last updated on 7/24/2012):

To git@rose-github.llnl.gov:lin32/rose.git
! [rejected]        origin/master -> master (non-fast forward)
error: failed to push some refs to 'git@rose-github.llnl.gov:lin32/rose.git'

---

Your master branch at [rose-github.llnl.gov:lin32/rose.git] cannot be automatically updated with [/nfs/casc/overture/ROSE/git/ROSE.git:master]

Please manually force the update:

Add the central repository as a remote, call it "nfs":

  $ git remote add nfs /nfs/casc/overture/ROSE/git/ROSE.git

1. First, try to manually perform a merge in your local repository:

  # 1. Checkout and update your Github's master branch
  $ git checkout master
  $ git pull origin master

  # 2. Merge the central master into your local master
  $ git pull nfs master
  <no merge conflicts>

  # 3. Synchronize your local master to your Github's master
  $ git push origin HEAD:refs/head/master

2. Otherwise, try to resolve the conflict.

3. Finally, if all else fails, force the synchronization:

  $ git push --force origin nfs/master:refs/heads/master

  WARNING: your master branch on Github will be overriden so make sure
  you have sufficient backups, and take precaution.

Please simply follow the email's instructions to force the update of your Github's master branch.

Past Software Experience

In the past, we have experimented with other code review tools:

Gerrit (Google)

In short:

Gerrit's user interface is not user-friendly (it's complex and therefore, more confusing). This is true, when compared to Github's Pull Request mechanism for code review.
Gerrit's remote API was not mature enough to handle our workflow. Additionally, we had to hack several things in order to slightly suit our needs. On the other hand, Github has a great remote API which is easily accessible through Ruby scripting, a very popular language for the domain of web interfaces and development.
Gerrit is not as popular as Github, which is important for our project to gain traction. Also, more people are familiar with Github so it makes it easier for them to use.