OpenSolaris

OSD Code Review Process

Laszlo Peter, 23th Oct, 2006

To review or not to review:

• typo fixes or similar trivial changes need not be reviewed
• patches submitted to bugzilla are exempt from internal review, but the bugzilla id must be included in the ChangeLog. (Note: you are welcome to use the code review alias to get your patch reviewed before submitting it upstream)
  • The engineer is responsible for keeping track of the bugzilla bug and taking action should the patch be rejected or a different fix is implemented by the community
  • re-work and resubmit the patch until mutually acceptable; or
  • if you think it won't get accepted upstream then you need to get it reviewed internally
• patches taken from upstream code are exempt from internal review, please indicate in ChangeLog where the patch is coming from.
• simply bumping to a new tarball version, removing upstream patches, trivial patch merges are exempt from review, however, please get all non-trivial patch changes reviewed; this is also a great opportunity to check the status of those patches
• spec file changes other than adding the Patch / %patch / %changelog entries are subject to review

Review process

• send your svn diff to jds-review@opensolaris.org, including the bugtraq or bugzilla id (when available) and a description if the %changelog entry (which should be in the patch) is not enough to understand what it does (Note: ideally, it should be descriptive enough)
• for all new spec files or spec files moved from one repository to another, specify which repository / section the spec file is going into, e.g. spec-files-other/experimental.
• all OSD engineers must be subscribed to the code review alias and anyone else is welcome to join.
• anyone can, and is encouraged to, comment on any code change
• comments, questions, answers, reviews are sent to the same alias
• bug category owners must review all patches in their category and respond. If the patch looks good and you have no comments, just reply with "approve"
• reviews are non-blocking: you can go ahead and commit if you are confident enough that the fix it good, but be aware that you may need to revert or amend your changes if your peers or the upstream community have issues with it
• new contributors get commit access after their first approved code change
• no unreviewed commits on last working day before a milestone build [FIXME: milestone build schedules must be published externally]
• you must address all questions or concerns
• reviewers are expected to respond to review requests within 3 working days, submitters should not wait for approval for longer than that
• be available 24-48 hours after you commit; if there are issues with your patch and noone is around to help and fix them, RE will undo your changes

Review guidelines

patches

• coding style must follow the coding style of the original sources
• the patch must not make the code Solaris specific, since that means that the patch cannot be accepted upstream
• the code must compile with both Sun Studio and gcc, however we don't expect people to compile and test with both compilers and on all supported platforms, architectures, etc.
• when using #ifdefs, consider checking for a certain feature, function call, library, etc. in configure.in rather than checking whether the OS is Solaris
• if you do use #ifdefs to check for Solaris, the correct form is #if defined(sun) && defined(SVR4) (this works with both gcc and Sun Studio)
• look out for interface changes that require ARC reviews, because they have an impact beyond the OSD project. See what sort of changes need ARC approval and when should changes to the architecture of the system be reviewed and/or approved
• look out for code changes that require changes to documentation (e.g. man pages)

spec file changes

• most packages are built using pre-module RPM spec files (base spec files) and a Solaris spec file that wraps them. Patches must be added to the base spec file whenever one exists, even if the patch is Solaris specific, so that all patches against a component are found at the same location
• all patches go into spec-files/patches,
• avoid using %ifos, it's evil
• make sure the appropriate build-time (BuildRequires) and run-time (Requires) dependencies are used
• use the directory macros: %{_bindir}, %{_libdir},...
• %files lists should use the appropriate default attributes:

 |attributes|directories

• use --without-foo or --disable-foo configure options for all features that we don't want to support
• look out for packaging changes (new packages, files moving from one packages to another).

Background

Motivation

• previously, there was no formal review process in OSD, even though we are not perfect ;) It was a pretty casual comment to a change in our commits list. Usually if the patch looked odd/interesting/otherwise someone would check it out and comment where necessary.
• as we were moving our subversion repository to opensolaris.org and inviting contributions from community members, we needed to have a process in place that works for both internal (Sun) and external (non-Sun) contributors
• changelog entries are often not descriptive enough to understand what the actual change is
• code reviews are a great opportunity for learning

Things considered when writing this process

• the process has to be simple, no filling out forms for half an hour
• nevertheless, engineers need to review their own patches one more time before submitting for a review and that in itself is a good thing
• the process must be the same for Sun and non-Sun contributors
• anyone should be able to comment on any patch
• patches written or reviewed by upstream community maintainer(s) are considered reviewed. The reason why we do this is basically because we've traditionally not had the resources or desire to do anything else. The total amount of code for the OSD consolidation is a *lot* bigger than anything in ON. While it would be nice to review everything, it's impractical, and there are many more experienced people in the community that are more capable of reviewing changes in their code. Obviously there are exceptions to the rule.

Tracking reviews

• the review alias is archived here
• longer term: reviews will be automatically tracked on a web page
• even longer term: metrics of the reviews, like
  • number of patches reviewed
  • number of patches approved without comments

Please send comments about this document to desktop-discuss@opensolaris.org.