Re: [Xen-devel] [PATCH] get_maintainers.pl: Enable running the script on unikraft repos

[Lars Kurth <lars.kurth@xxxxxxxxxx>]

Hi Julien,

On 16/08/2019, 11:55, "Julien Grall" <julien.grall@xxxxxxx> 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 <lars.kurth@xxxxxxxxxx>
    > ---
    > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
    > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
    > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
    > Cc: Jan Beulich <jbeulich@xxxxxxxx>
    > Cc: Julien Grall <julien.grall@xxxxxxx>
    > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
    > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
    > Cc: Tim Deegan <tim@xxxxxxx>
    > Cc: Wei Liu <wl@xxxxxxx>
    > 
    > CC: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
    > CC: Florian Schmidt <florian.schmidt@xxxxxxxxx>
    > CC: Felipe Huici <felipe.huici@xxxxxxxxx>
    > ---
    >   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

   

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