What about reviewing patches?
All this rant about this and that and noone reviews patches. Shame on you.
On Wed, Aug 13, 2008 at 5:07 PM, Karoly Negyesi wrote:
All this rant about this and that and noone reviews patches. Shame on you.
chx is right, and the patch review queue gets longer and longer. There's over 320 just waiting to be reviewed against D7 [1] and over 200 against D6 [2], and that's just for core. http://drupal.org/patch/review has just had a big overhaul, so if you've not done patch reviews before, start there, or jump into #drupal on irc and find many, many people willing to help get you started. [1] http://drupal.org/project/issues?projects=3060&states=8&versions=156281 [2] http://drupal.org/project/issues?projects=3060&versions=97368,280583
Op 13 aug 2008, om 18:07 heeft Karoly Negyesi het volgende geschreven:
All this rant about this and that and noone reviews patches. Shame on you.
Why? Reviewing patches would be much more fun if things would be committed sooner. Patches stay in their current mode for days/weeks and even months untill they got committed. I was one of the people who extensively tested and reviewed patches in the past, but completely lost my interest because of this. I'm sorry Karoly, this is not against you in person but that's how I and other people feel about it. Stefan
Stefan Nagtegaal wrote:
Reviewing patches would be much more fun if things would be committed sooner. Patches stay in their current mode for days/weeks and even months untill they got committed. On that note, why is there still no Drupal 7 branch maintainer? http://drupal.org/node/254640 has been postponed for months now, with no further explanation/discussion. A Drupal 7 maintainer would help eliminate that bottleneck in the patch cycle.
Why? Reviewing patches would be much more fun if things would be committed sooner. Patches stay in their current mode for days/weeks and even months untill they got committed.
This seems like its becoming even more of an issue as the project keeps growing. Its obviously a very major bottleneck in the developer workflow (some could argue its a partially intentional choice) but it does seem like its some discussion about possible ways of improving this situation would be helpful. Maybe speed of commitments isn't the key problem, but avoiding loosing tester's motivations is something to consider. Or tackle the committers speed if that is the underlying problem. Ideas?
Ryan Cross wrote:
Maybe speed of commitments isn't the key problem, but avoiding loosing tester's motivations is something to consider. Or tackle the committers speed if that is the underlying problem. Ideas?
Mhh.. what about (auto-)committing to a D7.x-next branch after two independent people have confirmed a patch as working? That would give the process of reviewing patches more resoluteness, also liberating the core CVS admins to decide what gets in and what not. Patches that don't proof as a problem in 7.x-next could then be cherry-picked to 7.x-dev... Of course this would make running the next-branch a risky business, but at least it helps getting out of the situation where perfectly good and simple patches are not applied for weeks and months because the concerning code is being completly rewritten on some other issue (module system revamp f.e.)... regards_marcel. -- "Obstacles are those frightful things you see when you take your eyes off your goal." -- Henry Ford (1863-1947) Change the world! Vote revolution: http://hfopi.org/vote-future
Mhh.. what about (auto-)committing to a D7.x-next branch after two independent people have confirmed a patch as working? That would give the process of reviewing patches more resoluteness, also liberating the core CVS admins to decide what gets in and what not. Patches that don't proof as a problem in 7.x-next could then be cherry-picked to 7.x-dev...
No-one runs 7.x, it's fairly unstable and isn't supported for either upgrades or security releases - and that's the case right up until release candidate, so I'm not sure what you're suggesting here. While commits are a bottleneck at the moment. Reviews are a bottleneck /all the time/. Nat PS. 10/1 odds on that more people reply to this thread than review patches over the next couple of days. I'd love to be wrong of course.
Marcel Partap wrote:
Ryan Cross wrote:
Maybe speed of commitments isn't the key problem, but avoiding loosing tester's motivations is something to consider. Or tackle the committers speed if that is the underlying problem. Ideas?
Mhh.. what about (auto-)committing to a D7.x-next branch after two independent people have confirmed a patch as working? That would give the process of reviewing patches more resoluteness, also liberating the core CVS admins to decide what gets in and what not. Patches that don't proof as a problem in 7.x-next could then be cherry-picked to 7.x-dev... Of course this would make running the next-branch a risky business, but at least it helps getting out of the situation where perfectly good and simple patches are not applied for weeks and months because the concerning code is being completly rewritten on some other issue (module system revamp f.e.)... regards_marcel.
Dear sweet Lord, NO! :) It's imperative that HEAD remain stable at all times, and that automated tests continue to pass. There are a lot of seemingly "simple" patches that have lots of subtle ways that they break things if you're not careful. If we get into the situation where HEAD is broken, development effectively stops until things are working again. It also makes a BIG difference who those two people are. Follow catch's advice. Catch is wise. The best shot you have of getting patches that you want in core is to become one of those rare patch reviewing ninjas like him. Then your voice being one of those two voices may very well be enough. -Angie
Angela Byron wrote: > careful. If we get into the situation where HEAD is broken, development > effectively stops until things are working again. Nooooooo, 'cause not HEAD ;X - 'next' or 'experimental' branch! > It also makes a BIG difference who those two people are. Follow catch's > advice. Catch is wise. The best shot you have of getting patches that > you want in core is to become one of those rare patch reviewing ninjas > like him. Then your voice being one of those two voices may very well be > enough. i'd love too but my excuse (as so many others') shall be: no real spare time atm. Also, of course i mostly care about bugs i run into myself, and i rarely run into any. Some people seem to report bugs using really obscure setups plus outdated modules... -- "Obstacles are those frightful things you see when you take your eyes off your goal." -- Henry Ford (1863-1947) Change the world! Vote revolution: http://hfopi.org/vote-future
careful. If we get into the situation where HEAD is broken, development effectively stops until things are working again.
Nooooooo, 'cause not HEAD ;X - 'next' or 'experimental' branch!
Drupal 7 is the 'next' or 'experimental' branch. It's also likely to have anything up to 1,000 people working on it (and at least about 50 at any one time) - so when it breaks, those people can't work.
i'd love too but my excuse (as so many others') shall be: no real spare time atm.
Some patch reviews take less than 30 seconds, especially when the issue queue is long and has lots of stale patches in it. You've already spent more than 30 seconds replying to this thread ;)
Also, of course i mostly care about bugs i run into myself, and i rarely run into any.
If you review some patches, the next time you report a bug, it's likely others will pay more attention to it. Nat
On Wed, Aug 13, 2008 at 12:52 PM, Nathaniel Catchpole <catch56@googlemail.com> wrote:
Some patch reviews take less than 30 seconds, especially when the issue queue is long and has lots of stale patches in it.
I'm not sure I follow. I review patches when I get the time, but I figure on at least 20 minutes -- read the issue, check out HEAD, apply the patch, create a new database, install the test site, download/install devel, generate stuff, see if the patch works as advertised, post my review. If I have a recent enough HEAD install I figure I can skip the database bit and just use my existing database, but it's still a decent chunk of time. What am I doing wrong? -- Katherine Senzee (ksenzee) esquaredworkshops.com
On Wed, Aug 13, 2008 at 9:08 PM, Katherine Senzee wrote:
On Wed, Aug 13, 2008 at 12:52 PM, Nathaniel Catchpole wrote:
Some patch reviews take less than 30 seconds, especially when the issue queue is long and has lots of stale patches in it.
I'm not sure I follow. I review patches when I get the time, but I figure on at least 20 minutes -- read the issue, check out HEAD, apply the patch, create a new database, install the test site, download/install devel, generate stuff, see if the patch works as advertised, post my review. If I have a recent enough HEAD install I figure I can skip the database bit and just use my existing database, but it's still a decent chunk of time. What am I doing wrong?
20 minutes is about right to do a proper review, but a lot of patches don't even apply any more, including RTBC ones. So if you already have HEAD installed (I pretty much only clean my database before or after reviewing a patch with an upgrade path or schema change), you can do this: // clean your HEAD of old changes. cvs -q diff | patch -p0 -R wget http://drupal.org/files/issues/somepatch.patch patch -p0 < somepatch.patch 1 out of 1 hunks failed etc. Mark to code needs work with 'needs a re-roll'. Maybe 45 seconds then, but not too much more than that. Also simpletest is a much, much easier and more comprehensive way to avoid regressions that installing devel, generating content and clicking around. So that stage is reduced to: admin/build/modules enable simpletest admin/build/testing run all tests Although you can go make a cup of tea while the tests run, there's little effort involved running them. You should obviously test the actual functionality of the patch on top of this, but that's a bit less RSI inducing. Nat
On Wednesday 13 August 2008, Nathaniel Catchpole wrote:
So if you already have HEAD installed (I pretty much only clean my database before or after reviewing a patch with an upgrade path or schema change), you can do this:
// clean your HEAD of old changes. cvs -q diff | patch -p0 -R
wget http://drupal.org/files/issues/somepatch.patch
patch -p0 < somepatch.patch
1 out of 1 hunks failed etc.
Mark to code needs work with 'needs a re-roll'.
Can't this process be automated? I think I remember something about a process that scans for stale patches - does it still exist?. The close-fixed-after-two-weeks bot seems to be doing a very nice work! -- Yuval Hager [@] yuval@avramzon.net
There's a patch against the coder module waiting to be reviewed (I know it's not core), but this patch will add functionality to coder which will allow you to run a code review on patches. You can paste in the patch, or just provide the URL to it. It might help speed up the process slightly. Cheers, Stella On Wed, Aug 13, 2008 at 10:55 PM, Yuval Hager <yuval@avramzon.net> wrote:
On Wednesday 13 August 2008, Nathaniel Catchpole wrote:
So if you already have HEAD installed (I pretty much only clean my database before or after reviewing a patch with an upgrade path or schema change), you can do this:
// clean your HEAD of old changes. cvs -q diff | patch -p0 -R
wget http://drupal.org/files/issues/somepatch.patch
patch -p0 < somepatch.patch
1 out of 1 hunks failed etc.
Mark to code needs work with 'needs a re-roll'.
Can't this process be automated? I think I remember something about a process that scans for stale patches - does it still exist?. The close-fixed-after-two-weeks bot seems to be doing a very nice work!
-- Yuval Hager [@] yuval@avramzon.net
Quoting Stella Power <stella@stellapower.net>:
There's a patch against the coder module waiting to be reviewed (I know it's not core), but this patch will add functionality to coder which will allow you to run a code review on patches. You can paste in the patch, or just provide the URL to it. It might help speed up the process slightly.
Then a project infrastructure process could just automate a run of the patch against coder and give a report (or a flag) in the issue. Earnie -- http://for-my-kids.com/ -- http://give-me-an-offer.com/
On Thu, Aug 14, 2008 at 2:11 PM, Earnie Boyd wrote:
Quoting Stella Power
There's a patch against the coder module waiting to be reviewed (I know it's not core), but this patch will add functionality to coder which will allow you to run a code review on patches. You can paste in the patch, or just provide the URL to it. It might help speed up the process slightly.
Then a project infrastructure process could just automate a run of the patch against coder and give a report (or a flag) in the issue.
This seems like a very reasonable thing to add into testing.drupal.org once it's up and running.
Quoting Nathaniel Catchpole <catch56@googlemail.com>:
This seems like a very reasonable thing to add into testing.drupal.org once it's up and running.
Cooler than a snowball on a hot tin roof! How often does the cron job run? Is the plan to submit the results to the issue where the patch resides? Do you plan for a similar system for contrib modules? At least we should get a link to http://testing.drupal.org/tests listed on the project issue page. Earnie -- http://for-my-kids.com/ -- http://give-me-an-offer.com/
On Thu, Aug 14, 2008 at 5:53 PM, Earnie Boyd wrote:
Cooler than a snowball on a hot tin roof! How often does the cron job run?
It doesn't at the moment.
Is the plan to submit the results to the issue where the patch resides? Yes. But the bot can't set status until http://drupal.org/node/271216 is resolved - if you (or anyone else) would like to see this actually happen, pitch in there.
Do you plan for a similar system for contrib modules? I don't know if the testbed is set up to handle contrib modules as well. That would be a natural extension of it though (although it'd have to be one module added at a time, and we don't have any contribs on api.drupal.org yet either).
Nat
Quoting Nathaniel Catchpole <catch56@googlemail.com>:
On Thu, Aug 14, 2008 at 5:53 PM, Earnie Boyd wrote:
Cooler than a snowball on a hot tin roof! How often does the cron job run?
It doesn't at the moment.
So is it being run by hand? I set at http://testing.drupal.org/tests text like "1 hour 27 min ago" under the "Submitted" column.
Is the plan to submit the results to the issue where the patch resides? Yes. But the bot can't set status until http://drupal.org/node/271216 is resolved - if you (or anyone else) would like to see this actually happen, pitch in there.
I don't know how much I have left to pitch (at least for August) but I've bookmarked it.
Do you plan for a similar system for contrib modules? I don't know if the testbed is set up to handle contrib modules as well. That would be a natural extension of it though (although it'd have to be one module added at a time, and we don't have any contribs on api.drupal.org yet either).
Well, that would be another plus for contrib. At least I have http://drupal.kollm.org/node/1 thanks to ax. Earnie -- http://for-my-kids.com/ -- http://give-me-an-offer.com/
Earnie Boyd wrote:
So is it being run by hand? I set at http://testing.drupal.org/tests text like "1 hour 27 min ago" under the "Submitted" column.
The status of testing.drupal.org is, at the moment, this: patches are applied, and marked "fail" if they don't apply properly. Tests are not actually run at the moment. But none of that matters, because nothing is posted back to the issue queue anyway.
Well, that would be another plus for contrib. At least I have http://drupal.kollm.org/node/1 thanks to ax.
But, how many contributed modules actually have their own tests written? No more than 10-20, certainly, which is a very small percentage. The workflow here needs to be more thought out; in addition, it's not all that reasonable to have a server running automated tests for almost 3000 contributed modules, not to mention different core versions, module dependencies, etc. -Charlie
On Aug 14, 2008, at 9:48 PM, Charlie Gordon wrote:
Earnie Boyd wrote:
So is it being run by hand? I set at http://testing.drupal.org/ tests text like "1 hour 27 min ago" under the "Submitted" column.
But, how many contributed modules actually have their own tests written? No more than 10-20, certainly, which is a very small percentage. The workflow here needs to be more thought out; in addition, it's not all that reasonable to have a server running automated tests for almost 3000 contributed modules, not to mention different core versions, module dependencies, etc.
-Charlie
Yes. Chicken and egg. If the test results were being posted back to d.o. it would encourage people to write more tests, especially if projects with no tests got a big "Testing status: FAIL" all over them. I think it is unreasonable NOT to have as many servers as are needed, running all tests for all module, all day, 24/7. We're no longer talking about an ecosystem financed by milk money. Getting iron isn't the problem. Someone please write a proposal to the Drupal Association in time for the next board meeting that the complete testing infrastructure and development resources should be financed. Or, alternatively, some Drupal shop out there with the capacity to make this happen step forward. We've invested a lot into testing, and it is clear that it can have a positive influence on the commit process (for core and contrib), so let's not settle for a single or a double. To hit a home run we need the testing infrastructure.
On Thu, Aug 14, 2008 at 4:08 PM, Robert Douglass <rob@robshouse.net> wrote:
Yes. Chicken and egg. If the test results were being posted back to d.o. it would encourage people to write more tests, especially if projects with no tests got a big "Testing status: FAIL" all over them.
I think it is unreasonable NOT to have as many servers as are needed, running all tests for all module, all day, 24/7. We're no longer talking about an ecosystem financed by milk money. Getting iron isn't the problem.
Someone please write a proposal to the Drupal Association in time for the next board meeting that the complete testing infrastructure and development resources should be financed. Or, alternatively, some Drupal shop out there with the capacity to make this happen step forward.
We've invested a lot into testing, and it is clear that it can have a positive influence on the commit process (for core and contrib), so let's not settle for a single or a double. To hit a home run we need the testing infrastructure.
If you're seriously interested in having a drupal shop or other step in to help handle this need, then we still need a proposal written to detail what is needed. Write up a proposal of needs and make it public. I'd love to help to help with hardware, bandwidth, etc. In fact my company has a bunch of old servers hanging around the office unused (we moved to EC2). However, I need to know what is needed and how to implement it. --- Kathleen Murtagh
On Thu, Aug 14, 2008 at 9:27 PM, Kathleen Murtagh wrote:
If you're seriously interested in having a drupal shop or other step in to help handle this need, then we still need a proposal written to detail what is needed. Write up a proposal of needs and make it public.
I'd love to help to help with hardware, bandwidth, etc. In fact my company has a bunch of old servers hanging around the office unused (we moved to EC2). However, I need to know what is needed and how to implement it.
--- Kathleen Murtagh
I've started a thread in the QA/Testing + Association groups to detail the steps required to the extent I know them, and get a proposal written up if that's needed - http://groups.drupal.org/node/13990 Nat
On Aug 14, 2008, at 2:08 PM, Robert Douglass wrote:
Someone please write a proposal to the Drupal Association in time for the next board meeting that the complete testing infrastructure and development resources should be financed.
And by the by the next Board meeting is next week, and then another one at Szeged. Yes there are many things on the agenda, but infrastructure is one area where a serious proposal might get some serious consideration, and normally the Board meets every 2 months or so. Laura
Quoting Charlie Gordon <cwgordon7@cwgordon.com>:
Earnie Boyd wrote:
So is it being run by hand? I set at http://testing.drupal.org/tests text like "1 hour 27 min ago" under the "Submitted" column.
The status of testing.drupal.org is, at the moment, this: patches are applied, and marked "fail" if they don't apply properly. Tests are not actually run at the moment. But none of that matters, because nothing is posted back to the issue queue anyway.
Even the fact that this is done would be a benefit if the developer could new he could depend on it being done within 5 or 10 minutes of posting the patch. The developer could check testing.drupal.org/tests to see if his patch applied properly. Earnie -- http://for-my-kids.com/ -- http://give-me-an-offer.com/
On Wed, Aug 13, 2008 at 5:55 PM, Yuval Hager <yuval@avramzon.net> wrote:
On Wednesday 13 August 2008, Nathaniel Catchpole wrote:
So if you already have HEAD installed (I pretty much only clean my database before or after reviewing a patch with an upgrade path or schema change), you can do this:
// clean your HEAD of old changes. cvs -q diff | patch -p0 -R
wget http://drupal.org/files/issues/somepatch.patch
patch -p0 < somepatch.patch
1 out of 1 hunks failed etc.
Mark to code needs work with 'needs a re-roll'.
Can't this process be automated? I think I remember something about a process that scans for stale patches - does it still exist?. The close-fixed-after-two-weeks bot seems to be doing a very nice work!
This is supposed to go here at some point http://testing.drupal.org/ -- Khalid M. Baheyeldin 2bits.com, Inc. http://2bits.com Drupal optimization, development, customization and consulting.
On Wed, Aug 13, 2008 at 3:45 PM, Nathaniel Catchpole <catch56@googlemail.com> wrote:
So if you already have HEAD installed (I pretty much only clean my database before or after reviewing a patch with an upgrade path or schema change), you can do this:
// clean your HEAD of old changes. cvs -q diff | patch -p0 -R
wget http://drupal.org/files/issues/somepatch.patch
patch -p0 < somepatch.patch
1 out of 1 hunks failed etc.
Mark to code needs work with 'needs a re-roll'.
Also simpletest is a much, much easier and more comprehensive way to avoid regressions that installing devel, generating content and clicking around. So that stage is reduced to: admin/build/modules enable simpletest admin/build/testing run all tests
Brilliant. We need more of this kind of cook-book, how to do something quickly and efficiently sort of documentation. I've been using cvs and patch for about 25 years, and I never thought to combine them in "cvs -q diff | patch -p0 -R". There's always something new to learn. (I think simply running "cvs update -C" gets you the same end result.)
You are doing nothing wrong, Katherine. Everybody reads their email with an email client that's been optimized with years and years of experience. Virtually no one has a Drupal development environment and tools which have been optimized to that extent. As you described, Katherine, it takes real time to install and test patches. I took about 2 minutes to write this, far less time than it would even take to go find a patch I was interested in, read the issue and download the patch, much less give it a thorough test and then write a review of that test. Folks who suggest you can do reviews in the time it takes to review or write a quick comment in email are wrong, wrong, wrong for 99.9% of the development community out there. ..chris On Wed, Aug 13, 2008 at 3:08 PM, Katherine Senzee <katherine@esquaredworkshops.com> wrote:
On Wed, Aug 13, 2008 at 12:52 PM, Nathaniel Catchpole <catch56@googlemail.com> wrote:
Some patch reviews take less than 30 seconds, especially when the issue queue is long and has lots of stale patches in it.
I'm not sure I follow. I review patches when I get the time, but I figure on at least 20 minutes -- read the issue, check out HEAD, apply the patch, create a new database, install the test site, download/install devel, generate stuff, see if the patch works as advertised, post my review. If I have a recent enough HEAD install I figure I can skip the database bit and just use my existing database, but it's still a decent chunk of time. What am I doing wrong?
-- Katherine Senzee (ksenzee) esquaredworkshops.com
"No real spare time." What an excuse. Somehow, people have spare time for all these rants, but no time for patch reviews? This does not make much sense to me. Does anyone care to explain? Dmitri On Wed, Aug 13, 2008 at 12:36 PM, Marcel Partap <mpartap@gmx.net> wrote:
Angela Byron wrote:
careful. If we get into the situation where HEAD is broken, development effectively stops until things are working again.
Nooooooo, 'cause not HEAD ;X - 'next' or 'experimental' branch!
It also makes a BIG difference who those two people are. Follow catch's advice. Catch is wise. The best shot you have of getting patches that you want in core is to become one of those rare patch reviewing ninjas like him. Then your voice being one of those two voices may very well be enough.
i'd love too but my excuse (as so many others') shall be: no real spare time atm. Also, of course i mostly care about bugs i run into myself, and i rarely run into any. Some people seem to report bugs using really obscure setups plus outdated modules...
-- "Obstacles are those frightful things you see when you take your eyes off your goal." -- Henry Ford (1863-1947)
Change the world! Vote revolution: http://hfopi.org/vote-future
On Wed, Aug 13, 2008 at 4:38 PM, Dmitri G <dmitrig01@gmail.com> wrote:
"No real spare time."
What an excuse.
Somehow, people have spare time for all these rants, but no time for patch reviews?
This does not make much sense to me.
Does anyone care to explain?
Dmitri
In terms of time management, I've been thinking a lot about this issue :) It comes down to patterns, motivation and task switching. Patterns help you do things swiftly, or unconsciously. Responding to a mailing list is an example of doing something that takes time unconsciously because we were checking our email _anyway_, what's writing one more? On the other hand, not having an environment set up, or a system in place for going over patches makes it difficult because there are too many conscious steps to address. There isn't a pattern or workflow set up, so the person has to consciously think of the next step and work to improve upon the process with their given environment. There isn't yet any swiftness to following a pattern, and you're well aware of the time that it will take. That segways nicely into motivation. In reading an email, we can be inspired or angered to reply - and besides, we were checking our email anyway. A person may not be inspired to move and do some patch reviews. And if they're not inspired, setting up and maintaining that environment to do the testing will be burdensome. Then is task switching. I know this one well :) Although the actual task itself may take a short amount of time, there is a significant amount of overhead when switching from one task to another. I can work on one project and put in a 12 hour productive day. However, if I were asked to do three projects, I would likely work on only two in the day, leaving the other out cold with only 6 productive hours, and 6 totally wasted hours due to overhead. I am, obviously, an extreme example of this phenomena. However, a "20 minute" patch review can very easily turn into an hour including the overhead of task switching. --- Kathleen Murtagh
On Wed, Aug 13, 2008 at 4:21 PM, Kathleen Murtagh <kathleen@ceardach.com> wrote:
In terms of time management, I've been thinking a lot about this issue :) It comes down to patterns, motivation and task switching.
Exactly. And tooling. The world of software development has been standardizing and optimizing the email client for more years and far more man-years than the "test patch files against CVS" environment.
Then is task switching. I know this one well :) Although the actual
6 totally wasted hours due to overhead. I am, obviously, an extreme example of this phenomena. However, a "20 minute" patch review can
Kathleen Murtagh
No, you're not an extreme case. You're just far more observant and informed about the issue than most. In their classic book Peopleware , Tom DeMarco and Tim Lister report on studies of programmers' productivity relative to the characteristics of their work environment. "It is clear that interruptions are a major cause of low productivity among programmers. Why? The problem is not the time needed to handle the interruptions themselves, but the time needed to get back into the programming problem. Everybody, no matter what they do, face a reorientation time when they return to their work after an interruption. When you are reading a magazine article and look up to answer a question, it takes you longer to read the next paragraph than if you had been reading continuously." -- from this article: http://www.ibm.com/developerworks/library/it-nielsen4/?dwzone=ibm
On Wed, Aug 13, 2008 at 1:38 PM, Dmitri G <dmitrig01@gmail.com> wrote:
"No real spare time."
What an excuse.
Somehow, people have spare time for all these rants, but no time for patch reviews?
This does not make much sense to me.
Does anyone care to explain?
Dmitri
Well I haven't had time for either but since you asked... I've burned out on reviewing because I'd get excited about a patch try to help it along and then.... nothing... it just gets ignored and never committed or commented on by a committer. Sort of hard to stay motivated to keep throwing good time after bad. andrew
andrew morton wrote
Well I haven't had time for either but since you asked... I've burned out on reviewing because I'd get excited about a patch try to help it along and then.... nothing... it just gets ignored and never committed or commented on by a committer. Sort of hard to stay motivated to keep throwing good time after bad.
Although I haven't either submitted nor reviewed patches for drupal core, I can acknowledge this point because I see this with modules. Many of the modules I need for 6.x were still in beta or not even upgraded to 6.x, so I tried to write and patch a lot of stuff by myself. With some modules, I got feedback quite fast and so I went on patching and improving for those. But when you spent two weeks full-time work to add a feature (like attaching files for comments to use that in forums) and then get nothing but silence for weeks, not even a "I will look at or test this", then you definitely lose motivation to do any other patch for this module. I'm not blaming module maintainers for this, since they might just not have enough time to care about all this or more important things (or "real life" :-)) to do or whatever. Although this is the view from the other side, I'm sure this is the same reviewing. People just want feedback about work they did, otherwise they are disappointed and stop contributing. cu, Frank -- Dipl.-Inform. Frank Steiner Web: http://www.bio.ifi.lmu.de/~steiner/ Lehrstuhl f. Bioinformatik Mail: http://www.bio.ifi.lmu.de/~steiner/m/ LMU, Amalienstr. 17 Phone: +49 89 2180-4049 80333 Muenchen, Germany Fax: +49 89 2180-99-4049 * Rekursion kann man erst verstehen, wenn man Rekursion verstanden hat. *
On Thursday 14 August 2008, Frank Steiner wrote:
I'm not blaming module maintainers for this, since they might just not have enough time to care about all this or more important things (or "real life" :-)) to do or whatever.
Well, I do blame them. Responsible maintainers *should* respond within two weeks to *any* bug they have on the issue queue. Any polite response would be better than silence. Only a handful of complicatedmodules can skip this rule, but we all know their maintainers are responsible anyway :) My own experience of Drupal core patches was also demotivating, where patches were discussed to every minor detail, not always in a professional way, but sometimes in a personal way, not without inappropriate comments or innuendos here and there. I have not reviewed enough to claim this counts for statistics, but it was enough to keep me away from core for a long time. -- Yuval Hager [@] yuval@avramzon.net
Somehow, people have spare time for all these rants, but no time for patch reviews? This does not make much sense to me. Does anyone care to explain?
I've never bought this line of reasoning. Drupal core would be worst off if the same amount of time to send a flippant email was spent on a patch review. It's like comparing firing a gun to cleaning a gun. -- Morbus Iff ( god less america ) Technical: http://www.oreillynet.com/pub/au/779 Enjoy: http://www.disobey.com/ and http://www.videounderbelly.com/ aim: akaMorbus / skype: morbusiff / icq: 2927491 / jabber.org: morbus
Not if you just approve everything Morbus. M On Aug 13, 2008, at 7:06 PM, Morbus Iff wrote:
Somehow, people have spare time for all these rants, but no time for patch reviews? This does not make much sense to me. Does anyone care to explain?
I've never bought this line of reasoning. Drupal core would be worst off if the same amount of time to send a flippant email was spent on a patch review. It's like comparing firing a gun to cleaning a gun.
-- Morbus Iff ( god less america ) Technical: http://www.oreillynet.com/pub/au/779 Enjoy: http://www.disobey.com/ and http://www.videounderbelly.com/ aim: akaMorbus / skype: morbusiff / icq: 2927491 / jabber.org: morbus
Angela Byron wrote:
Marcel Partap wrote:
Mhh.. what about (auto-)committing to a D7.x-next branch after two independent people have confirmed a patch as working?
Dear sweet Lord, NO! :)
It does'nt have to be auto-committed, but if 2-3 persons have reviewed and marked RTBC, the status can escalate into something like TRTBC (Thoroughly RTBC). This may have a nice effect on both the committers, as they will be more focused, and on the rest of us, since the more we review, the faster things will probably be committed.
Quoting Zohar Stolar <z.stolar@gmail.com>:
Angela Byron wrote:
Marcel Partap wrote:
Mhh.. what about (auto-)committing to a D7.x-next branch after two independent people have confirmed a patch as working?
Dear sweet Lord, NO! :)
It does'nt have to be auto-committed, but if 2-3 persons have reviewed and marked RTBC, the status can escalate into something like TRTBC (Thoroughly RTBC). This may have a nice effect on both the committers, as they will be more focused, and on the rest of us, since the more we review, the faster things will probably be committed.
Won't help. We have a large funnel and the end of the funnel is a committer. There is no way for the committer to catch up with all commits. We need more committers to resolve that issue. It'd be interesting and bazaar to see another repository of Drupal core where all CVS account holders could commit patches after review. I wonder just how broken its HEAD would be. Earnie -- http://for-my-kids.com/ -- http://give-me-an-offer.com/
Won't help. We have a large funnel and the end of the funnel is a committer. There is no way for the committer to catch up with all commits. We need more committers to resolve that issue.
Hard to argue with that. However - is there anyway we can make the committers job easier? and related, is there a way we can get work that in "the funnel" to keep moving farther to the end so that people don't get discouraged being stuck on the sidelines.
It'd be interesting and bazaar to see another repository of Drupal core where all CVS account holders could commit patches after review. I wonder just how broken its HEAD would be.
I agree that a free-for-all could be pretty messy (especially without a reasonable transition) but I would point out that there are lots of projects that operate with more than 2-3 committers. I believe I've seen references to projects like svn having ~150 "core" committers or apache having 100+ committers.
On Thu, Aug 14, 2008 at 7:35 AM, Zohar Stolar wrote:
It does'nt have to be auto-committed, but if 2-3 persons have reviewed and marked RTBC, the status can escalate into something like TRTBC (Thoroughly RTBC).
This is what RTBC is, adding an extra status called Really Really Reviewed and Tested by the Community and Thoroughly Ready to be Committed (RRTBCTRBC). Due to the shortage of reviewers though, patches don't always get confirmed as RTBC by two or more people, so it's not as solid as it should be - but that's a problem of lack of reviews, not patch statuses. Nat
Nathaniel Catchpole wrote:
On Thu, Aug 14, 2008 at 7:35 AM, Zohar Stolar wrote:
It does'nt have to be auto-committed, but if 2-3 persons have reviewed and marked RTBC, the status can escalate into something like TRTBC (Thoroughly RTBC).
This is what RTBC is, adding an extra status called Really Really Reviewed and Tested by the Community and Thoroughly Ready to be Committed (RRTBCTRBC). Due to the shortage of reviewers though, patches don't always get confirmed as RTBC by two or more people, so it's not as solid as it should be - but that's a problem of lack of reviews, not patch statuses.
I agree. The people complaining about the long RTBC queue are missing the point. The point is that there's a much, much longer "needs review" queue, and there always has been. While some of those RTBC patches will eventually get committed, none of the "needs review" patches will unless we get some help. -Angie
The people complaining about the long RTBC queue are missing the point. The point is that there's a much, much longer "needs review" queue, and there always has been.
Perhaps they are missing *your* point. The RTBC queue is a big problem as well. Many patches in RTBC represent a massive effort by several people and it is a complete shame to let them sit there, silently. Losing a baserunner on 3rd base is much more painful than losing one at second base. We terribly need a branch maintainer or two for D7. Lack of throughput at RTBC demotivates the 'needs review' queue, and even before that - patch creation. Examples of RTBC silence: 1. make drupal_set_title() use check_plain() by default. - http://drupal.org/node/242873 2. No way to flush form errors during iterative programatic form submission - http://drupal.org/node/180063 3. Node revisions test fails if admin has a personal timezone offset - http://drupal.org/node/283246
Moshe Weitzman wrote:
The people complaining about the long RTBC queue are missing the point. The point is that there's a much, much longer "needs review" queue, and there always has been.
Perhaps they are missing *your* point. The RTBC queue is a big problem as well. Many patches in RTBC represent a massive effort by several people and it is a complete shame to let them sit there, silently. Losing a baserunner on 3rd base is much more painful than losing one at second base. We terribly need a branch maintainer or two for D7. Lack of throughput at RTBC demotivates the 'needs review' queue, and even before that - patch creation.
True. But exactly *one* person can do something about the RTBC queue. 2,000+ people can do something about the CNR queue. Let's have a community outcry about the RTBC queue once the situation is reversed, and we have 325 patches waiting for core committers' blessing, and only 40 patches that need community review. Until then, we have work to do. -Angie
On Thu, Aug 14, 2008 at 6:29 PM, Angela Byron wrote:
True. But exactly *one* person can do something about the RTBC queue. 2,000+ people can do something about the CNR queue.
I think the main issue /at the moment/ is that this is probably the longest in several releases that there's only been one person able to do anything about the RTBC queue. Drupal 5 was released on January 15th 2007 [1], Gabor was given commit access on April 17, 2007[2], three months later. Drupal 6 was released on February 13, 2008 [3], it's now 6 months later and there's no sign of a branch maintainer yet. Additionally, the last core commit by Steven Wittens was July 26, 2007 [4], three weeks after code freeze [5] - so there's actually two less people committing to HEAD than during the Drupal 6 code thaw, and Gabor was employed full time working by Acquia just to work on core commits from August [6] as well. However, even with three core committers, or two and one person employed full time, reviews have always been the primary bottleneck.
Let's have a community outcry about the RTBC queue once the situation is reversed, and we have 325 patches waiting for core committers' blessing, and only 40 patches that need community review. Until then, we have work to do.
-Angie
It's pretty easy to make a difference to the RTBC queue as well, at least when it's as long as it is now, I just went through everything that had been untouched for 5 weeks or more, bumped everything that still applied, found 2-3 that needed re-rolling, and one that was in entirely the wrong queue. Quite a few patches left would probably get in easier if they had simpletests written for them too - so people could find those and help them out if simpletests are missing. For now, RTBC patches against D7 are down to ~ 30 from over twice that a few days ago (there's been some commits as well recently, although 30 is still quite a lot considering many of them are heavily tested and nearly all should be valid against HEAD). We'll see if it goes up or down from here. Like you said, a review queue of about 40 would be pretty good - as long as the RTBC queue goes down by the same proportion (about 5-15). Would be much, much less work nursing patches once the initial pain was over with. Nat [1] http://drupal.org/node/109494 [2] http://buytaert.net/gabor-hojtsy [3] http://drupal.org/node/221219 [4] http://drupal.org/cvs?commit=74849 [5] http://drupal.org/drupal-6.0-code-freeze [6] http://hojtsy.hu/blog/2008-feb-15/drupal-6-maintainers-perspective
On Aug 14, 2008, at 10:29 AM, Angela Byron wrote:
True. But exactly *one* person can do something about the RTBC queue. 2,000+ people can do something about the CNR queue.
Let's have a community outcry about the RTBC queue once the situation is reversed, and we have 325 patches waiting for core committers' blessing, and only 40 patches that need community review. Until then, we have work to do.
As someone who's done an awful lot of core patch reviews over the years, I feel compelled to chime in here. If we've got a single bottleneck on the RTBC queue, how is mustering the forces of 2000+ people to exacerbate that problem going to help anything? Things sitting in RTBC are a *HUGE* waste of developer time. The sheer volume of hours wasted by things having to be re- rolled since they were RTBC once but no one committed for months is mind boggling. Not to mention the cascading effect of other patches that are constantly re-rolled when something finally does get in. I'm certainly not motivated to work in the core issue queue much anymore, except out of necessity. I've just wasted far too many hours re-rolling, re-wrangling reviews, etc, etc, trying to get something in. So long as things sit in RTBC for weeks on end, there's very little motivation for me (and clearly many others) to try to get stuff from CNR to RTBC. The point of patches and patch reviews are to improve Drupal. That only happens once a patch *lands*. So long as one person is the bottleneck for patches landing, there's no point in shaming, moralizing, or otherwise cajoling people to review more patches. Given a single maintainer, I think the core development workflow should make more use of the patch spotlight[1] and be something like: - Dries says the 1-5 patches he cares about and is willing to follow and comment on at any given time. - People who want to help core work on those patches until they're *committed*. - i++; Just about anything else is a giant waste of time it seems. Cheers, -Derek (dww) [1] http://drupal.org/patch/spotlight
On Aug 14, 2008, at 8:15 PM, Derek Wright wrote:
Given a single maintainer, I think the core development workflow should make more use of the patch spotlight[1] and be something like:
- Dries says the 1-5 patches he cares about and is willing to follow and comment on at any given time. - People who want to help core work on those patches until they're *committed*. - i++;
Just about anything else is a giant waste of time it seems.
This is an excellent workflow. If you look at core commit messages, it isn't for lack of commits that things seem stalled. In the time that people have been complaining here, our Chief Bottleneck has committed 14 patches. A bit above average, but take the time to scroll through the pages. You'll be impressed. http://drupal.org/project/cvs/3060
On Aug 14, 2008, at 10:29 AM, Angela Byron wrote:
Let's have a community outcry about the RTBC queue once the situation is reversed, and we have 325 patches waiting for core committers' blessing, and only 40 patches that need community review. Until then, we have work to do.
@Angie: Hear hear! And while I'm at it, let me just say that a patch review doesn't take 20 minutes. Unless the patch only has < 5 lines of changed code in it, it takes an hour~ to review it. This is how long it takes me to read the entire issue queue to figure out which version of the patch is the community approved one, identify the goals and desires of the patch and weed out the scope creep, download the patch, reset my testing sandbox from the last time I ran one of these, install the core or contrib that I'm about to test against, go forth and familiarize myself with the existing functionality that's about to be changed or altered by this patch, apply the patch against its intended destination, re-familiarize myself with the intended set of changes, actually test those changes to see if they work, methodically approach the targeted subset of functional code to see if I can think of a way to break it, break it, observe the results of that breakage, and then reliably report the results back to the issue queue n a way that can be understood by all parties involved. And that's not even including the Simpletests that pass/fail, of the lack of Simpletest(s) for certain areas of the code that should really be tested, and would therefore hold back an RTBC status from being bestowed upon the issue. Just reading that last paragraph should make your head hurt. It did me, and I wrote it. The one thing that speeds up the testing process for me, and I mean greater than 50% is a clear set of testing instructions. If I can find an issue that has less than 12 replies, and an attached patch, I'll jump on it because it should be far easier to discern what the intent of the patch is and test accordingly. On the other hand, once an issue reaches > 15 comments, or has more than two patches attached to it, I automatically assign it a "one precious hour of my personal time" sticker, and am far less motivated to tackle the thing. The only way that an issue can really force me to test it is if I see recent action on the part of the person who actually makes the commit, or a clearly defined two-sentence instructional on how to test this patch from the coder's mindset at the time they wrote it. Which brings me to the end of this little diatribe, but because I strive to never point out a problem without also proffering a solution, I'd like to ask if we might be able to create a 5-line textarea for each issue in the queue that would store the Patch Testing Instructions. Old issues would not have any info in this field, but if a developer wished to receive some near-instant reviews from the community, they could write up a one-paragraph set of instructions on how to adequately test this patch so that it's committable. We can place this field at the top of each issue as a collapsed div, and proffer an expand/collapse jQuerified link right below the First unread comment Most recent comment Add new comment [+]Patch review instructions -- Senpai
participants (25)
-
andrew morton -
Angela Byron -
Charlie Gordon -
Chris Johnson -
Derek Wright -
Dmitri G -
Earnie Boyd -
Frank Steiner -
Karoly Negyesi -
Katherine Senzee -
Kathleen Murtagh -
Khalid Baheyeldin -
Laura Scott -
Marcel Partap -
Michael Haggerty -
Morbus Iff -
Moshe Weitzman -
Nathaniel Catchpole -
Robert Douglass -
Ryan Cross -
Senpai -
Stefan Nagtegaal -
Stella Power -
Yuval Hager -
Zohar Stolar