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

Mrinal Pandey mrinalmni at gmail.com
Sun Jul 26 14:04:12 UTC 2020


On 20/07/26 01:09PM, Lukas Bulwahn wrote:
> 
> 
> 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.

Sir,

I apologize if this has been causing trouble lately.
I have been using neomutt for sending patches and Gmail for conversation.
Could you please let me know once the issues you have seen with my mailing
client so that I can address them?
> 
> > 
> > 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?

Okay, I would send patches to fix this.

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

Is there anything I can do to fix this?
> 
> > 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.

It would be great if we could do this.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.linuxfoundation.org/pipermail/linux-kernel-mentees/attachments/20200726/27d87510/attachment.sig>


More information about the Linux-kernel-mentees mailing list