automation: Creating a patchwork instance to improve pre-commit build testing

automation: Creating a patchwork instance to improve pre-commit build testing

[Lars Kurth]

Hi all,

Wei and Doug have recently been improving [xen.git] / automation / build, implementing some of the functionality outlined in [1] (see "CI / CD" heading). 

We are intending to take every patch series that is sent to xen-devel and create a branch at https://gitlab.com/xen-project/xen.git scoped to something like "ml/<msg-id>". ml stands for mailing list. The goal is to run whatever basic CI tests we can to help reviewers and contributors. Wei and Doug have gotten things to the point where we can compile against a number of distros and they continue to expand that functionality. 

What is missing, is the capability to take patch series from the mailing list and use the posted patches to trigger the CI/CD functionality. We are planning to use Patchwork because it provides functionality to allow us to do this via the Patchwork API (https://patchwork.readthedocs.io/en/latest/api/rest/) Originally Doug suggested he could just try this via one of his own VMs, but the recent introduction of the GDPR creates risks and obligations for everyone (both individuals and companies) who processes Xen Project related mailing list data (including git meta-data). To avoid this, we were planning to run patchwork on a new Xen Project hosted VM, for the sole purpose of using the Patchwork API. 

At this stage, we are not intending to expose the patchwork UI.

# How does this impact me?
The contribution workflow is *not* impacted by this change, but once up and running the following will happen once you post a patch or patch series to xen-devel:
* Patchwork will take patch series from the mailing list and applies it
* CI/DC testing is triggered
* A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)

This does mean though that series which do not build or show other issues, will likely not be reviewed until the tests pass. This would lessen the burden on reviewers, as they will know whether the code submitted builds on a wide array of environments. 

# Supported build environments (for now)
For now, we are only supporting this on x86. The exact list of distros we build against can be found in [xen.git] / automation / build 
Arm support will be added later, once the missing pieces are in place (see [1] section "What OSs (architectures) should we support?") 

# What about checkpatch.pl?
Once clang-format changes have been completed, Wei and Doug are going to look at automating code style checks as well. Iurii Artemenko is working on clang-format improvements that will allow us to work around the issues outlined in [2]. There are still 3 missing pieces (see [3]). 

I don't think this is a controversial proposal, but I wanted to ask whether there are any objections or suggestions. I included the minios-devel list, as the Unikraft project is planning to introduce patchwork into their workflow.

Best Regards
Lars


# References
[1] https://lists.xenproject.org/archives/html/xen-devel/2018-07/msg00126.html 

[2] https://xen.markmail.org/thread/pc2dhjgbgrdcz5sb
[3] Coding style patterns used in Xen that we cannot yet support

Braces should be omitted for blocks with a single statement. e.g.,
 if ( condition )
     single_statement();
 
Comments
--------
 
 Only C style /* ... */ comments are to be used.  C++ style // comments
 should not be used.  Multi-word comments should begin with a capital
 letter.  Comments containing a single sentence may end with a full
 stop; comments containing several sentences must have a full stop
 after each sentence.
 
 Multi-line comment blocks should start and end with comment markers on
 separate lines and each line should begin with a leading '*'.
 
 /*
  * Example, multi-line comment block.
  *
  * Note beginning and end markers on separate lines and leading '*'.
  */
 
Emacs local variables
---------------------
A comment block containing local variables for emacs is permitted at
the end of files.  It should be:
 
 /*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
  * c-basic-offset: 4
  * indent-tabs-mode: nil
  * End:
  */


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Jan Beulich]

>>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
> # How does this impact me?
> The contribution workflow is *not* impacted by this change, but once up and 
> running the following will happen once you post a patch or patch series to 
> xen-devel:
> * Patchwork will take patch series from the mailing list and applies it
> * CI/DC testing is triggered
> * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
> 
> This does mean though that series which do not build or show other issues, 
> will likely not be reviewed until the tests pass. This would lessen the 
> burden on reviewers, as they will know whether the code submitted builds on a 
> wide array of environments. 

So how are dependencies between series intended to be dealt with? It
is not uncommon for someone to say "applies only on top of xyz". The
implication of "will likely not be reviewed until the tests pass" seems
unsuitable to me in such a case.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Andrew Cooper]

On 24/07/18 10:06, Jan Beulich wrote:
>>>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
>> # How does this impact me?
>> The contribution workflow is *not* impacted by this change, but once up and 
>> running the following will happen once you post a patch or patch series to 
>> xen-devel:
>> * Patchwork will take patch series from the mailing list and applies it
>> * CI/DC testing is triggered
>> * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
>>
>> This does mean though that series which do not build or show other issues, 
>> will likely not be reviewed until the tests pass. This would lessen the 
>> burden on reviewers, as they will know whether the code submitted builds on a 
>> wide array of environments. 
> So how are dependencies between series intended to be dealt with? It
> is not uncommon for someone to say "applies only on top of xyz". The
> implication of "will likely not be reviewed until the tests pass" seems
> unsuitable to me in such a case.

99% of submissions aren't "applies on top of xyz".

Alternative, how about we see about not blocking underlying patches for
unreasonable periods of time?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Jan Beulich]

>>> On 24.07.18 at 11:14, <andrew.cooper3@citrix.com> wrote:
> On 24/07/18 10:06, Jan Beulich wrote:
>>>>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
>>> # How does this impact me?
>>> The contribution workflow is *not* impacted by this change, but once up and 
>>> running the following will happen once you post a patch or patch series to 
>>> xen-devel:
>>> * Patchwork will take patch series from the mailing list and applies it
>>> * CI/DC testing is triggered
>>> * A test report will be sent as a mail to the patch or the series (aka the 
> 00 patch of the series)
>>>
>>> This does mean though that series which do not build or show other issues, 
>>> will likely not be reviewed until the tests pass. This would lessen the 
>>> burden on reviewers, as they will know whether the code submitted builds on 
> a 
>>> wide array of environments. 
>> So how are dependencies between series intended to be dealt with? It
>> is not uncommon for someone to say "applies only on top of xyz". The
>> implication of "will likely not be reviewed until the tests pass" seems
>> unsuitable to me in such a case.
> 
> 99% of submissions aren't "applies on top of xyz".
> 
> Alternative, how about we see about not blocking underlying patches for
> unreasonable periods of time?

Well, I'm all for it, but I don't expect us to get there any time soon.
Just take the recent example of my indirect call patching series
depending on another series that I had submitted over 4 months ago.
Along those lines, the oldest (non-RFC) series I have in my to-be-
reviewed folder is from November - what if the author submitted
anything depending on it?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Wei Liu]

On Tue, Jul 24, 2018 at 03:06:08AM -0600, Jan Beulich wrote:
> >>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
> > # How does this impact me?
> > The contribution workflow is *not* impacted by this change, but once up and 
> > running the following will happen once you post a patch or patch series to 
> > xen-devel:
> > * Patchwork will take patch series from the mailing list and applies it
> > * CI/DC testing is triggered
> > * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
> > 
> > This does mean though that series which do not build or show other issues, 
> > will likely not be reviewed until the tests pass. This would lessen the 
> > burden on reviewers, as they will know whether the code submitted builds on a 
> > wide array of environments. 
> 
> So how are dependencies between series intended to be dealt with? It
> is not uncommon for someone to say "applies only on top of xyz". The
> implication of "will likely not be reviewed until the tests pass" seems
> unsuitable to me in such a case.
> 

We have been asking everyone to rebase to staging before posting a new
version for a long time.  It is natural for the bot to assume that
everything should apply on top of staging. That would provide most value
to the community.

For special cases like you just mention, we should aim to provide
mechanisms to manually appoint a branch to be tested.

Wei.

> Jan
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Jan Beulich]

>>> On 24.07.18 at 11:24, <wei.liu2@citrix.com> wrote:
> On Tue, Jul 24, 2018 at 03:06:08AM -0600, Jan Beulich wrote:
>> >>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
>> > # How does this impact me?
>> > The contribution workflow is *not* impacted by this change, but once up and 
> 
>> > running the following will happen once you post a patch or patch series to 
>> > xen-devel:
>> > * Patchwork will take patch series from the mailing list and applies it
>> > * CI/DC testing is triggered
>> > * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
>> > 
>> > This does mean though that series which do not build or show other issues, 
>> > will likely not be reviewed until the tests pass. This would lessen the 
>> > burden on reviewers, as they will know whether the code submitted builds on a 
>> > wide array of environments. 
>> 
>> So how are dependencies between series intended to be dealt with? It
>> is not uncommon for someone to say "applies only on top of xyz". The
>> implication of "will likely not be reviewed until the tests pass" seems
>> unsuitable to me in such a case.
>> 
> 
> We have been asking everyone to rebase to staging before posting a new
> version for a long time.  It is natural for the bot to assume that
> everything should apply on top of staging. That would provide most value
> to the community.
> 
> For special cases like you just mention, we should aim to provide
> mechanisms to manually appoint a branch to be tested.

I'm afraid I disagree again: Tools used should not be dictated. I'm
using quilt, not git for my work, and hence I don't maintain any
branches anywhere.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Wei Liu]

On Tue, Jul 24, 2018 at 03:34:51AM -0600, Jan Beulich wrote:
> >>> On 24.07.18 at 11:24, <wei.liu2@citrix.com> wrote:
> > On Tue, Jul 24, 2018 at 03:06:08AM -0600, Jan Beulich wrote:
> >> >>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
> >> > # How does this impact me?
> >> > The contribution workflow is *not* impacted by this change, but once up and 
> > 
> >> > running the following will happen once you post a patch or patch series to 
> >> > xen-devel:
> >> > * Patchwork will take patch series from the mailing list and applies it
> >> > * CI/DC testing is triggered
> >> > * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
> >> > 
> >> > This does mean though that series which do not build or show other issues, 
> >> > will likely not be reviewed until the tests pass. This would lessen the 
> >> > burden on reviewers, as they will know whether the code submitted builds on a 
> >> > wide array of environments. 
> >> 
> >> So how are dependencies between series intended to be dealt with? It
> >> is not uncommon for someone to say "applies only on top of xyz". The
> >> implication of "will likely not be reviewed until the tests pass" seems
> >> unsuitable to me in such a case.
> >> 
> > 
> > We have been asking everyone to rebase to staging before posting a new
> > version for a long time.  It is natural for the bot to assume that
> > everything should apply on top of staging. That would provide most value
> > to the community.
> > 
> > For special cases like you just mention, we should aim to provide
> > mechanisms to manually appoint a branch to be tested.
> 
> I'm afraid I disagree again: Tools used should not be dictated. I'm
> using quilt, not git for my work, and hence I don't maintain any
> branches anywhere.

Alright.

First, I don't think I said that only git would be supported.
Git is the most prevalent VCS nowadays, and most developers use it, so
it would make sense to support it first.  If you want quilt, we can
certainly look into that. But I'm afraid if you don't say what you
specifically need, nothing can be done in that regard.

Second, it is up to individual whether they want to use a certain tool
or not. If you don't want to use this infrastructure for whatever
reason, that's OK. You're only missing out all the work in the community
has done, but you should be able to use your own workflow just fine.

Wei.

> 
> Jan
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Jan Beulich]

>>> On 24.07.18 at 11:43, <wei.liu2@citrix.com> wrote:
> On Tue, Jul 24, 2018 at 03:34:51AM -0600, Jan Beulich wrote:
>> >>> On 24.07.18 at 11:24, <wei.liu2@citrix.com> wrote:
>> > On Tue, Jul 24, 2018 at 03:06:08AM -0600, Jan Beulich wrote:
>> >> >>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
>> >> > # How does this impact me?
>> >> > The contribution workflow is *not* impacted by this change, but once up and 
>> > 
>> >> > running the following will happen once you post a patch or patch series to 
>> >> > xen-devel:
>> >> > * Patchwork will take patch series from the mailing list and applies it
>> >> > * CI/DC testing is triggered
>> >> > * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
>> >> > 
>> >> > This does mean though that series which do not build or show other issues, 
>> >> > will likely not be reviewed until the tests pass. This would lessen the 
>> >> > burden on reviewers, as they will know whether the code submitted builds on a 
>> >> > wide array of environments. 
>> >> 
>> >> So how are dependencies between series intended to be dealt with? It
>> >> is not uncommon for someone to say "applies only on top of xyz". The
>> >> implication of "will likely not be reviewed until the tests pass" seems
>> >> unsuitable to me in such a case.
>> >> 
>> > 
>> > We have been asking everyone to rebase to staging before posting a new
>> > version for a long time.  It is natural for the bot to assume that
>> > everything should apply on top of staging. That would provide most value
>> > to the community.
>> > 
>> > For special cases like you just mention, we should aim to provide
>> > mechanisms to manually appoint a branch to be tested.
>> 
>> I'm afraid I disagree again: Tools used should not be dictated. I'm
>> using quilt, not git for my work, and hence I don't maintain any
>> branches anywhere.
> 
> Alright.
> 
> First, I don't think I said that only git would be supported.
> Git is the most prevalent VCS nowadays, and most developers use it, so
> it would make sense to support it first.  If you want quilt, we can
> certainly look into that. But I'm afraid if you don't say what you
> specifically need, nothing can be done in that regard.

Well, if you thought of other than git, then I'm afraid I lack
understanding of where such a "branch" should be coming from.
My first and foremost requirement is that, as stated pretty close
to the top, the contribution workflow be *not* impacted. Any
setting up of anything that I'd need to do would be contrary to
that.

> Second, it is up to individual whether they want to use a certain tool
> or not. If you don't want to use this infrastructure for whatever
> reason, that's OK. You're only missing out all the work in the community
> has done, but you should be able to use your own workflow just fine.

Then I maybe misunderstood Lars'es mail: I've gained the
impression that the picking up of patches would be automatic,
i.e. without me telling to system to do so. As it would presumably
send its (failure) mails back to the author, I'd expect to get what
effectively is spam in the described case.

I'm afraid my personal bar for any such automation is pretty
high: There must not ever be any negative effect from such an
addition. Positive effects would of course be very welcome. I
realize this is an unrealistic goal, but it should at least come
close (perhaps after some initial learning phase). But this implies
that at least in theory it is possible to come close in the first
place, which I can't take for given with the information I've been
provided so far.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Wei Liu]

On Tue, Jul 24, 2018 at 04:04:05AM -0600, Jan Beulich wrote:
> >>> On 24.07.18 at 11:43, <wei.liu2@citrix.com> wrote:
> > On Tue, Jul 24, 2018 at 03:34:51AM -0600, Jan Beulich wrote:
> >> >>> On 24.07.18 at 11:24, <wei.liu2@citrix.com> wrote:
> >> > On Tue, Jul 24, 2018 at 03:06:08AM -0600, Jan Beulich wrote:
> >> >> >>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
> >> >> > # How does this impact me?
> >> >> > The contribution workflow is *not* impacted by this change, but once up and 
> >> > 
> >> >> > running the following will happen once you post a patch or patch series to 
> >> >> > xen-devel:
> >> >> > * Patchwork will take patch series from the mailing list and applies it
> >> >> > * CI/DC testing is triggered
> >> >> > * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
> >> >> > 
> >> >> > This does mean though that series which do not build or show other issues, 
> >> >> > will likely not be reviewed until the tests pass. This would lessen the 
> >> >> > burden on reviewers, as they will know whether the code submitted builds on a 
> >> >> > wide array of environments. 
> >> >> 
> >> >> So how are dependencies between series intended to be dealt with? It
> >> >> is not uncommon for someone to say "applies only on top of xyz". The
> >> >> implication of "will likely not be reviewed until the tests pass" seems
> >> >> unsuitable to me in such a case.
> >> >> 
> >> > 
> >> > We have been asking everyone to rebase to staging before posting a new
> >> > version for a long time.  It is natural for the bot to assume that
> >> > everything should apply on top of staging. That would provide most value
> >> > to the community.
> >> > 
> >> > For special cases like you just mention, we should aim to provide
> >> > mechanisms to manually appoint a branch to be tested.
> >> 
> >> I'm afraid I disagree again: Tools used should not be dictated. I'm
> >> using quilt, not git for my work, and hence I don't maintain any
> >> branches anywhere.
> > 
> > Alright.
> > 
> > First, I don't think I said that only git would be supported.
> > Git is the most prevalent VCS nowadays, and most developers use it, so
> > it would make sense to support it first.  If you want quilt, we can
> > certainly look into that. But I'm afraid if you don't say what you
> > specifically need, nothing can be done in that regard.
> 
> Well, if you thought of other than git, then I'm afraid I lack
> understanding of where such a "branch" should be coming from.

Well even CVS has the concept of branch. Mercurial also has branch (but
not the same as git branch). But I admit I was mostly thinking about git
branches. It would be strange for me to not think about git as first
approximation because Xen uses git as the official VCS.

Anyway, I don't see much point in arguing this more. I can only say this
again: if you want other tools, this can be done in principle, but at
the very least you need to provide insight on your workflow, and the
community will see about what to do.

> My first and foremost requirement is that, as stated pretty close
> to the top, the contribution workflow be *not* impacted. Any
> setting up of anything that I'd need to do would be contrary to
> that.
> 

If you don't use it, you don't need to set up anything (other than a
filter?), you won't be impacted. Does this make sense?

> > Second, it is up to individual whether they want to use a certain tool
> > or not. If you don't want to use this infrastructure for whatever
> > reason, that's OK. You're only missing out all the work in the community
> > has done, but you should be able to use your own workflow just fine.
> 
> Then I maybe misunderstood Lars'es mail: I've gained the
> impression that the picking up of patches would be automatic,
> i.e. without me telling to system to do so. As it would presumably
> send its (failure) mails back to the author, I'd expect to get what
> effectively is spam in the described case.
> 
> I'm afraid my personal bar for any such automation is pretty
> high: There must not ever be any negative effect from such an
> addition. Positive effects would of course be very welcome. I
> realize this is an unrealistic goal, but it should at least come
> close (perhaps after some initial learning phase). But this implies
> that at least in theory it is possible to come close in the first
> place, which I can't take for given with the information I've been
> provided so far.

Then I'm afraid the only suggestion I get for you at the moment is to
add a filter to dump those emails to /dev/null -- you already realised
that's an unrealistic goal (at least at the beginning).

Wei.

> 
> Jan
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Lars Kurth]

On 24/07/2018, 11:19, "Wei Liu" <wei.liu2@citrix.com> wrote:
    On Tue, Jul 24, 2018 at 04:04:05AM -0600, Jan Beulich wrote:
    > I'm afraid my personal bar for any such automation is pretty
    > high: There must not ever be any negative effect from such an
    > addition. Positive effects would of course be very welcome. I
    > realize this is an unrealistic goal, but it should at least come
    > close (perhaps after some initial learning phase). But this implies
    > that at least in theory it is possible to come close in the first
    > place, which I can't take for given with the information I've been
    > provided so far.
    
    Then I'm afraid the only suggestion I get for you at the moment is to
    add a filter to dump those emails to /dev/null -- you already realised
    that's an unrealistic goal (at least at the beginning).
    
    Wei.
    
First of all, there should only be mail (aka spam) if there was a failure. 

Hopefully such failures will be fairly rare in the long term: as people 
learn that  they are expected to submit patches that build on all 
platforms, one  would expect that they test this *before* submitting 
patches.

And maybe we can gradually phase this in: aka have the contributor 
add something like CC xengit-testbot@xenproject.org to the series.
At some point later, we could always trigger a CI build. Or we could 
add a tag in the subject line, e.g. [CI-TESTED PATCH ...] or something 
like it, which triggers the test run.

Maybe it would also be possible that contributors can send patches
to xengit-testbot@xenproject.org without CC'ing xen-devel
to test whether their patches would pass the CI test. Or provide 
some alternative to doing so. 

Regards
Lars


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Julien Grall]

Hi Lars,

On 24/07/18 11:33, Lars Kurth wrote:
> 
> On 24/07/2018, 11:19, "Wei Liu" <wei.liu2@citrix.com> wrote:
>      On Tue, Jul 24, 2018 at 04:04:05AM -0600, Jan Beulich wrote:
>      > I'm afraid my personal bar for any such automation is pretty
>      > high: There must not ever be any negative effect from such an
>      > addition. Positive effects would of course be very welcome. I
>      > realize this is an unrealistic goal, but it should at least come
>      > close (perhaps after some initial learning phase). But this implies
>      > that at least in theory it is possible to come close in the first
>      > place, which I can't take for given with the information I've been
>      > provided so far.
>      
>      Then I'm afraid the only suggestion I get for you at the moment is to
>      add a filter to dump those emails to /dev/null -- you already realised
>      that's an unrealistic goal (at least at the beginning).
>      
>      Wei.
>      
> First of all, there should only be mail (aka spam) if there was a failure.

This seems a little strange to only send e-mail on failure. How do you 
differentiate between the bot has successfully tested that series and 
the series is still in queue then?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Lars Kurth]

On 24/07/2018, 11:50, "Julien Grall" <julien.grall@arm.com> wrote:

    Hi Lars,
    
    On 24/07/18 11:33, Lars Kurth wrote:
    > 
    > On 24/07/2018, 11:19, "Wei Liu" <wei.liu2@citrix.com> wrote:
    >      On Tue, Jul 24, 2018 at 04:04:05AM -0600, Jan Beulich wrote:
    >      > I'm afraid my personal bar for any such automation is pretty
    >      > high: There must not ever be any negative effect from such an
    >      > addition. Positive effects would of course be very welcome. I
    >      > realize this is an unrealistic goal, but it should at least come
    >      > close (perhaps after some initial learning phase). But this implies
    >      > that at least in theory it is possible to come close in the first
    >      > place, which I can't take for given with the information I've been
    >      > provided so far.
    >      
    >      Then I'm afraid the only suggestion I get for you at the moment is to
    >      add a filter to dump those emails to /dev/null -- you already realised
    >      that's an unrealistic goal (at least at the beginning).
    >      
    >      Wei.
    >      
    > First of all, there should only be mail (aka spam) if there was a failure.
    
    This seems a little strange to only send e-mail on failure. How do you 
    differentiate between the bot has successfully tested that series and 
    the series is still in queue then?
    
Yes, that would be a trade-off to minimize "spam"

It seems to me there are a number of options we have and thus some decisions
that need to be made.
 
1: Do we trigger a CI cycle for *every* patch?
2: Do we have an opt-in or op-out (e.g. through a tag, a specific CC, etc.) for patches
3: Do we report results back to xen-devel or to a separate list
Looking at Linux 0 day, they report failures to a separate list - see https://lists.01.org/pipermail/kbuild-all/2018-July/thread.html
They also only seem to report failures

I am not quite sure what QEMU does. But I can't see any bot messages on their list archives

4: Who else, besides the author should get a mail
The patch submitters should definitely get a mail, the question is whether people on the CC list should also get one

5: Do we report back on success or only on failure?
See question by Julien

6: What exactly do we report back
Aka what is in the actual mail


 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Anthony PERARD]

On Tue, Jul 24, 2018 at 11:23:34AM +0000, Lars Kurth wrote:
> I am not quite sure what QEMU does. But I can't see any bot messages on their list archives

The bot from: is no-reply@patchew.org, for e.g.:
- coding style:
https://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg01428.html
- build failure:
https://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg01329.html

Those mails are been CCed to all. Only failure are sent.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[George Dunlap]

On 07/24/2018 12:23 PM, Lars Kurth wrote:
> 
> On 24/07/2018, 11:50, "Julien Grall" <julien.grall@arm.com> wrote:
> 
>     Hi Lars,
>     
>     On 24/07/18 11:33, Lars Kurth wrote:
>     > 
>     > On 24/07/2018, 11:19, "Wei Liu" <wei.liu2@citrix.com> wrote:
>     >      On Tue, Jul 24, 2018 at 04:04:05AM -0600, Jan Beulich wrote:
>     >      > I'm afraid my personal bar for any such automation is pretty
>     >      > high: There must not ever be any negative effect from such an
>     >      > addition. Positive effects would of course be very welcome. I
>     >      > realize this is an unrealistic goal, but it should at least come
>     >      > close (perhaps after some initial learning phase). But this implies
>     >      > that at least in theory it is possible to come close in the first
>     >      > place, which I can't take for given with the information I've been
>     >      > provided so far.
>     >      
>     >      Then I'm afraid the only suggestion I get for you at the moment is to
>     >      add a filter to dump those emails to /dev/null -- you already realised
>     >      that's an unrealistic goal (at least at the beginning).
>     >      
>     >      Wei.
>     >      
>     > First of all, there should only be mail (aka spam) if there was a failure.
>     
>     This seems a little strange to only send e-mail on failure. How do you 
>     differentiate between the bot has successfully tested that series and 
>     the series is still in queue then?
>     
> Yes, that would be a trade-off to minimize "spam"
> 
> It seems to me there are a number of options we have and thus some decisions
> that need to be made.
>  
> 1: Do we trigger a CI cycle for *every* patch?

In a world with infinite resources, yes, because we want to detect
broken bisections.  My guess is that this would be too
resource-intensive for the real world.

No matter what, I'd prefer only one email per series; Definitely *don't*
want a success email for every patch.

> 2: Do we have an opt-in or op-out (e.g. through a tag, a specific CC, etc.) for patches

Opt-out.

> 3: Do we report results back to xen-devel or to a separate list
> Looking at Linux 0 day, they report failures to a separate list - see https://lists.01.org/pipermail/kbuild-all/2018-July/thread.html
> They also only seem to report failures
> 
> I am not quite sure what QEMU does. But I can't see any bot messages on their list archives
[snip]
> 5: Do we report back on success or only on failure?
> See question by Julien

I'd start with having the bot respond to 00/NN exactly once, both on
success and failure.

> 4: Who else, besides the author should get a mail
> The patch submitters should definitely get a mail, the question is whether people on the CC list should also get one

I think the bot should reply-to-all.  Maybe we can add an opt-out to our
website, so that the bot won't reply to you if you don't want it to.

> 6: What exactly do we report back
> Aka what is in the actual mail

A link to the git branch it created (if the patch applied), or a snippet
of the rejection message if it didn't.

Success / failure, with a link to a page containing the various tests
run, so people can see which one failed and investigate the failures.

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[George Dunlap]

On 07/24/2018 04:26 PM, George Dunlap wrote:
> On 07/24/2018 12:23 PM, Lars Kurth wrote:
>>
>> On 24/07/2018, 11:50, "Julien Grall" <julien.grall@arm.com> wrote:
>>
>>     Hi Lars,
>>     
>>     On 24/07/18 11:33, Lars Kurth wrote:
>>     > 
>>     > On 24/07/2018, 11:19, "Wei Liu" <wei.liu2@citrix.com> wrote:
>>     >      On Tue, Jul 24, 2018 at 04:04:05AM -0600, Jan Beulich wrote:
>>     >      > I'm afraid my personal bar for any such automation is pretty
>>     >      > high: There must not ever be any negative effect from such an
>>     >      > addition. Positive effects would of course be very welcome. I
>>     >      > realize this is an unrealistic goal, but it should at least come
>>     >      > close (perhaps after some initial learning phase). But this implies
>>     >      > that at least in theory it is possible to come close in the first
>>     >      > place, which I can't take for given with the information I've been
>>     >      > provided so far.
>>     >      
>>     >      Then I'm afraid the only suggestion I get for you at the moment is to
>>     >      add a filter to dump those emails to /dev/null -- you already realised
>>     >      that's an unrealistic goal (at least at the beginning).
>>     >      
>>     >      Wei.
>>     >      
>>     > First of all, there should only be mail (aka spam) if there was a failure.
>>     
>>     This seems a little strange to only send e-mail on failure. How do you 
>>     differentiate between the bot has successfully tested that series and 
>>     the series is still in queue then?
>>     
>> Yes, that would be a trade-off to minimize "spam"
>>
>> It seems to me there are a number of options we have and thus some decisions
>> that need to be made.
>>  
>> 1: Do we trigger a CI cycle for *every* patch?
> 
> In a world with infinite resources, yes, because we want to detect
> broken bisections.  My guess is that this would be too
> resource-intensive for the real world.
> 
> No matter what, I'd prefer only one email per series; Definitely *don't*
> want a success email for every patch.

What about having "check-bisectability" as a separate test?  Rather than
doing a full build test from a clean tree for every possible distro, we
could do something like

for patch in $patches; do
  patch -p1 < $patch
  make
done

That should catch most bisection-breaking issues without being overly
resource-intensive.

From the CI's perspective, you'd be running on the whole series, and
check-bisectability would be a single sub-test.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1163 bytes --]

FWIW,

On Tue, 2018-07-24 at 16:26 +0100, George Dunlap wrote:
> On 07/24/2018 12:23 PM, Lars Kurth wrote:
> > 
> > It seems to me there are a number of options we have and thus some
> > decisions
> > that need to be made.

> > 2: Do we have an opt-in or op-out (e.g. through a tag, a specific
> > CC, etc.) for patches
> 
> Opt-out.
>
+1

> > 5: Do we report back on success or only on failure?
> > See question by Julien
> 
> I'd start with having the bot respond to 00/NN exactly once, both on
> success and failure.
> 
+1

> > 4: Who else, besides the author should get a mail
> > The patch submitters should definitely get a mail, the question is
> > whether people on the CC list should also get one
> 
> I think the bot should reply-to-all.  Maybe we can add an opt-out to
> our
> website, so that the bot won't reply to you if you don't want it to.
> 
+1

:-)

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Doug Goldstein]

On Tue, Jul 24, 2018 at 04:04:05AM -0600, Jan Beulich wrote:
> >>> On 24.07.18 at 11:43, <wei.liu2@citrix.com> wrote:
> > On Tue, Jul 24, 2018 at 03:34:51AM -0600, Jan Beulich wrote:
> >> >>> On 24.07.18 at 11:24, <wei.liu2@citrix.com> wrote:
> >> > On Tue, Jul 24, 2018 at 03:06:08AM -0600, Jan Beulich wrote:
> >> >> >>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
> >> >> > # How does this impact me?
> >> >> > The contribution workflow is *not* impacted by this change, but once up and 
> >> > 
> >> >> > running the following will happen once you post a patch or patch series to 
> >> >> > xen-devel:
> >> >> > * Patchwork will take patch series from the mailing list and applies it
> >> >> > * CI/DC testing is triggered
> >> >> > * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
> >> >> > 
> >> >> > This does mean though that series which do not build or show other issues, 
> >> >> > will likely not be reviewed until the tests pass. This would lessen the 
> >> >> > burden on reviewers, as they will know whether the code submitted builds on a 
> >> >> > wide array of environments. 
> >> >> 
> >> >> So how are dependencies between series intended to be dealt with? It
> >> >> is not uncommon for someone to say "applies only on top of xyz". The
> >> >> implication of "will likely not be reviewed until the tests pass" seems
> >> >> unsuitable to me in such a case.
> >> >> 
> >> > 
> >> > We have been asking everyone to rebase to staging before posting a new
> >> > version for a long time.  It is natural for the bot to assume that
> >> > everything should apply on top of staging. That would provide most value
> >> > to the community.
> >> > 
> >> > For special cases like you just mention, we should aim to provide
> >> > mechanisms to manually appoint a branch to be tested.
> >> 
> >> I'm afraid I disagree again: Tools used should not be dictated. I'm
> >> using quilt, not git for my work, and hence I don't maintain any
> >> branches anywhere.
> > 
> > Alright.
> > 
> > First, I don't think I said that only git would be supported.
> > Git is the most prevalent VCS nowadays, and most developers use it, so
> > it would make sense to support it first.  If you want quilt, we can
> > certainly look into that. But I'm afraid if you don't say what you
> > specifically need, nothing can be done in that regard.
> 
> Well, if you thought of other than git, then I'm afraid I lack
> understanding of where such a "branch" should be coming from.
> My first and foremost requirement is that, as stated pretty close
> to the top, the contribution workflow be *not* impacted. Any
> setting up of anything that I'd need to do would be contrary to
> that.
> 
> > Second, it is up to individual whether they want to use a certain tool
> > or not. If you don't want to use this infrastructure for whatever
> > reason, that's OK. You're only missing out all the work in the community
> > has done, but you should be able to use your own workflow just fine.
> 
> Then I maybe misunderstood Lars'es mail: I've gained the
> impression that the picking up of patches would be automatic,
> i.e. without me telling to system to do so. As it would presumably
> send its (failure) mails back to the author, I'd expect to get what
> effectively is spam in the described case.
> 
> I'm afraid my personal bar for any such automation is pretty
> high: There must not ever be any negative effect from such an
> addition. Positive effects would of course be very welcome. I
> realize this is an unrealistic goal, but it should at least come
> close (perhaps after some initial learning phase). But this implies
> that at least in theory it is possible to come close in the first
> place, which I can't take for given with the information I've been
> provided so far.
> 
> Jan

I hope you're not advocating for no progress until the perfect system is
achieved without giving anyone the opportunity to develop a system since
its impossible to develop a perfect system in the first go.

The ultimate goal here is to take patches that are posted to the mailing
list, apply them on top of staging and build them against a variety of
compiler combos coming from different distros. The results would then be
emailed as a reply to the cover letter. The idea is that this would help
maintainers/reviewers out as they could tell the submitter that it won't
get reviewed until it at least compiles.

The first improvement to the entire system I'd like to make is automatic
code-style checking. But that is depending on clang-format landing the
Xen code-style plugin.

Would you at least agree that this would be useful to some maintainers
and something some subset of folks would like to see? This isn't
necessarily targeted at code that you personally submit but folks that
are less frequent contributors. I've seen on numerous occasions a new
contributor making a patch against an outdated branch and this would
help there for example.

--
Doug

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Lars Kurth]

On 24/07/2018, 10:34, "Jan Beulich" <JBeulich@suse.com> wrote:

    >>> On 24.07.18 at 11:24, <wei.liu2@citrix.com> wrote:
    > On Tue, Jul 24, 2018 at 03:06:08AM -0600, Jan Beulich wrote:
    >> >>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
    >> > # How does this impact me?
    >> > The contribution workflow is *not* impacted by this change, but once up and 
    > 
    >> > running the following will happen once you post a patch or patch series to 
    >> > xen-devel:
    >> > * Patchwork will take patch series from the mailing list and applies it
    >> > * CI/DC testing is triggered
    >> > * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
    >> > 
    >> > This does mean though that series which do not build or show other issues, 
    >> > will likely not be reviewed until the tests pass. This would lessen the 
    >> > burden on reviewers, as they will know whether the code submitted builds on a 
    >> > wide array of environments. 
    >> 
    >> So how are dependencies between series intended to be dealt with? It
    >> is not uncommon for someone to say "applies only on top of xyz". The
    >> implication of "will likely not be reviewed until the tests pass" seems
    >> unsuitable to me in such a case.
    >> 
    > 
    > We have been asking everyone to rebase to staging before posting a new
    > version for a long time.  It is natural for the bot to assume that
    > everything should apply on top of staging. That would provide most value
    > to the community.
    > 
    > For special cases like you just mention, we should aim to provide
    > mechanisms to manually appoint a branch to be tested.
    
    I'm afraid I disagree again: Tools used should not be dictated. I'm
    using quilt, not git for my work, and hence I don't maintain any
    branches anywhere.
    
Jan, I have to clarify what I meant by "will likely not be reviewed until the tests pass".

At the end of the day, the CI loop is simply providing reviewers with more information. Reviewers then make a decision on whether they want to review a series or not.

Now in most cases, reviewers will rightly not want to review series which don't build on all platforms, which is why I added "will likely not be reviewed until the tests pass". 
But in some cases - such as the case of dependencies - there is a reason to still review the series. And there may be other reasons to do so.

Regards
Lars
    
    

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[George Dunlap]

On 07/24/2018 10:34 AM, Jan Beulich wrote:
>>>> On 24.07.18 at 11:24, <wei.liu2@citrix.com> wrote:
>> On Tue, Jul 24, 2018 at 03:06:08AM -0600, Jan Beulich wrote:
>>>>>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
>>>> # How does this impact me?
>>>> The contribution workflow is *not* impacted by this change, but once up and 
>>
>>>> running the following will happen once you post a patch or patch series to 
>>>> xen-devel:
>>>> * Patchwork will take patch series from the mailing list and applies it
>>>> * CI/DC testing is triggered
>>>> * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
>>>>
>>>> This does mean though that series which do not build or show other issues, 
>>>> will likely not be reviewed until the tests pass. This would lessen the 
>>>> burden on reviewers, as they will know whether the code submitted builds on a 
>>>> wide array of environments. 
>>>
>>> So how are dependencies between series intended to be dealt with? It
>>> is not uncommon for someone to say "applies only on top of xyz". The
>>> implication of "will likely not be reviewed until the tests pass" seems
>>> unsuitable to me in such a case.
>>>
>>
>> We have been asking everyone to rebase to staging before posting a new
>> version for a long time.  It is natural for the bot to assume that
>> everything should apply on top of staging. That would provide most value
>> to the community.
>>
>> For special cases like you just mention, we should aim to provide
>> mechanisms to manually appoint a branch to be tested.
> 
> I'm afraid I disagree again: Tools used should not be dictated. I'm
> using quilt, not git for my work, and hence I don't maintain any
> branches anywhere.

Well it's never been our habit to review patch series sent against
random private branches.  (x86-next not being a random private branch.)
The idea would be that you put a tag in the message somewhere that
indicates what the patchbot should do.  This would probably be just the
message-id of the patch series, given that (steady state) your previous
series would have had the bot reply to it with a link.  Something like this:

Prerequisite-series: <5B506A6E02000078001D5D17@prv1-mh.provo.novell.com>

If the sender doesn't add the prerequisite series, then of course it
won't apply; but the reviewer can choose to ignore the failure in that case.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Doug Goldstein]

On Tue, Jul 24, 2018 at 03:32:09PM +0100, George Dunlap wrote:
> On 07/24/2018 10:34 AM, Jan Beulich wrote:
> >>>> On 24.07.18 at 11:24, <wei.liu2@citrix.com> wrote:
> >> On Tue, Jul 24, 2018 at 03:06:08AM -0600, Jan Beulich wrote:
> >>>>>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
> >>>> # How does this impact me?
> >>>> The contribution workflow is *not* impacted by this change, but once up and 
> >>
> >>>> running the following will happen once you post a patch or patch series to 
> >>>> xen-devel:
> >>>> * Patchwork will take patch series from the mailing list and applies it
> >>>> * CI/DC testing is triggered
> >>>> * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
> >>>>
> >>>> This does mean though that series which do not build or show other issues, 
> >>>> will likely not be reviewed until the tests pass. This would lessen the 
> >>>> burden on reviewers, as they will know whether the code submitted builds on a 
> >>>> wide array of environments. 
> >>>
> >>> So how are dependencies between series intended to be dealt with? It
> >>> is not uncommon for someone to say "applies only on top of xyz". The
> >>> implication of "will likely not be reviewed until the tests pass" seems
> >>> unsuitable to me in such a case.
> >>>
> >>
> >> We have been asking everyone to rebase to staging before posting a new
> >> version for a long time.  It is natural for the bot to assume that
> >> everything should apply on top of staging. That would provide most value
> >> to the community.
> >>
> >> For special cases like you just mention, we should aim to provide
> >> mechanisms to manually appoint a branch to be tested.
> > 
> > I'm afraid I disagree again: Tools used should not be dictated. I'm
> > using quilt, not git for my work, and hence I don't maintain any
> > branches anywhere.
> 
> Well it's never been our habit to review patch series sent against
> random private branches.  (x86-next not being a random private branch.)
> The idea would be that you put a tag in the message somewhere that
> indicates what the patchbot should do.  This would probably be just the
> message-id of the patch series, given that (steady state) your previous
> series would have had the bot reply to it with a link.  Something like this:
> 
> Prerequisite-series: <5B506A6E02000078001D5D17@prv1-mh.provo.novell.com>
> 
> If the sender doesn't add the prerequisite series, then of course it
> won't apply; but the reviewer can choose to ignore the failure in that case.
> 
>  -George

So fwiw, there's a tool called git-series (the author says he's working
on integrating its functionality into the git upstream) that does
exactly this. Most of my recent patches have been sent with it and
you'll actually see what commit from staging my patches are based on and
if I wanted I could record instead the message-id of a posted series it
would depend on. I've spoken with the patchwork folks about support that
tag as well.

--
Doug

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Minios-devel] automation: Creating a patchwork instance to improve pre-commit build testing

[Lars Kurth]

[-- Attachment #1.1: Type: text/plain, Size: 2058 bytes --]



> On 24 Jul 2018, at 10:24, Wei Liu <wei.liu2@citrix.com> wrote:
> 
> On Tue, Jul 24, 2018 at 03:06:08AM -0600, Jan Beulich wrote:
>>>>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
>>> # How does this impact me?
>>> The contribution workflow is *not* impacted by this change, but once up and 
>>> running the following will happen once you post a patch or patch series to 
>>> xen-devel:
>>> * Patchwork will take patch series from the mailing list and applies it
>>> * CI/DC testing is triggered
>>> * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
>>> 
>>> This does mean though that series which do not build or show other issues, 
>>> will likely not be reviewed until the tests pass. This would lessen the 
>>> burden on reviewers, as they will know whether the code submitted builds on a 
>>> wide array of environments. 
>> 
>> So how are dependencies between series intended to be dealt with? It
>> is not uncommon for someone to say "applies only on top of xyz". The
>> implication of "will likely not be reviewed until the tests pass" seems
>> unsuitable to me in such a case.
>> 
> 
> We have been asking everyone to rebase to staging before posting a new
> version for a long time.  It is natural for the bot to assume that
> everything should apply on top of staging. That would provide most value
> to the community.
> 
> For special cases like you just mention, we should aim to provide
> mechanisms to manually appoint a branch to be tested.

Wei, Doug: I have another question, which is mainly for my own understanding. 

Right now we allow posting of patches to Linux, Qemu, xen.git, OSSTEST, ... to xen-devel. The planned CI infrastructure only applies to xen.git. Have you thought about how to handle such cases? 

We probably don't want to spam Linux and Qemu lists with results from a test bot. 
And we want to minimise false positives. Some patches may be identifiable through subject lines (e.g. OSSTEST in subject lines)

Lars



[-- Attachment #1.2: Type: text/html, Size: 9409 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Minios-devel] automation: Creating a patchwork instance to improve pre-commit build testing

[Wei Liu]

On Tue, Jul 24, 2018 at 10:38:24AM +0100, Lars Kurth wrote:
> 
> 
> > On 24 Jul 2018, at 10:24, Wei Liu <wei.liu2@citrix.com> wrote:
> > 
> > On Tue, Jul 24, 2018 at 03:06:08AM -0600, Jan Beulich wrote:
> >>>>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
> >>> # How does this impact me?
> >>> The contribution workflow is *not* impacted by this change, but once up and 
> >>> running the following will happen once you post a patch or patch series to 
> >>> xen-devel:
> >>> * Patchwork will take patch series from the mailing list and applies it
> >>> * CI/DC testing is triggered
> >>> * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
> >>> 
> >>> This does mean though that series which do not build or show other issues, 
> >>> will likely not be reviewed until the tests pass. This would lessen the 
> >>> burden on reviewers, as they will know whether the code submitted builds on a 
> >>> wide array of environments. 
> >> 
> >> So how are dependencies between series intended to be dealt with? It
> >> is not uncommon for someone to say "applies only on top of xyz". The
> >> implication of "will likely not be reviewed until the tests pass" seems
> >> unsuitable to me in such a case.
> >> 
> > 
> > We have been asking everyone to rebase to staging before posting a new
> > version for a long time.  It is natural for the bot to assume that
> > everything should apply on top of staging. That would provide most value
> > to the community.
> > 
> > For special cases like you just mention, we should aim to provide
> > mechanisms to manually appoint a branch to be tested.
> 
> Wei, Doug: I have another question, which is mainly for my own understanding. 
> 
> Right now we allow posting of patches to Linux, Qemu, xen.git,
> OSSTEST, ... to xen-devel. The planned CI infrastructure only applies
> to xen.git. Have you thought about how to handle such cases? 

No. I haven't.  We may be able to use some heuristics here.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Minios-devel] automation: Creating a patchwork instance to improve pre-commit build testing

[Lars Kurth]

On 24/07/2018, 10:46, "Wei Liu" <wei.liu2@citrix.com> wrote:

    On Tue, Jul 24, 2018 at 10:38:24AM +0100, Lars Kurth wrote:
    > 
    > 
    > > On 24 Jul 2018, at 10:24, Wei Liu <wei.liu2@citrix.com> wrote:
    > > 
    > > On Tue, Jul 24, 2018 at 03:06:08AM -0600, Jan Beulich wrote:
    > >>>>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
    > >>> # How does this impact me?
    > >>> The contribution workflow is *not* impacted by this change, but once up and 
    > >>> running the following will happen once you post a patch or patch series to 
    > >>> xen-devel:
    > >>> * Patchwork will take patch series from the mailing list and applies it
    > >>> * CI/DC testing is triggered
    > >>> * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
    > >>> 
    > >>> This does mean though that series which do not build or show other issues, 
    > >>> will likely not be reviewed until the tests pass. This would lessen the 
    > >>> burden on reviewers, as they will know whether the code submitted builds on a 
    > >>> wide array of environments. 
    > >> 
    > >> So how are dependencies between series intended to be dealt with? It
    > >> is not uncommon for someone to say "applies only on top of xyz". The
    > >> implication of "will likely not be reviewed until the tests pass" seems
    > >> unsuitable to me in such a case.
    > >> 
    > > 
    > > We have been asking everyone to rebase to staging before posting a new
    > > version for a long time.  It is natural for the bot to assume that
    > > everything should apply on top of staging. That would provide most value
    > > to the community.
    > > 
    > > For special cases like you just mention, we should aim to provide
    > > mechanisms to manually appoint a branch to be tested.
    > 
    > Wei, Doug: I have another question, which is mainly for my own understanding. 
    > 
    > Right now we allow posting of patches to Linux, Qemu, xen.git,
    > OSSTEST, ... to xen-devel. The planned CI infrastructure only applies
    > to xen.git. Have you thought about how to handle such cases? 
    
    No. I haven't.  We may be able to use some heuristics here.
    
Or an alternative would be to say: if you want to use the test bot then CC xengit-testbot@xenproject.org (or something like it) when you submit the series. That would also get around Jan's issue with dependent series: you would simply not add the CC, when you know it won't build without a dependency.

Lars
    

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Minios-devel] automation: Creating a patchwork instance to improve pre-commit build testing

[Andrew Cooper]

On 24/07/18 10:57, Lars Kurth wrote:
>
> On 24/07/2018, 10:46, "Wei Liu" <wei.liu2@citrix.com> wrote:
>
>     On Tue, Jul 24, 2018 at 10:38:24AM +0100, Lars Kurth wrote:
>     > 
>     > 
>     > > On 24 Jul 2018, at 10:24, Wei Liu <wei.liu2@citrix.com> wrote:
>     > > 
>     > > On Tue, Jul 24, 2018 at 03:06:08AM -0600, Jan Beulich wrote:
>     > >>>>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
>     > >>> # How does this impact me?
>     > >>> The contribution workflow is *not* impacted by this change, but once up and 
>     > >>> running the following will happen once you post a patch or patch series to 
>     > >>> xen-devel:
>     > >>> * Patchwork will take patch series from the mailing list and applies it
>     > >>> * CI/DC testing is triggered
>     > >>> * A test report will be sent as a mail to the patch or the series (aka the 00 patch of the series)
>     > >>> 
>     > >>> This does mean though that series which do not build or show other issues, 
>     > >>> will likely not be reviewed until the tests pass. This would lessen the 
>     > >>> burden on reviewers, as they will know whether the code submitted builds on a 
>     > >>> wide array of environments. 
>     > >> 
>     > >> So how are dependencies between series intended to be dealt with? It
>     > >> is not uncommon for someone to say "applies only on top of xyz". The
>     > >> implication of "will likely not be reviewed until the tests pass" seems
>     > >> unsuitable to me in such a case.
>     > >> 
>     > > 
>     > > We have been asking everyone to rebase to staging before posting a new
>     > > version for a long time.  It is natural for the bot to assume that
>     > > everything should apply on top of staging. That would provide most value
>     > > to the community.
>     > > 
>     > > For special cases like you just mention, we should aim to provide
>     > > mechanisms to manually appoint a branch to be tested.
>     > 
>     > Wei, Doug: I have another question, which is mainly for my own understanding. 
>     > 
>     > Right now we allow posting of patches to Linux, Qemu, xen.git,
>     > OSSTEST, ... to xen-devel. The planned CI infrastructure only applies
>     > to xen.git. Have you thought about how to handle such cases? 
>     
>     No. I haven't.  We may be able to use some heuristics here.
>     
> Or an alternative would be to say: if you want to use the test bot then CC xengit-testbot@xenproject.org (or something like it) when you submit the series. That would also get around Jan's issue with dependent series: you would simply not add the CC, when you know it won't build without a dependency.

If you require contributors to opt into automation, people will won't
know or forget, and reviewers will first have to ask people to submit
full series again CC'ing the correct bot.

-100 to any idea which requires an opt-in.  It should be active by default.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: automation: Creating a patchwork instance to improve pre-commit build testing

[Lars Kurth]

On 24/07/2018, 10:06, "Jan Beulich" <JBeulich@suse.com> wrote:
    >>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
    > This does mean though that series which do not build or show other issues, 
    > will likely not be reviewed until the tests pass. This would lessen the 
    > burden on reviewers, as they will know whether the code submitted builds on a 
    > wide array of environments. 
    
    So how are dependencies between series intended to be dealt with? It
    is not uncommon for someone to say "applies only on top of xyz". The
    implication of "will likely not be reviewed until the tests pass" seems
    unsuitable to me in such a case.

We should look at how this is done in communities which have systems in place that do some off-list verification of patches, such as qemu and linux (0 day test service). 

Obviously in such cases the test bot would return results for a fail. The sensible thing to do would be the following:
* For the submitter of the patch to notify the reviewer(s) to highlight the test failure/dependency 
* For the reviewer to spot the dependency

In any case, the reviewer would have to decide whether to review a series which cannot be automatically build tested off list at that stage. 

Thinking about it a bit more, there are also two places at which things can go wrong:
a) Failure to apply the patch => this would probably be the most likely outcome with a dependency
b) Failure to build => if there was a missing dependency then probably fail in ALL build environments

In other words, there should be some tell-tales for this case, which can probably be highlighted in the bot results

Regards
Lars
    

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Minios-devel] automation: Creating a patchwork instance to improve pre-commit build testing

[Yuri Volchkov]

Hi All!

Lars Kurth <lars.kurth@citrix.com> writes:

> On 24/07/2018, 10:06, "Jan Beulich" <JBeulich@suse.com> wrote:
>     >>> On 23.07.18 at 18:40, <lars.kurth@citrix.com> wrote:
>     > This does mean though that series which do not build or show other issues, 
>     > will likely not be reviewed until the tests pass. This would lessen the 
>     > burden on reviewers, as they will know whether the code submitted builds on a 
>     > wide array of environments. 
>     
>     So how are dependencies between series intended to be dealt with? It
>     is not uncommon for someone to say "applies only on top of xyz". The
>     implication of "will likely not be reviewed until the tests pass" seems
>     unsuitable to me in such a case.
>
> We should look at how this is done in communities which have systems in place that do some off-list verification of patches, such as qemu and linux (0 day test service). 
>
> Obviously in such cases the test bot would return results for a fail. The sensible thing to do would be the following:
> * For the submitter of the patch to notify the reviewer(s) to highlight the test failure/dependency 
> * For the reviewer to spot the dependency
This would probably make sense to send notification to the address from
which the Patchwork gets emails for parsing. In case of successfully
passed test, the bot can send an email with "Tested-by" tag, which will
appear automatically in the commit message in the patchwork (similar to
"Reviewed-by").

If you do not want to have "Tested-by ci-bot", just email with free text
would be fine, because it will appear on the Patchwork's web interface
anyways. In such a case, we could even send CI messages *only* to the
patchwork, without flooding the mailing list. And whoever interested in
reviewing the patch, will just look up the email from the bot on the web
page related to this patch.

In my personal opinion, just sending CI email as "reply-all" is fine. I
do not mind having an extra email per patch in my mailbox.

--Yuri.

>
> In any case, the reviewer would have to decide whether to review a series which cannot be automatically build tested off list at that stage. 
>
> Thinking about it a bit more, there are also two places at which things can go wrong:
> a) Failure to apply the patch => this would probably be the most likely outcome with a dependency
> b) Failure to build => if there was a missing dependency then probably fail in ALL build environments
>
> In other words, there should be some tell-tales for this case, which can probably be highlighted in the bot results
>
> Regards
> Lars
>     
>
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/minios-devel

-- 
Yuri Volchkov
Software Specialist

NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Minios-devel] automation: Creating a patchwork instance to improve pre-commit build testing

[Jan Beulich]

>>> On 24.07.18 at 13:48, <yuri.volchkov@neclab.eu> wrote:
> In my personal opinion, just sending CI email as "reply-all" is fine. I
> do not mind having an extra email per patch in my mailbox.

This is exactly what I'm afraid of - when you're Cc-ed on a lot of
patches, you may then also get a lot of mails here. And no, other
than suggested elsewhere, I'm never going to have a rule to push
all mails matching certain criteria right into trash - there's always
the risk of a false positive. It is imo _always_ the sending side
which needs to judge who needs to be on the To/Cc lists of a mail,
never the receiving side to "paper over" mistakes the sender has
made.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Minios-devel] automation: Creating a patchwork instance to improve pre-commit build testing

[Lars Kurth]

On 24/07/2018, 13:00, "Jan Beulich" <JBeulich@suse.com> wrote:

    >>> On 24.07.18 at 13:48, <yuri.volchkov@neclab.eu> wrote:
    > In my personal opinion, just sending CI email as "reply-all" is fine. I
    > do not mind having an extra email per patch in my mailbox.
    
    This is exactly what I'm afraid of - when you're Cc-ed on a lot of
    patches, you may then also get a lot of mails here. And no, other
    than suggested elsewhere, I'm never going to have a rule to push
    all mails matching certain criteria right into trash - there's always
    the risk of a false positive. It is imo _always_ the sending side
    which needs to judge who needs to be on the To/Cc lists of a mail,
    never the receiving side to "paper over" mistakes the sender has
    made.
    
I believe there is quite a bit of freedom on how we would implement this.

@Doug: please correct me if this is wrong.

For example: we could do something like the following
* Contributor sends series to xen-devel@ (or if necessary to some 
alias or a different new list)
* Patchbot to take mail off list and run the tests
* Patchbot to augment the original mail(s) with embedded 
test results and/or Tested-by: tags to and send it to xen-devel@
* Augmented mail to be sent to xen-devel@ as if it came from
sender - although this may cause problems with some mail
clients

Or we could push the burden onto the contributor, e.g.
* Contributor to send series to test service 
* Contributor will get results (including some URL pointing to results)
* If succeeded or there is another good reason to send the series: 
Contributor to send mail to xen-devel@ with a reference to the results 
in the patch

Lars
    
    
    

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Minios-devel] automation: Creating a patchwork instance to improve pre-commit build testing

[George Dunlap]

On 07/24/2018 01:45 PM, Lars Kurth wrote:
> 
> 
> On 24/07/2018, 13:00, "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>     >>> On 24.07.18 at 13:48, <yuri.volchkov@neclab.eu> wrote:
>     > In my personal opinion, just sending CI email as "reply-all" is fine. I
>     > do not mind having an extra email per patch in my mailbox.
>     
>     This is exactly what I'm afraid of - when you're Cc-ed on a lot of
>     patches, you may then also get a lot of mails here. And no, other
>     than suggested elsewhere, I'm never going to have a rule to push
>     all mails matching certain criteria right into trash - there's always
>     the risk of a false positive. It is imo _always_ the sending side
>     which needs to judge who needs to be on the To/Cc lists of a mail,
>     never the receiving side to "paper over" mistakes the sender has
>     made.
>     
> I believe there is quite a bit of freedom on how we would implement this.
> 
> @Doug: please correct me if this is wrong.
> 
> For example: we could do something like the following
> * Contributor sends series to xen-devel@ (or if necessary to some 
> alias or a different new list)
> * Patchbot to take mail off list and run the tests
> * Patchbot to augment the original mail(s) with embedded 
> test results and/or Tested-by: tags to and send it to xen-devel@
> * Augmented mail to be sent to xen-devel@ as if it came from
> sender - although this may cause problems with some mail
> clients
> 
> Or we could push the burden onto the contributor, e.g.
> * Contributor to send series to test service 
> * Contributor will get results (including some URL pointing to results)
> * If succeeded or there is another good reason to send the series: 
> Contributor to send mail to xen-devel@ with a reference to the results 
> in the patch

I don't see what the problem is in having a single response to the
thread saying that the test was run, the result of the run, and a link
to a page about it.  It's certainly less mail than I get in the course
of a normal review cycle about patch series I'm not interested in.

I mean, suppose we just had a really enthusiastic contributor who made
it their personal goal to test and build every patch that was sent to
the list.  Would anyone really complain about a single extra mail per
series, when a typical series generates dozens of human-generated mails
anyway?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Minios-devel] automation: Creating a patchwork instance to improve pre-commit build testing

[Jan Beulich]

>>> On 24.07.18 at 16:01, <george.dunlap@citrix.com> wrote:
> I don't see what the problem is in having a single response to the
> thread saying that the test was run, the result of the run, and a link
> to a page about it.  It's certainly less mail than I get in the course
> of a normal review cycle about patch series I'm not interested in.
> 
> I mean, suppose we just had a really enthusiastic contributor who made
> it their personal goal to test and build every patch that was sent to
> the list.  Would anyone really complain about a single extra mail per
> series, when a typical series generates dozens of human-generated mails
> anyway?

Well, I agree one can view at this from different angles. Your
perspective looks to be that with there already being so much
mail, a little more doesn't hurt. I'm on the position that every
unnecessary mail is a problem. For (long) series the one extra
mail perhaps is indeed not only tolerable but helpful (albeit even
there it would rather be one mail per version, which may
become increasingly pointless as only very small changes get
done between versions). For individual patches (one liners to
take the extreme) there often is just a single response with an
ack. The bot would increase that volume by a whopping 50%
(or 100% for anyone for their own patches).

With all this said, just to be clear: I'm not against improvements
here or anywhere else, but their price needs to be reasonable.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Minios-devel] automation: Creating a patchwork instance to improve pre-commit build testing

[George Dunlap]

On 07/24/2018 04:44 PM, Jan Beulich wrote:
>>>> On 24.07.18 at 16:01, <george.dunlap@citrix.com> wrote:
>> I don't see what the problem is in having a single response to the
>> thread saying that the test was run, the result of the run, and a link
>> to a page about it.  It's certainly less mail than I get in the course
>> of a normal review cycle about patch series I'm not interested in.
>>
>> I mean, suppose we just had a really enthusiastic contributor who made
>> it their personal goal to test and build every patch that was sent to
>> the list.  Would anyone really complain about a single extra mail per
>> series, when a typical series generates dozens of human-generated mails
>> anyway?
> 
> Well, I agree one can view at this from different angles. Your
> perspective looks to be that with there already being so much
> mail, a little more doesn't hurt. I'm on the position that every
> unnecessary mail is a problem. For (long) series the one extra
> mail perhaps is indeed not only tolerable but helpful (albeit even
> there it would rather be one mail per version, which may
> become increasingly pointless as only very small changes get
> done between versions). For individual patches (one liners to
> take the extreme) there often is just a single response with an
> ack. The bot would increase that volume by a whopping 50%
> (or 100% for anyone for their own patches).
> 
> With all this said, just to be clear: I'm not against improvements
> here or anywhere else, but their price needs to be reasonable.

OK, well why don't we give it a try, and if people find the mail spammy
we can add a "do-not-mail" list that the bot will avoid sending mail to.
If there's a series someone in the do-not-mail list decides they want
information on, it shouldn't be too difficult to find it from the status
page.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Minios-devel] automation: Creating a patchwork instance to improve pre-commit build testing

[Julien Grall]

On 24/07/18 13:45, Lars Kurth wrote:
> 
> 
> On 24/07/2018, 13:00, "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>      >>> On 24.07.18 at 13:48, <yuri.volchkov@neclab.eu> wrote:
>      > In my personal opinion, just sending CI email as "reply-all" is fine. I
>      > do not mind having an extra email per patch in my mailbox.
>      
>      This is exactly what I'm afraid of - when you're Cc-ed on a lot of
>      patches, you may then also get a lot of mails here. And no, other
>      than suggested elsewhere, I'm never going to have a rule to push
>      all mails matching certain criteria right into trash - there's always
>      the risk of a false positive. It is imo _always_ the sending side
>      which needs to judge who needs to be on the To/Cc lists of a mail,
>      never the receiving side to "paper over" mistakes the sender has
>      made.
>      
> I believe there is quite a bit of freedom on how we would implement this.
> 
> @Doug: please correct me if this is wrong.
> 
> For example: we could do something like the following
> * Contributor sends series to xen-devel@ (or if necessary to some
> alias or a different new list)
> * Patchbot to take mail off list and run the tests
> * Patchbot to augment the original mail(s) with embedded
> test results and/or Tested-by: tags to and send it to xen-devel@
> * Augmented mail to be sent to xen-devel@ as if it came from
> sender - although this may cause problems with some mail
> clients
> 
> Or we could push the burden onto the contributor, e.g.
> * Contributor to send series to test service
> * Contributor will get results (including some URL pointing to results)
> * If succeeded or there is another good reason to send the series:
> Contributor to send mail to xen-devel@ with a reference to the results
> in the patch

I would prefer the first solution. If you push the burden onto the 
contributor, you increase potential discrepancy between the series 
tested and sent.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel