get_maintainers.pl: Enable running the script on unikraft repos

Commit Message

Lars Kurth Aug. 16, 2019, 10:42 a.m. UTC

Unikraft repos follow the same syntax as xen.git with the
following exceptions:
* MAINTAINERS files are called MAINTAINERS.md
* M: ... etc blocks are preceded by whitespaces for rendering as
  markup files

This change will
- load MAINTAINERS.md if MAINTAINERS is not present
- deal with indented M: ... blocks

Signed-off-by: Lars Kurth <[e-mail]>
---
Cc: Andrew Cooper <[e-mail]>
Cc: George Dunlap <[e-mail]>
Cc: Ian Jackson <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Julien Grall <[e-mail]>
Cc: Konrad Rzeszutek Wilk <[e-mail]>
Cc: Stefano Stabellini <[e-mail]>
Cc: Tim Deegan <[e-mail]>
Cc: Wei Liu <[e-mail]>

CC: Simon Kuenzer <[e-mail]>
CC: Florian Schmidt <[e-mail]>
CC: Felipe Huici <[e-mail]>
---
 scripts/get_maintainer.pl | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Julien Grall Aug. 16, 2019, 10:55 a.m. UTC | #1

Hi Lars,

On 16/08/2019 11:42, Lars Kurth wrote:
> Unikraft repos follow the same syntax as xen.git with the
> following exceptions:
> * MAINTAINERS files are called MAINTAINERS.md
> * M: ... etc blocks are preceded by whitespaces for rendering as
>    markup files

There is an other difference. The "fallback" category is "UNIKRAFT GENERAL" and 
not "THE REST".

> 
> This change will
> - load MAINTAINERS.md if MAINTAINERS is not present
> - deal with indented M: ... blocks

One process question. Does it mean Unikraft folks will have to checkout Xen in 
order to use {add, get}_maintainers.pl? If so, would it make sense to have 
add_maintainers.pl and script_maintainers.pl in a separate repo that can be 
added as submodule?

> 
> Signed-off-by: Lars Kurth <[e-mail]>
> ---
> Cc: Andrew Cooper <[e-mail]>
> Cc: George Dunlap <[e-mail]>
> Cc: Ian Jackson <[e-mail]>
> Cc: Jan Beulich <[e-mail]>
> Cc: Julien Grall <[e-mail]>
> Cc: Konrad Rzeszutek Wilk <[e-mail]>
> Cc: Stefano Stabellini <[e-mail]>
> Cc: Tim Deegan <[e-mail]>
> Cc: Wei Liu <[e-mail]>
> 
> CC: Simon Kuenzer <[e-mail]>
> CC: Florian Schmidt <[e-mail]>
> CC: Felipe Huici <[e-mail]>
> ---
>   scripts/get_maintainer.pl | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index f1e9c904ee..bdb09f8f65 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -284,12 +284,18 @@ if (!top_of_tree($xen_path)) {
>   my @typevalue = ();
>   my %keyword_hash;
>   
> -open (my $maint, '<', "${xen_path}MAINTAINERS")
> -    or die "$P: Can't open MAINTAINERS: $!\n";
> +my $maint;
> +my $maintainers_file = "MAINTAINERS";
> +if (! open ($maint, '<', ${xen_path}.$maintainers_file)) {
> +    $maintainers_file = "MAINTAINERS.md";
> +    open ($maint, '<', ${xen_path}.$maintainers_file)
> +        or die "$P: Can't open MAINTAINERS or MAINTAINERS.md: $!\n";
> +}
> +
>   while (<$maint>) {
>       my $line = $_;
>   
> -    if ($line =~ m/^([A-Z]):\s*(.*)/) {
> +    if ($line =~ m/^\s*([A-Z]):\s*(.*)/) {

As you allow space before the blocks M:, would not this catch the example at the 
beginning of the file?

	M: Mail patches to: FullName <address@domain>
	R: Designated reviewer: FullName <address@domain>

>   	my $type = $1;
>   	my $value = $2;
>   
> @@ -421,7 +427,7 @@ foreach my $file (@ARGV) {
>       }
>       if ($from_filename) {
>   	push(@files, $file);
> -	if ($file ne "MAINTAINERS" && -f $file && ($keywords || $file_emails)) {
> +	if ($file ne $maintainers_file && -f $file && ($keywords || $file_emails)) {
>   	    open(my $f, '<', $file)
>   		or die "$P: Can't open $file: $!\n";
>   	    my $text = do { local($/) ; <$f> };
> 

Cheers,

Anthony PERARD Aug. 16, 2019, 11:17 a.m. UTC | #2

On Fri, Aug 16, 2019 at 11:55:16AM +0100, Julien Grall wrote:
> Hi Lars,
> 
> On 16/08/2019 11:42, Lars Kurth wrote:
> > Unikraft repos follow the same syntax as xen.git with the
> > following exceptions:
> > * MAINTAINERS files are called MAINTAINERS.md
> > * M: ... etc blocks are preceded by whitespaces for rendering as
> >    markup files
> 
> There is an other difference. The "fallback" category is "UNIKRAFT GENERAL"
> and not "THE REST".
> 
> > 
> > This change will
> > - load MAINTAINERS.md if MAINTAINERS is not present
> > - deal with indented M: ... blocks
> 
> One process question. Does it mean Unikraft folks will have to checkout Xen
> in order to use {add, get}_maintainers.pl? If so, would it make sense to
> have add_maintainers.pl and script_maintainers.pl in a separate repo that
> can be added as submodule?

Shouldn't instead the Unikraft repo have it's one get_maintainers
script? xen.git's script doesn't needs to have support for every single
repo available on earth and Unikraft is a different project anyway.

Usually, projects with a MAINTAINERS file have there own get_maintainers
script.

Cheers,

Lars Kurth Aug. 16, 2019, 11:41 a.m. UTC | #3

Hi Julien,

On 16/08/2019, 11:55, "Julien Grall" <[e-mail]> wrote:

    Hi Lars,
    
    On 16/08/2019 11:42, Lars Kurth wrote:
    > Unikraft repos follow the same syntax as xen.git with the
    > following exceptions:
    > * MAINTAINERS files are called MAINTAINERS.md
    > * M: ... etc blocks are preceded by whitespaces for rendering as
    >    markup files
    
    There is an other difference. The "fallback" category is "UNIKRAFT GENERAL" and 
    not "THE REST".

That is right. But currently get_maintainers.pl: totally ignores the headlines. 
It just reads
M: ...
R: ...
Etc.

What is different is that "UNIKRAFT GENERAL" does not contain
F: entries, which have to be added, otherwise no e-mail addresses
from the section are added. So I was going to send a patch to fix this.
    
    > This change will
    > - load MAINTAINERS.md if MAINTAINERS is not present
    > - deal with indented M: ... blocks
    
    One process question. Does it mean Unikraft folks will have to checkout Xen in 
    order to use {add, get}_maintainers.pl? If so, would it make sense to have 
    add_maintainers.pl and script_maintainers.pl in a separate repo that can be 
    added as submodule?

The way how the code is written they would either have to check out the 
repo or just get the two scripts and put them in the same directory somewhere
on the path.

For things like mini-os, xtf, osstest, ... you would always have a xen.git somewhere
In the unikraft case that is different. We could break it out, but maybe we can do this
at a later point in time.

    > Signed-off-by: Lars Kurth <[e-mail]>
    > ---
    > Cc: Andrew Cooper <[e-mail]>
    > Cc: George Dunlap <[e-mail]>
    > Cc: Ian Jackson <[e-mail]>
    > Cc: Jan Beulich <[e-mail]>
    > Cc: Julien Grall <[e-mail]>
    > Cc: Konrad Rzeszutek Wilk <[e-mail]>
    > Cc: Stefano Stabellini <[e-mail]>
    > Cc: Tim Deegan <[e-mail]>
    > Cc: Wei Liu <[e-mail]>
    > 
    > CC: Simon Kuenzer <[e-mail]>
    > CC: Florian Schmidt <[e-mail]>
    > CC: Felipe Huici <[e-mail]>
    > ---
    >   scripts/get_maintainer.pl | 14 ++++++++++----
    >   1 file changed, 10 insertions(+), 4 deletions(-)
    > 
    > diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
    > index f1e9c904ee..bdb09f8f65 100755
    > --- a/scripts/get_maintainer.pl
    > +++ b/scripts/get_maintainer.pl
    > @@ -284,12 +284,18 @@ if (!top_of_tree($xen_path)) {
    >   my @typevalue = ();
    >   my %keyword_hash;
    >   
    > -open (my $maint, '<', "${xen_path}MAINTAINERS")
    > -    or die "$P: Can't open MAINTAINERS: $!\n";
    > +my $maint;
    > +my $maintainers_file = "MAINTAINERS";
    > +if (! open ($maint, '<', ${xen_path}.$maintainers_file)) {
    > +    $maintainers_file = "MAINTAINERS.md";
    > +    open ($maint, '<', ${xen_path}.$maintainers_file)
    > +        or die "$P: Can't open MAINTAINERS or MAINTAINERS.md: $!\n";
    > +}
    > +
    >   while (<$maint>) {
    >       my $line = $_;
    >   
    > -    if ($line =~ m/^([A-Z]):\s*(.*)/) {
    > +    if ($line =~ m/^\s*([A-Z]):\s*(.*)/) {
    
    As you allow space before the blocks M:, would not this catch the example at the 
    beginning of the file?
    
    	M: Mail patches to: FullName <address@domain>
    	R: Designated reviewer: FullName <address@domain>

Good point. When I tested it (I sent the last few patches with the change in it), 
it didn't pick up the e-mail addresses. However, when I check with regex101.com
it is picked up.

Which means that the values are pushed to @typevalue, aka
{ "R",  "Designated reviewer: FullName <address@domain>" }
Etc.

@typevalue seem to be processed by find_first_section(), 
find_starting_index(), find_ending_index() 

And then basically the entries are not processed because the
block in the example is not consistent and fails the validation
further down the line

For example:
./scripts/get_maintainer.pl --email --git -f .
Etc.

Don't list the e-mail addresses in the examples
    
So, what I proposed is probably far too fragile to make sense.
And doing something which is more accurate will probably 
require major surgery to the scripts.

I can look at this in a bit more detail and see whether there
is a way to handle this.

But I don't want to invest the time to do this really as this
is probably rather complex.

@Simon, @Florian, @Felipe: would you be willing to
change the MAINTAINERS files in your repos such that
we don't have to implement lots of magic to make the
patch sending helper scripts work for you?

Regards
Lars

Lars Kurth Aug. 16, 2019, 12:05 p.m. UTC | #4

Added Paul Durrant

On 16/08/2019, 12:17, "Anthony PERARD" <[e-mail]> wrote:

    On Fri, Aug 16, 2019 at 11:55:16AM +0100, Julien Grall wrote:
    > Hi Lars,
    > 
    > On 16/08/2019 11:42, Lars Kurth wrote:
    > > Unikraft repos follow the same syntax as xen.git with the
    > > following exceptions:
    > > * MAINTAINERS files are called MAINTAINERS.md
    > > * M: ... etc blocks are preceded by whitespaces for rendering as
    > >    markup files
    > 
    > There is an other difference. The "fallback" category is "UNIKRAFT GENERAL"
    > and not "THE REST".
    > 
    > > 
    > > This change will
    > > - load MAINTAINERS.md if MAINTAINERS is not present
    > > - deal with indented M: ... blocks
    > 
    > One process question. Does it mean Unikraft folks will have to checkout Xen
    > in order to use {add, get}_maintainers.pl? If so, would it make sense to
    > have add_maintainers.pl and script_maintainers.pl in a separate repo that
    > can be added as submodule?
    
    Shouldn't instead the Unikraft repo have it's one get_maintainers
    script? xen.git's script doesn't needs to have support for every single
    repo available on earth and Unikraft is a different project anyway.
    
    Usually, projects with a MAINTAINERS file have there own get_maintainers
    script.
    
Well: Unikraft is part of the Xen Project. 

When I started to clean up the contribution docs it became apparent that
there is a lot of inconsistency. Ideally our contribution guide [0] would apply 
to pretty much *all* sub projects which use a mailing list based workflows

Consistency makes life for developers and also newcomers much easier. And the 
number of new devs who seem to trip over inconsistencies between projects are 
quite large (we had 3  cases in 3 weeks which I noticed).

If we start improving our CI infrastructure (which we are), it would be nice if other 
sub projects had the possibility to easily hook into it or replicate it. But that does require 
some consistency.

That's why I submitted [1] and [2]

Sub-projects with mail based workflows
1: Hypervisor
2: Hypervisor related repos (livepatch-build-tools, mini-os, xtf, ...)
3: Windows PV Drivers - which will require changes to their MAINTAINERS file 
4: Unikraft

Supporting 1 - 3 should be straightforward because you would almost always
have xen.git checked out. 4 is more different.

When I did [1] supporting Unikraft looked quite straightforward, which is
why I submitted this patch. But, given the issues Julien highlighted maybe that 
is not the case. 

I think for now, maybe we should focus on 1 - 3 and let the Unikraft devs decide 
how to approach this. Then we can revisit the question of where to store these
scripts. For now, I think requiring to have xen.git checked out is OK.

Regards
Lars

[0] https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches     
[1] https://lists.xenproject.org/archives/html/xen-devel/2019-08/threads.html#01575
[2] https://lists.xenproject.org/archives/html/xen-devel/2019-08/threads.html#01581

Paul Durrant Aug. 16, 2019, 12:09 p.m. UTC | #5

> -----Original Message-----
> From: Lars Kurth <[e-mail]>
> Sent: 16 August 2019 13:05
> To: Anthony Perard <[e-mail]>; Julien Grall <[e-mail]>
> Cc: [e-mail]; Felipe Huici <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Wei Liu <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>; George
> Dunlap <[e-mail]>; Andrew Cooper <[e-mail]>; Ian Jackson
> <[e-mail]>; Tim (Xen.org) <[e-mail]>; Florian Schmidt <[e-mail]>;
> Jan Beulich <[e-mail]>; Simon Kuenzer <[e-mail]>; Paul Durrant
> <[e-mail]>
> Subject: Re: [Xen-devel] [PATCH] get_maintainers.pl: Enable running the script on unikraft repos
> 
> Added Paul Durrant
> 
> On 16/08/2019, 12:17, "Anthony PERARD" <[e-mail]> wrote:
> 
>     On Fri, Aug 16, 2019 at 11:55:16AM +0100, Julien Grall wrote:
>     > Hi Lars,
>     >
>     > On 16/08/2019 11:42, Lars Kurth wrote:
>     > > Unikraft repos follow the same syntax as xen.git with the
>     > > following exceptions:
>     > > * MAINTAINERS files are called MAINTAINERS.md
>     > > * M: ... etc blocks are preceded by whitespaces for rendering as
>     > >    markup files
>     >
>     > There is an other difference. The "fallback" category is "UNIKRAFT GENERAL"
>     > and not "THE REST".
>     >
>     > >
>     > > This change will
>     > > - load MAINTAINERS.md if MAINTAINERS is not present
>     > > - deal with indented M: ... blocks
>     >
>     > One process question. Does it mean Unikraft folks will have to checkout Xen
>     > in order to use {add, get}_maintainers.pl? If so, would it make sense to
>     > have add_maintainers.pl and script_maintainers.pl in a separate repo that
>     > can be added as submodule?
> 
>     Shouldn't instead the Unikraft repo have it's one get_maintainers
>     script? xen.git's script doesn't needs to have support for every single
>     repo available on earth and Unikraft is a different project anyway.
> 
>     Usually, projects with a MAINTAINERS file have there own get_maintainers
>     script.
> 
> Well: Unikraft is part of the Xen Project.
> 
> When I started to clean up the contribution docs it became apparent that
> there is a lot of inconsistency. Ideally our contribution guide [0] would apply
> to pretty much *all* sub projects which use a mailing list based workflows
> 
> Consistency makes life for developers and also newcomers much easier. And the
> number of new devs who seem to trip over inconsistencies between projects are
> quite large (we had 3  cases in 3 weeks which I noticed).
> 
> If we start improving our CI infrastructure (which we are), it would be nice if other
> sub projects had the possibility to easily hook into it or replicate it. But that does require
> some consistency.
> 
> That's why I submitted [1] and [2]
> 
> Sub-projects with mail based workflows
> 1: Hypervisor
> 2: Hypervisor related repos (livepatch-build-tools, mini-os, xtf, ...)
> 3: Windows PV Drivers - which will require changes to their MAINTAINERS file
> 4: Unikraft
> 
> Supporting 1 - 3 should be straightforward because you would almost always
> have xen.git checked out. 4 is more different.

I'd say that folks building 3 are unlikely to have xen.git checked out.

  Paul

Lars Kurth Aug. 16, 2019, 12:20 p.m. UTC | #6

On 16/08/2019, 13:09, "Paul Durrant" <[e-mail]> wrote:

    > -----Original Message-----
    > From: Lars Kurth <[e-mail]>
    > Sent: 16 August 2019 13:05
    > To: Anthony Perard <[e-mail]>; Julien Grall <[e-mail]>
    > Cc: [e-mail]; Felipe Huici <[e-mail]>; Stefano Stabellini
    > <[e-mail]>; Wei Liu <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>; George
    > Dunlap <[e-mail]>; Andrew Cooper <[e-mail]>; Ian Jackson
    > <[e-mail]>; Tim (Xen.org) <[e-mail]>; Florian Schmidt <[e-mail]>;
    > Jan Beulich <[e-mail]>; Simon Kuenzer <[e-mail]>; Paul Durrant
    > <[e-mail]>
    > Subject: Re: [Xen-devel] [PATCH] get_maintainers.pl: Enable running the script on unikraft repos
    > 
    > Added Paul Durrant
    > 
    > On 16/08/2019, 12:17, "Anthony PERARD" <[e-mail]> wrote:
    > 
    >     On Fri, Aug 16, 2019 at 11:55:16AM +0100, Julien Grall wrote:
    >     > Hi Lars,
    >     >
    >     > On 16/08/2019 11:42, Lars Kurth wrote:
    >     > > Unikraft repos follow the same syntax as xen.git with the
    >     > > following exceptions:
    >     > > * MAINTAINERS files are called MAINTAINERS.md
    >     > > * M: ... etc blocks are preceded by whitespaces for rendering as
    >     > >    markup files
    >     >
    >     > There is an other difference. The "fallback" category is "UNIKRAFT GENERAL"
    >     > and not "THE REST".
    >     >
    >     > >
    >     > > This change will
    >     > > - load MAINTAINERS.md if MAINTAINERS is not present
    >     > > - deal with indented M: ... blocks
    >     >
    >     > One process question. Does it mean Unikraft folks will have to checkout Xen
    >     > in order to use {add, get}_maintainers.pl? If so, would it make sense to
    >     > have add_maintainers.pl and script_maintainers.pl in a separate repo that
    >     > can be added as submodule?
    > 
    >     Shouldn't instead the Unikraft repo have it's one get_maintainers
    >     script? xen.git's script doesn't needs to have support for every single
    >     repo available on earth and Unikraft is a different project anyway.
    > 
    >     Usually, projects with a MAINTAINERS file have there own get_maintainers
    >     script.
    > 
    > Well: Unikraft is part of the Xen Project.
    > 
    > When I started to clean up the contribution docs it became apparent that
    > there is a lot of inconsistency. Ideally our contribution guide [0] would apply
    > to pretty much *all* sub projects which use a mailing list based workflows
    > 
    > Consistency makes life for developers and also newcomers much easier. And the
    > number of new devs who seem to trip over inconsistencies between projects are
    > quite large (we had 3  cases in 3 weeks which I noticed).
    > 
    > If we start improving our CI infrastructure (which we are), it would be nice if other
    > sub projects had the possibility to easily hook into it or replicate it. But that does require
    > some consistency.
    > 
    > That's why I submitted [1] and [2]
    > 
    > Sub-projects with mail based workflows
    > 1: Hypervisor
    > 2: Hypervisor related repos (livepatch-build-tools, mini-os, xtf, ...)
    > 3: Windows PV Drivers - which will require changes to their MAINTAINERS file
    > 4: Unikraft
    > 
    > Supporting 1 - 3 should be straightforward because you would almost always
    > have xen.git checked out. 4 is more different.
    
    I'd say that folks building 3 are unlikely to have xen.git checked out.
    
Good to know

If there was tooling available that simplifies your workflow, would 
members of the Windows PV Drivers sub-project be willing to use them?

It maybe that for Windows PV Drivers the workflow is not at all command
line based and people tend to use an IDE instead.

Lars

Paul Durrant Aug. 16, 2019, 12:28 p.m. UTC | #7

> -----Original Message-----
> From: Lars Kurth <[e-mail]>
> Sent: 16 August 2019 13:20
> To: Paul Durrant <[e-mail]>; Anthony Perard <[e-mail]>; Julien Grall
> <[e-mail]>
> Cc: [e-mail]; Felipe Huici <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Wei Liu <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>; George
> Dunlap <[e-mail]>; Andrew Cooper <[e-mail]>; Ian Jackson
> <[e-mail]>; Tim (Xen.org) <[e-mail]>; Florian Schmidt <[e-mail]>;
> Jan Beulich <[e-mail]>; Simon Kuenzer <[e-mail]>
> Subject: Re: [Xen-devel] [PATCH] get_maintainers.pl: Enable running the script on unikraft repos
> 
> 
> 
> On 16/08/2019, 13:09, "Paul Durrant" <[e-mail]> wrote:
> 
>     > -----Original Message-----
>     > From: Lars Kurth <[e-mail]>
>     > Sent: 16 August 2019 13:05
>     > To: Anthony Perard <[e-mail]>; Julien Grall <[e-mail]>
>     > Cc: [e-mail]; Felipe Huici <[e-mail]>; Stefano Stabellini
>     > <[e-mail]>; Wei Liu <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>;
> George
>     > Dunlap <[e-mail]>; Andrew Cooper <[e-mail]>; Ian Jackson
>     > <[e-mail]>; Tim (Xen.org) <[e-mail]>; Florian Schmidt
> <[e-mail]>;
>     > Jan Beulich <[e-mail]>; Simon Kuenzer <[e-mail]>; Paul Durrant
>     > <[e-mail]>
>     > Subject: Re: [Xen-devel] [PATCH] get_maintainers.pl: Enable running the script on unikraft repos
>     >
>     > Added Paul Durrant
>     >
>     > On 16/08/2019, 12:17, "Anthony PERARD" <[e-mail]> wrote:
>     >
>     >     On Fri, Aug 16, 2019 at 11:55:16AM +0100, Julien Grall wrote:
>     >     > Hi Lars,
>     >     >
>     >     > On 16/08/2019 11:42, Lars Kurth wrote:
>     >     > > Unikraft repos follow the same syntax as xen.git with the
>     >     > > following exceptions:
>     >     > > * MAINTAINERS files are called MAINTAINERS.md
>     >     > > * M: ... etc blocks are preceded by whitespaces for rendering as
>     >     > >    markup files
>     >     >
>     >     > There is an other difference. The "fallback" category is "UNIKRAFT GENERAL"
>     >     > and not "THE REST".
>     >     >
>     >     > >
>     >     > > This change will
>     >     > > - load MAINTAINERS.md if MAINTAINERS is not present
>     >     > > - deal with indented M: ... blocks
>     >     >
>     >     > One process question. Does it mean Unikraft folks will have to checkout Xen
>     >     > in order to use {add, get}_maintainers.pl? If so, would it make sense to
>     >     > have add_maintainers.pl and script_maintainers.pl in a separate repo that
>     >     > can be added as submodule?
>     >
>     >     Shouldn't instead the Unikraft repo have it's one get_maintainers
>     >     script? xen.git's script doesn't needs to have support for every single
>     >     repo available on earth and Unikraft is a different project anyway.
>     >
>     >     Usually, projects with a MAINTAINERS file have there own get_maintainers
>     >     script.
>     >
>     > Well: Unikraft is part of the Xen Project.
>     >
>     > When I started to clean up the contribution docs it became apparent that
>     > there is a lot of inconsistency. Ideally our contribution guide [0] would apply
>     > to pretty much *all* sub projects which use a mailing list based workflows
>     >
>     > Consistency makes life for developers and also newcomers much easier. And the
>     > number of new devs who seem to trip over inconsistencies between projects are
>     > quite large (we had 3  cases in 3 weeks which I noticed).
>     >
>     > If we start improving our CI infrastructure (which we are), it would be nice if other
>     > sub projects had the possibility to easily hook into it or replicate it. But that does require
>     > some consistency.
>     >
>     > That's why I submitted [1] and [2]
>     >
>     > Sub-projects with mail based workflows
>     > 1: Hypervisor
>     > 2: Hypervisor related repos (livepatch-build-tools, mini-os, xtf, ...)
>     > 3: Windows PV Drivers - which will require changes to their MAINTAINERS file
>     > 4: Unikraft
>     >
>     > Supporting 1 - 3 should be straightforward because you would almost always
>     > have xen.git checked out. 4 is more different.
> 
>     I'd say that folks building 3 are unlikely to have xen.git checked out.
> 
> Good to know
> 
> If there was tooling available that simplifies your workflow, would
> members of the Windows PV Drivers sub-project be willing to use them?
> 

Sure, but our MAINTAINERS files are trivial, and most code changes are made by Owen and myself anyway. Not sure there's really a need for any extra tooling.

> It maybe that for Windows PV Drivers the workflow is not at all command
> line based and people tend to use an IDE instead.
> 

Some folks may use the Visual Studio IDE. I don't, and the Jenkins build workers do rely on the python (pre-9.0) or powershell (9.0 onwards) scripts so building via command line does need to be tested by contributors.

  Paul

Julien Grall Aug. 16, 2019, 1:22 p.m. UTC | #8

On 16/08/2019 12:41, Lars Kurth wrote:
> Hi Julien,

Hi Lars,

> 
> On 16/08/2019, 11:55, "Julien Grall" <[e-mail]> wrote:
> 
>      Hi Lars,
>      
>      On 16/08/2019 11:42, Lars Kurth wrote:
>      > Unikraft repos follow the same syntax as xen.git with the
>      > following exceptions:
>      > * MAINTAINERS files are called MAINTAINERS.md
>      > * M: ... etc blocks are preceded by whitespaces for rendering as
>      >    markup files
>      
>      There is an other difference. The "fallback" category is "UNIKRAFT GENERAL" and
>      not "THE REST".
> 
> That is right. But currently get_maintainers.pl: totally ignores the headlines.
> It just reads
> M: ...
> R: ...
> Etc.

Not really, get_maintainer_role() will return the headline. This is for instance 
used to avoid CC "THE REST" when there are other maintainers:

             # Check the role, if it is not "THE REST" then the file is not
             # only maintained by "THE REST".
             if ( get_maintainer_role($line) ne "supporter:THE REST" ) {
                     $file_maintained_by_the_rest = 0;
             }

 From my understanding "UNIKRAFT GENERAL" behaves exactly the same way.

The roles can also be printed with the option --roles.

> 
> What is different is that "UNIKRAFT GENERAL" does not contain
> F: entries, which have to be added, otherwise no e-mail addresses
> from the section are added. So I was going to send a patch to fix this.
>      
>      > This change will
>      > - load MAINTAINERS.md if MAINTAINERS is not present
>      > - deal with indented M: ... blocks
>      
>      One process question. Does it mean Unikraft folks will have to checkout Xen in
>      order to use {add, get}_maintainers.pl? If so, would it make sense to have
>      add_maintainers.pl and script_maintainers.pl in a separate repo that can be
>      added as submodule?
> 
> The way how the code is written they would either have to check out the
> repo or just get the two scripts and put them in the same directory somewhere
> on the path.
> 
> For things like mini-os, xtf, osstest, ... you would always have a xen.git somewhere
> In the unikraft case that is different. We could break it out, but maybe we can do this
> at a later point in time.

Fair point.

> 
>      > Signed-off-by: Lars Kurth <[e-mail]>
>      > ---
>      > Cc: Andrew Cooper <[e-mail]>
>      > Cc: George Dunlap <[e-mail]>
>      > Cc: Ian Jackson <[e-mail]>
>      > Cc: Jan Beulich <[e-mail]>
>      > Cc: Julien Grall <[e-mail]>
>      > Cc: Konrad Rzeszutek Wilk <[e-mail]>
>      > Cc: Stefano Stabellini <[e-mail]>
>      > Cc: Tim Deegan <[e-mail]>
>      > Cc: Wei Liu <[e-mail]>
>      >
>      > CC: Simon Kuenzer <[e-mail]>
>      > CC: Florian Schmidt <[e-mail]>
>      > CC: Felipe Huici <[e-mail]>
>      > ---
>      >   scripts/get_maintainer.pl | 14 ++++++++++----
>      >   1 file changed, 10 insertions(+), 4 deletions(-)
>      >
>      > diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
>      > index f1e9c904ee..bdb09f8f65 100755
>      > --- a/scripts/get_maintainer.pl
>      > +++ b/scripts/get_maintainer.pl
>      > @@ -284,12 +284,18 @@ if (!top_of_tree($xen_path)) {
>      >   my @typevalue = ();
>      >   my %keyword_hash;
>      >
>      > -open (my $maint, '<', "${xen_path}MAINTAINERS")
>      > -    or die "$P: Can't open MAINTAINERS: $!\n";
>      > +my $maint;
>      > +my $maintainers_file = "MAINTAINERS";
>      > +if (! open ($maint, '<', ${xen_path}.$maintainers_file)) {
>      > +    $maintainers_file = "MAINTAINERS.md";
>      > +    open ($maint, '<', ${xen_path}.$maintainers_file)
>      > +        or die "$P: Can't open MAINTAINERS or MAINTAINERS.md: $!\n";
>      > +}
>      > +
>      >   while (<$maint>) {
>      >       my $line = $_;
>      >
>      > -    if ($line =~ m/^([A-Z]):\s*(.*)/) {
>      > +    if ($line =~ m/^\s*([A-Z]):\s*(.*)/) {
>      
>      As you allow space before the blocks M:, would not this catch the example at the
>      beginning of the file?
>      
>      	M: Mail patches to: FullName <address@domain>
>      	R: Designated reviewer: FullName <address@domain>
> 
> Good point. When I tested it (I sent the last few patches with the change in it),
> it didn't pick up the e-mail addresses. However, when I check with regex101.com
> it is picked up.
> 
> Which means that the values are pushed to @typevalue, aka
> { "R",  "Designated reviewer: FullName <address@domain>" }
> Etc.
> 
> @typevalue seem to be processed by find_first_section(),
> find_starting_index(), find_ending_index()
> 
> And then basically the entries are not processed because the
> block in the example is not consistent and fails the validation
> further down the line
> 
> For example:
> ./scripts/get_maintainer.pl --email --git -f .
> Etc.
> 
> Don't list the e-mail addresses in the examples
>      
> So, what I proposed is probably far too fragile to make sense.
> And doing something which is more accurate will probably
> require major surgery to the scripts.
> 
> I can look at this in a bit more detail and see whether there
> is a way to handle this.
> 
> But I don't want to invest the time to do this really as this
> is probably rather complex.
> 
> @Simon, @Florian, @Felipe: would you be willing to
> change the MAINTAINERS files in your repos such that
> we don't have to implement lots of magic to make the
> patch sending helper scripts work for you?

Modifying MAINTAINERS file in Unikraft looks the safest and less intrusive to to 
do. Let see what the maintainers think :).

Cheers,

Patch

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index f1e9c904ee..bdb09f8f65 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -284,12 +284,18 @@  if (!top_of_tree($xen_path)) {
 my @typevalue = ();
 my %keyword_hash;
 
-open (my $maint, '<', "${xen_path}MAINTAINERS")
-    or die "$P: Can't open MAINTAINERS: $!\n";
+my $maint;
+my $maintainers_file = "MAINTAINERS";
+if (! open ($maint, '<', ${xen_path}.$maintainers_file)) {
+    $maintainers_file = "MAINTAINERS.md";
+    open ($maint, '<', ${xen_path}.$maintainers_file)
+        or die "$P: Can't open MAINTAINERS or MAINTAINERS.md: $!\n";
+}
+
 while (<$maint>) {
     my $line = $_;
 
-    if ($line =~ m/^([A-Z]):\s*(.*)/) {
+    if ($line =~ m/^\s*([A-Z]):\s*(.*)/) {
 	my $type = $1;
 	my $value = $2;
 
@@ -421,7 +427,7 @@  foreach my $file (@ARGV) {
     }
     if ($from_filename) {
 	push(@files, $file);
-	if ($file ne "MAINTAINERS" && -f $file && ($keywords || $file_emails)) {
+	if ($file ne $maintainers_file && -f $file && ($keywords || $file_emails)) {
 	    open(my $f, '<', $file)
 		or die "$P: Can't open $file: $!\n";
 	    my $text = do { local($/) ; <$f> };