diff mbox series

[RFC,v2,02/10] MAINTAINERS: Introduce V: entry for tests

Message ID 20231205184503.79769-3-Nikolai.Kondrashov@redhat.com
State New
Headers show
Series MAINTAINERS: Introduce V: entry for tests | expand

Commit Message

Nikolai Kondrashov Dec. 5, 2023, 6:02 p.m. UTC
Introduce a new 'V:' ("Verify") entry to MAINTAINERS. The entry accepts
a test suite command which is proposed to be executed for each
contribution to the subsystem.

Extend scripts/get_maintainer.pl to support retrieving the V: entries
when '--test' option is specified.

Require the entry values to not contain the '@' character, so they could
be distinguished from emails (always) output by get_maintainer.pl. Make
scripts/checkpatch.pl check that they don't.

Update entry ordering in both scripts/checkpatch.pl and
scripts/parse-maintainers.pl.

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 Documentation/process/submitting-patches.rst | 18 ++++++++++++++++++
 MAINTAINERS                                  |  7 +++++++
 scripts/checkpatch.pl                        | 10 +++++++++-
 scripts/get_maintainer.pl                    | 17 +++++++++++++++--
 scripts/parse-maintainers.pl                 |  3 ++-
 5 files changed, 51 insertions(+), 4 deletions(-)

Comments

Nikolai Kondrashov Dec. 6, 2023, 4:23 p.m. UTC | #1
On 12/6/23 10:12, David Gow wrote:
> I'm pretty happy with this personally, though I definitely think we
> need the support for tests which aren't just executable scripts (e.g.
> the docs in patch 6).
> 
> The get_maintailer.pl bits, and hence the requirement to not include
> '@', feel a little bit 'off': I'd rather get_maintainer.pl kept emails
> and tests separate by some other means (either having --test _only_
> print tests, not emails at all, or by giving them a prefix like
> 'TEST:' or something). But that is diverging more from the existing
> behaviour of get_maintainer.pl, so I could go either way.
> 
> Otherwise, this looks pretty good. I'll give it a proper test tomorrow
> alongside the other patches.

Thanks for the review, David!

Yeah, I don't like the '@' bit myself, but it seems to be the path of least 
resistance right now (not necessarily the best one, of course).

I'm up for adding an option to get_maintainer.pl that disables email output, 
if people like that, though.

Nick
Joe Perches Dec. 6, 2023, 4:38 p.m. UTC | #2
On Wed, 2023-12-06 at 18:23 +0200, Nikolai Kondrashov wrote:
> On 12/6/23 10:12, David Gow wrote:
> > I'm pretty happy with this personally, though I definitely think we
> > need the support for tests which aren't just executable scripts (e.g.
> > the docs in patch 6).
> > 
> > The get_maintailer.pl bits, and hence the requirement to not include
> > '@', feel a little bit 'off': I'd rather get_maintainer.pl kept emails
> > and tests separate by some other means (either having --test _only_
> > print tests, not emails at all, or by giving them a prefix like
> > 'TEST:' or something). But that is diverging more from the existing
> > behaviour of get_maintainer.pl, so I could go either way.
> > 
> > Otherwise, this looks pretty good. I'll give it a proper test tomorrow
> > alongside the other patches.
> 
> Thanks for the review, David!
> 
> Yeah, I don't like the '@' bit myself, but it seems to be the path of least 
> resistance right now (not necessarily the best one, of course).
> 
> I'm up for adding an option to get_maintainer.pl that disables email output, 
> if people like that, though.

That already exists though I don't understand the
specific requirement here

--nom --nol --nor

from
$ ./scripts/get_maintainer.pl --help
[]
    --m => include maintainer(s) if any
    --r => include reviewer(s) if any
    --n => include name 'Full Name <addr@domain.tld>'
    --l => include list(s) if any
[]
   Most options have both positive and negative forms.
      The negative forms for --<foo> are --no<foo> and --no-<foo>.
diff mbox series

Patch

diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 86d346bcb8ef0..f034feaf1369e 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -228,6 +228,24 @@  You should be able to justify all violations that remain in your
 patch.
 
 
+Test your changes
+-----------------
+
+Test the patch to the best of your ability. Check the MAINTAINERS file for the
+subsystem(s) you are changing to see if there are any **V:** entries
+proposing particular test suite commands. E.g.::
+
+  V: tools/testing/kunit/run_checks.py
+
+Supplying the ``--test`` option to ``scripts/get_maintainer.pl`` adds **V:**
+entries to its output.
+
+Execute the commands, if any, to test your changes.
+
+All commands must be executed from the root of the source tree. Each command
+outputs usage information, if an -h/--help option is specified.
+
+
 Select the recipients for your patch
 ------------------------------------
 
diff --git a/MAINTAINERS b/MAINTAINERS
index 788be9ab5b733..e6d0777e21657 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -59,6 +59,13 @@  Descriptions of section entries and preferred order
 	      matches patches or files that contain one or more of the words
 	      printk, pr_info or pr_err
 	   One regex pattern per line.  Multiple K: lines acceptable.
+	V: *Test suite* proposed for execution. The command that could be
+	   executed to verify changes to the maintained subsystem.
+	   Must be executed from the root of the source tree.
+	   Must support the -h/--help option.
+	   Cannot contain '@' character.
+	   V: tools/testing/kunit/run_checks.py
+	   One test suite per line.
 
 Maintainers List
 ----------------
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 25fdb7fda1128..a184e576c980b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3657,7 +3657,7 @@  sub process {
 				}
 			}
 # check MAINTAINERS entries for the right ordering too
-			my $preferred_order = 'MRLSWQBCPTFXNK';
+			my $preferred_order = 'MRLSWQBCPVTFXNK';
 			if ($rawline =~ /^\+[A-Z]:/ &&
 			    $prevrawline =~ /^[\+ ][A-Z]:/) {
 				$rawline =~ /^\+([A-Z]):\s*(.*)/;
@@ -3683,6 +3683,14 @@  sub process {
 					}
 				}
 			}
+# check MAINTAINERS V: entries are valid
+			if ($rawline =~ /^\+V:\s*(.*)/) {
+				my $name = $1;
+				if ($name =~ /@/) {
+					ERROR("TEST_PROPOSAL_INVALID",
+					      "Test proposal cannot contain '\@' character\n" . $herecurr);
+				}
+			}
 		}
 
 		if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 37901c2298388..804215a7477db 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -53,6 +53,7 @@  my $output_section_maxlen = 50;
 my $scm = 0;
 my $tree = 1;
 my $web = 0;
+my $test = 0;
 my $subsystem = 0;
 my $status = 0;
 my $letters = "";
@@ -270,6 +271,7 @@  if (!GetOptions(
 		'scm!' => \$scm,
 		'tree!' => \$tree,
 		'web!' => \$web,
+		'test!' => \$test,
 		'letters=s' => \$letters,
 		'pattern-depth=i' => \$pattern_depth,
 		'k|keywords!' => \$keywords,
@@ -319,13 +321,14 @@  if ($sections || $letters ne "") {
     $status = 0;
     $subsystem = 0;
     $web = 0;
+    $test = 0;
     $keywords = 0;
     $keywords_in_file = 0;
     $interactive = 0;
 } else {
-    my $selections = $email + $scm + $status + $subsystem + $web;
+    my $selections = $email + $scm + $status + $subsystem + $web + $test;
     if ($selections == 0) {
-	die "$P:  Missing required option: email, scm, status, subsystem or web\n";
+	die "$P:  Missing required option: email, scm, status, subsystem, web or test\n";
     }
 }
 
@@ -634,6 +637,7 @@  my %hash_list_to;
 my @list_to = ();
 my @scm = ();
 my @web = ();
+my @test = ();
 my @subsystem = ();
 my @status = ();
 my %deduplicate_name_hash = ();
@@ -665,6 +669,11 @@  if ($web) {
     output(@web);
 }
 
+if ($test) {
+    @test = uniq(@test);
+    output(@test);
+}
+
 exit($exit);
 
 sub self_test {
@@ -850,6 +859,7 @@  sub get_maintainers {
     @list_to = ();
     @scm = ();
     @web = ();
+    @test = ();
     @subsystem = ();
     @status = ();
     %deduplicate_name_hash = ();
@@ -1072,6 +1082,7 @@  MAINTAINER field selection options:
   --status => print status if any
   --subsystem => print subsystem name if any
   --web => print website(s) if any
+  --test => print test(s) if any
 
 Output type options:
   --separator [, ] => separator for multiple entries on 1 line
@@ -1382,6 +1393,8 @@  sub add_categories {
 		push(@scm, $pvalue . $suffix);
 	    } elsif ($ptype eq "W") {
 		push(@web, $pvalue . $suffix);
+	    } elsif ($ptype eq "V") {
+		push(@test, $pvalue . $suffix);
 	    } elsif ($ptype eq "S") {
 		push(@status, $pvalue . $suffix);
 	    }
diff --git a/scripts/parse-maintainers.pl b/scripts/parse-maintainers.pl
index 2ca4eb3f190d6..dbc2b79bcaa46 100755
--- a/scripts/parse-maintainers.pl
+++ b/scripts/parse-maintainers.pl
@@ -44,6 +44,7 @@  usage: $P [options] <pattern matching regexes>
       B:  URI for bug tracking/submission
       C:  URI for chat
       P:  URI or file for subsystem specific coding styles
+      V:  Test suite name
       T:  SCM tree type and location
       F:  File and directory pattern
       X:  File and directory exclusion pattern
@@ -73,7 +74,7 @@  sub by_category($$) {
 
 sub by_pattern($$) {
     my ($a, $b) = @_;
-    my $preferred_order = 'MRLSWQBCPTFXNK';
+    my $preferred_order = 'MRLSWQBCPVTFXNK';
 
     my $a1 = uc(substr($a, 0, 1));
     my $b1 = uc(substr($b, 0, 1));