| Message ID | 20231205184503.79769-3-Nikolai.Kondrashov@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | MAINTAINERS: Introduce V: entry for tests | expand |
On Tue, 2023-12-05 at 20:02 +0200, Nikolai Kondrashov wrote: > Require the entry values to not contain the '@' character, so they could > be distinguished from emails (always) output by get_maintainer.pl. Why is this useful? Why the need to distinguish?
On Wed, 6 Dec 2023 at 02:45, Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com> wrote: > > 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> > --- 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. Cheers, -- David
On 12/5/23 20:58, Joe Perches wrote: > On Tue, 2023-12-05 at 20:02 +0200, Nikolai Kondrashov wrote: >> Require the entry values to not contain the '@' character, so they could >> be distinguished from emails (always) output by get_maintainer.pl. > > Why is this useful? > Why the need to distinguish? This is because get_maintainer.pl seems to insist on *always* outputting emails. I guess that was its sole original purpose, and it stuck to it? It kinda makes sense to me, especially given the name of the script, but at the same time, as a consequence you can't query *only* the tests (or anything but emails, really). Therefore we have to be able to somehow filter out the emails from the get_maintainer.pl output, to get only the tests. Email addresses *have* to have the '@' character (seems to be the only reliable thing about them :D), so naturally I chose that as the way to distinguish them from the tests. It's ugly, but considering the get_maintainer.pl legacy, it's good enough. I don't mind changing get_maintainer.pl, though, to stop it from outputting emails (e.g. given an option), if that works for everyone involved. Nick
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
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>.
On 12/6/23 18:38, Joe Perches wrote: > 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>. > Thanks, Joe! Yeah, I already explored that way, but it seems to be explicitly forbidden: $ scripts/get_maintainer.pl --nom --nol --nor 0001-dt-bindings-mailbox-convert-bcm2835-mbox-bindings-to.patch scripts/get_maintainer.pl: Please select at least 1 email option So, I assumed there is a reason and an intention behind this behavior and went the other way. Nick
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));
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(-)