[RFC PATCH 2/3] Maintainers.txt: add wildcard path association for Arm/AArch64

[RFC PATCH 2/3] Maintainers.txt: add wildcard path association for Arm/AArch64

[Leif Lindholm]

Add Ard and Leif as responsible for any path matching
F: */Arm/
F: */AArch64/

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Maintainers.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index cd32f9b00170..e415f51468d5 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -82,6 +82,14 @@ EDK II Releases:
 W: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
 M: Liming Gao <liming.gao@intel.com>
 
+EDK II Architectures:
+---------------------
+ARM, AARCH64
+F: */AArch64/
+F: */Arm/
+M: Leif Lindholm <leif.lindholm@linaro.org>
+M: Ard Biesheuvel <ard.biesheuvel@linaro.org>
+
 EDK II Packages:
 ----------------
 ArmPkg
-- 
2.11.0

Re: [edk2-devel] [RFC PATCH 2/3] Maintainers.txt: add wildcard path association for Arm/AArch64

[Laszlo Ersek]

Hi Leif,

On 06/14/19 22:21, Leif Lindholm wrote:
> Add Ard and Leif as responsible for any path matching
> F: */Arm/
> F: */AArch64/
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  Maintainers.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index cd32f9b00170..e415f51468d5 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -82,6 +82,14 @@ EDK II Releases:
>  W: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
>  M: Liming Gao <liming.gao@intel.com>
>  
> +EDK II Architectures:
> +---------------------
> +ARM, AARCH64
> +F: */AArch64/
> +F: */Arm/
> +M: Leif Lindholm <leif.lindholm@linaro.org>
> +M: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> +
>  EDK II Packages:
>  ----------------
>  ArmPkg
> 

I'm a bit confused now.

* In the blurb, you write,

"Wildcard support is not fully filesystem compliant except in *first* or
last position in the path"

(emphasis mine). That would invalidate the present patch (because I
gather your intent is to match any pathname that has *any* component
called Aarch64 or Arm).

* Upon checking the third patch in the series, the "first position"
exception doesn't seem to be implemented actually. Which should make the
present patch work, in practice. And, we could ignore the statement in
the blurb (given that the blurb is never captured in the git history).

* However... the explanation from the first patch of the series, namely

+     F:   */net/*         all files in "any top level directory"/net

conflicts with the script then!


So right now, we have 6 locations:
(a) the blurb
(b) the "All patches CC:d here" section from Maintainers.txt
(c) the "Tianocore Stewards" section from Maintainers.txt
(d) the "F: */net/*" example pattern from Maintainers.txt
(e) the present patch
(f) the script (patch #3)

Locations (a) through (d) say that "*" in the first position matches top
level entries only, while (d) and (e) state "*" in the first position
matches any -- possibly multi-component -- pathname prefix.

It's one thing that we can make the script in patch #3 conform to what
the blurb says. But, that still leaves us with the problem that patches
#1 and #2, considered together, use "*" in the first position in
opposite senses.

Thanks,
Laszlo

Re: [edk2-devel] [RFC PATCH 2/3] Maintainers.txt: add wildcard path association for Arm/AArch64

[Leif Lindholm]

On Thu, Jun 20, 2019 at 05:29:55PM +0200, Laszlo Ersek wrote:
> Hi Leif,
> 
> On 06/14/19 22:21, Leif Lindholm wrote:
> > Add Ard and Leif as responsible for any path matching
> > F: */Arm/
> > F: */AArch64/
> > 
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  Maintainers.txt | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/Maintainers.txt b/Maintainers.txt
> > index cd32f9b00170..e415f51468d5 100644
> > --- a/Maintainers.txt
> > +++ b/Maintainers.txt
> > @@ -82,6 +82,14 @@ EDK II Releases:
> >  W: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
> >  M: Liming Gao <liming.gao@intel.com>
> >  
> > +EDK II Architectures:
> > +---------------------
> > +ARM, AARCH64
> > +F: */AArch64/
> > +F: */Arm/
> > +M: Leif Lindholm <leif.lindholm@linaro.org>
> > +M: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > +
> >  EDK II Packages:
> >  ----------------
> >  ArmPkg
> > 
> 
> I'm a bit confused now.
> 
> * In the blurb, you write,
> 
> "Wildcard support is not fully filesystem compliant except in *first* or
> last position in the path"
> 
> (emphasis mine). That would invalidate the present patch (because I
> gather your intent is to match any pathname that has *any* component
> called Aarch64 or Arm).

What I actually meant by that was that it matches for files in the
top directory (but not directories), as well as at the end.
Will reword.

> * Upon checking the third patch in the series, the "first position"
> exception doesn't seem to be implemented actually. Which should make the
> present patch work, in practice. And, we could ignore the statement in
> the blurb (given that the blurb is never captured in the git history).
> 
> * However... the explanation from the first patch of the series, namely
> 
> +     F:   */net/*         all files in "any top level directory"/net
> 
> conflicts with the script then!

Well, that remains a copy-paste from the QEMU description.
I would be tempted to keep current behaviour and simply not support
that particular case for now - updating the documentation to describe
actual behaviour.

> So right now, we have 6 locations:
> (a) the blurb

Needs rewording (as per above).

> (b) the "All patches CC:d here" section from Maintainers.txt

*  - all files in top-level directory
*/ - all files in all subdirectories of top-level directory

So, yes, I definitely *cheat* with transforming the second one to
'.*/.*', but it works :)
(but more below on '*')

> (c) the "Tianocore Stewards" section from Maintainers.txt

The same.

> (d) the "F: */net/*" example pattern from Maintainers.txt

Cut & paste error in documentation.

> (e) the present patch
> (f) the script (patch #3)

(more)
You are correct however - the 'F: *' special case isn't actually
working as intended. Currently that returns the expected result
because it hits the '<default>' rule.
I'll rework that.

Regards,

Leif

> Locations (a) through (d) say that "*" in the first position matches top
> level entries only, while (d) and (e) state "*" in the first position
> matches any -- possibly multi-component -- pathname prefix.
> 
> It's one thing that we can make the script in patch #3 conform to what
> the blurb says. But, that still leaves us with the problem that patches
> #1 and #2, considered together, use "*" in the first position in
> opposite senses.
> 
> Thanks,
> Laszlo

Re: [RFC PATCH 2/3] Maintainers.txt: add wildcard path association for Arm/AArch64

[Wu, Hao A]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Saturday, June 15, 2019 4:21 AM
> To: devel@edk2.groups.io
> Cc: Feng, Bob C; Gao, Liming; Andrew Fish; Laszlo Ersek; Kinney, Michael D;
> Wu, Hao A
> Subject: [RFC PATCH 2/3] Maintainers.txt: add wildcard path association for
> Arm/AArch64
> 
> Add Ard and Leif as responsible for any path matching
> F: */Arm/
> F: */AArch64/
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  Maintainers.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index cd32f9b00170..e415f51468d5 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -82,6 +82,14 @@ EDK II Releases:
>  W: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-
> Planning
>  M: Liming Gao <liming.gao@intel.com>
> 
> +EDK II Architectures:
> +---------------------
> +ARM, AARCH64
> +F: */AArch64/
> +F: */Arm/


Hello Leif,

I want to confirm a couple of usage model for the wildcard character.

Maybe they have been explained with your response to Laszlo's comments,
but I just want to double-confirm with my using cases.

1. Matching multiple levels of directories

For the below 2 folders:
MdeModulePkg/Bus/Ufs/
MdeModulePkg/Bus/Pci/UfsPciHcDxe/

I can use:
MdeModulePkg/*Ufs*/

to match them all, right?


2. Matching header files (usually within the Include/ directory)

In some cases, a feature may include some drivers + some include header
files. So for the below case:
MdeModulePkg/Include/Library/HiiLib.h
MdeModulePkg/Include/Guid/HiiResourceSampleHii.h
MdeModulePkg/Universal/HiiDatabaseDxe/
MdeModulePkg/Universal/HiiResourcesSampleDxe/
MdeModulePkg/Library/UefiHiiLib/

Should I use:
MdeModulePkg/*Hii*/

to match the drivers(libraries) and the headers all or should I use:
MdeModulePkg/*Hii*/
MdeModulePkg/Include/*Hii*.h

instead?

Best Regards,
Hao Wu


> +M: Leif Lindholm <leif.lindholm@linaro.org>
> +M: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> +
>  EDK II Packages:
>  ----------------
>  ArmPkg
> --
> 2.11.0

Re: [RFC PATCH 2/3] Maintainers.txt: add wildcard path association for Arm/AArch64

[Leif Lindholm]

On Wed, Jul 03, 2019 at 07:52:46AM +0000, Wu, Hao A wrote:
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > Sent: Saturday, June 15, 2019 4:21 AM
> > To: devel@edk2.groups.io
> > Cc: Feng, Bob C; Gao, Liming; Andrew Fish; Laszlo Ersek; Kinney, Michael D;
> > Wu, Hao A
> > Subject: [RFC PATCH 2/3] Maintainers.txt: add wildcard path association for
> > Arm/AArch64
> > 
> > Add Ard and Leif as responsible for any path matching
> > F: */Arm/
> > F: */AArch64/
> > 
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  Maintainers.txt | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/Maintainers.txt b/Maintainers.txt
> > index cd32f9b00170..e415f51468d5 100644
> > --- a/Maintainers.txt
> > +++ b/Maintainers.txt
> > @@ -82,6 +82,14 @@ EDK II Releases:
> >  W: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-
> > Planning
> >  M: Liming Gao <liming.gao@intel.com>
> > 
> > +EDK II Architectures:
> > +---------------------
> > +ARM, AARCH64
> > +F: */AArch64/
> > +F: */Arm/
> 
> 
> Hello Leif,
> 
> I want to confirm a couple of usage model for the wildcard character.
> 
> Maybe they have been explained with your response to Laszlo's comments,
> but I just want to double-confirm with my using cases.
> 
> 1. Matching multiple levels of directories
> 
> For the below 2 folders:
> MdeModulePkg/Bus/Ufs/
> MdeModulePkg/Bus/Pci/UfsPciHcDxe/
> 
> I can use:
> MdeModulePkg/*Ufs*/
> 
> to match them all, right?

Yes, that is how it currently works (I think?). It is unclear to me if
there is consensus on how we want it to work in the future.

> 2. Matching header files (usually within the Include/ directory)
> 
> In some cases, a feature may include some drivers + some include header
> files. So for the below case:
> MdeModulePkg/Include/Library/HiiLib.h
> MdeModulePkg/Include/Guid/HiiResourceSampleHii.h
> MdeModulePkg/Universal/HiiDatabaseDxe/
> MdeModulePkg/Universal/HiiResourcesSampleDxe/
> MdeModulePkg/Library/UefiHiiLib/
> 
> Should I use:
> MdeModulePkg/*Hii*/
> 
> to match the drivers(libraries) and the headers all or should I use:
> MdeModulePkg/*Hii*/
> MdeModulePkg/Include/*Hii*.h

My view is that this second option would be required.
If MdeModulePkg/*Hii*/ matches on the .h files, that is effectively a
bug - is that the behaviour you are currently seeing?

Best Regards,

Leif

Re: [RFC PATCH 2/3] Maintainers.txt: add wildcard path association for Arm/AArch64

[Wu, Hao A]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Wednesday, July 03, 2019 6:44 PM
> To: Wu, Hao A
> Cc: devel@edk2.groups.io
> Subject: Re: [RFC PATCH 2/3] Maintainers.txt: add wildcard path association
> for Arm/AArch64
> 
> On Wed, Jul 03, 2019 at 07:52:46AM +0000, Wu, Hao A wrote:
> > > -----Original Message-----
> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > Sent: Saturday, June 15, 2019 4:21 AM
> > > To: devel@edk2.groups.io
> > > Cc: Feng, Bob C; Gao, Liming; Andrew Fish; Laszlo Ersek; Kinney, Michael
> D;
> > > Wu, Hao A
> > > Subject: [RFC PATCH 2/3] Maintainers.txt: add wildcard path association
> for
> > > Arm/AArch64
> > >
> > > Add Ard and Leif as responsible for any path matching
> > > F: */Arm/
> > > F: */AArch64/
> > >
> > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > ---
> > >  Maintainers.txt | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/Maintainers.txt b/Maintainers.txt
> > > index cd32f9b00170..e415f51468d5 100644
> > > --- a/Maintainers.txt
> > > +++ b/Maintainers.txt
> > > @@ -82,6 +82,14 @@ EDK II Releases:
> > >  W: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> Release-
> > > Planning
> > >  M: Liming Gao <liming.gao@intel.com>
> > >
> > > +EDK II Architectures:
> > > +---------------------
> > > +ARM, AARCH64
> > > +F: */AArch64/
> > > +F: */Arm/
> >
> >
> > Hello Leif,
> >
> > I want to confirm a couple of usage model for the wildcard character.
> >
> > Maybe they have been explained with your response to Laszlo's comments,
> > but I just want to double-confirm with my using cases.
> >
> > 1. Matching multiple levels of directories
> >
> > For the below 2 folders:
> > MdeModulePkg/Bus/Ufs/
> > MdeModulePkg/Bus/Pci/UfsPciHcDxe/
> >
> > I can use:
> > MdeModulePkg/*Ufs*/
> >
> > to match them all, right?
> 
> Yes, that is how it currently works (I think?). It is unclear to me if
> there is consensus on how we want it to work in the future.


Yes, the proposed BaseTools script is working for:
MdeModulePkg/*Ufs*/

to match changes made in both directories.

I am also fine (and prefer) for the wildcard character to match multiple
levels of folders.


> 
> > 2. Matching header files (usually within the Include/ directory)
> >
> > In some cases, a feature may include some drivers + some include header
> > files. So for the below case:
> > MdeModulePkg/Include/Library/HiiLib.h
> > MdeModulePkg/Include/Guid/HiiResourceSampleHii.h
> > MdeModulePkg/Universal/HiiDatabaseDxe/
> > MdeModulePkg/Universal/HiiResourcesSampleDxe/
> > MdeModulePkg/Library/UefiHiiLib/
> >
> > Should I use:
> > MdeModulePkg/*Hii*/
> >
> > to match the drivers(libraries) and the headers all or should I use:
> > MdeModulePkg/*Hii*/
> > MdeModulePkg/Include/*Hii*.h
> 
> My view is that this second option would be required.
> If MdeModulePkg/*Hii*/ matches on the .h files, that is effectively a
> bug - is that the behaviour you are currently seeing?


The script is working like the 2nd option, and I think it is expected.
So, there seems to me no bug in my tests.


I do met a little problem that for a patch that touches multiple features,
the contacts order in the output is not ideally perfect:

Content in Maintainers.txt:
'''
MdeModulePkg
F: MdeModulePkg/
W: https://github.com/tianocore/tianocore.github.io/wiki/MdeModulePkg
M: Jian J Wang <jian.j.wang@intel.com>
M: Hao A Wu <hao.a.wu@intel.com>

F: MdeModulePkg/*Ufs*/
R: Ufs Guy <ufs@test.com>

F: MdeModulePkg/Include/*Hii*.h
F: MdeModulePkg/*Hii*/
R: Hii Guy <hii@test.com>
'''

Script output:
'''
$ py BaseTools/Scripts/GetMaintainer.py HEAD
MdeModulePkg/Bus/Pci/UfsPciHcDxe/UfsPciHcDxe.inf
MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.uni
  Jian J Wang <jian.j.wang@intel.com>
  Hao A Wu <hao.a.wu@intel.com>
  Ufs Guy <ufs@test.com>
  devel@edk2.groups.io
  Hii Guy <hii@test.com>
'''

The 'Hii Guy' is at the end of the list.

But this is a rather minor problem and does not impact the use at all.

Best Regards,
Hao Wu

> 
> Best Regards,
> 
> Leif

Re: [RFC PATCH 2/3] Maintainers.txt: add wildcard path association for Arm/AArch64

[Leif Lindholm]

On Thu, Jul 04, 2019 at 06:14:04AM +0000, Wu, Hao A wrote:
> I do met a little problem that for a patch that touches multiple features,
> the contacts order in the output is not ideally perfect:
> 
> Content in Maintainers.txt:
> '''
> MdeModulePkg
> F: MdeModulePkg/
> W: https://github.com/tianocore/tianocore.github.io/wiki/MdeModulePkg
> M: Jian J Wang <jian.j.wang@intel.com>
> M: Hao A Wu <hao.a.wu@intel.com>
> 
> F: MdeModulePkg/*Ufs*/
> R: Ufs Guy <ufs@test.com>
> 
> F: MdeModulePkg/Include/*Hii*.h
> F: MdeModulePkg/*Hii*/
> R: Hii Guy <hii@test.com>
> '''
> 
> Script output:
> '''
> $ py BaseTools/Scripts/GetMaintainer.py HEAD
> MdeModulePkg/Bus/Pci/UfsPciHcDxe/UfsPciHcDxe.inf
> MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.uni
>   Jian J Wang <jian.j.wang@intel.com>
>   Hao A Wu <hao.a.wu@intel.com>
>   Ufs Guy <ufs@test.com>
>   devel@edk2.groups.io
>   Hii Guy <hii@test.com>
> '''
> 
> The 'Hii Guy' is at the end of the list.

Totally agree. The idea was to keep the first revision simple.
Another thing missing (that I would argue is even more important than
listed order), is an indication of *why* said person is listed.

> But this is a rather minor problem and does not impact the use at all.

Good. I will respin the set based on Laszlo's feedback on previous
version and resubmit.

Best Regards,

Leif

Re: [RFC PATCH 2/3] Maintainers.txt: add wildcard path association for Arm/AArch64

[Wu, Hao A]

> -----Original Message-----
> From: Wu, Hao A
> Sent: Wednesday, July 03, 2019 3:53 PM
> To: 'Leif Lindholm'
> Cc: devel@edk2.groups.io
> Subject: RE: [RFC PATCH 2/3] Maintainers.txt: add wildcard path association
> for Arm/AArch64
> 
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > Sent: Saturday, June 15, 2019 4:21 AM
> > To: devel@edk2.groups.io
> > Cc: Feng, Bob C; Gao, Liming; Andrew Fish; Laszlo Ersek; Kinney, Michael D;
> > Wu, Hao A
> > Subject: [RFC PATCH 2/3] Maintainers.txt: add wildcard path association for
> > Arm/AArch64
> >
> > Add Ard and Leif as responsible for any path matching
> > F: */Arm/
> > F: */AArch64/
> >
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  Maintainers.txt | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Maintainers.txt b/Maintainers.txt
> > index cd32f9b00170..e415f51468d5 100644
> > --- a/Maintainers.txt
> > +++ b/Maintainers.txt
> > @@ -82,6 +82,14 @@ EDK II Releases:
> >  W: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-
> > Planning
> >  M: Liming Gao <liming.gao@intel.com>
> >
> > +EDK II Architectures:
> > +---------------------
> > +ARM, AARCH64
> > +F: */AArch64/
> > +F: */Arm/
> 
> 
> Hello Leif,
> 
> I want to confirm a couple of usage model for the wildcard character.
> 
> Maybe they have been explained with your response to Laszlo's comments,
> but I just want to double-confirm with my using cases.
> 
> 1. Matching multiple levels of directories
> 
> For the below 2 folders:
> MdeModulePkg/Bus/Ufs/
> MdeModulePkg/Bus/Pci/UfsPciHcDxe/
> 
> I can use:
> MdeModulePkg/*Ufs*/
> 
> to match them all, right?
> 
> 
> 2. Matching header files (usually within the Include/ directory)
> 
> In some cases, a feature may include some drivers + some include header
> files. So for the below case:
> MdeModulePkg/Include/Library/HiiLib.h
> MdeModulePkg/Include/Guid/HiiResourceSampleHii.h
> MdeModulePkg/Universal/HiiDatabaseDxe/
> MdeModulePkg/Universal/HiiResourcesSampleDxe/
> MdeModulePkg/Library/UefiHiiLib/
> 
> Should I use:
> MdeModulePkg/*Hii*/
> 
> to match the drivers(libraries) and the headers all or should I use:
> MdeModulePkg/*Hii*/
> MdeModulePkg/Include/*Hii*.h
> 
> instead?
> 
> Best Regards,
> Hao Wu


Posting to the rfc list as well.

Best Regards,
Hao Wu


> 
> 
> > +M: Leif Lindholm <leif.lindholm@linaro.org>
> > +M: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > +
> >  EDK II Packages:
> >  ----------------
> >  ArmPkg
> > --
> > 2.11.0