[Xen-devel] [PATCH v2] Allow get_maintainer.pl / add_maintainers.pl scripts to be called outside of xen.git

[Xen-devel] [PATCH v2] Allow get_maintainer.pl / add_maintainers.pl scripts to be called outside of xen.git

[Lars Kurth]

Use-case: Allow using both scripts on xen repositories such as
mini-os.git, osstest.git,

Tool changes:
* add_maintainers.pl: $get_maintainer inherits path from $0
* get_maintainer.pl: warn (instead fo die) when called
  from a different tree

Assumptions: the repository contains a MAINTAINERS file that
follows the same conventions as the file in xen.git

A suggested template

========================================================
This file follows the same conventions as outlined in
xen.git:MAINTAINERS. Please refer to the file in xen.git
for more information.

THE REST
M:	MAINTAINER1 <maintainer1@email.com>
M:      MAINTAINER2 <maintainer2@email.com>
L:	xen-devel@lists.xenproject.org
S:	Supported
F:	*/
========================================================

Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
---
 scripts/add_maintainers.pl |  4 ++--
 scripts/get_maintainer.pl  | 13 +++++++++++--
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/scripts/add_maintainers.pl b/scripts/add_maintainers.pl
index 09e9f6609f..5a6d0f631b 100755
--- a/scripts/add_maintainers.pl
+++ b/scripts/add_maintainers.pl
@@ -26,9 +26,9 @@ sub insert ($$$$);
 sub hastag ($$);
 
 # Tool Variables
-my $get_maintainer      = "./scripts/get_maintainer.pl";
-
 my $tool = $0;
+my $get_maintainer = $tool;
+$get_maintainer =~ s/add_maintainers/get_maintainer/;
 my $usage = <<EOT;
 OPTIONS:
 --------
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 174dfb7e40..f1e9c904ee 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -266,8 +266,17 @@ if ($email &&
 }
 
 if (!top_of_tree($xen_path)) {
-    die "$P: The current directory does not appear to be "
-	. "a Xen source tree.\n";
+    # Do not exit, but print an error message to STDERR to allow calling
+    # the tool from xen-related repos such as mini-os.git,
+    # live patch-build-tools.git, etc
+    print STDERR "$P:\n".
+          "=====================================================\n".
+          "WARNING: The current directory does not appear to be \n".
+	  "the xen.git source tree.\n\n".
+          "The tool works outside of the xen.git tree, if the\n".
+          "MAINTAINERS file follows the same format as that of\n".
+          "xen.git. Use at your own peril.\n".
+          "=====================================================\n";
 }
 
 ## Read MAINTAINERS for type/value pairs
-- 
2.13.0


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

Re: [Xen-devel] [PATCH v2] Allow get_maintainer.pl / add_maintainers.pl scripts to be called outside of xen.git

[Julien Grall]

Hi Lars,

Thank you for the patch.

On 15/08/2019 18:22, Lars Kurth wrote:
> Use-case: Allow using both scripts on xen repositories such as
> mini-os.git, osstest.git,
> 
> Tool changes:
> * add_maintainers.pl: $get_maintainer inherits path from $0
> * get_maintainer.pl: warn (instead fo die) when called
>    from a different tree
> 
> Assumptions: the repository contains a MAINTAINERS file that
> follows the same conventions as the file in xen.git
> 
> A suggested template
> 
> ========================================================
> This file follows the same conventions as outlined in
> xen.git:MAINTAINERS. Please refer to the file in xen.git
> for more information.
> 
> THE REST
> M:	MAINTAINER1 <maintainer1@email.com>
> M:      MAINTAINER2 <maintainer2@email.com>
> L:	xen-devel@lists.xenproject.org
> S:	Supported
> F:	*/
> ========================================================
> 
> Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
> ---
>   scripts/add_maintainers.pl |  4 ++--
>   scripts/get_maintainer.pl  | 13 +++++++++++--
>   2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/add_maintainers.pl b/scripts/add_maintainers.pl
> index 09e9f6609f..5a6d0f631b 100755
> --- a/scripts/add_maintainers.pl
> +++ b/scripts/add_maintainers.pl
> @@ -26,9 +26,9 @@ sub insert ($$$$);
>   sub hastag ($$);
>   
>   # Tool Variables
> -my $get_maintainer      = "./scripts/get_maintainer.pl";
> -
>   my $tool = $0;
> +my $get_maintainer = $tool;
> +$get_maintainer =~ s/add_maintainers/get_maintainer/;
>   my $usage = <<EOT;
>   OPTIONS:
>   --------
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index 174dfb7e40..f1e9c904ee 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -266,8 +266,17 @@ if ($email &&
>   }
>   
>   if (!top_of_tree($xen_path)) {
> -    die "$P: The current directory does not appear to be "
> -	. "a Xen source tree.\n";
> +    # Do not exit, but print an error message to STDERR to allow calling
> +    # the tool from xen-related repos such as mini-os.git,
> +    # live patch-build-tools.git, etc
> +    print STDERR "$P:\n".
> +          "=====================================================\n".
> +          "WARNING: The current directory does not appear to be \n".
> +	  "the xen.git source tree.\n\n".
> +          "The tool works outside of the xen.git tree, if the\n".
> +          "MAINTAINERS file follows the same format as that of\n".
> +          "xen.git. Use at your own peril.\n".
> +          "=====================================================\n";

 From my understanding, any use on mini-os.git & co will be legitimate. However, 
we still print the WARNING in those cases.

Usually WARNING means something needs attention. As most of the users will 
likely copy/paste from the wiki, we are going to have report asking why the 
WARNING is there.

I think it would make sense to try to downgrade the message a bit when possible. 
For instance, we could check if the section "THE REST" is present in the file 
MAINTAINERS. If not, this is likely not a file we are able to support.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2] Allow get_maintainer.pl / add_maintainers.pl scripts to be called outside of xen.git

[Lars Kurth]

On 16/08/2019, 11:01, "Julien Grall" <julien.grall@arm.com> wrote:

    Hi Lars,
    
    Thank you for the patch.
    
    On 15/08/2019 18:22, Lars Kurth wrote:
    > Use-case: Allow using both scripts on xen repositories such as
    > mini-os.git, osstest.git,
    > 
    > Tool changes:
    > * add_maintainers.pl: $get_maintainer inherits path from $0
    > * get_maintainer.pl: warn (instead fo die) when called
    >    from a different tree
    > 
    > Assumptions: the repository contains a MAINTAINERS file that
    > follows the same conventions as the file in xen.git
    > 
    > A suggested template
    > 
    > ========================================================
    > This file follows the same conventions as outlined in
    > xen.git:MAINTAINERS. Please refer to the file in xen.git
    > for more information.
    > 
    > THE REST
    > M:	MAINTAINER1 <maintainer1@email.com>
    > M:      MAINTAINER2 <maintainer2@email.com>
    > L:	xen-devel@lists.xenproject.org
    > S:	Supported
    > F:	*/
    > ========================================================
    > 
    > Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
    > ---
    >   scripts/add_maintainers.pl |  4 ++--
    >   scripts/get_maintainer.pl  | 13 +++++++++++--
    >   2 files changed, 13 insertions(+), 4 deletions(-)
    > 
    > diff --git a/scripts/add_maintainers.pl b/scripts/add_maintainers.pl
    > index 09e9f6609f..5a6d0f631b 100755
    > --- a/scripts/add_maintainers.pl
    > +++ b/scripts/add_maintainers.pl
    > @@ -26,9 +26,9 @@ sub insert ($$$$);
    >   sub hastag ($$);
    >   
    >   # Tool Variables
    > -my $get_maintainer      = "./scripts/get_maintainer.pl";
    > -
    >   my $tool = $0;
    > +my $get_maintainer = $tool;
    > +$get_maintainer =~ s/add_maintainers/get_maintainer/;
    >   my $usage = <<EOT;
    >   OPTIONS:
    >   --------
    > diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
    > index 174dfb7e40..f1e9c904ee 100755
    > --- a/scripts/get_maintainer.pl
    > +++ b/scripts/get_maintainer.pl
    > @@ -266,8 +266,17 @@ if ($email &&
    >   }
    >   
    >   if (!top_of_tree($xen_path)) {
    > -    die "$P: The current directory does not appear to be "
    > -	. "a Xen source tree.\n";
    > +    # Do not exit, but print an error message to STDERR to allow calling
    > +    # the tool from xen-related repos such as mini-os.git,
    > +    # live patch-build-tools.git, etc
    > +    print STDERR "$P:\n".
    > +          "=====================================================\n".
    > +          "WARNING: The current directory does not appear to be \n".
    > +	  "the xen.git source tree.\n\n".
    > +          "The tool works outside of the xen.git tree, if the\n".
    > +          "MAINTAINERS file follows the same format as that of\n".
    > +          "xen.git. Use at your own peril.\n".
    > +          "=====================================================\n";
    
     From my understanding, any use on mini-os.git & co will be legitimate. However, 
    we still print the WARNING in those cases.
    
    Usually WARNING means something needs attention. As most of the users will 
    likely copy/paste from the wiki, we are going to have report asking why the 
    WARNING is there.
    
    I think it would make sense to try to downgrade the message a bit when possible. 
    For instance, we could check if the section "THE REST" is present in the file 
    MAINTAINERS. If not, this is likely not a file we are able to support.
    
I thought about this and it is not as easy as it seems, because the script only parses
M: ... &c lines

Maybe the best way to address this would be to include some identifier into the
MAINTAINERS file (after the header with all the definitions). 

FORMAT: xen-project-maintainers <version>  
(note that this is not currently picked up by the tool)

Or

V: xen-project-maintainers <version>  
(note that this would be picked up by the tool)

Then any compliant version is easily identifiable and the warning can be supressed.

That would make maintainers look like 

============
....
V:  xen-project-maintainers 1.6

ACPI
M:	Jan Beulich <jbeulich@suse.com>  
...
============

This seems to be more of a can of worms than I thought.

Cheers
Lars


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

Re: [Xen-devel] [PATCH v2] Allow get_maintainer.pl / add_maintainers.pl scripts to be called outside of xen.git

[Julien Grall]

On 16/08/2019 13:17, Lars Kurth wrote:
> On 16/08/2019, 11:01, "Julien Grall" <julien.grall@arm.com> wrote:
>       From my understanding, any use on mini-os.git & co will be legitimate. However,
>      we still print the WARNING in those cases.
>      
>      Usually WARNING means something needs attention. As most of the users will
>      likely copy/paste from the wiki, we are going to have report asking why the
>      WARNING is there.
>      
>      I think it would make sense to try to downgrade the message a bit when possible.
>      For instance, we could check if the section "THE REST" is present in the file
>      MAINTAINERS. If not, this is likely not a file we are able to support.
>      
> I thought about this and it is not as easy as it seems, because the script only parses
> M: ... &c lines

The script is able to parse the section name. See get_maintainer_role().

Although, I am not sure how early the function can get called.

But...

> 
> Maybe the best way to address this would be to include some identifier into the
> MAINTAINERS file (after the header with all the definitions).
> 
> FORMAT: xen-project-maintainers <version>
> (note that this is not currently picked up by the tool)
> 
> Or
> 
> V: xen-project-maintainers <version>
> (note that this would be picked up by the tool)

Any of these solutions are also a potential alternative.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2] Allow get_maintainer.pl / add_maintainers.pl scripts to be called outside of xen.git

[Lars Kurth]

> On 16 Aug 2019, at 14:28, Julien Grall <julien.grall@arm.com> wrote:
> 
> 
> 
> On 16/08/2019 13:17, Lars Kurth wrote:
>> On 16/08/2019, 11:01, "Julien Grall" <julien.grall@arm.com> wrote:
>>      From my understanding, any use on mini-os.git & co will be legitimate. However,
>>     we still print the WARNING in those cases.
>>          Usually WARNING means something needs attention. As most of the users will
>>     likely copy/paste from the wiki, we are going to have report asking why the
>>     WARNING is there.
>>          I think it would make sense to try to downgrade the message a bit when possible.
>>     For instance, we could check if the section "THE REST" is present in the file
>>     MAINTAINERS. If not, this is likely not a file we are able to support.
>>     I thought about this and it is not as easy as it seems, because the script only parses
>> M: ... &c lines
> 
> The script is able to parse the section name. See get_maintainer_role().
> 
> Although, I am not sure how early the function can get called.
> 
> But...

That may make it feasible to go down that route.
Incidentially both Linux as well as QEMU MAINTAINERs files use the same syntax
as us (with a few extra tags which we don't have)

Not sure whether this would be a problem

>> Maybe the best way to address this would be to include some identifier into the
>> MAINTAINERS file (after the header with all the definitions).
>> FORMAT: xen-project-maintainers <version>
>> (note that this is not currently picked up by the tool)
>> Or
>> V: xen-project-maintainers <version>
>> (note that this would be picked up by the tool)
> 
> Any of these solutions are also a potential alternative.

I will see what others think and take it from there

Lars


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

Re: [Xen-devel] [PATCH v2] Allow get_maintainer.pl / add_maintainers.pl scripts to be called outside of xen.git

[Lars Kurth]

On 16/08/2019, 06:43, "Lars Kurth" <lars.kurth.xen@gmail.com> wrote:

    
    
    > On 16 Aug 2019, at 14:28, Julien Grall <julien.grall@arm.com> wrote:
    > 
    > 
    > 
    > On 16/08/2019 13:17, Lars Kurth wrote:
    >> On 16/08/2019, 11:01, "Julien Grall" <julien.grall@arm.com> wrote:
    >>      From my understanding, any use on mini-os.git & co will be legitimate. However,
    >>     we still print the WARNING in those cases.
    >>          Usually WARNING means something needs attention. As most of the users will
    >>     likely copy/paste from the wiki, we are going to have report asking why the
    >>     WARNING is there.
    >>          I think it would make sense to try to downgrade the message a bit when possible.
    >>     For instance, we could check if the section "THE REST" is present in the file
    >>     MAINTAINERS. If not, this is likely not a file we are able to support.
    >>     I thought about this and it is not as easy as it seems, because the script only parses
    >> M: ... &c lines
    > 
    > The script is able to parse the section name. See get_maintainer_role().
    > 
    > Although, I am not sure how early the function can get called.
    > 
    > But...
    
    That may make it feasible to go down that route.
    Incidentially both Linux as well as QEMU MAINTAINERs files use the same syntax
    as us (with a few extra tags which we don't have)
    
    Not sure whether this would be a problem
    
    >> Maybe the best way to address this would be to include some identifier into the
    >> MAINTAINERS file (after the header with all the definitions).
    >> FORMAT: xen-project-maintainers <version>
    >> (note that this is not currently picked up by the tool)
    >> Or
    >> V: xen-project-maintainers <version>
    >> (note that this would be picked up by the tool)
    > 
    > Any of these solutions are also a potential alternative.
    
    I will see what others think and take it from there
    
Hi all. I would like to get this resolved and was looking for 
opinions. The thread is about enabling usage of get_maintainer.pl / 
add_maintainers.pl on sister repositories for xen.git, such as 
xtf.git, osstest.git, mini-os.git, ... to have a consistent tools story 
and make patch submission for newcomers easier. We have 
several options:

1) Warn if the tools are applied outside the Xen tree
Julian felt this is likely confusing

2) Do not warn under some conditions
2.1) Use THE REST as identifier to avoid the warning
Cons: Warning would disappear because Linux and QEMU also have THE REST 
This may not be an issue as both MAINTAINERS files follow the same format
However, there may be subtle differences in behaviour for unusual options 
for the get_maintainer.pl script as we have not been tracking all changes

2.2) Introduce a unique identifier in MAINTAINERS
This would imply introducing a unique identifier for xen related
MAINTAINER files
Pros: More accurate
Cons: Pollutes file format

I don’t have a strong opinion and will follow majority consensus.
Maybe people can vote on the options and I will just implement
what most people prefer

Lars

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

Re: [Xen-devel] [PATCH v2] Allow get_maintainer.pl / add_maintainers.pl scripts to be called outside of xen.git

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3807 bytes --]

On Fri, 23 Aug 2019, Lars Kurth wrote:
> 16/08/2019, 06:43, "Lars Kurth" <lars.kurth.xen@gmail.com> wrote:
>     > On 16 Aug 2019, at 14:28, Julien Grall <julien.grall@arm.com> wrote:
>     > 
>     > 
>     > 
>     > On 16/08/2019 13:17, Lars Kurth wrote:
>     >> On 16/08/2019, 11:01, "Julien Grall" <julien.grall@arm.com> wrote:
>     >>      From my understanding, any use on mini-os.git & co will be legitimate. However,
>     >>     we still print the WARNING in those cases.
>     >>          Usually WARNING means something needs attention. As most of the users will
>     >>     likely copy/paste from the wiki, we are going to have report asking why the
>     >>     WARNING is there.
>     >>          I think it would make sense to try to downgrade the message a bit when possible.
>     >>     For instance, we could check if the section "THE REST" is present in the file
>     >>     MAINTAINERS. If not, this is likely not a file we are able to support.
>     >>     I thought about this and it is not as easy as it seems, because the script only parses
>     >> M: ... &c lines
>     > 
>     > The script is able to parse the section name. See get_maintainer_role().
>     > 
>     > Although, I am not sure how early the function can get called.
>     > 
>     > But...
>     
>     That may make it feasible to go down that route.
>     Incidentially both Linux as well as QEMU MAINTAINERs files use the same syntax
>     as us (with a few extra tags which we don't have)
>     
>     Not sure whether this would be a problem
>     
>     >> Maybe the best way to address this would be to include some identifier into the
>     >> MAINTAINERS file (after the header with all the definitions).
>     >> FORMAT: xen-project-maintainers <version>
>     >> (note that this is not currently picked up by the tool)
>     >> Or
>     >> V: xen-project-maintainers <version>
>     >> (note that this would be picked up by the tool)
>     > 
>     > Any of these solutions are also a potential alternative.
>     
>     I will see what others think and take it from there
>     
> Hi all. I would like to get this resolved and was looking for 
> opinions. The thread is about enabling usage of get_maintainer.pl / 
> add_maintainers.pl on sister repositories for xen.git, such as 
> xtf.git, osstest.git, mini-os.git, ... to have a consistent tools story 
> and make patch submission for newcomers easier. We have 
> several options:
> 
> 1) Warn if the tools are applied outside the Xen tree
> Julian felt this is likely confusing
> 
> 2) Do not warn under some conditions
> 2.1) Use THE REST as identifier to avoid the warning
> Cons: Warning would disappear because Linux and QEMU also have THE REST 
> This may not be an issue as both MAINTAINERS files follow the same format
> However, there may be subtle differences in behaviour for unusual options 
> for the get_maintainer.pl script as we have not been tracking all changes
> 
> 2.2) Introduce a unique identifier in MAINTAINERS
> This would imply introducing a unique identifier for xen related
> MAINTAINER files
> Pros: More accurate
> Cons: Pollutes file format
> 
> I don’t have a strong opinion and will follow majority consensus.
> Maybe people can vote on the options and I will just implement
> what most people prefer
 
Any of these options are OK, really. Aside from the other subprojects, I
think one interesting case to consider is when a user calls
get_maintainer.pl on tools/qemu-xen-dir, which should return a warning
or error because that's QEMU, not Xen.

So, I think it would be best to go with 2.2) introducing a new tag to
distinguish the Xen MAINTAINERS file from the QEMU MAINTAINERS file, so
that we can properly return a warning for tools/qemu-xen-dir, but at the
same time it could work fine for mini-os.git.

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [Xen-devel] [PATCH v2] Allow get_maintainer.pl / add_maintainers.pl scripts to be called outside of xen.git

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v2] Allow get_maintainer.pl / add_maintainers.pl scripts to be called outside of xen.git"):
> On Fri, 23 Aug 2019, Lars Kurth wrote:
> > Hi all. I would like to get this resolved and was looking for 
> > opinions. The thread is about enabling usage of get_maintainer.pl / 
> > add_maintainers.pl on sister repositories for xen.git, such as 
> > xtf.git, osstest.git, mini-os.git, ... to have a consistent tools story 
> > and make patch submission for newcomers easier. We have 
> > several options:
> > 
> > 1) Warn if the tools are applied outside the Xen tree
> > Julian felt this is likely confusing
> > 
> > 2) Do not warn under some conditions
> > 2.1) Use THE REST as identifier to avoid the warning
> > Cons: Warning would disappear because Linux and QEMU also have THE REST 
> > This may not be an issue as both MAINTAINERS files follow the same format
> > However, there may be subtle differences in behaviour for unusual options 
> > for the get_maintainer.pl script as we have not been tracking all changes
> > 
> > 2.2) Introduce a unique identifier in MAINTAINERS
> > This would imply introducing a unique identifier for xen related
> > MAINTAINER files
> > Pros: More accurate
> > Cons: Pollutes file format

I think the "Con" for 2.2 is very minor indeed.

> Any of these options are OK, really. Aside from the other subprojects, I
> think one interesting case to consider is when a user calls
> get_maintainer.pl on tools/qemu-xen-dir, which should return a warning
> or error because that's QEMU, not Xen.
> 
> So, I think it would be best to go with 2.2) introducing a new tag to
> distinguish the Xen MAINTAINERS file from the QEMU MAINTAINERS file, so
> that we can properly return a warning for tools/qemu-xen-dir, but at the
> same time it could work fine for mini-os.git.

I agree that 2.2 is the best answer.  I would be content with any of
the above options.

Ian.

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