[flashrom] New development guidelines after 0.9.7

[Carl-Daniel Hailfinger]

Am 18.07.2013 03:21 schrieb Stefan Tauner:
> Our current development model with elaborate reviews has not been
> working for a while now. Therefore, beginning with the release of
> flashrom 0.9.7 our strategy on merging patches will change radically.
>
> The current rules regarding committing to subversion can be found here:
> http://flashrom.org/Development_Guidelines#Committing
> Essentially they prohibit merging non-trivial patches without explicit
> consent from another community member and is not too specific on the
> requirements for an ack.
>
> Below is my proposal for the new rules which works with implicit
> consent after timeouts. The exact time values are completely open for
> discussion of course.
>
> I would also like to introduce some kind of stable branch where we can
> commit important backported patches, if need be, without releasing all
> non-critical fixes committed in the meantime (as was the case with
> 0.9.6.1 for example). I would not have a problem doing this with git,
> but I am not so sure if I want to try that with svn... let's pretend
> there is a community consensus to continue with svn for now.
> Is that sensible to do? AFAIK this would produce odd revision numbers
> (which is one of the main arguments pro svn).

svn branch management is actually quite simple, and revision numbers are
still linear (though with gaps in each branch).

Quite a few projects have a different version number scheme for stable
and development branches to make differentiating easier. The most
frequently used approaches are:

Odd/even release version number (variant 1): See Linux 2.4.x (stable
series), 2.5.x (unstable series), 2.6.x (stable series). IMHO not really
suitable for us because we don't envision a few years of unstable
development before making all new features available in a new stable
release.
Odd/even release version number (variant 2): 1.0.[01234] (stable
versions), 1.1 (unstable version, no explicit numbering), 1.2.[01234]
(stable versions). Drawback: There's no intrinsic property of odd
release numbers which tells people to avoid them. We might have lots of
people trying to use the latest major release even if it is an unstable
release. May or may not be fixable with lots of documentation telling
people to avoid odd-numbered releases.
Special version marker for unstable series (variant 3): 1.0.[01234]
(stable versions), 1.0.99 (unstable development branch), 1.1.[01234]
(stable versions). Suffers from the same drawback as variant 2, but
maybe to a lesser degree.
Special version marker for unstable series (variant 4): 1.0.[01234]
(stable versions), 1.1-pre (unstable development branch), 1.1.[01234]
(stable versions). Has non-numeric versions for the development branch,
might confuse users. Was used in earlier Linux days for development
activity in a stable series.


> I would also like to clean up the commit permissions and name the ones
> with commit rights in the wiki.

Listing the active committers in the wiki is an excellent idea from a
publicity perspective, but will it make breakins easier for attackers?
There's always svn log to find out the committers, so the security
argument is probably moot.


> IMHO the following ppl should have commit rights:
> carldani, stefanct, dhendrix, kmalkki, idwer

Yes. I'd also like to have ruik for IMC stuff, and xvilka for reverse
engineering stuff, and Mathias Krause and Nico Huber had some good
patches in the past as well as some still review-pending patches.


> I am not sure about uwe and twice11, because they are (sadly) not even
> active on IRC or the ml.

>From a patch-writing perspective, uwe has been inactive about a year
longer than twice11. That said, I wouldn't disable their accounts just
yet. It feels wrong to do that.

There's also stepan and oxygene. Hm. Neither of them are a security risk
(it's stepan's server and oxygene is/was doing some maintenance/admin
stuff on the server as well).


> Everyone else's permission should be revoked.

Yes.


> = Reviews =
>
> Contributors without commit rights should receive at least a preliminary review within one week of submission by some committer.
> At minimum this should include a broad indication of acceptance or rejection of...
> * the idea/rationale/motivation,
> * the implementation
> respectively.
>
> In general, reviews should focus on the architectural changes and things that affect flashrom as a whole.
> This includes (but is by no means limited to) changes in APIs and types, safety, portability, extensibility, and maintainability.
> The purpose of reviews is not to create perfect patches, but to steer development in the right direction and produce consensus within the community.
> The goal of each patch should be to improve the state of the project - it does not need to fix all problems of the respective field perfectly.

Do we also want a no-regression policy or is a patch allowed to break
some things to improve other things as long as a fixing followup patch
is in the queue?


> NB: New contributors may need more detailed advices and should be told about minor issues like formatting problems more precisely.
> The result of a review should either be a 
>
> == Acked-by ==
>
> * Trivial stuff like...
> ** compile fixes (that are tested on all or at least some affected and some ''unaffected'' platforms),
> ** marking boards/chips/programmers as tested, and
> ** typo and whitespace fixes etc.
> :do not need an Acked-by line from someone else than the author.
> * If the patch is something like a board enable, new chip(set) support etc. (i.e. no refactoring, no architectural impact), you can get an ack from someone who is not an experienced developer after he/she has tested the patch, or ack yourself if proof exists that it worked.

Even if the new code is copy-pasted instead of adding a new switch/ID to
existing code?


> * However, for more complicated patches you should get a review from someone who knows the code well if possible.
> ** If no one replies to the patch submission on the mailing list within 2 weeks, you may assume consent by the community and ack yourself.

While I'm not totally happy about this, it's the only way to remove the
no-review roadblocks. And with a separate development branch, we can
always have a stabilization phase shortly before a release where we
tighten the rules a bit.


> ** If, however, one of the known community members comments on your submission and requests changes or further information/a rationale etc., you need to come to some consensus with that member about the points complained about (only).
> *** If your answer (e.g. in form of a new iteration of the patch including the requested changes or a mail containing the requested information) does not get any replies for three weeks, you may ack yourself.
> *** When consensus in all points have been reached, but no explicit ack has been sent, then the submission falls under the same timeout rule again; i.e. if no one replies to your last e-mail within 2 weeks, you may ack yourself.
> * Contributions from people without commit rights should not be treated much differently.
> : Maintainers should merge them if there is no objection from the community within the timeframes and according to the rule set described above.
>
> = Committing =
>
> Those with commit rights to the (currently subversion-based) source repository should follow the following rules.
>
> * Reviews
> ** Changes should generally be discussed and reviewed before committing, see [[Development_Guidelines#Reviews|above]]
> * The commit log
> ** Should start with a short sentence summing up the changes of the patch.
> ** Optionally this should be prefixed with a topic or file name if the changes are targeting one area only.
> ** End the sentence with a full stop followed by an empty line.
> ** The body of the commit log should be short, but descriptive: If anyone involved in flashrom reads your comment in a year, she/he shall still be able to understand what your commit is about, without analyzing the code (what was the motivation for the change? what are the differences to the previous behavior?).
> ** At the end it has to list Signed-off-by lines of all authors and at least one Acked-by line.
> ** Optionally one can add Tested-by lines to give credit to testers.
> * Committing
> ** If you ack something and the sender has commit rights, let him commit unless he asks you to commit.
> *: If the sender does not have commit rights, wait at least 24 hours after sending the ack to give him/her an opportunity to add final refinements or comments.

A notice to the sender "acked-by <me>, will commit within 24 hours
unless you want to change or cancel the patch" would be nice.

> ** After committing please reply to the mailing thread that contains the patch with at least the revision in the body to notify anyone involved of the commit and document it for references in the future and in patchwork or similar facilities.
> * Don't share your login(s)/password(s) (should be obvious, but sadly it is not for everyone :)

An excellent summary of a hopefully better way to manage flashrom
contributions in the future!

Thanks a lot for your work!

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/