[Ksummit-discuss] [CORE TOPIC] Recruitment (Reviewers, Testers, Maintainers, Hobbyists)

[Rafael J. Wysocki]

On Thursday, July 09, 2015 04:38:54 PM Darren Hart wrote:
> On Thu, Jul 09, 2015 at 11:31:49PM +0100, David Woodhouse wrote:
> > On Thu, 2015-07-09 at 12:37 -0700, Darren Hart wrote:
> > > I spend a highly disproportionate amount of my time, relative to measurable
> > > quality impact to the kernel, going over the nuances of submitting patches.
> > > 
> > > 1) Must have a complete commit message
> > > 2) DCO goes above the ---
> > > 3) Include a patch changelog, do so below ---
> > > 4) Cc maintainers :-)
> > > 5) Checkpatch... checkpatch... checkpatch...
> > > 6) Compiler warnings
> > > 7) CodingStyle :-)
> > > 8) Use ascii or utf8 character encodings
> > > 
> > > All of these are distractions from reviewing the code. I'm often on 
> > > version 3 of a series before I'm actually reviewing content.
> > 
> > I don't think it's entirely appropriate to call all of those
> > 'distractions'.
> > 
> > The "content" in question is a proposed change to the code base. Such a
> > change requires a coherent commit message which will make sense when we
> > look back at this commit in the future. We will need to understand what
> > was happening and why, in order to fix it or tweak it for new
> > circumstances.
> > 
> > Doing that is *not* a peculiarity of the Linux 'process'. It is basic
> > engineering discipline, and it is *part* of the work. Just like the
> > task of breaking a change down into incremental, bisectable commits —
> > which I'm surprised wasn't in your list.
> > 
> > Yes, there are a lot of people who *lack* that basic engineering
> > discipline, to the point where it *can* start to look like it's a Linux
> > -only requirement. But it's not. And there's not a lot we can do if an
> > "engineer" lacks it, short of educating them. That part isn't a process
> > issue.
> 
> Fair enough. Agreed.
> 
> Small functional changes isn't something that is readily automated, and it's the
> kind of feedback I do expect to give as a maintainer. That's why it isn't on the
> list. I understand your point that some things that are on the list are just
> good SW Eng 101 material - in practice, it still wastes my time :-) as it can
> be automated.
> 
> > 
> > Likewise, compiler warnings. If your developers are actually
> > *submitting* code with unresolved compiler warnings, again there's not
> > a lot we can do to help them or you. Apart from confiscating their
> > keyboard.
> 
> In the simplest case, sure. However, it isn't uncommon for config fuzz testing
> or different compiler versions to result in compiler warnings that the original
> submitter could legitimately not seen. Right now my choice is to ignore their
> code or spend the time to educate - but either way I have to build it and find
> the error. That step could be automated.

Yeah.  That's why I have the bleeding-edge branch. :-)

> > Coding style, again, isn't a Linux-specific thing. All large code bases
> > attempt to have some kind of consistency to make the code readable, and
> > anyone attempting to work on *any* code base should expect to become
> > familiar with the idioms and conventions (and charset choice) of the
> > code they're working on.
> > 
> > These *aren't* process issues, in my view. What you're saying with some
> > of these is that you're having to do *basic* (non-Linux-specific)
> > education of people who want to contribute code, and that *that*
> > doesn't scale. Which is true, but it's hard to know how to address
> > that, except with programs like GSoC etc.
> > 
> > Josh suggests that we should provide a service that people could push
> > code to and 'get automated feedback on what needs fixing'... but isn't
> > that what checkpatch was for? OK, a local tool can't cross-compile it
> > for you on every platform we support, but it can do a lot of stuff
> > short of that.
> > 
> > I do like the idea of a 'test' mailing list which receives patches and
> > checks them for corruption though.
> 
> I believe we're in agreement on some additional automation being available to
> a broader audience would catch more errors with less maintainer/reviewer time.

That service only requires somebody with a kernel.org account and enough time
to apply *all* patches they get (except for the ones that don't apply, of course)
and push the result out into a public git branch on a regular basis.

That might be a robot, but arguably not running on kernel.org for safety
reasons.

> As to what should be expected from contributors... I don't know. I wish everyone
> was as meticulous as you and so many other kernel developers (it's one of the
> things I really appreciate about Linux kernel development), and something I
> personally strive for. The reality is, they aren't, and since we're talking
> about recruiting, my point was by implementing some of these things we can
> maintain our standards while reducing maintainer/reviewer overhead.

With all tools/scripts/0-days/whatever in place there always is the time when
*you* (the maintainer) have to decide whether or not to apply the patch (and
sometimes when to apply it too), so you have to look at it and understand
what it is doing.

Fortunately or not, no kind of software is able to understand things as of
today, so you need a human for that job.  And *that* really should be the
job for reviewers (including maintainers in a reviewer role).  All of the
coding style/build errors/white space/etc is secondary in principle.
However, people often request things to be cleaned up to start with, because
those small details prevent them from being able to understand the general
idea in the first place.

So the automation is only useful here as long as it helps patch submitters to
get their work to the point at which it can be understood by a reviewer in
a relatively short time.  Otherwise, it is just not helpful in that respect.

For example, if someone sends me a patch that doesn't apply, but that I can
easily understand and I think "Yeah, that's a good idea!", I may fix it up
and make it build and so on just fine.  But without understanding, I pretty
much can't do anything with it even if it got all of the details perfect.

Thanks,
Rafael