On 09/12/2017 04:14 PM, Aleksander Alekseev wrote: > Hello, hackers! > > Thanks to the work of Thomas Munro now we have a CI for the patches on > the commitfest [1]. Naturally there is still room for improvement, but > in any case it's much, much better than nothing. > > After a short discussion [2] we agreed (or at least no one objected) > to determine the patches that don't apply / don't compile / don't > pass regression tests and have "Needs Review" status, change the > status of these patches to "Waiting on Author" and write a report > (this one) with a CC to the authors. As all we know, we are short on > reviewers and this action will save them a lot of time. Here [3] you > can find a script I've been using to find such patches. > > I rechecked the list manually and did my best to exclude the patches > that were updated recently or that depend on other patches. However > there still a chance that your patch got to the list by a mistake. > In this case please let me know.>
With all due respect, it's hard not to see this as a disruption of the current CF. I agree automating the patch processing is a worthwhile goal, but we're not there yet and it seems somewhat premature. Let me explain why I think so: (1) You just changed the status of 10-15% open patches. I'd expect things like this to be consulted with the CF manager, yet I don't see any comments from Daniel. Considering he's been at the Oslo PUG meetup today, I doubt he was watching hackers very closely. (2) You gave everyone about 4 hours to object, ending 3PM UTC, which excludes about one whole hemisphere where it's either too early or too late for people to respond. I'd say waiting for >24 hours would be more appropriate. (3) The claim that "on one objected" is somewhat misleading, I guess. I myself objected to automating this yesterday, and AFAICS Thomas Munro shares the opinion that we're not ready for automating it. (4) You say you rechecked the list manually - can you elaborate what you checked? Per reports from others, some patches seem to "not apply" merely because "git apply" is quite strict. Have you actually tried applying / compiling the patches yourself? (5) I doubt "does not apply" is actually sufficient to move the patch to "waiting on author". For example my statistics patch was failing to apply merely due to 821fb8cdbf lightly touching the SGML docs, changing "type" to "kind" on a few places. Does that mean the patch can't get any reviews until the author fixes it? Hardly. But after switching it to "waiting on author" that's exactly what's going to happen, as people are mostly ignoring patches in that state. (6) It's generally a good idea to send a message the patch thread whenever the status is changed, otherwise the patch authors may not notice the change for a long time. I don't see any such messages, certainly not in "my" patch thread. I object to changing the patch status merely based on the script output. It's a nice goal, but we need to do the legwork first, otherwise it'll be just annoying and disrupting. I suggest we inspect the reported patches manually, investigate whether the failures are legitimate or not, and eliminate as many false positives as possible. Once we are happy with the accuracy, we can enable it again. kind regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
