[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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

Hi Lars,

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: ...

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 
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 <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 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>" }

@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 .

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 :).


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.