[Linux-kernel-mentees] [PATCH] checkpatch: Fix SPDX license check for script files

Mrinal Pandey mrinalmni at gmail.com
Sun Jul 26 10:23:16 UTC 2020


On Sun, Jul 26, 2020 at 11:43 AM Lukas Bulwahn <lukas.bulwahn at gmail.com>
wrote:

>
>
> On Sat, 25 Jul 2020, Mrinal Pandey wrote:
>
> > In all the script files(except yaml), SPDX license identifier is expected
>
> Why do believe a yaml file is a script file? Where did you read that?
>
> To me, this reads like:
>
> All fish(except zebras) live in water...
>
> I think you can just drop that note on yaml.
>
> > on the second line, the first line being the shebang. Sometimes, the
> > diff content includes the SPDX licensing information but excludes the
> > shebang when a commit is made to a script file.
> >
>
> You can be more explicit here. Sometimes? You can exactly say when and how
> that happens.
>
> > This can be seen in the commits like commit 37f8173dd849
> ("locking/atomics:
> > Flip fallbacks and instrumentation") and commit 075c8aa79d54 ("selftests:
> > forwarding: tc_actions.sh: add matchall mirror test"). In these cases
> > checkpatch issues a false positive warning:
> > "Misplaced SPDX-License-Identifier tag - use line 1 instead".
> >
> > I noticed this false positive, while running checkpatch on the set of
> > commits from v5.7 to v5.8-rc1 of the kernel, on the said commits.
> > This false positive exists in checkpatch since commit a8da38a9cf0e
> > ("checkpatch: add test for SPDX-License-Identifier on wrong line #")
> > when the corresponding rule was first added.
> >
> > The existing logic looks for a shebang in the patch/file being checked
> > and if a shebang is encountered it directs checkpatch to expect
> > license on the second line by setting `$checklicenseline` to `2`.
> >
>
> You can drop those `quotes` around variable names and numbers.
>
> This can be written shorter:
> Currently, if checkpatch find a shebang in line 1, it expects the
> license identifier in line 2 instead of line 1.
>
> > However, this approach doesn't work when we don't have a shebang in the
> > patch and `$checklicensline` continues to be `1` in this case leading to
> > the unnecessary warning.
> >
>
> However, this only works when both line 1 and 2 are part of the patch.
>
> > Fix this by setting `$checklicenseline` to `2` by checking if the patch
> > comes from a script for the cases when the shebang doesn't appear in the
> > patch.
> >
>
> You are not really fixing it. You are improving the heuristics.
>
> So:
>
> Reduce the false warnings for such patches on scripts, by checking file
> extension.
>
> Alternatively, you could actually also check the file permissions of the
> file.
>
> Sir,

There are 577 executable files in the kernel, out of which 53 don't have a
shebang in the first line.

Moreover, in these 53 files, 8 should have definitely had it as they are
.py or .sh files.
The other files are .dts, .dtsi, .S, .h, .c files,
scripts/atomic/atomics.tbl file, Documentation/Changes file and all the
files
inside scripts/atomic/fallbacks that are without extension.
If I am correct none of these should have a shebang but I am not very sure
about files inside scripts/atomic/fallbacks.

If we check for file extension we warn wrongly for 120 files iff shebang
doesn't appear in the patch but if we check for
permissions we would warn wrongly for 53 files. I think this is better than
checking for extension.
Should we use this heuristic in the new patch I send?

Thank you.

>
>
> Improve the patch; if it looks good then, it could be suitable for the
> larger discussion on lkml.
>

> Lukas
>
> > Signed-off-by: Mrinal Pandey <mrinalmni at gmail.com>
> > ---
> >  scripts/checkpatch.pl | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 4c820607540b..80dfa83ed0fb 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3167,7 +3167,7 @@ sub process {
> >
> >  # check for using SPDX license tag at beginning of files
> >               if ($realline == $checklicenseline) {
> > -                     if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
> > +                     if ($rawline =~ /^[ \+]\s*\#\!\s*\// || $realfile
> =~ /.*\.\(py\|sh\|pl\|awk\|tc\)/) {
> >                               $checklicenseline = 2;
> >                       } elsif ($rawline =~ /^\+/) {
> >                               my $comment = "";
> > --
> > 2.25.1
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linuxfoundation.org/pipermail/linux-kernel-mentees/attachments/20200726/343013d3/attachment-0001.html>


More information about the Linux-kernel-mentees mailing list