Draft Public Code Review Proposal

[Dr Andrew John Hughes]

On 03:45 Thu 14 Jul     , Dalibor Topic wrote:

> Next item on my draft list is code review. The proposal below is
  inspired by the developer guide section on code review, and in part
  inspired by the processes in OpenJDK 6. It tries to encourage
  transparency, and getting more eyes on the code, while at the same
  time allowing established practices, like asking component leads for
  additional reviews, to continue to work.

> Public code review:

>  Preamble: The new infrastructure is not ready yet, so we won't
> switch to it for the next release in OpenJDK (i.e. 7u2, as 7u1 is a
> CPU) just yet. The process here may change for 7u4.

What is this 'CPU'?  You refer to it both here and in the last
document, but I assume it's not Central Processing Unit.

> Pre-requisite: Before asking for code review, make sure your fix is
  applied on the most recent revision of the code you intend to work
  on, that there are no build errors on the supported platforms for
  the release you're working on, and that any included or otherwise
  applicable tests pass without failures on all relevant platforms. If
  you're not able to build or test on all supported platforms, you
  MUST let the reviewers know which ones you were able to build and
  test on, as well as whether you anticipate any issues on the
  platforms you haven't been able to build and test on.
> 

These rules are very stringent.  I doubt anyone outside Oracle is going
to be able to build and test on all platforms.  Running the full test suite
on any platform is non-trivial and something that should be provided by
mainline infrastructure, not something every developer has to set up.

> Rule 0: Code reviews for public JDK 7 Update forests MUST be done
> publicly: Either through e-mail on jdk7u-dev at openjdk.java.net
> mailing list, or using some other suitable public mechanism. If a
> code review is not done on the jdk7u-dev at openjdk.java.net mailing
> list, as part of the approval request for inclusion of the fix into
> a public JDK 7 Updates forest, which you MUST send to the
> jdk7u-dev at openjdk.java.net mailing list, a URL for the public code
> review MUST be provided.

> Rule 1: If the content of a changeset submitted for review for a
> public JDK 7 Update forest is the same as the corresponding
> changeset submitted or committed for JDK 8, then it does not have to
> be resubmitted. In that case its URL will suffice.

> Rule 2: If the content of a changeset submitted for review for a public JDK 7 Update forest differs from the corresponding JDK 8 changeset, or if there is no corresponding JDK 8 changeset, then the changeset MUST be submitted publicly for review:
> 
>     * A very small changeset MAY be submitted as a simple patch, as described in http://openjdk.java.net/contribute/
> 
>     * Other than that, changesets SHOULD be submitted as webrevs on the code review server, cr.openjdk.java.net.
> 
>     * Alternatively, a URL for the webrev of the changeset MAY be provided
> 

What is the reasoning behind continuing to use webrevs?  They separate the patch
from the discussion and there seems to be no guarantee that they won't just disappear,
as they aren't archived with the mails.

> Rule 3: A changeset submitted for the JDK 7 Update mainline forest
> (jdk7u) MUST be reviewed by at least one reviewer. The maintainer
> responsible for approval of the changeset MAY request additional,
> specific reviewers to review the changeset, e. g. component leads.

> Rule 4: A changeset submitted for a JDK 7 Update release forest
> (like jdk7u2, once it's branched off) MUST be reviewed by at least
> two reviewers. The maintainer responsible for approval of the
> changeset MAY request additional, specific reviewers to review the
> changeset, e. g. component leads.

> cheers, dalibor topic 

Cheers,
-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37