| Message ID | 20240717093752.50595-1-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scripts/checkpatch: emit a warning if an imported file is touched | expand |
On Wed, Jul 17, 2024 at 11:37:52AM +0200, Stefano Garzarella wrote: > If a file imported from Linux is touched, emit a warning and suggest > using scripts/update-linux-headers.sh > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > scripts/checkpatch.pl | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index ff373a7083..b0e8266fa2 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -1374,6 +1374,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_imported_file = 0; > my $non_utf8_charset = 0; > > our @report = (); > @@ -1673,8 +1674,17 @@ sub process { > # ignore non-hunk lines and lines being removed > next if (!$hunk_line || $line =~ /^-/); > > -# ignore files that are being periodically imported from Linux > - next if ($realfile =~ /^(linux-headers|include\/standard-headers)\//); > +# ignore files that are being periodically imported from Linux and emit a warning > + if ($realfile =~ /^(linux-headers|include\/standard-headers)\//) { > + if (!$reported_imported_file) { > + $reported_imported_file = 1; > + WARN("added, moved or deleted file(s) " . > + "imported from Linux, are you using " . > + "scripts/update-linux-headers.sh?\n" . > + $herecurr); This is a good hint, but can we add a further check that is a fatal error, if the headers are changed in the same commit as non-header changes. When importing headers, they should only ever be in a self-contained patch with nothing else touched. > + } > + next; > + } With regards, Daniel
On Wed, Jul 17 2024, Stefano Garzarella <sgarzare@redhat.com> wrote: > If a file imported from Linux is touched, emit a warning and suggest > using scripts/update-linux-headers.sh > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > scripts/checkpatch.pl | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index ff373a7083..b0e8266fa2 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -1374,6 +1374,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_imported_file = 0; > my $non_utf8_charset = 0; > > our @report = (); > @@ -1673,8 +1674,17 @@ sub process { > # ignore non-hunk lines and lines being removed > next if (!$hunk_line || $line =~ /^-/); > > -# ignore files that are being periodically imported from Linux > - next if ($realfile =~ /^(linux-headers|include\/standard-headers)\//); > +# ignore files that are being periodically imported from Linux and emit a warning > + if ($realfile =~ /^(linux-headers|include\/standard-headers)\//) { > + if (!$reported_imported_file) { > + $reported_imported_file = 1; > + WARN("added, moved or deleted file(s) " . > + "imported from Linux, are you using " . > + "scripts/update-linux-headers.sh?\n" . > + $herecurr); > + } > + next; > + } Thanks, that looks useful -- just two comments (sorry, my perl-fu is low): - Is there a way to check that this is a proper linux headers update? We'd have to rely on heuristics, but OTOH, we also usually want a headers update to use a certain format ($SUBJECT containing "headers update", patch description pointing to the version this update was done against.) Not sure if it is worth actually trying to figure this out. - A common issue is headers changes mixed in with other code changes, which should not happen -- can we check for that as well and advise to either do a headers update, or use a placeholder patch? > > #trailing whitespace > if ($line =~ /^\+.*\015/) {
On Wed, Jul 17, 2024 at 10:50:51AM GMT, Daniel P. Berrangé wrote: >On Wed, Jul 17, 2024 at 11:37:52AM +0200, Stefano Garzarella wrote: >> If a file imported from Linux is touched, emit a warning and suggest >> using scripts/update-linux-headers.sh >> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> --- >> scripts/checkpatch.pl | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index ff373a7083..b0e8266fa2 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -1374,6 +1374,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_imported_file = 0; >> my $non_utf8_charset = 0; >> >> our @report = (); >> @@ -1673,8 +1674,17 @@ sub process { >> # ignore non-hunk lines and lines being removed >> next if (!$hunk_line || $line =~ /^-/); >> >> -# ignore files that are being periodically imported from Linux >> - next if ($realfile =~ /^(linux-headers|include\/standard-headers)\//); >> +# ignore files that are being periodically imported from Linux and emit a warning >> + if ($realfile =~ /^(linux-headers|include\/standard-headers)\//) { >> + if (!$reported_imported_file) { >> + $reported_imported_file = 1; >> + WARN("added, moved or deleted file(s) " . >> + "imported from Linux, are you using " . >> + "scripts/update-linux-headers.sh?\n" . >> + $herecurr); > >This is a good hint, but can we add a further check that is a fatal error, >if the headers are changed in the same commit as non-header changes. When >importing headers, they should only ever be in a self-contained patch >with nothing else touched. Yep, good point! I'll add that check in v2. Thanks, Stefano
On Wed, Jul 17, 2024 at 11:58:46AM GMT, Cornelia Huck wrote: >On Wed, Jul 17 2024, Stefano Garzarella <sgarzare@redhat.com> wrote: > >> If a file imported from Linux is touched, emit a warning and suggest >> using scripts/update-linux-headers.sh >> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> --- >> scripts/checkpatch.pl | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index ff373a7083..b0e8266fa2 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -1374,6 +1374,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_imported_file = 0; >> my $non_utf8_charset = 0; >> >> our @report = (); >> @@ -1673,8 +1674,17 @@ sub process { >> # ignore non-hunk lines and lines being removed >> next if (!$hunk_line || $line =~ /^-/); >> >> -# ignore files that are being periodically imported from Linux >> - next if ($realfile =~ /^(linux-headers|include\/standard-headers)\//); >> +# ignore files that are being periodically imported from Linux and emit a warning >> + if ($realfile =~ /^(linux-headers|include\/standard-headers)\//) { >> + if (!$reported_imported_file) { >> + $reported_imported_file = 1; >> + WARN("added, moved or deleted file(s) " . >> + "imported from Linux, are you using " . >> + "scripts/update-linux-headers.sh?\n" . >> + $herecurr); >> + } >> + next; >> + } > >Thanks, that looks useful -- just two comments (sorry, my perl-fu is >low): Same perl-fu here ;-P >- Is there a way to check that this is a proper linux headers update? > We'd have to rely on heuristics, but OTOH, we also usually want a > headers update to use a certain format ($SUBJECT containing "headers > update", patch description pointing to the version this update was > done against.) Not sure if it is worth actually trying to figure this > out. I think it can be done though I think we should formalize it somewhere first, or integrate the generation of the commit in the scripts/update-linux-headers.sh At that point here we can add a check based on that. >- A common issue is headers changes mixed in with other code changes, > which should not happen -- can we check for that as well and advise > to either do a headers update, or use a placeholder patch? Yeah, Daniel suggested the same, I'll address in v2. Thanks, Stefano
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index ff373a7083..b0e8266fa2 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1374,6 +1374,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_imported_file = 0; my $non_utf8_charset = 0; our @report = (); @@ -1673,8 +1674,17 @@ sub process { # ignore non-hunk lines and lines being removed next if (!$hunk_line || $line =~ /^-/); -# ignore files that are being periodically imported from Linux - next if ($realfile =~ /^(linux-headers|include\/standard-headers)\//); +# ignore files that are being periodically imported from Linux and emit a warning + if ($realfile =~ /^(linux-headers|include\/standard-headers)\//) { + if (!$reported_imported_file) { + $reported_imported_file = 1; + WARN("added, moved or deleted file(s) " . + "imported from Linux, are you using " . + "scripts/update-linux-headers.sh?\n" . + $herecurr); + } + next; + } #trailing whitespace if ($line =~ /^\+.*\015/) {
If a file imported from Linux is touched, emit a warning and suggest using scripts/update-linux-headers.sh Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- scripts/checkpatch.pl | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)