| Message ID | 20200806153331.23907-1-cfontana@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] checkpatch: detect missing changes to trace-events | expand |
Claudio Fontana <cfontana@suse.de> writes: > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- > scripts/checkpatch.pl | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > We could do something similar to MAINTAINERS for trace-events, > ie warning about files added, moved, deleted if we don't see > an update to a trace-events file. I like the idea, but... > To make it more solid it would be better to check the > actual file contents for #include "trace.h" or "trace-root.h", > but I guess this is not possible/practice from checkpatch? ... I'm also concerned about false positives. > If we could only warn for files moved that actually include > trace.h or trace-root.h, we could avoid a lot of false positives. The existing MAINTAINERS check warns even when an existing pattern covers the new file, e.g. when I create or rename a file scripts/qapi/* or qapi/*.json. The former is definitely a false positive, and mildly annoying. The latter, however, could be a true positive: even though the new file is covered by the "QAPI Schema" section, *additional* coverage by some other section may be called for, just like qapi/machine.json is additionally covered by "Machine core". So, even if checkpatch.pl looked at more than just the patch, suppressing false positives would involve guesswork. The new trace-events check is simpler: it's *always* a false positive when the file doesn't include trace.h or trace-root.h. Complication: it could include via some header. I figure that's rare enough to be ignored. Howver, checkpatch.pl checks *patches* by design[*]. It doesn't read the patched files to get more context, or additional files. Perhaps it's simply the wrong place both for the MAINTAINERS check and the trace-events check. We put the MAINTAINERS check there, because it exists and developers run it. "make check-source" would be another option, except it doesn't exist. CI would be yet another option, but we should be careful about what to check only in CI. [*] There's -f to check a source file, which checks "diff -u /dev/null $filename".
On 8/7/20 8:21 AM, Markus Armbruster wrote: > Claudio Fontana <cfontana@suse.de> writes: > >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> --- >> scripts/checkpatch.pl | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> We could do something similar to MAINTAINERS for trace-events, >> ie warning about files added, moved, deleted if we don't see >> an update to a trace-events file. > > I like the idea, but... > >> To make it more solid it would be better to check the >> actual file contents for #include "trace.h" or "trace-root.h", >> but I guess this is not possible/practice from checkpatch? > > ... I'm also concerned about false positives. > >> If we could only warn for files moved that actually include >> trace.h or trace-root.h, we could avoid a lot of false positives. > > The existing MAINTAINERS check warns even when an existing pattern > covers the new file, e.g. when I create or rename a file scripts/qapi/* > or qapi/*.json. The former is definitely a false positive, and mildly > annoying. The latter, however, could be a true positive: even though > the new file is covered by the "QAPI Schema" section, *additional* > coverage by some other section may be called for, just like > qapi/machine.json is additionally covered by "Machine core". So, even > if checkpatch.pl looked at more than just the patch, suppressing false > positives would involve guesswork. > > The new trace-events check is simpler: it's *always* a false positive > when the file doesn't include trace.h or trace-root.h. > > Complication: it could include via some header. I figure that's rare > enough to be ignored. Agreed with all the above. I think something like this would be useful (and not annoying) only if we could at least limit the false positives by checking that the file effectively includes trace.h or trace-root.h . > > Howver, checkpatch.pl checks *patches* by design[*]. It doesn't read > the patched files to get more context, or additional files. Right - if we do end up checking the patched files for this feature though (thus breaking this design rule), would this have any other consequence beyond providing this feature? We need to call checkpatch.pl from the top srcdir anyway, and if we don't find the patched file or we don't match trace.h or trace-root.h in there, well we just don't emit the warning... > > Perhaps it's simply the wrong place both for the MAINTAINERS check and > the trace-events check. We put the MAINTAINERS check there, because it > exists and developers run it. "make check-source" would be another > option, except it doesn't exist. CI would be yet another option, but we > should be careful about what to check only in CI. > > > [*] There's -f to check a source file, which checks "diff -u /dev/null > $filename". > Won't insist, would just have found it useful to avoid forgetting that piece when moving stuff around. Just for kicks I will send an RFC v2 to see what it could look like attempting to look for trace.h and trace-root.h, probably it's something I would personally use. Ciao, thanks! Claudio
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index bd3faa154c..1c8afebed5 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1300,6 +1300,7 @@ sub process { my $in_header_lines = $file ? 0 : 1; my $in_commit_log = 0; #Scanning lines before patch my $reported_maintainer_file = 0; + my $reported_trace_events_file = 0; my $non_utf8_charset = 0; our @report = (); @@ -1524,15 +1525,24 @@ sub process { if ($line =~ /^\s*MAINTAINERS\s*\|/) { $reported_maintainer_file = 1; } - +# similar check for trace-events + if ($line =~ /^\s*trace-events\s*\|/) { + $reported_trace_events_file = 1; + } # Check for added, moved or deleted files - if (!$reported_maintainer_file && !$in_commit_log && + if (!$in_commit_log && ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ || $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ || ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ && (defined($1) || defined($2))))) { - $reported_maintainer_file = 1; - WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr); + if (!$reported_maintainer_file) { + $reported_maintainer_file = 1; + WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr); + } + if (!$reported_trace_events_file) { + $reported_trace_events_file = 1; + WARN("added, moved or deleted file(s), does trace-events need updating?\n" . $herecurr); + } } # Check for wrappage within a valid hunk of the file
Signed-off-by: Claudio Fontana <cfontana@suse.de> --- scripts/checkpatch.pl | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) We could do something similar to MAINTAINERS for trace-events, ie warning about files added, moved, deleted if we don't see an update to a trace-events file. To make it more solid it would be better to check the actual file contents for #include "trace.h" or "trace-root.h", but I guess this is not possible/practice from checkpatch? If we could only warn for files moved that actually include trace.h or trace-root.h, we could avoid a lot of false positives. Thanks, Claudio