Hey, A couple of comments..
Laszlo (Laca) Peter wrote: > - Motivation > > * currently, there is no formal review process in JDS, even though > we are not perfect ;) For historical background, previously it was a pretty casual comment to a change in our commits list. Each set of changes to the repository generates a mail sent to everyone who has an account. Usually if the patch looks odd/interesting/otherwise someone will check it out and comment where necessary. We hope to have this commits list available on opensolaris.org too, FWIW. > * changelog entries are often not descriptive enough to understand > what the actual change is To the extent that some entries don't even exist. One thing that I don't think Laca mentioned was the need to edit both ChangeLog and Solaris/ChangeLog where necessary. There are useful scripts [prepare-ChangeLog.pl or otherwise] to help you generate these. ChangeLog entries are not required for very, very trivial changes like typos. > * code reviews are a great opportunity for learning Not just code, but also how we've traditionally done things... > * 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 JDS 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. > * 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 Generally we're aiming to get the patch committed upstream *first* before we bring it back to jds-spec-files. > - 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 ] I wonder if a separate list is needed - could this be part of the eventual commits list, though I wonder if people will want to filter into separate folders. > * new contributors get commit access after their first approved > code change Sweet! :) > - 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 Do we really need this? What does it give us? > > - 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 That's impractical to ask that the person has to compile the code with both compilers. I assume it's 'ok' to build, test, commit, and then let the build machines catch any problems with the patch for other platforms/compilers? Do we intend to make the build mails public as well? Which alias? Otherwise looks good to me - Laca, feel like making a document out of it and posting to the JDS project website? Glynn
