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

[Lukas Bulwahn]

On Sun, 26 Jul 2020, Mrinal Pandey wrote:

> 
> 
> 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.
> 

First, please fix your email client. It is broken. If you want to work in 
the kernel community, your email client needs to work as defined in the 
kernel documentation.

Read the kernel documentation on email clients and fix your client.

> 
> 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.

How about fixing those eight?

> The other files are .dts, .dtsi, .S, .h, .c files, scripts/atomic/atomics.tbl file, Documentation/Changes file

It seems wrong that those have an executable flag in the first place.

That could be fixed as well.

> 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.
> 

Probably that whole atomics stuff needs some documentation and cleanup.

> 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?
>

Yes, go ahead a rework your patch.


Lukas
 
> 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
>       >
>       >
> 
> 
>