Hi,
I noticed that many people are waiting for reviews/commits of their patches. This can be a frustrating experience, so please let me explain the situation.
Writing a patch is a _lot_ easier than reviewing it thoroughly. Even one-liners can have a profound unwanted impact on code flow which is not visible at a first glance. Reviewing a patch is time-consuming. I usually spend at least twice the time reviewing a patch compared to writing it myself. Writing a patch is also much more pleasant than reviewing it. Writing a patch is an act of creation whereas reviewing a patch is about finding flaws.
That said, reviewing a patch thoroughly (that also includes reading the code around it, the code it calls and the code it is called from) is a great way to start with development. If you want to get your hands dirty, I recommend looking for unapplied patches and reviewing them. Don't be discouraged if someone else posts a review in the meantime, your review is still valuable. If you find the same things another reviewer has mentioned, still write about them (you did find them as well, you deserve the credit). Write about everything you checked even if it is OK. That shows others what you reviewed and how you worked.
If you already know the codebase well, _please_ try to review unapplied patches.
Patch reviewers are essential for the success of our project because: - they keep buggy code out - they keep bad design out - they help patch submitters to improve their code - they ensure steadily increasing code quality - they show the patch submitter that someone cared about the patch.
If you have unapplied patches in your queue, please reply to them with a small note ("ping" etc.) so reviewers can find them and look at them.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
If you have unapplied patches in your queue, please reply to them with a small note ("ping" etc.) so reviewers can find them and look at them.
I would like to request a better patch management system than this mailing list. The fact the above "ping" is now a part of our development process is a very strong indication that things are not functioning very well.
It's easy to say "use a tracker" but that is not practical for some developers because it is centralized, and that doesn't work when you are not always online.
One idea is tracker email integration, but that isn't ideal either.
Another is to have a patch repository based on a distributed VCS.
How do other projects solve this problem? Experiences anyone?
//Peter
On Sat, May 30, 2009 at 9:01 PM, Peter Stuge peter@stuge.se wrote:
I would like to request a better patch management system than this mailing list. The fact the above "ping" is now a part of our development process is a very strong indication that things are not functioning very well.
it's a hard problem. I'm on several projects. They are all non-ideal in some way. Linux sucks in patches at the rate of 30,000 a year or so; that's fine performance but some feel (me included) that the kernel is "de-cohering": it no longer has the small tight feel and coherence of vision that it might have once had. Plan 9 still has the same tight feel and coherence but at a cost: important patches seem to linger on the vine for (i am not kidding here) years .
Coreboot is trickier than a kernel, as trivial errors can lead to systems that can not be recovered. I especially avoid acking flashrom patches because I can't test most of them. Others I know don't like to NAK, but they're not comfortable with an ACK either; they don't like the code but they don't want to hold up progress.
All in all, I think the process works. Yes, it is not ideal. Yes, it could be better, but so could everything.
ron
ron minnich wrote:
I would like to request a better patch management system than this mailing list.
it's a hard problem.
..
All in all, I think the process works.
I don't, when patches are untracked and linger for however long until the author or someone else sends a ping, at which point they may get some more attention, or will just continue to linger.
Especially when there is no feedback, there might then just as well be no community at all.
It also means that it is _impossible_ to get any kind of overview.
Yes, it is not ideal. Yes, it could be better, but so could everything.
Why settle just because it's hard? Then there would be no coreboot. :)
//Peter
On Sun, May 31, 2009 at 4:41 PM, Peter Stuge peter@stuge.se wrote:
ron minnich wrote:
I would like to request a better patch management system than this mailing list.
it's a hard problem.
..
All in all, I think the process works.
I don't, when patches are untracked and linger for however long until the author or someone else sends a ping, at which point they may get some more attention, or will just continue to linger.
all patches or some patches? Every time I svn, which is almost daily, there are changes.
Clearly, some patches are being reviewed and acked.
Especially when there is no feedback,
OK, I'm guilty here. I don't review every patch. Does anyone? Could anyone?
There is some bit of burden here on people who submit patches. A patch needs to be very clear as to: - what is the issue - how is the issue solved - why is it solved in this way
We do get patches from time to time that are confusing. I see it happen. We get a patch, for example, which solves a problem that nobody understood to be a problem, and that is not clear as to how it solves the problem. It is sometimes unclear that the patch is a good idea. At the same time, it's hard to take the time to write up just why (in my case) I'm not comfortable with or don't understand the patch.
No code review system, however automated, can fix that kind of problem.
Again, I see this problem on other projects. On these other projects I've seen: - patches that fix a design problem that people aren't willing to admit is a design problem - patches that fix an implementation problem -- same story, it's hard to get people to get it and at some point you give up - patches that add a very nice capability -- people argue about silly things and avoid the main point - patches that fix a problem -- badly -- such that nobody ever takes it in, and it dies a lingering, but well-deserved death.
I've seen all these scenarios on coreboot. I've submitted bad patches that got NAK'ed or died. I've submitted stuff I thought we ought to have, same experience. I've wrongly ACK'ed and NAK'ed patches.
It happens. We're fallible. But again, v2 rev level is 4224 or so, which means that in some way, the system worked at least 4000 times.
All that said, if somebody wants to try out and evangelize one of these improved systems, and it can improve our life, why not try it? Computers are there to remove drudgery from our lives and smooth our path for us -- let's use them :-)
thanks
ron
Am 31.05.2009 23:14, schrieb ron minnich:
No code review system, however automated, can fix that kind of problem.
No, but they give a good overview at outstanding reviews, so less stuff ends up on the floor.
Patrick
Good points Ron. I've also been on both ends of the spectrum.
Patrick Georgi wrote:
No code review system, however automated, can fix that kind of problem.
No, but they give a good overview at outstanding reviews, so less stuff ends up on the floor.
This elegantly summarizes the point I tried to make. I'll have a look at the links David posted. Thanks David!
//Peter
Peter Stuge schrieb:
This elegantly summarizes the point I tried to make. I'll have a look at the links David posted. Thanks David!
One thing I found lacking with all those tools so far was proper integration with a mail feed, which seems to be a hard requirement for coreboot.
It's not that hard to automatically fetch all attachments that look like patches from the mailing list and figure out which of our subprojects it belongs to. It's very hard to automatically figure out if a patch is committed, so the ticket in the patch queue / review tool could be closed.
That means, that we'd either have to force people to use the tool instead of just the mailing list (good luck with that), or we'd need to clean up the database of that tool manually (good luck with that, too).
The other missing feature in review board half a year ago, when I installed a test instance of it, was support for patch queues, eg. 6 successive patches that build on each other. That would require rather ugly workarounds. It might be supported by now.
Patrick
On Sun, May 31, 2009 at 05:14:37PM -0400, ron minnich wrote:
On Sun, May 31, 2009 at 4:41 PM, Peter Stuge peter@stuge.se wrote:
ron minnich wrote:
I would like to request a better patch management system than this mailing list.
it's a hard problem.
..
All in all, I think the process works.
I don't, when patches are untracked and linger for however long until the author or someone else sends a ping, at which point they may get some more attention, or will just continue to linger.
all patches or some patches? Every time I svn, which is almost daily, there are changes.
Clearly, some patches are being reviewed and acked.
Most of them, yes.
I personally think our review/commit process is working very well. Sure, sometimes patches take a little longer to review/test, but that's not a problem of the process itself, it's simply because we have a limited number of developers with a limited amount of time.
There is only one thing I suggest for improvement: if a patch doesn't reveice answers (review, test, ack, nack) after 2 weeks or so, the developer (or pretty much any user/developer on this list) should either
(a) Post a *ping* mail, maybe the patch was just not noticed or forgotten and will then be swiftly reviewed/committed.
(b) Add a trac issue, containing the URL to the mailing list post of the patch, so that it doesn't get forgotten.
Other than that, the process works just fine, and I strongly believe that adding _more_ (web-based or other) tools in the mix will make things worse, not better. We already have trac for issues, no need for more tools.
Uwe.
Uwe Hermann wrote:
I strongly believe that adding _more_ (web-based or other) tools in the mix will make things worse, not better.
That surprises me. I think helpful tools are always an improvement.
We already have trac for issues, no need for more tools.
Except we don't have Trac. Noone uses Trac, so noone else will use it either. It isn't used because it isn't helpful enough for us as a group.
This thread is about tools which actually would be helpful, not about adding $RANDOMSTUFF which doesn't help us. It seems a little harsh to rule out all tools after one has been evaluated. (Thanks Patrick!)
//Peter
On 01.06.2009 15:31 Uhr, Uwe Hermann wrote:
Most of them, yes.
I personally think our review/commit process is working very well. Sure, sometimes patches take a little longer to review/test, but that's not a problem of the process itself, it's simply because we have a limited number of developers with a limited amount of time.
There is only one thing I suggest for improvement: if a patch doesn't reveice answers (review, test, ack, nack) after 2 weeks or so, the developer (or pretty much any user/developer on this list) should either
(a) Post a *ping* mail, maybe the patch was just not noticed or forgotten and will then be swiftly reviewed/committed.
(b) Add a trac issue, containing the URL to the mailing list post of the patch, so that it doesn't get forgotten.
(c) If you think your patch is ignored / not handled properly, please do send a separate mail to rminnich@gmail.com and/or stepan@coresystems.de (maybe other core developers might want to offer the same?) discussing the issue. This can significantly speed up the process.
Other than that, the process works just fine, and I strongly believe that adding _more_ (web-based or other) tools in the mix will make things worse, not better. We already have trac for issues, no need for more tools.
If you have a soup, adding 10 knifes does not solve the problem. And it looks like there simply is no spoon for some problems.
Stefan
On Sun, May 31, 2009 at 10:56 AM, ron minnich rminnich@gmail.com wrote:
On Sat, May 30, 2009 at 9:01 PM, Peter Stuge peter@stuge.se wrote:
I would like to request a better patch management system than this mailing list. The fact the above "ping" is now a part of our development process is a very strong indication that things are not functioning very well.
it's a hard problem. I'm on several projects. They are all non-ideal in some way. Linux sucks in patches at the rate of 30,000 a year or so; that's fine performance but some feel (me included) that the kernel is "de-cohering": it no longer has the small tight feel and coherence of vision that it might have once had. Plan 9 still has the same tight feel and coherence but at a cost: important patches seem to linger on the vine for (i am not kidding here) years .
Coreboot is trickier than a kernel, as trivial errors can lead to systems that can not be recovered. I especially avoid acking flashrom patches because I can't test most of them. Others I know don't like to NAK, but they're not comfortable with an ACK either; they don't like the code but they don't want to hold up progress.
All in all, I think the process works. Yes, it is not ideal. Yes, it could be better, but so could everything.
ron
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Indeed, changing the whole process might not be worth the effort considering the trade-offs. There are some tools that can make the current process a lot less painful, however, specifically web-based code review dashboardshttp://ostatic.com/blog/open-source-code-review-tools. Review Board http://www.review-board.org/ from the folks at VMWare looks really good, and Rietveld http://code.google.com/p/rietveld/, which is a relatively new open-source fork of Google's Mondrianhttp://www.youtube.com/watch?v=sMql3Di4Kgc#t=25m20scode review tool, is quite helpful as well though it seems behind RB at the moment.
Am Sonntag, den 31.05.2009, 06:01 +0200 schrieb Peter Stuge:
[…]
How do other projects solve this problem? Experiences anyone?
I saw patchwork [1] on kernel.org [2]. Has this program been considered and does anyone have experience with this?
Bests,
Paul
[1] http://ozlabs.org/~jk/projects/patchwork/ [2] http://patchwork.kernel.org/
I've had really good luck with Git on other projects, but I haven't tried it with coreboot yet.
wt
On Sun, Jun 28, 2009 at 12:46 AM, Paul Menzelpaulepanter@users.sourceforge.net wrote:
Am Sonntag, den 31.05.2009, 06:01 +0200 schrieb Peter Stuge:
[…]
How do other projects solve this problem? Experiences anyone?
I saw patchwork [1] on kernel.org [2]. Has this program been considered and does anyone have experience with this?
Bests,
Paul
[1] http://ozlabs.org/~jk/projects/patchwork/ [2] http://patchwork.kernel.org/
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
On Sun, 28 Jun 2009 09:46:19 +0200, Paul Menzel paulepanter@users.sourceforge.net wrote:
Am Sonntag, den 31.05.2009, 06:01 +0200 schrieb Peter Stuge:
[…]
How do other projects solve this problem? Experiences anyone?
I saw patchwork [1] on kernel.org [2]. Has this program been considered and does anyone have experience with this?
Bests,
Paul
[1] http://ozlabs.org/~jk/projects/patchwork/ [2] http://patchwork.kernel.org/
Wow good find! I really like this, but will it work with Subversion?
Git-svn works pretty well as a two way mapper from svn<->git.
wt
On Sun, Jun 28, 2009 at 2:39 AM, Joseph Smithjoe@settoplinux.org wrote:
On Sun, 28 Jun 2009 09:46:19 +0200, Paul Menzel paulepanter@users.sourceforge.net wrote:
Am Sonntag, den 31.05.2009, 06:01 +0200 schrieb Peter Stuge:
[…]
How do other projects solve this problem? Experiences anyone?
I saw patchwork [1] on kernel.org [2]. Has this program been considered and does anyone have experience with this?
Bests,
Paul
[1] http://ozlabs.org/~jk/projects/patchwork/ [2] http://patchwork.kernel.org/
Wow good find! I really like this, but will it work with Subversion?
-- Thanks, Joseph Smith Set-Top-Linux www.settoplinux.org
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Paul Menzel wrote:
How do other projects solve this problem? Experiences anyone?
I saw patchwork [1] on kernel.org [2].
Great find!
Has this program been considered and does anyone have experience with this?
I don't think so. I would very much like to have this available for our list(s). I already have fastcgi running so if it seems like it would be a little messy on coreboot.org, I could host. Patrick?
Joseph Smith wrote:
I really like this, but will it work with Subversion?
Sure, it doesn't really do anything with the repo, it just collects patches from the mailing list.
//Peter
-
Carl-Daniel Hailfinger -
David Hendricks -
Joseph Smith -
Patrick Georgi -
Paul Menzel -
Peter Stuge -
ron minnich -
Stefan Reinauer -
Uwe Hermann -
Warren Turkal