Apply Patchseries Script

Apply Patchseries Script

[Patrick Robb]

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

Hello all,

The Community Lab is reviewing and rewriting some parts of our (currently
internal) apply patchseries script. The reasons are:

1. We want to remove any dependency the apply script has on our internal
infrastructure, so that it can be upstreamed and utilized by others in the
community.

2. We want to add in new features like “depends-on patch” applying (like
ovsrobot is doing currently)

3. Some DPDK project processes have changed (like moving next branches from
the main repo to being their own distinct repos). We have added on
workarounds along the way to account for this, but an overall rework is now
in order to clean up our process.

Before we do the work and attempt to upstream the script, I want to verify
with the community that our current assumptions regarding the apply
patchseries process are appropriate and should not be tweaked. Assumptions:

1. There are two inputs, A. The pw series url and B. The branch output of
pw_maintainers_cli.py

2. Do not apply and run if the series is an RFC series

3. Always check out to the current head of tree when applying a patch,
regardless of whether the tree state has changed between patch submission
and patch application in CI.

4. If the cover letter contains “depends-on,” extract the dependency series
id(s), apply those, then attempt to apply the patch

5. If patch does not cleanly apply to the branch supplied by
pw_maintainers_cli.py, attempt to apply on dpdk main. If this also fails,
report an apply failure.

6. If apply is successful, attempt a sanity build, and report a build
failure if that fails. If it succeeds, proceed with all CI testing.

Note: The Community Lab does not currently use pw-client. If it is better
for the CI community, we could stop maintaining a dedicated script for the
apply process, try moving the pw-client, and direct our efforts at patching
pw-client with the goal of adding support for features like depends-on. Are
other labs using pw-client right now and do you recommend it?

[-- Attachment #2: Type: text/html, Size: 8994 bytes --]

Re: Apply Patchseries Script

[Thomas Monjalon]

27/09/2023 18:31, Patrick Robb:
> Hello all,
> 
> The Community Lab is reviewing and rewriting some parts of our (currently
> internal) apply patchseries script. The reasons are:
> 
> 1. We want to remove any dependency the apply script has on our internal
> infrastructure, so that it can be upstreamed and utilized by others in the
> community.

Good news.

> 2. We want to add in new features like “depends-on patch” applying (like
> ovsrobot is doing currently)

Yes would be fantastic.

> 3. Some DPDK project processes have changed (like moving next branches from
> the main repo to being their own distinct repos). We have added on
> workarounds along the way to account for this, but an overall rework is now
> in order to clean up our process.

I don't think it changed.

> Before we do the work and attempt to upstream the script, I want to verify
> with the community that our current assumptions regarding the apply
> patchseries process are appropriate and should not be tweaked. Assumptions:
> 
> 1. There are two inputs, A. The pw series url and B. The branch output of
> pw_maintainers_cli.py

So it is only 1 input, because B can be deduced from A.

> 2. Do not apply and run if the series is an RFC series

Not sure about this requirement.
What is the problem in running tests on RFC?

> 3. Always check out to the current head of tree when applying a patch,
> regardless of whether the tree state has changed between patch submission
> and patch application in CI.

I don't think it is reasonable to look for the exact tree state
of patch submission, so yes I agree to use the head of the tree.
If it becomes quickly non applicable, then the author needs to update.
It does not happen frequently.

> 4. If the cover letter contains “depends-on,” extract the dependency series
> id(s), apply those, then attempt to apply the patch

Yes

> 5. If patch does not cleanly apply to the branch supplied by
> pw_maintainers_cli.py, attempt to apply on dpdk main. If this also fails,
> report an apply failure.

Yes

> 6. If apply is successful, attempt a sanity build, and report a build
> failure if that fails. If it succeeds, proceed with all CI testing.

Yes

> Note: The Community Lab does not currently use pw-client. If it is better
> for the CI community, we could stop maintaining a dedicated script for the
> apply process, try moving the pw-client, and direct our efforts at patching
> pw-client with the goal of adding support for features like depends-on. Are
> other labs using pw-client right now and do you recommend it?

In general I think it is a good idea to use common tools.
About adding depends-on support, it looks a great idea.
Other projects could use the same syntax then.
That's the same for the CI support in patchwork: we invented it in DPDK.

Re: Apply Patchseries Script

[Patrick Robb]

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

On Wed, Sep 27, 2023 at 4:22 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> 27/09/2023 18:31, Patrick Robb:
>
> > 2. Do not apply and run if the series is an RFC series
>
> Not sure about this requirement.
> What is the problem in running tests on RFC?
>
> I see that currently ovsrobot and UNH Lab have rules saying don't test on
RFC series, and Loongson and Intel do test on RFC series. I'm guessing the
thinking was something like "RFC patches are at least one stage away from
merge, and probably do not represent the final state of the patch, so CI
testing is not very valuable." On the other hand, I'm sure in many cases
getting that early feedback, even for an RFC, is helpful to developers.
I'll bring it up in the CI testing meeting tomorrow and see if any of the
CI testing people have an opinion. Anyways, I think all labs should have
the same policy, be it testing or not testing on RFC patches.

Thanks for the feedback.

[-- Attachment #2: Type: text/html, Size: 1298 bytes --]

Re: Apply Patchseries Script

[Aaron Conole]

Patrick Robb <probb@iol.unh.edu> writes:

> On Wed, Sep 27, 2023 at 4:22 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
>  27/09/2023 18:31, Patrick Robb:
>
>  > 2. Do not apply and run if the series is an RFC series
>
>  Not sure about this requirement.
>  What is the problem in running tests on RFC?
>
> I see that currently ovsrobot and UNH Lab have rules saying don't test on RFC series, and Loongson and Intel do test on
> RFC series. I'm guessing the thinking was something like "RFC patches are at least one stage away from merge, and
> probably do not represent the final state of the patch, so CI testing is not very valuable." On the other hand, I'm sure in
> many cases getting that early feedback, even for an RFC, is helpful to developers. I'll bring it up in the CI testing
> meeting tomorrow and see if any of the CI testing people have an opinion. Anyways, I think all labs should have the
> same policy, be it testing or not testing on RFC patches. 

We do currently skip running RFCs as well.  IIRC they were eating into
our timing budget on Travis, and we never bothered to re-evaluate after
the switch to github actions.  I think it would be good to discuss it.

> Thanks for the feedback. 

Re: Apply Patchseries Script

[Aaron Conole]

Thomas Monjalon <thomas@monjalon.net> writes:

> 27/09/2023 18:31, Patrick Robb:
>> Hello all,
>> 
>> The Community Lab is reviewing and rewriting some parts of our (currently
>> internal) apply patchseries script. The reasons are:
>> 
>> 1. We want to remove any dependency the apply script has on our internal
>> infrastructure, so that it can be upstreamed and utilized by others in the
>> community.
>
> Good news.
>
>> 2. We want to add in new features like “depends-on patch” applying (like
>> ovsrobot is doing currently)
>
> Yes would be fantastic.
>
>> 3. Some DPDK project processes have changed (like moving next branches from
>> the main repo to being their own distinct repos). We have added on
>> workarounds along the way to account for this, but an overall rework is now
>> in order to clean up our process.
>
> I don't think it changed.
>
>> Before we do the work and attempt to upstream the script, I want to verify
>> with the community that our current assumptions regarding the apply
>> patchseries process are appropriate and should not be tweaked. Assumptions:
>> 
>> 1. There are two inputs, A. The pw series url and B. The branch output of
>> pw_maintainers_cli.py
>
> So it is only 1 input, because B can be deduced from A.
>
>> 2. Do not apply and run if the series is an RFC series
>
> Not sure about this requirement.
> What is the problem in running tests on RFC?
>
>> 3. Always check out to the current head of tree when applying a patch,
>> regardless of whether the tree state has changed between patch submission
>> and patch application in CI.
>
> I don't think it is reasonable to look for the exact tree state
> of patch submission, so yes I agree to use the head of the tree.
> If it becomes quickly non applicable, then the author needs to update.
> It does not happen frequently.

+1

>> 4. If the cover letter contains “depends-on,” extract the dependency series
>> id(s), apply those, then attempt to apply the patch
>
> Yes
>
>> 5. If patch does not cleanly apply to the branch supplied by
>> pw_maintainers_cli.py, attempt to apply on dpdk main. If this also fails,
>> report an apply failure.
>
> Yes
>
>> 6. If apply is successful, attempt a sanity build, and report a build
>> failure if that fails. If it succeeds, proceed with all CI testing.
>
> Yes
>
>> Note: The Community Lab does not currently use pw-client. If it is better
>> for the CI community, we could stop maintaining a dedicated script for the
>> apply process, try moving the pw-client, and direct our efforts at patching
>> pw-client with the goal of adding support for features like depends-on. Are
>> other labs using pw-client right now and do you recommend it?
>
> In general I think it is a good idea to use common tools.
> About adding depends-on support, it looks a great idea.
> Other projects could use the same syntax then.
> That's the same for the CI support in patchwork: we invented it in DPDK.

Re: Apply Patchseries Script

[David Marchand]

On Thu, Sep 28, 2023 at 2:05 PM Aaron Conole <aconole@redhat.com> wrote:
>
> Patrick Robb <probb@iol.unh.edu> writes:
>
> > On Wed, Sep 27, 2023 at 4:22 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> >  27/09/2023 18:31, Patrick Robb:
> >
> >  > 2. Do not apply and run if the series is an RFC series
> >
> >  Not sure about this requirement.
> >  What is the problem in running tests on RFC?
> >
> > I see that currently ovsrobot and UNH Lab have rules saying don't test on RFC series, and Loongson and Intel do test on
> > RFC series. I'm guessing the thinking was something like "RFC patches are at least one stage away from merge, and
> > probably do not represent the final state of the patch, so CI testing is not very valuable." On the other hand, I'm sure in
> > many cases getting that early feedback, even for an RFC, is helpful to developers. I'll bring it up in the CI testing
> > meeting tomorrow and see if any of the CI testing people have an opinion. Anyways, I think all labs should have the
> > same policy, be it testing or not testing on RFC patches.
>
> We do currently skip running RFCs as well.  IIRC they were eating into
> our timing budget on Travis, and we never bothered to re-evaluate after
> the switch to github actions.  I think it would be good to discuss it.

I once missed some issues in a RFC of mine that I only discovered when
sending the first non-RFC patch.

As far as time budget is concerned now, I don't think testing RFC is
that much of an issue.
We have way more people sending 3-4 revisions in a single day than
people sending RFCs :-).


-- 
David Marchand