[Qemu-devel] Patch checking bot

[Qemu-devel] Patch checking bot

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

Hi,
At KVM Forum 2014 we discussed a patch checking bot that automates patch
format checking and smoke testing:

1. Did the patch submitter include Signed-off-by?
2. Does checkpatch.pl pass?
3. Does the patch apply to qemu.git/master?
4. Does each patch compile?
5. Does the series pass make check and qemu-iotests?

Here are some thoughts on the patch checker:

If a patch series passes successfully, no email is sent.  If a patch
series fails, an email with the errors is sent as a reply to the patch
series email thread.  The patch submitter can then respond in case there
are false positive (e.g. from checkpatch.pl) - the bot doesn't care
about replies but it tells the human reviewers and maintainers what the
patch submitter intends to do.

The bot should detect new patches within 15 minutes so humans can rely
on it to perform these basic checks before they review the patch series.

There should be a web page showing the check status of each patch series
on the mailing list.  This allows anyone to see which patch series have
passed, failed, or are pending check.

Ideas on the implementation:

The "patches" tool allows querying patch series on the mailing list.  It
can be used to apply patches to a git tree and display patches in mbox
format:

  https://github.com/stefanha/patches/tree/stefanha-tweaks

Patch series contain untrusted code so it is critical that operations
are performed inside a sandbox.  Otherwise people could send email to
qemu-devel@nongnu.org with Makefile or checkpatch.pl changes that get
executed with the bot's privileges!

Use docker or lxc to run a container for builds.  The root file system
should be fresh for each build so previous builds cannot affect later
ones.  The container cannot have external networking connectivity (for
security).

Include automated deployment scripts so bot instances can be created
easily.  Here is an example of automated deployment scripts written with
Fabric that I use for VM that builds the QEMU "patches" database:

  https://github.com/stefanha/qemu-patches

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] Patch checking bot

[Peter Maydell]

On 20 October 2014 11:25, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Hi,
> At KVM Forum 2014 we discussed a patch checking bot that automates patch
> format checking and smoke testing:
>
> 1. Did the patch submitter include Signed-off-by?
> 2. Does checkpatch.pl pass?
> 3. Does the patch apply to qemu.git/master?
> 4. Does each patch compile?
> 5. Does the series pass make check and qemu-iotests?
>
> Here are some thoughts on the patch checker:
>
> If a patch series passes successfully, no email is sent.  If a patch
> series fails, an email with the errors is sent as a reply to the patch
> series email thread.  The patch submitter can then respond in case there
> are false positive (e.g. from checkpatch.pl) - the bot doesn't care
> about replies but it tells the human reviewers and maintainers what the
> patch submitter intends to do.

Probably also worth having a feature where the cover
letter or patch can have a "patchchecker: no" line in
it to tell the bot to ignore something, so people can
avoid it sending lots of mail for patch series they
know don't apply to mainline (eg ones which depend on
a previous series).

-- PMM

Re: [Qemu-devel] Patch checking bot

[Fam Zheng]

On Mon, Oct 20, 2014 at 6:25 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Hi,
> At KVM Forum 2014 we discussed a patch checking bot that automates patch
> format checking and smoke testing:
>
> 1. Did the patch submitter include Signed-off-by?
> 2. Does checkpatch.pl pass?
> 3. Does the patch apply to qemu.git/master?
> 4. Does each patch compile?
> 5. Does the series pass make check and qemu-iotests?
>
> Here are some thoughts on the patch checker:
>
> If a patch series passes successfully, no email is sent.  If a patch
> series fails, an email with the errors is sent as a reply to the patch
> series email thread.  The patch submitter can then respond in case there
> are false positive (e.g. from checkpatch.pl) - the bot doesn't care
> about replies but it tells the human reviewers and maintainers what the
> patch submitter intends to do.
>
> The bot should detect new patches within 15 minutes so humans can rely
> on it to perform these basic checks before they review the patch series.
>
> There should be a web page showing the check status of each patch series
> on the mailing list.  This allows anyone to see which patch series have
> passed, failed, or are pending check.
>
> Ideas on the implementation:
>
> The "patches" tool allows querying patch series on the mailing list.  It
> can be used to apply patches to a git tree and display patches in mbox
> format:
>
>   https://github.com/stefanha/patches/tree/stefanha-tweaks
>
> Patch series contain untrusted code so it is critical that operations
> are performed inside a sandbox.  Otherwise people could send email to
> qemu-devel@nongnu.org with Makefile or checkpatch.pl changes that get
> executed with the bot's privileges!
>
> Use docker or lxc to run a container for builds.  The root file system
> should be fresh for each build so previous builds cannot affect later
> ones.  The container cannot have external networking connectivity (for
> security).

A small question: if the container doesn't have network connectivity,
where does the bot's checking scripts read the patch mail from?

>
> Include automated deployment scripts so bot instances can be created
> easily.  Here is an example of automated deployment scripts written with
> Fabric that I use for VM that builds the QEMU "patches" database:
>
>   https://github.com/stefanha/qemu-patches

Re: [Qemu-devel] Patch checking bot

[John Snow]

On 10/20/2014 10:08 AM, Peter Maydell wrote:
> On 20 October 2014 11:25, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> Hi,
>> At KVM Forum 2014 we discussed a patch checking bot that automates patch
>> format checking and smoke testing:
>>
>> 1. Did the patch submitter include Signed-off-by?
>> 2. Does checkpatch.pl pass?
>> 3. Does the patch apply to qemu.git/master?
>> 4. Does each patch compile?
>> 5. Does the series pass make check and qemu-iotests?
>>
>> Here are some thoughts on the patch checker:
>>
>> If a patch series passes successfully, no email is sent.  If a patch
>> series fails, an email with the errors is sent as a reply to the patch
>> series email thread.  The patch submitter can then respond in case there
>> are false positive (e.g. from checkpatch.pl) - the bot doesn't care
>> about replies but it tells the human reviewers and maintainers what the
>> patch submitter intends to do.
>
> Probably also worth having a feature where the cover
> letter or patch can have a "patchchecker: no" line in
> it to tell the bot to ignore something, so people can
> avoid it sending lots of mail for patch series they
> know don't apply to mainline (eg ones which depend on
> a previous series).
>
> -- PMM
>

Maybe it should still check what it can, but squelch the reply to list. 
Certain automatic checks may still be of value, even if it doesn't apply 
to master. If we have a website where we can check the bot and patch 
status, having some output might be nicer than allowing arbitrary skips.

Further, maybe the bot could be trained to check the patch series target 
to see what branch it's supposed to apply to and go from there. If 
people are good about labeling their stable patches, the bot should be 
able to check those as well.

This might help us tighten up and formalize our subject formatting 
rules, which would probably also help maintainer work-flow by allowing 
more robust automation. e.g., "Sorry, you submitted a patch, but I 
couldn't identify which branch/component you're trying to patch against!"

I believe at KVM Forum it was also mentioned that it'd be nice to have 
the bot reply to patch series where the proper maintainers were missed 
with a "Hey, I added in <so and so> who maintains <this file you 
touched>, please include it next time!"

-- 
—js

Re: [Qemu-devel] Patch checking bot

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1479 bytes --]

On Mon, Oct 20, 2014 at 03:08:35PM +0100, Peter Maydell wrote:
> On 20 October 2014 11:25, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > Hi,
> > At KVM Forum 2014 we discussed a patch checking bot that automates patch
> > format checking and smoke testing:
> >
> > 1. Did the patch submitter include Signed-off-by?
> > 2. Does checkpatch.pl pass?
> > 3. Does the patch apply to qemu.git/master?
> > 4. Does each patch compile?
> > 5. Does the series pass make check and qemu-iotests?
> >
> > Here are some thoughts on the patch checker:
> >
> > If a patch series passes successfully, no email is sent.  If a patch
> > series fails, an email with the errors is sent as a reply to the patch
> > series email thread.  The patch submitter can then respond in case there
> > are false positive (e.g. from checkpatch.pl) - the bot doesn't care
> > about replies but it tells the human reviewers and maintainers what the
> > patch submitter intends to do.
> 
> Probably also worth having a feature where the cover
> letter or patch can have a "patchchecker: no" line in
> it to tell the bot to ignore something, so people can
> avoid it sending lots of mail for patch series they
> know don't apply to mainline (eg ones which depend on
> a previous series).

The bot would send 1 email reply with a report of all errors.  That
doesn't seem too noisy.

That said, a header line to ignore the series is easy to implement so we
might as well.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] Patch checking bot

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

On Tue, Oct 21, 2014 at 04:19:52PM +0800, Fam Zheng wrote:
> On Mon, Oct 20, 2014 at 6:25 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > Use docker or lxc to run a container for builds.  The root file system
> > should be fresh for each build so previous builds cannot affect later
> > ones.  The container cannot have external networking connectivity (for
> > security).
> 
> A small question: if the container doesn't have network connectivity,
> where does the bot's checking scripts read the patch mail from?

The bot runs outside the container.  It uses 'patches fetch' to grab the
latest patches database every 15 minutes or so.

When a new patch series is detected, it creates a new container and
places an mbox that git-apply(1) can process inside the container.

The rest happens inside the container:

  cd qemu
  scripts/checkpatch.pl </tmp/patches.mbox
  git apply </tmp/patches.mbox
  ./configure ... && make
  make check check-block

Once the checker completes the bot can update the status web page and
send out an error report email, if necessary.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] Patch checking bot

[Andreas Färber]

[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]

Hi,

Am 22.10.2014 um 12:03 schrieb Stefan Hajnoczi:
> On Mon, Oct 20, 2014 at 03:08:35PM +0100, Peter Maydell wrote:
>> On 20 October 2014 11:25, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>> At KVM Forum 2014 we discussed a patch checking bot that automates patch
>>> format checking and smoke testing:
>>>
>>> 1. Did the patch submitter include Signed-off-by?
>>> 2. Does checkpatch.pl pass?
>>> 3. Does the patch apply to qemu.git/master?
>>> 4. Does each patch compile?
>>> 5. Does the series pass make check and qemu-iotests?
>>>
>>> Here are some thoughts on the patch checker:
>>>
>>> If a patch series passes successfully, no email is sent.  If a patch
>>> series fails, an email with the errors is sent as a reply to the patch
>>> series email thread.  The patch submitter can then respond in case there
>>> are false positive (e.g. from checkpatch.pl) - the bot doesn't care
>>> about replies but it tells the human reviewers and maintainers what the
>>> patch submitter intends to do.
>>
>> Probably also worth having a feature where the cover
>> letter or patch can have a "patchchecker: no" line in
>> it to tell the bot to ignore something, so people can
>> avoid it sending lots of mail for patch series they
>> know don't apply to mainline (eg ones which depend on
>> a previous series).
> 
> The bot would send 1 email reply with a report of all errors.  That
> doesn't seem too noisy.

Anthony had implemented that at some point and at times it spammed quite
a lot. No real objection, just reminding.

> That said, a header line to ignore the series is easy to implement so we
> might as well.

Anthony had simply detected [PATCH treename n/m] as not applying to
master iirc, which was also useful to human inbox readers.

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]