[Buildroot] [RFC] Effective Patchwork Use and Patch Review

[Buildroot] [RFC] Effective Patchwork Use and Patch Review

[Ryan Barnett]

Thomas De Schampheleire, all,

In the massive thread that has become [PATCH 0/3] Support for
out-of-tree Buildroot customization, Thomas De Schampheleire
brought up the following:

Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote on
09/13/2013 02:35:04 AM:

> 2. sometimes patches get very little, if any, response. Especially in
> the beginning of my contributions, this was very frustrating. I pinged
> patches, re-sent them, even multiple times, and still not even a
> comment was given. I can imagine that not everyone continues trying,
> and several people will just give up, tagging the buildroot community
> as 'not accepting patches'.

> The situation has certainly improved since then, I think: the
> introduction of patchwork helps keeping track of open patches, and
> although I have not kept numbers I think the community certainly has
> grown, and there are now several more people systematically reviewing
> patches so that the burden does not fall entirely on Peter and
> yourself.

> I think we should still keep an eye on this though. I think it still
> happens that patches are sent by 'new' people in the community, and
> there is little or no reaction. Sometimes the patch does not really
> fit in our general belief of how buildroot should be, but maybe we
> don't always dare to say it. Or sometimes the patch is so specific
> that it is hard to  give a good review, so one waits until someone
> else with more experience in that area gives some comments, which may
> never happen.

> Ideally, patches should stay no longer than a fixed amount of time in
> patchwork. Either the patch is given some comments and we
> wait/encourage for the submitter to send an updated version, or we
> reject the patch with some explanation, or we ask more details so we
> can either accept/reject it with more background. This may not be
> feasible for all patches. Sometimes there is simply no-one who really
> needs the extra feature/fix except the submitter himself, but that is
> not necessarily a reason not to accept the patch.

One thing that I've found difficult is finding an effective way for
someone new to buildroot community to perform reviews on patches they
find of interest. What I mean by effective is a way that takes little
time to pull the patch down and then mark it up for review and send it
back out. Our email client in my work environment isn't ideal.
However, we do have ways of sending email directly using git-email.

The thing is that I haven't been able find good documentation on how
to effectively use Patchworks for review (pwclient). I don't
necessarily need someone to tell me how to exactly use it but rather
maybe an example workflow for interacting with patchworks with
"pwclient" and maybe some example helper scripts for formatting
replies to these patches.

So I guess the two things that I would like to see eventually be
addressed by provide a little more detail some how are:

1) Guidelines for providing reviews comments on patches

2) Some examples of how to effectively use patchworks for reviewing

If somebody is willing to chime in on this, I can maybe look to
document this feedback under the contributing to buildroot section
of the manual.

------------------------------------------------------------
Ryan J Barnett / Software Engineer / Platform SW
MS 137-157, 855 35th St NE, Cedar Rapids, IA, 52498-3161, US
Phone: 319-263-3880 / VPN: 263-3880 
rjbarnet at rockwellcollins.com
www.rockwellcollins.com

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20130913/a48433a2/attachment.html>

[Buildroot] [RFC] Effective Patchwork Use and Patch Review

[Thomas Petazzoni]

Dear Ryan Barnett,

On Fri, 13 Sep 2013 11:41:41 -0500, Ryan Barnett wrote:

> One thing that I've found difficult is finding an effective way for
> someone new to buildroot community to perform reviews on patches they
> find of interest. What I mean by effective is a way that takes little
> time to pull the patch down and then mark it up for review and send it
> back out. Our email client in my work environment isn't ideal.
> However, we do have ways of sending email directly using git-email.
> 
> The thing is that I haven't been able find good documentation on how
> to effectively use Patchworks for review (pwclient). I don't
> necessarily need someone to tell me how to exactly use it but rather
> maybe an example workflow for interacting with patchworks with
> "pwclient" and maybe some example helper scripts for formatting
> replies to these patches.

patchwork is really not here to help with the reviewing process. It's
mainly of use for the Buildroot maintainer to keep track of which
patches remain to be merged/rejected.

All the reviewing process takes place on the mailing list, using e-mail
clients. We don't use patchwork at all for this.

The only use of patchwork you may be interested in is to apply patches
locally in order to be able to test them. But you don't even need
pwclient for that. You can simply do:

wget -O - http://patchwork.ozlabs.org/patch/274867/mbox/ | git am

This will fetch a patch from patchwork and apply it to the current
branch.

But besides that, all the review process simply consists in replying to
the patch on the mailing list.

> So I guess the two things that I would like to see eventually be
> addressed by provide a little more detail some how are:
> 
> 1) Guidelines for providing reviews comments on patches

As said above, it's really just "hit reply in your mailer", and provide
your comments at the relevant places in the patch itself.

> 2) Some examples of how to effectively use patchworks for reviewing

patchwork is of little use for reviewing. The only thing it is used for
is to know the list of patches that are still pending to be applied.

I believe you might have seen patchwork like gerrit, but it is not:
patchwork is really a patch tracking tool, not a patch reviewing tool.
At the time we introduced patchwork, I believe we discussed gerrit and
things like that, but if I remember correctly, we thought that
patchwork was already a good step forward, and we didn't want to move
to a more complicated tool in one step.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[Buildroot] [RFC] Effective Patchwork Use and Patch Review

[Carsten Schoenert]

Hello Ryan,

On 14.09.2013 07:47, Thomas Petazzoni wrote:
> wget -O - http://patchwork.ozlabs.org/patch/274867/mbox/ | git am
> 
> This will fetch a patch from patchwork and apply it to the current
> branch.
> 
> But besides that, all the review process simply consists in replying to
> the patch on the mailing list.
...

Wolfram Sang gave this year at the Fosdem a talk how he is maintaining
the I2C subsystem in the kernel. His talk was right after Thomas talk
about the ARM support in the Linux kernel. ;)

Wolfram talks a lot about *his* workflow while maintaining the I2C
subsystem.

Unfortunately Wolfram doesn't uploaded his slides, but the interesting
parts are the live demos about that. So you may want see the video from
this talk?

https://archive.fosdem.org/2013/schedule/event/maintaining_a_kernel_subsystem/

Regards
Carsten

[Buildroot] [RFC] Effective Patchwork Use and Patch Review

[Thomas De Schampheleire]

Hi,

On Sat, Sep 14, 2013 at 7:47 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Ryan Barnett,
>
> On Fri, 13 Sep 2013 11:41:41 -0500, Ryan Barnett wrote:
>
>
> patchwork is really not here to help with the reviewing process. It's
> mainly of use for the Buildroot maintainer to keep track of which
> patches remain to be merged/rejected.
>
> All the reviewing process takes place on the mailing list, using e-mail
> clients. We don't use patchwork at all for this.

Agreed. When I wrote that patchwork improved the situation, I meant
that patchwork provided a good overview of outstanding patches, both
for reviewers as for maintainers.

>
> The only use of patchwork you may be interested in is to apply patches
> locally in order to be able to test them. But you don't even need
> pwclient for that. You can simply do:
>
> wget -O - http://patchwork.ozlabs.org/patch/274867/mbox/ | git am
>
> This will fetch a patch from patchwork and apply it to the current
> branch.

This is also how I use patchwork.
As I wrote before, I'm using Mercurial for my buildroot development,
and specifically the mq extension that is similar to git's quilt, in
which case I can simply do:

hg qimport http://patchwork.ozlabs.org/patch/274867/mbox/

which does the same as:

wget -O - http://patchwork.ozlabs.org/patch/274867/mbox/ | hg qimport

but with less typing.

Best regards,
Thomas

[Buildroot] [RFC] Effective Patchwork Use and Patch Review

[Ryan Barnett]

Thomas(es),

Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote on 
09/15/2013 09:23:35 AM:

> Hi,
> 
> On Sat, Sep 14, 2013 at 7:47 AM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > Dear Ryan Barnett,
> >
> > On Fri, 13 Sep 2013 11:41:41 -0500, Ryan Barnett wrote:
> >
> >
> > patchwork is really not here to help with the reviewing process. It's
> > mainly of use for the Buildroot maintainer to keep track of which
> > patches remain to be merged/rejected.
> >
> > All the reviewing process takes place on the mailing list, using 
e-mail
> > clients. We don't use patchwork at all for this.
> 
> Agreed. When I wrote that patchwork improved the situation, I meant
> that patchwork provided a good overview of outstanding patches, both
> for reviewers as for maintainers.

OK that fine. I was just wondering if anyone had used patchworks
to write some scripts to pull patches down and then mark-up and send 
the review back out over email. My email client is a pain for this so
I was just wondering if there was a way to not use it...

Under Section 10.2 of the manual, would it be useful to update this 
say explicitly state that the review takes place over the mailing 
list? Currently I think the section implies it that but doesn't
say it explicitly. 

Thanks,
-Ryan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20130916/752167b1/attachment.html>

[Buildroot] [RFC] Effective Patchwork Use and Patch Review

[Thomas Petazzoni]

Dear Ryan Barnett,

On Mon, 16 Sep 2013 09:36:44 -0500, Ryan Barnett wrote:

> > Agreed. When I wrote that patchwork improved the situation, I meant
> > that patchwork provided a good overview of outstanding patches, both
> > for reviewers as for maintainers.
> 
> OK that fine. I was just wondering if anyone had used patchworks
> to write some scripts to pull patches down and then mark-up and send 
> the review back out over email. My email client is a pain for this so
> I was just wondering if there was a way to not use it...

No, the only person using scripts for patchwork is really the
maintainer, so that when he applies patches, they are automatically
marked as "Applied" on the patchwork.

Also, since I (and a few other core developers) have some kind of admin
access to patchwork, we can mark patches as Superseded, Rejected, etc.
But we do this with the web interface.

> Under Section 10.2 of the manual, would it be useful to update this 
> say explicitly state that the review takes place over the mailing 
> list? Currently I think the section implies it that but doesn't
> say it explicitly. 

Sure, if anything is unclear in the manual, it's certainly worth making
it explicit.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com