Draft Public Code Review Proposal

[David Holmes]

Hi Dalibor,

On 14/07/2011 11:45 AM, 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.
>
> 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.
>
> 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.

At present who comprises the set of authors, commiters and reviewers for the 
jdk7u project?

> 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.

I don't understand what you mean by resubmitted. Do you just mean that we 
can give the URL to the original JDK8 webrev? I assume review is still 
necessary.

> 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
>
> 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.

So I don't understand how rules 3 and 4 interact. Let's assume that jdk7u 
exists and we haven't yet forked off jdk7u2. I have a fix that must be in 
7u2 so I push to the jdk7u-dev repo. According to the above this requires a 
single reviewer. When 7u2 repos are created I assume they will contain the 
current contains of 7u mainline - is that right? Is it after that point that 
commits to 7u2 require two reviews (presumably because by that stage we 
anticipate 7u2 is fairly close and so we need to be extra careful when doing 
commits)?

I assume that the approval of the Project and Technical leads are in 
addition to reviews themselves. What is the process for obtaining approval? 
I would expect that approval, in principle at least, for a given CR needs to 
be given upfront, and then again once the final code change is ready. What 
are the expectations and limitations on placing things into an update release?

FYI I have a fix (7039182) that is in 8 and needs to be in 7u2 and I'd like 
to move on this asap, so don't mind being the initial tester of the new 
process, as it were. For good measure this is actually a non-public CR so 
we'll see how that part of the process goes too ;-)

Cheers,
David Holmes


> cheers,
> dalibor topic