Hi!
The reason of why I write this to you is simple: my patches have stuck again and I don't know what to do. But since this happening not with me only and not this happens regularly, I have thought about looking at the problem a bit wider.
Is there any code review system for patches that are sent to wine-patches? Or just some developers monitor wine-patches and say something if they have something to say? Or, how does Alexander make decisions regarding committing patches?
I've been thinking about some kind of web-based system, like one used at http://codereview.chromium.org/
It would be nice, if there was sime kind of bot, that would grab all patches from wine-patches, sort them, make some initial analysis (maybe, run local test git repository, make various tests, check that threre are no conflicts, check code style (at least whitespace changes) etc). Then it could set a flag and report back to the sender. For instance it would flag "tests fail" and send details back to sender. Or "whitespace change detected" and send back the quoted diff parts.
Then, somebody of developers might review it and put his mark/comments that would go to wine-devel (if necessary), or to sender directly.
There could be simple templates for test reviews for common problems, and tags, such as
"sure?" that means, that more explanations are needed why that implementation is correct. "too hackish" that means that style or code itself is of bad quality. "whitespace" whitespacechange detected. "tests required" tests are really required for this patch to go in. "okay", means that reviewer recommends the patch for commitment. "wrong", means that patch is totally wrong ..etc
Some of these tags mean that report goes directly to developer, and patch is marked as obsolete, others make system send some feedback to wine-dev.
It would take just few clicks for reviewer to get it done. This would provide good feedback, also this would help the one who makes the decisions to see that others have already checked that patch and what they think about that.
Sometimes "no feedback" means that you just dont know - is there something wrong? Or maybe your patch wos not understood correctly? Or there are doubts that what you did is right? Or what? It would be better to have feedback like "are you serious?" than nothing at all :)
P.S: the patches in question are http://www.winehq.org/pipermail/wine-patches/2009-April/072009.html and http://www.winehq.org/pipermail/wine-patches/2009-April/072013.html . They both are soo simple, that I even don't know how to ask of what's wrong with them :)
On Tue, Apr 21, 2009 at 2:44 AM, Igor Tarasov tarasov.igor@gmail.com wrote:
Hi!
The reason of why I write this to you is simple: my patches have stuck again and I don't know what to do. But since this happening not with me only and not this happens regularly, I have thought about looking at the problem a bit wider.
Is there any code review system for patches that are sent to wine-patches? Or just some developers monitor wine-patches and say something if they have something to say? Or, how does Alexander make decisions regarding committing patches?
It's an open list, and many times, developers do comment. But if the patch is not so obviously wrong, or no one knows that code area well, it may not get a reply, even if correct.
I've been thinking about some kind of web-based system, like one used at http://codereview.chromium.org/
It would be nice, if there was sime kind of bot, that would grab all patches from wine-patches, sort them, make some initial analysis (maybe, run local test git repository, make various tests, check that threre are no conflicts, check code style (at least whitespace changes) etc). Then it could set a flag and report back to the sender. For instance it would flag "tests fail" and send details back to sender. Or "whitespace change detected" and send back the quoted diff parts.
We had this for a while. Dan Kegel wrote patchwatcher (http://wiki.winehq.org/PatchWatcher) precisely for this. It's been offline for a while since he's been remodeling his house. If you've got a spare machine and a bit of perl knowledge, should only be a few minutes to set up.
Then, somebody of developers might review it and put his mark/comments that would go to wine-devel (if necessary), or to sender directly.
Patchwatcher does this.
There could be simple templates for test reviews for common problems, and tags, such as
"sure?" that means, that more explanations are needed why that implementation is correct. "too hackish" that means that style or code itself is of bad quality. "whitespace" whitespacechange detected. "tests required" tests are really required for this patch to go in. "okay", means that reviewer recommends the patch for commitment. "wrong", means that patch is totally wrong ..etc
Some of these tags mean that report goes directly to developer, and patch is marked as obsolete, others make system send some feedback to wine-dev.
It would take just few clicks for reviewer to get it done. This would provide good feedback, also this would help the one who makes the decisions to see that others have already checked that patch and what they think about that.
Sometimes "no feedback" means that you just dont know - is there something wrong? Or maybe your patch wos not understood correctly? Or there are doubts that what you did is right? Or what? It would be better to have feedback like "are you serious?" than nothing at all :)
P.S: the patches in question are http://www.winehq.org/pipermail/wine-patches/2009-April/072009.html and http://www.winehq.org/pipermail/wine-patches/2009-April/072013.html . They both are soo simple, that I even don't know how to ask of what's wrong with them :)
-- Igor
Sometimes "no feedback" means that you just dont know - is there something wrong? Or maybe your patch wos not understood correctly? Or there are doubts that what you did is right? Or what? It would be better to have feedback like "are you serious?" than nothing at all :)
I've tried to update the 'why was my patch ignored' section in the Wiki: http://wiki.winehq.org/DeveloperFaq#head-1e5a50563dbdb221702cbde246b23f5de08... to reflect what I understand from Alexandre.
Not sure if any of it applies in your case, but I thought it'd be good to have it in the mailing list archives.
Cheers,
Jeremy
2009/4/21 Jeremy White jwhite@winehq.org:
Sometimes "no feedback" means that you just dont know - is there something wrong? Or maybe your patch wos not understood correctly? Or there are doubts that what you did is right? Or what? It would be better to have feedback like "are you serious?" than nothing at all :)
I've tried to update the 'why was my patch ignored' section in the Wiki: http://wiki.winehq.org/DeveloperFaq#head-1e5a50563dbdb221702cbde246b23f5de08... to reflect what I understand from Alexandre.
Not sure if any of it applies in your case, but I thought it'd be good to have it in the mailing list archives.
I found http://wiki.jswindle.com/index.php/Coding_Hints:Part_1 that may be useful as a checklist.
I was trying to find the interview with Alexandre where he lists 10 things that cause a patch to be rejected (e.g. using C++ comments). Does anyone know the link for this?
- Reece
Reece Dunn wrote:
2009/4/21 Jeremy White jwhite@winehq.org:
Sometimes "no feedback" means that you just dont know - is there something wrong? Or maybe your patch wos not understood correctly? Or there are doubts that what you did is right? Or what? It would be better to have feedback like "are you serious?" than nothing at all :)
I've tried to update the 'why was my patch ignored' section in the Wiki: http://wiki.winehq.org/DeveloperFaq#head-1e5a50563dbdb221702cbde246b23f5de08... to reflect what I understand from Alexandre.
Not sure if any of it applies in your case, but I thought it'd be good to have it in the mailing list archives.
I found http://wiki.jswindle.com/index.php/Coding_Hints:Part_1 that may be useful as a checklist.
I was trying to find the interview with Alexandre where he lists 10 things that cause a patch to be rejected (e.g. using C++ comments). Does anyone know the link for this?
- Reece
Do you mean:
http://winehq.org/wwn/353#News:%20WineConf%202008
(It's not 10 reasons though).
2009/4/21 Paul Vriens paul.vriens.wine@gmail.com:
The best point: "Are the planets aligned in just the right way and none of the above reject? Yes : ACCEPT". I guess there were some problems with this point yesterday :)
2009/4/21 Paul Vriens paul.vriens.wine@gmail.com:
Reece Dunn wrote:
2009/4/21 Jeremy White jwhite@winehq.org:
Sometimes "no feedback" means that you just dont know - is there something wrong? Or maybe your patch wos not understood correctly? Or there are doubts that what you did is right? Or what? It would be better to have feedback like "are you serious?" than nothing at all :)
I've tried to update the 'why was my patch ignored' section in the Wiki:
http://wiki.winehq.org/DeveloperFaq#head-1e5a50563dbdb221702cbde246b23f5de08... to reflect what I understand from Alexandre.
Not sure if any of it applies in your case, but I thought it'd be good to have it in the mailing list archives.
I found http://wiki.jswindle.com/index.php/Coding_Hints:Part_1 that may be useful as a checklist.
I was trying to find the interview with Alexandre where he lists 10 things that cause a patch to be rejected (e.g. using C++ comments). Does anyone know the link for this?
- Reece
Do you mean:
http://winehq.org/wwn/353#News:%20WineConf%202008
(It's not 10 reasons though).
Yes, that's what I meant.
- Reece
-
Austin English -
Igor Tarasov -
Jeremy White -
Paul Vriens -
Reece Dunn