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).
I would also like to clean up the commit permissions and name the ones with commit rights in the wiki. IMHO the following ppl should have commit rights: carldani, stefanct, dhendrix, kmalkki, idwer I am not sure about uwe and twice11, because they are (sadly) not even active on IRC or the ml. Everyone else's permission should be revoked.
= 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. 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. * 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. ** 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. ** 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 :)
On Wed, Jul 17, 2013 at 7:21 PM, Stefan Tauner < stefan.tauner@student.tuwien.ac.at> wrote:
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.
Not to add undue overhead to the project, but why not go to git and gerrit? Automates a lot of this stuff and is already setup on coreboot.org.
Marc
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
On Fri, 19 Jul 2013 00:30:52 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 18.07.2013 03:21 schrieb Stefan Tauner:
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).
that's what I would call odd. it is still better comprehensible than hash numbers, but I already dislike the missing revisions for tags and not knowing which branch a commit belongs to makes those numbers pretty useless to me.
Quite a few projects have a different version number scheme for stable and development branches to make differentiating easier. […]
Why would we care? I don't plan to release anything else than stable releases. I should have explained that in the mail and it should probably end up in the wiki.
My suggestion would be to not change the release management too much. The only difference actually is that I would like to extend the stabilization phase without prohibiting merging new patches. How would that work? By branching at the beginning of the stabilization phase (which is quite similar to what we did with 0.9.5 and 0.9.6 by accident more or less :) - this should probably be tagged as RC.
Example: We will eventually tag 0.9.7 and start to merge patches according to the new rules. Every merged patch is also a candidate for the stable branch (on top of 0.9.7 to end up in a 0.9.7.1 release if need be). I have not thought about a process for that, but as conservative as we are it is rather obvious what we would want to merge there... fixes for regressions of earlier stable releases for example. I don't think we need extra rules for that and there will be enough time to discuss such patches (i.e. no timeout rule needed).
The decision about when to release stable updates can probably be reached together with the discussion of which individual patches to merge with stable. If we really need a written rule I would suggest tagging a release after each patch merged to the stable branch (maybe with a cool-down phase of 24 hours).
Then after a while (e.g. the usual 6-12 months) we will want to do a new release. We create a new branch from the current dev branch and tag it as 0.9.8-rc1. We continue to merge patches according to the rules to the dev branch, but merge them on top of the rc too if need be. If need be we tag more rcs on the rc branch and eventually the new stable release.
Rinse, repeat. No obvious semantic changes to release numbering.
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.
Security through obscurity and that from you? I take that as a joke. BTW how about signed commits and transfers via ssh? :P Really this shouldn't be about security.
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 see commit rights to be equivalent to having some kind of maintainer status. That requires all these points: - must be active on the ml or IRC on a regular basis (and by active I dont mean just being subscribed or lurking in the channel :) - must know at least parts of the code very well - must contribute to patch discussions/reviews on a regular basis
Contributing patches alone is not enough for that or not even required actually. Nico's patches are good no question, but that's all I have seen from him. Similarly I would not deem Mathias an active member of the community.
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.
As said, patch writing is not so important to me.
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).
Every unused login is a security risk. But that's not what I am thinking about. Those with commit rights should be the active members, that do the reviews, the merging and the ones that can be mailed, asked and nagged about things, i.e. it is a responsibility too. That's also why I would like to mention them in the wiki. What should a contributor do if no one reviews his patch in time to get his patch in? He should nag someone with commit rights and hence this should be obvious. And it is not obvious atm... I have no idea who (still) has commit rights. Everyone who ever did a commit?
= 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?
I am not sure I want to have too many rules. The reason I put that sentence in was to remind everyone that perfect patches are hard and take a long time (and still have bugs).
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?
Ask the original author, I just rephrased that point a bit :P No rule can make common sense obsolete... if copying code is the saner choice in some situations then it is the right thing to do. And after all the refactoring I have done I think I have the right to say that :)
- 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.
No one is happy about the current situation either. I hope the release management stuff is clear now...
- 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.
This should be the semantics the receiver should read when getting an ack. But it certainly does not hurt if we write that in the ack mail, yes.
I hope everything is somewhat clear... 3am, gn8
I have now added the guidelines to the respective wiki page: http://flashrom.org/Development_Guidelines
I think it is a bit sad that almost all discussion in this thread is focused on the VCS to use and not about the guidelines. Apparently emotions regarding the tools are much more intense than about the created work.
On Fri, Jul 19, 2013 at 4:31 AM, Stefan Tauner < stefan.tauner@student.tuwien.ac.at> wrote:
I think it is a bit sad that almost all discussion in this thread is focused on the VCS to use and not about the guidelines. Apparently emotions regarding the tools are much more intense than about the created work.
Maybe it's a sign?
(Hint: re-read *all* the stuff you wrote, and then reconsider what Marc said about automation)
-
Carl-Daniel Hailfinger -
David Hendricks -
Marc Jones -
Stefan Tauner