pw_maintainers_cli: enhance tree selection

  From: Pavan Nikhilesh <pbhagavatula@marvell.com>

When longest prefix match doesnt find a suitable tree, pick the
tree which has the highest modified file count instead of defauting
to main tree.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 tools/pw_maintainers_cli.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
  

> -----Original Message-----
> From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
> Sent: Friday, September 29, 2023 2:05 PM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; alialnu@nvidia.com;
> aconole@redhat.com
> Cc: ci@dpdk.org; Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
> Subject: [PATCH] pw_maintainers_cli: enhance tree selection
> 
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> When longest prefix match doesnt find a suitable tree, pick the
> tree which has the highest modified file count instead of defauting
> to main tree.
> 

This change is need to find the correct branch when a patch has a specification
Change followed by a implementation of driver layer example:

https://patches.dpdk.org/project/dpdk/list/?series=29675

> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
>  tools/pw_maintainers_cli.py | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/pw_maintainers_cli.py b/tools/pw_maintainers_cli.py
> index c7b5ba0..4aa46ae 100755
> --- a/tools/pw_maintainers_cli.py
> +++ b/tools/pw_maintainers_cli.py
> @@ -46,6 +46,7 @@ import re
>  import argparse
>  import fnmatch
> 
> +from collections import Counter
>  from requests.exceptions import HTTPError
> 
>  from git_pw import config
> @@ -276,6 +277,7 @@ class Maintainers(object):
>            dpdk-next-crypto + dpdk-next-virtio = dpdk
>            dpdk-next-net-intel + dpdk-next-net-mlx = dpdk-next-net
>          """
> +        highest_tree = Counter(tree_list).most_common(1)[0][0]
>          # Make sure the list is unique.
>          tree_list = list(set(tree_list))
> 
> @@ -287,7 +289,9 @@ class Maintainers(object):
>              os.path.commonprefix(_tree_list).rstrip('-').replace(
>                      'dpdk-next-net-virtio', 'dpdk-next-virtio')
>          # There is no 'dpdk-next' named tree.
> -        if common_prefix.endswith('dpdk-next') or
> common_prefix.endswith('/'):
> +        if common_prefix.endswith('dpdk-next'):
> +            common_prefix = highest_tree
> +        elif common_prefix.endswith('/'):
>              common_prefix = 'git://dpdk.org/dpdk'
>          return common_prefix
> 
> --
> 2.25.1
  

On Fri, Sep 29, 2023 at 11:21 AM Pavan Nikhilesh Bhagavatula
<pbhagavatula@marvell.com> wrote:
> > -----Original Message-----
> > From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
> > Sent: Friday, September 29, 2023 2:05 PM
> > To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; alialnu@nvidia.com;
> > aconole@redhat.com
> > Cc: ci@dpdk.org; Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
> > Subject: [PATCH] pw_maintainers_cli: enhance tree selection
> >
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >
> > When longest prefix match doesnt find a suitable tree, pick the
> > tree which has the highest modified file count instead of defauting
> > to main tree.
> >

This script helps with the delegation but nothing prevents submitters
or maintainers from updating this in patchwork themselves.

I did not test it but this heuristic seems complex in how it is
affected by existing git history.
It makes this script harder to understand / predict the outcome.
I fear side effects.


>
> This change is need to find the correct branch when a patch has a specification
> Change followed by a implementation of driver layer example:

This should be in the commitlog from the start as it exposes the need,
rather than the solution implemented by the patch.

>
> https://patches.dpdk.org/project/dpdk/list/?series=29675
  

> On Fri, Sep 29, 2023 at 11:21 AM Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com> wrote:
> > > -----Original Message-----
> > > From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
> > > Sent: Friday, September 29, 2023 2:05 PM
> > > To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; alialnu@nvidia.com;
> > > aconole@redhat.com
> > > Cc: ci@dpdk.org; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>
> > > Subject: [PATCH] pw_maintainers_cli: enhance tree selection
> > >
> > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > >
> > > When longest prefix match doesnt find a suitable tree, pick the
> > > tree which has the highest modified file count instead of defauting
> > > to main tree.
> > >
> 
> This script helps with the delegation but nothing prevents submitters
> or maintainers from updating this in patchwork themselves.
>

The script is used by CI to decide which branch to apply the patches on
and run the tests.

For example in the CI run http://mails.dpdk.org/archives/test-report/2023-September/468555.html
the script incorrectly picks dpdk main tree instead of dpdk-next-event and fails.

 
> I did not test it but this heuristic seems complex in how it is
> affected by existing git history.
> It makes this script harder to understand / predict the outcome.
> I fear side effects.
> 

It is not based on git history, It picks the tree based on the files that were changed in the patch.
Also, it only effects the case where its not able to find the common tree.

> 
> >
> > This change is need to find the correct branch when a patch has a
> specification
> > Change followed by a implementation of driver layer example:
> 
> This should be in the commitlog from the start as it exposes the need,
> rather than the solution implemented by the patch.
> 

I can change this in the next version.

> >
> > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__patches.dpdk.org_project_dpdk_list_-3Fseries-
> 3D29675&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=E3SgYMjtKCMVsB-
> fmvgGV3o-
> g_fjLhk5Pupi9ijohpc&m=QbsEVS0vy8B7ZVY0WPAMjlHbfOxASgG_XP4P7_pri
> aunQeURVo3SJaIvt3GsjfMe&s=kdARtlZA5zKbOuliw3uJFntF-
> IjXGY8OXli9SkD2rDg&e=
> 
> 
> 
> --
> David Marchand
  

29/09/2023 11:21, Pavan Nikhilesh Bhagavatula:
> 
> > -----Original Message-----
> > From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
> > Sent: Friday, September 29, 2023 2:05 PM
> > To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; alialnu@nvidia.com;
> > aconole@redhat.com
> > Cc: ci@dpdk.org; Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
> > Subject: [PATCH] pw_maintainers_cli: enhance tree selection
> > 
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > 
> > When longest prefix match doesnt find a suitable tree, pick the
> > tree which has the highest modified file count instead of defauting
> > to main tree.
> > 
> 
> This change is need to find the correct branch when a patch has a specification
> Change followed by a implementation of driver layer example:
> 
> https://patches.dpdk.org/project/dpdk/list/?series=29675

That's expected: when a series touches more than a tree scope,
it goes to main.
But that's not the issue here.
Both eventdev lib, test and drivers belong to the eventdev tree.
So why it is not already delegated to eventdev?
Please dig more.
  

> >
> > > -----Original Message-----
> > > From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
> > > Sent: Friday, September 29, 2023 2:05 PM
> > > To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; alialnu@nvidia.com;
> > > aconole@redhat.com
> > > Cc: ci@dpdk.org; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>
> > > Subject: [PATCH] pw_maintainers_cli: enhance tree selection
> > >
> > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > >
> > > When longest prefix match doesnt find a suitable tree, pick the
> > > tree which has the highest modified file count instead of defauting
> > > to main tree.
> > >
> >
> > This change is need to find the correct branch when a patch has a
> specification
> > Change followed by a implementation of driver layer example:
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__patches.dpdk.org_project_dpdk_list_-3Fseries-
> 3D29675&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=E3SgYMjtKCMVsB-
> fmvgGV3o-g_fjLhk5Pupi9ijohpc&m=WKN1erKSjH9MuEyyRs-
> R3jYC5geZeUeNipx2GQlQpQSuQQCet70Torr1oSdZNvZp&s=6XFs2Ggg5G0VFd
> YEaRexumEXM5JKc2vs5dmrRNcIUZw&e=
> 
> That's expected: when a series touches more than a tree scope,
> it goes to main.
> But that's not the issue here.
> Both eventdev lib, test and drivers belong to the eventdev tree.
> So why it is not already delegated to eventdev?
> Please dig more.
> 
> 

The main issue is the driver implementation touches common/cnxk which ties it to 
next-net-mrvl tree which causes the conflict.

We have few options here, 
1. Based on max number of files touched per tree (Current patch)
2. Ignore  driver/common from tree selection when there is a conflict with other tree.
3. When there is a conflict between trees choose the common tree instead of company specific tree 
    Example, dpdk-next-eventdev + dpdk-next-net-mrvl = dpdk-next-eventdev
  

29/09/2023 12:54, Pavan Nikhilesh Bhagavatula:
> > > From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
> > > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > >
> > > > When longest prefix match doesnt find a suitable tree, pick the
> > > > tree which has the highest modified file count instead of defauting
> > > > to main tree.
> > > >
> > >
> > > This change is need to find the correct branch when a patch has a
> > specification
> > > Change followed by a implementation of driver layer example:
> > >
> > > https://urldefense.proofpoint.com/v2/url?u=https-
> > 3A__patches.dpdk.org_project_dpdk_list_-3Fseries-
> > 3D29675&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=E3SgYMjtKCMVsB-
> > fmvgGV3o-g_fjLhk5Pupi9ijohpc&m=WKN1erKSjH9MuEyyRs-
> > R3jYC5geZeUeNipx2GQlQpQSuQQCet70Torr1oSdZNvZp&s=6XFs2Ggg5G0VFd
> > YEaRexumEXM5JKc2vs5dmrRNcIUZw&e=
> > 
> > That's expected: when a series touches more than a tree scope,
> > it goes to main.
> > But that's not the issue here.
> > Both eventdev lib, test and drivers belong to the eventdev tree.
> > So why it is not already delegated to eventdev?
> > Please dig more.
> > 
> > 
> 
> The main issue is the driver implementation touches common/cnxk which ties it to 
> next-net-mrvl tree which causes the conflict.
> 
> We have few options here, 
> 1. Based on max number of files touched per tree (Current patch)
> 2. Ignore  driver/common from tree selection when there is a conflict with other tree.
> 3. When there is a conflict between trees choose the common tree instead of company specific tree 
>     Example, dpdk-next-eventdev + dpdk-next-net-mrvl = dpdk-next-eventdev

I have a preference for option 2.
  

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, September 29, 2023 4:40 PM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; alialnu@nvidia.com;
> aconole@redhat.com; ci@dpdk.org; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>
> Subject: Re: [EXT] Re: [PATCH] pw_maintainers_cli: enhance tree selection
> 
> 29/09/2023 12:54, Pavan Nikhilesh Bhagavatula:
> > > > From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
> > > > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > >
> > > > > When longest prefix match doesnt find a suitable tree, pick the
> > > > > tree which has the highest modified file count instead of
> > > > > defauting to main tree.
> > > > >
> > > >
> > > > This change is need to find the correct branch when a patch has a
> > > specification
> > > > Change followed by a implementation of driver layer example:
> > > >
> > > > https://urldefense.proofpoint.com/v2/url?u=https-
> > > 3A__patches.dpdk.org_project_dpdk_list_-3Fseries-
> > > 3D29675&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=E3SgYMjtKCMVsB-
> > > fmvgGV3o-g_fjLhk5Pupi9ijohpc&m=WKN1erKSjH9MuEyyRs-
> > >
> R3jYC5geZeUeNipx2GQlQpQSuQQCet70Torr1oSdZNvZp&s=6XFs2Ggg5G0VFd
> > > YEaRexumEXM5JKc2vs5dmrRNcIUZw&e=
> > >
> > > That's expected: when a series touches more than a tree scope, it
> > > goes to main.
> > > But that's not the issue here.
> > > Both eventdev lib, test and drivers belong to the eventdev tree.
> > > So why it is not already delegated to eventdev?
> > > Please dig more.
> > >
> > >
> >
> > The main issue is the driver implementation touches common/cnxk which
> > ties it to next-net-mrvl tree which causes the conflict.
> >
> > We have few options here,
> > 1. Based on max number of files touched per tree (Current patch) 2.
> > Ignore  driver/common from tree selection when there is a conflict with other
> tree.
> > 3. When there is a conflict between trees choose the common tree instead of
> company specific tree
> >     Example, dpdk-next-eventdev + dpdk-next-net-mrvl =
> > dpdk-next-eventdev
> 
> I have a preference for option 2.

+1 for option 2.

> 
> 
>