Dear LinuxBIOS committers,
I activated a set of commit rules on the svn repository:
- commits require a Signed-Off-By: tag now.
Commits without Signed-Off-By: are no longer possible. The format is:
Signed-Off-By: Name email@address
You are not supposed to put your name in there except you explained on the mailing list beforehand why you are going to do so.
Note: If this is abused we will start checking the supplied email addresses (and possibly mails to the mailinglist ;-)
The goal is to get all of us to have your code reviewed properly.
- empty commit messages are not accepted anymore
any commit with an empty commit message or a commit message consisting of Signed-Off-By: only are blocked.
----- ;-) ----- I think about adding some metric for defining change log quality (ie. minimum of 2 lines description per kilobyte of patch committed) but I will let this idea settle until the next set of really bad commit messages comes up. ----- ;-) -----
Comments?
Stefan
I don't understand the process.
How does it work? Do I do svn commit, and someone else signs it off?
can you tell me more?
ron
* ron minnich rminnich@gmail.com [061021 00:41]:
I don't understand the process.
The goal of the process is to make changes trackable even after a longer period of time. Therefore there needs to be a connection between checkins and the discussion on the associated topics.
This is going to happen via the tracker eventually, as soon as all infrastructure is in place.
So each patch should "belong to" either a feature enhancement or a bug fix (whereas "cleanup" would count as the later). As in: No code goes in that does not serve a (documented) purpose. At some point when we are doing LinuxBIOS v4 or v5, we might even go as far as "no code goes in that has no automated test case that the code does what it is required to do"
And no code should go in that has not been reviewed. We do need to find a solution on review timeouts (code has to be reviewed within ... hours/days or it can go in without,.. thats what Yinghai did with the AM2 code while we were at the symposium)
Until the tracker side of things is up and running, we should send mails to the mailing list, as we've been doing recently, and have someone review it there. As we have been doing it already.
Note: This is not about restricting people to do their check ins. It is still possible to sign off your own patches and get them committed this way. But this should not be used to render the whole review and documentation tracability process useless.
If you add a new board and dont change any other code, nobody will care if you sign it off yourself, because without the hardware nobody will be able to give anything but well-intentioned hints.
How does it work? Do I do svn commit, and someone else signs it off?
For now:
- send a patch to the mailing list and/or ask someone to review it. - If you get the review within some amount of time, go ahead and check it in with the "foreign" signed-off-by - If you dont get the review, send another mail explaining why you check the code in anyways (ie. This is 3 lines of code adding a new flash chip to "flashrom", no review required) and commit the patch signed off by yourself.
For trivial stuff waiting for a review might not make sense or be necessary. For infrastructure changes and cross-board features however, we should take the time to do reviews.
Stefan
The goal of the process is to make changes trackable even after a longer period of time. Therefore there needs to be a connection between checkins and the discussion on the associated topics.
There's a mailing list archive, every patch gets sent to the mailing list for review --> it's trackable (although a pain to find the right threads sometimes, esp. if people break the ML threads).
As in: No code goes in that does not serve a (documented) purpose.
A good check in comment would state that purpose anyway.
At some point when we are doing LinuxBIOS v4 or v5, we might even go as far as "no code goes in that has no automated test case that the code does what it is required to do"
Good :-)
And no code should go in that has not been reviewed. We do need to find a solution on review timeouts (code has to be reviewed within ... hours/days or it can go in without,.. thats what Yinghai did with the AM2 code while we were at the symposium)
How about: only maintainers (of the code under discussion) can approve a patch (and they can approve their own). Patches should _still_ be sent to the mailing list of course (and in most cases it would be prudent to not check in before it has been out for review for a few days).
Until the tracker side of things is up and running, we should send mails to the mailing list, as we've been doing recently, and have someone review it there. As we have been doing it already.
Please even with a tracker keep it on the mailing list as well. Mail works offline, is distributed, has built-in redundancy -- all things that a centralised web-based thing does not.
Let's keep the whole "process" thing as flexible and unobtrusive as possible?
Segher
Hi,
On Sat, Oct 21, 2006 at 11:35:53AM +0200, Segher Boessenkool wrote:
There's a mailing list archive, every patch gets sent to the mailing list for review --> it's trackable (although a pain to find the right threads sometimes, esp. if people break the ML threads).
Yeah. I would say every patch _must_ be sent to the list for review, no exceptions.
Trivial patches (e.g. a new chip for flashrom) can then be checked in after a few hours or a day if nobody complains, but for more complex patches the review time should be longer.
I agree that all of the review and patch management should happen on the list. The issue tracker should only be used for tracking bugs (when a commit fixes a bug in the tracker, the commit message should say "Closes: #1234" or some such thing; Trac for example will then automatically link to the respective issue/bug).
The issue tracker can also be used for long-term issues (so we don't forget about such things), and maybe release planning/goals etc.
Another important thing for the issue tracker: _Users_ should be able to use it to submit bug reports, feature requests etc.
As in: No code goes in that does not serve a (documented) purpose.
A good check in comment would state that purpose anyway.
Definately. I like the idea to reject commits with empty log messages.
At some point when we are doing LinuxBIOS v4 or v5, we might even go as far as "no code goes in that has no automated test case that the code does what it is required to do"
Good :-)
Yeah. Not easy to do for such low-level software, but a good idea.
How about: only maintainers (of the code under discussion) can approve a patch (and they can approve their own). Patches should _still_ be sent to the mailing list of course (and in most cases it would be prudent to not check in before it has been out for review for a few days).
I don't think there are clear maintainers of subsystems (as in the Linux kernel). Neither is there a Benevolent Dictator (Linus) here. Both is a good thing, IMHO.
As for the sign-off mechanism: the two main points it should achieve, I think, is this:
1. Review of _all_ patches.
2. Tracking of who is the author and copyright owner of the code, and making sure that all contributed code is GPL'd.
After reading http://lxr.linux.no/source/Documentation/SubmittingPatches http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt I think the way the process works in the Linux kernel would be applicable to LinuxBIOS, too.
Here's my proposal:
* Every patch _must_ be sent to the list. In the body it should contain at least one 'Signed-off-by: John Doe foo@example.com' line.
The Signed-off-by means that the person listed there is the author (or co-author) and copyright owner of the code _and_ has made sure that it doesn't contain any non-GPL-compatible parts _and_ agrees to license the code under the GPL (version 2 or any later version).
All files in the patch _must_ contain the GPL header with exact 'Copyright (C) 2006 John Doe foo@example.com' lines. If someone want to license the code under a license different to the 'GPL (version 2 or any later version)' the mail should explicitly say so, _and_ the files in the patch should then have the license header of that other license (e.g. BSD license, GPLv2, etc.).
Of course, the license _must_ be GPL-compatible at least, or else the patch will be rejected.
* The patch is reviewed on the list. If someone modifies the code and posts a new version to the list, he adds himself to the list of Signed-off-by lines, e.g.
Signed-off-by: John Doe foo@example.com Signed-off-by: Jane Doe bar@example.com
* If a reviewer merely looked over the patch and thinks it's ok and should be applied, he replies to the mail with
Acked-by: John Reviewer baz@example.com
This only means he reviewed the patch, he does _not_ modify it, and thus is not an author/co-author, and thus doesn't own the copyright on the code.
* When the review process is finished and there's consensus that the patch is fine, it gets committed (usually by the person who wrote it, or by someone else if the person doesn't have an svn account).
The commit log then _must_ contain _all_ Signed-off-by and Acked-by lines the patch gathered during the review process. Also, it _must_ contain a good, descriptive explanation what the patch does, and why (but that should always be the case anyway). If it fixes a bug from the issue tracker, it should be referenced.
Example commit message:
Add support for component X of chipset XYZ (Closes: #1234).
Signed-off-by: John Doe foo@example.com Signed-off-by: Jane Doe bar@example.com Acked-by: John Reviewer baz@example.com
Optional:
* We _could_ introduce a rule that every (non-trivial) patch needs at least one Signed-off-by and at least one Acked-by line. This ensures that at least one other developer looked over the patch and didn't spot any glaring obvious breakage.
This can be enforced by an svn pre-commit-hook. For _trivial_ patches the author can then use
Signed-off-by: John Doe foo@example.com Acked-by: John Doe foo@example.com
to override the pre-commit-hook. This also allows easy searching for patches which probably got no review and for punishing the guilty ;-)
Please even with a tracker keep it on the mailing list as well. Mail works offline, is distributed, has built-in redundancy -- all things that a centralised web-based thing does not.
[...]
Let's keep the whole "process" thing as flexible and unobtrusive as possible?
ACK.
I think using the Linux kernel process is nice, because it's well-established and well-known, so contributors who know the kernel-process, feel right at home when contributing to LinuxBIOS.
Also, I suggest to always use the _same_ name, email (and case and spelling) for the Signed-off-by/Acked-by lines for consistency reasons. E.g. don't use "Ron G. Minnich", "ron minnich", "Ronald G. minnich", "ronald g. minnich" etc... Choose one and use it consistently.
Another suggestion for the pre-commit-hook:
* Make it check for the exact spelling of 'Signed-off-by:' (that's what the kernel uses (instead of Signed-Off-By:). * Make a Name and an email address required (already done?)
Uwe.
* Uwe Hermann uwe@hermann-uwe.de [061022 18:07]:
I agree that all of the review and patch management should happen on the list. The issue tracker should only be used for tracking bugs (when a commit fixes a bug in the tracker, the commit message should say "Closes: #1234" or some such thing; Trac for example will then automatically link to the respective issue/bug).
I think we can easily reach this. All tracker changes can be sent to the list, similar to what OLPC does.
I am all for putting all patches into the tracker. For several reasons:
- if I make a patch A and it is superseded by patch B, I need to be able to track this.
- Feature enhancements should be handled like bug fixes as well to make the change process simpler-
- Every patch therefore is a to-be-tracked bug/issue, and should go to the tracker.
If there's no bug, there's no need to change the code, right?
- If there is some issue that requires action on developer side, this issue has a distinct status. an issue can be resolved, or open, or in progress, etc. On a mailing list we dont have this status. I have issues in my inbox that reflect open threads, though being several weeks old. This is not going to change, but it should be handled properly. Just having such stuff in mails _does_ make important issues go down.
I want to point out Uwe's very useful mail of un-checked in patches on the mailing list. I guess some are still open, but I forgot.
Having them in an issue tracker at one point would make life a lot easier.
- Last but not least: That is how we've already been doing it since quite a while with the roundup issue tracker. But since roundup pretty much sucks people stopped using it.
The issue tracker can also be used for long-term issues (so we don't forget about such things), and maybe release planning/goals etc.
Yes, we do want milestones and handle them publically so people can plan on interactions to get those solved.
Another important thing for the issue tracker: _Users_ should be able to use it to submit bug reports, feature requests etc.
full ack. This is a more modal form of doing support.
As in: No code goes in that does not serve a (documented) purpose.
A good check in comment would state that purpose anyway.
this is the wrong direction. The reason has to be documented _before_ the checkin, not after the checkin. I agree though that we need better checkin comments.
How about: only maintainers (of the code under discussion) can approve a patch (and they can approve their own). Patches should _still_ be sent to the mailing list of course (and in most cases it would be prudent to not check in before it has been out for review for a few days).
I don't think there are clear maintainers of subsystems (as in the Linux kernel). Neither is there a Benevolent Dictator (Linus) here. Both is a good thing, IMHO.
currently we have a couple of people who are long-term contributors and looked at many different pieces of code to be able to judge whether code "fits in the structure" of LinuxBIOS, and there are some others that have 100% indepth hardware knowledge that is required to make things work. Those are arguing which things should go in until someone feels determined to check things in. So we dont have a voters system, but we dont really have a dictator system as well.
The grub project had this discussion recently, and Johan Rydberg had an interesting suggestion (which did not find too much positive feedback from the "dictators" though ;):
------- 8< ------- From: Johan Rydberg jrydberg@night.trouble.net To: grub-devel@gnu.org Subject: Let core developers "vote" to accept patches. [..] In other projects that I've worked on, we've used a system where the core developers can either give a +1 or a -1 on a contributed patch. If a patch receives two +1's or more, it is accepted (if there are no -1's) and committed by one of the developers.
This lowers the pressure on the maintainers, and hopefully will speed up the development.
Though, the core developers need to be a bit careful. They should not give +1's or -1's on a patch that they either (1) do not fully understand, or that (2) applies to a section of the code that they are not knowledgeable about. It is then better to either state that they do not know the area well enough, and maybe give the patch a +0, or be silent.
The downside is that some developers will never feel that they are knowledgeable enough to comment on a contribution; resulting in that the maintainer will have to do it. The same situation we are in now, more or less.
The upside is that the maintainers can focus on other things than just reviewing patches; maybe getting some new features in. Also, trivial and small patches will get quicker review and get merged into the tree in a swift manner.
Please note:
This is in no way an attempt to overrun any of our maintainers, it is just an attempt to make things go a bit smoother and faster in the times when the maintainers are busy with other things. [..] ~j ------- 8< -------
As for the sign-off mechanism: the two main points it should achieve, I think, is this:
- Review of _all_ patches.
ack!
- Tracking of who is the author and copyright owner of the code, and making sure that all contributed code is GPL'd.
ack!
If a reviewer merely looked over the patch and thinks it's ok and should be applied, he replies to the mail with
Acked-by: John Reviewer baz@example.com
This only means he reviewed the patch, he does _not_ modify it, and thus is not an author/co-author, and thus doesn't own the copyright on the code.
So in addition to one Signed-off-by: we would need at least one other Signed-off-by or at least one Acked-by?
When the review process is finished and there's consensus that the patch is fine, it gets committed (usually by the person who wrote it, or by someone else if the person doesn't have an svn account).
The commit log then _must_ contain _all_ Signed-off-by and Acked-by lines the patch gathered during the review process.
which argues for putting the patches into the tracker. grabbing this stuff together in the mailing list archives is fine for stuff that goes in after 1-4 days, but will be hell for bigger things..
Also, it _must_ contain a good, descriptive explanation what the patch does, and why (but that should always be the case anyway). If it fixes a bug from the issue tracker, it should be referenced.
This description should also go to the tracker, so it can be copied to the commit message from there. There should also be a link to the tracker. ie. This commit adds the feature described in isse #2342.
This is why we wanted a tracker with VC support after all, right?
Optional:
- We _could_ introduce a rule that every (non-trivial) patch needs at least one Signed-off-by and at least one Acked-by line. This ensures that at least one other developer looked over the patch and didn't spot any glaring obvious breakage.
Maybe a second signed-off-by should be handled like an acked-by, because we can assume that someone participating in a patch will acknowledge its inclusion in the tree.
Please even with a tracker keep it on the mailing list as well. Mail works offline, is distributed, has built-in redundancy -- all things that a centralised web-based thing does not.
[...]
Let's keep the whole "process" thing as flexible and unobtrusive as possible?
ACK.
It still needs to be a process though. We've had enough uber-hackers destroying the tree because they thought they just know better than all the rest of us. If people do not want to cooperate, they can do their own fork, right. So we might very well enforce cooperation in this project. ;-)
And a set of rules that apply to anyone is 100% to be preferred over the endless arguing about why one party insists on the other doing this and that even though its only overhead and it doesnt really matter because the tree compiles (meaning: for one single board).
I think using the Linux kernel process is nice, because it's well-established and well-known, so contributors who know the kernel-process, feel right at home when contributing to LinuxBIOS.
ack.
Also, I suggest to always use the _same_ name, email (and case and spelling) for the Signed-off-by/Acked-by lines for consistency reasons. E.g. don't use "Ron G. Minnich", "ron minnich", "Ronald G. minnich", "ronald g. minnich" etc... Choose one and use it consistently.
It should at least be the same email address.
Also: Who should be doing reviews? Only other committers? Or anyone?
Another suggestion for the pre-commit-hook:
- Make it check for the exact spelling of 'Signed-off-by:' (that's what the kernel uses (instead of Signed-Off-By:).
ok changed.
- Make a Name and an email address required (already done?)
currently only an email is required. any regex welcome ;)
Uwe.
Uwe Hermann http://www.hermann-uwe.de http://www.it-services-uh.de | http://www.crazy-hacks.org http://www.holsham-traders.de | http://www.unmaintained-free-software.org
-- linuxbios mailing list linuxbios@linuxbios.org http://www.openbios.org/mailman/listinfo/linuxbios
so, for my benefit, what does the process look like.
I make a change. I do this: svn diff and mail it to the list, with a signed-off-by: on it, and then what? how are you enforcing this approved-by sequence? I am getting a bit lost in the discussion, and I need to make some commits this week.
ron
* ron minnich rminnich@gmail.com [061023 02:29]:
so, for my benefit, what does the process look like.
I make a change. I do this: svn diff and mail it to the list, with a signed-off-by: on it, and then what?
Then people review it and send their Acked-by: to the list. If you have the first Acked-by: you check it in.
how are you enforcing this approved-by sequence? I am getting a bit lost in the discussion, and I need to make some commits this week.
svn commit hooks are scanning the commit messages and acting upon stuff that is there.
Stefan
On 10/23/06, Stefan Reinauer stepan@coresystems.de wrote:
- ron minnich rminnich@gmail.com [061023 02:29]:
so, for my benefit, what does the process look like.
I make a change. I do this: svn diff and mail it to the list, with a signed-off-by: on it, and then what?
Then people review it and send their Acked-by: to the list. If you have the first Acked-by: you check it in.
There is a slight complication with this. Not a really big deal but one everyone should remember. Svn diff does not show moves or renames. Only adds and modifies.
Those can only be done by someone with commit privlidges. So they will have to either describe what they want to do or create a patch with non svn methods.
Hi,
On Mon, Oct 23, 2006 at 08:30:10AM -0500, Richard Smith wrote:
There is a slight complication with this. Not a really big deal but one everyone should remember. Svn diff does not show moves or renames. Only adds and modifies.
Those can only be done by someone with commit privlidges. So they will have to either describe what they want to do or create a patch with non svn methods.
You can work around this with a small trick. Just do an 'svn add foo.c' or 'svn rm foo.c'(possible even without svn commit permissions). The 'svn diff' will then include the removed/added file in the patch.
Uwe.
Uwe Hermann wrote:
You can work around this with a small trick. Just do an 'svn add foo.c' or 'svn rm foo.c'?(possible even without svn commit permissions). The 'svn diff' will then include the removed/added file in the patch.
Noted.. That probably needs to be added somewhere in the developer docs or some sort of hacking howto. It caused me much confusion on the patches I got for the 440bx reorg.
BTW, YingHai, we should delete the dk8 that does not work, correct, the dk8-htx is the working version?
thanks
ron
YES. delete DK8HTX dir.
why Iwill instead of iwill dir. also the MB dir.?
all others are low case.
YH
On 10/23/06, ron minnich rminnich@gmail.com wrote:
BTW, YingHai, we should delete the dk8 that does not work, correct, the dk8-htx is the working version?
thanks
ron
-- linuxbios mailing list linuxbios@linuxbios.org http://www.openbios.org/mailman/listinfo/linuxbios
* yhlu yinghailu@gmail.com [061024 09:58]:
YES. delete DK8HTX dir.
why Iwill instead of iwill dir. also the MB dir.?
all others are low case.
good point. we should rename it.
Any objections anyone?
* Stefan Reinauer stepan@coresystems.de [061024 10:49]:
- yhlu yinghailu@gmail.com [061024 09:58]:
YES. delete DK8HTX dir.
why Iwill instead of iwill dir. also the MB dir.?
all others are low case.
good point. we should rename it.
done.
Hi,
On Tue, Oct 24, 2006 at 12:58:18AM -0700, yhlu wrote:
all others are low case.
Yes, they should be. There are some more instances where they aren't.
Attached patch fixes the superio/NSC -> superio/nsc instance.
It survives the abuild-test.
Signed-off-by: Uwe Hermann uwe@hermann-uwe.de
Uwe.
* Uwe Hermann uwe@hermann-uwe.de [061024 18:48]:
Hi,
On Tue, Oct 24, 2006 at 12:58:18AM -0700, yhlu wrote:
all others are low case.
Yes, they should be. There are some more instances where they aren't.
Attached patch fixes the superio/NSC -> superio/nsc instance.
It survives the abuild-test.
I assume it additionally needs an svn mv --force ...?
Signed-off-by: Uwe Hermann uwe@hermann-uwe.de
Acked-by: Stefan Reinauer stepan@coresystems.de
On Tue, Oct 24, 2006 at 07:05:05PM +0200, Stefan Reinauer wrote:
I assume it additionally needs an svn mv --force ...?
Probably, but I never used 'svn mv --force'. I was thinking of something like this:
patch < foo.patch svn ci cd src/superio svn mv NSC nsc svn ci
If the --force method works in one step (one commit) it's probably nice, too. However, the above method makes the changes really transparent and easy to understand to somebody reading the svn logs.
Uwe.
* Uwe Hermann uwe@hermann-uwe.de [061024 23:45]:
On Tue, Oct 24, 2006 at 07:05:05PM +0200, Stefan Reinauer wrote:
I assume it additionally needs an svn mv --force ...?
Probably, but I never used 'svn mv --force'. I was thinking of something like this:
patch < foo.patch svn ci cd src/superio svn mv NSC nsc svn ci
If the --force method works in one step (one commit) it's probably nice, too. However, the above method makes the changes really transparent and easy to understand to somebody reading the svn logs.
Yes, above is fine of course. Not sure if svn is handling the mv --force correctly . (which might be why you have to force it in the first place)
I activated a set of commit rules on the svn repository:
commits require a Signed-Off-By: tag now.
Commits without Signed-Off-By: are no longer possible. The format is:
Signed-Off-By: Name email@address
You are not supposed to put your name in there except you explained on the mailing list beforehand why you are going to do so.
Since this process (and its goal) is totally different from the Linux Signed-off-by, could you _please_ name it something else?
The goal is to get all of us to have your code reviewed properly.
Segher
Hi,
On Sat, Oct 21, 2006 at 11:25:19AM +0200, Segher Boessenkool wrote:
Since this process (and its goal) is totally different from the Linux Signed-off-by, could you _please_ name it something else?
Yeah, either that or use the kernel conventions (see my other mail). No reason to reinvent the wheel...
Uwe.
-
Richard Smith -
ron minnich -
Segher Boessenkool -
Stefan Reinauer -
Uwe Hermann -
yhlu