Hi all,
The JDS subversion repository (spec-files) is soon moving to
svn.opensolaris.org. At the same time, we are planning to
introduce a simple code review process that would apply to
both Sun and non-Sun contributors. Below is the proposal
for the review process, comments are welcome.
Thanks,
Laca
---8<---
JDS Code Review Process
- Background
- Motivation
* currently, there is no formal review process in JDS, even though
we are not perfect ;)
* as we are moving our subversion repository to opensolaris.org and
inviting contributions from community members, we need 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
- 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-code-review at 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)
[ FIXME: the above mailing is yet to be created ]
* the subject must be: "review: <module> <description>"
where <description> is typically the same as the description part
of a patch name, for examples:
review: nautilus build fix
review: evolution-data-server shared libdb support
review: ekiga branding changes
* all JDS 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
* your changes are considered approved if no issues are raised within
3 working days or if all issues raised are addressed
* 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
- tracking reviews
* the review alias is archived [FIXME: link to archives 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
- review guidelines (draft)
- 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
* 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
* look out for code changes that require changes to documentation
(e.g. man pages)
- spec file changes
* patches must be added to the Linux 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
* patches added to Linux spec files go into jds-spec-files/patches,
patches added to Solaris spec files (because there is no Linux spec
file) go into jds-spec-files/Solaris/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:
root:sys for /etc, /var
root:bin for /usr/*
root:other for a few directories:
/usr/lib/%{_arch64}/pkgconfig
/usr/lib/pkgconfig
/usr/share/applications
/usr/share/pixmaps
/usr/share/aclocal
/usr/share/application-registry
/usr/share/doc
/usr/share/gnome
/usr/share/mime-info
/usr/share/locale
/usr/share/icons RECURSIVELY!
/var/lib
* 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).
