Re: [Linux-kernel-mentees] [PATCH] checkpatch: Fix SPDX license check for scripts

[Lukas Bulwahn <lukas.bulwahn@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5153 bytes --]



On Tue, 14 Jul 2020, Mrinal Pandey wrote:

> 
> 
> On Tue, Jul 14, 2020 at 1:16 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> 
> 
>       On Mon, 13 Jul 2020, Mrinal Pandey wrote:
> 
>       > In all the scripts, the SPDX license should be on the second line,
>       > the first line being the "sh-bang", but checkpatch issues a warning
>       > "Misplaced SPDX-License-Identifier tag - use line 1 instead" for the
>       > scripts that have SPDX license in the second line.
>       >
>       > However, this warning is not issued when checkpatch is run on a file using
>       > `-f` option. The case for files has been handled gracefully by changing
>       > `$checklicenseline` to `2` but a corresponding check when running checkpatch
>       > on a commit hash is missing.
>       >
>       > 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 commits which modified
>       > a script file.
>       >
>       > This check is missing in checkpatch since commit a8da38a9cf0e
>       > ("checkpatch: add test for SPDX-License-Identifier on wrong line #")
>       > when the corresponding rule was first commited.
>       >
>       > Fix this by setting `$checklicenseline` to `2` when the diff content that
>       > is being checked originates from a script, thus, informing checkpatch that
>       > the SPDX license should be on the second line.
>       >
>       > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
>       > ---
>       >  scripts/checkpatch.pl | 3 +++
>       >  1 file changed, 3 insertions(+)
>       >
>       > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>       > index 4c820607540b..bbffd0c4449d 100755
>       > --- a/scripts/checkpatch.pl
>       > +++ b/scripts/checkpatch.pl
>       > @@ -3218,6 +3218,9 @@ sub process {
>       >               next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
>       > 
>       >  # check for using SPDX-License-Identifier on the wrong line number
>       > +             if ($realfile =~ /^scripts/) {
>       > +                    $checklicenseline = 2;
>       > +             }
> 
>       I think this is somehow wrong here. The check for checklicenseline = 2
>       looks very different above.
> 
>       Why does -f work and using a patch file not work?
> 
> 
> Sir,
> 
> I am going to explain my observation based on file `scripts/atomic/gen-atomic-fallback.sh` and
> commit hash `37f8173dd849`.
> 
> If we are checking against the file, `checklicenseline` is set to 1 and when `realline` is 1 the above
> `if` block is triggered, then we check if this line is of the form `#!/` using the regular expression
> `^[ \+]\s*\#\!\s*\/`. If this is the case we set `checklicenseline` to `2` informing checkpatch that it should
> expect license on the second line and this works all fine for a file.
> The `if` block below my proposed changes evaluates to false in this case and thus it emits no false warning.
> 
> However, If we are checking a diff content, the above `if` block is not triggered at all. This is
> because `realline` stores the actual line number of the line we are checking currently out of diff content.
> This value is 2 because SPDX identifier is indeed at the second line in the file but `checklicenseline` is still
> `1`.
> `realline` will never become equal to 1 again and thus the above `if` condition will never be true in this case.
> Even if the above `if` block is triggered it would not update `checklicenseline` to 2 as the regular expression
> is not satisfied since we don't have sh-bang in diff content and just the SPDX tag.
> If we don't do this, the `if` block below evaluates to true when `realline` is 2 and `checklicensline` is `1`
> leading
> to the emission of a false warning.
> 

So, maybe this whole logic needs to be reworked. If you do not know the 
first line, you need to have a different criteria in the first place
to determine if you expect the license tag in the first or the second, 
e.g., the file extension, and then checking line 1 for a shebang is just
sanity checking. If it is of a specific file extension, you know line 1
and it is not a shebang, that is probably worth noting as a different 
recommendation in checkpatch.pl anyway.

Lukas


> So, what I did was to check if the diff content we are checking actually comes from a script, if yes, we can set
> `checklicenseline` to `2` to avoid this confusion.
>

Why would you think that scripts are only in scripts?

How about first listing all files where the SPDX tag is in line 2 in the 
current repository, e.g., v5.8-rc5?

Then, we look at that list and determine a suitable criteria for looking 
in line 2 for the SPDX tag.

Lukas

> Please let me know if this is reasonable.
> 
> Thank you.
>  
> 
>       Lukas
> 
> 
>       >               if ($realline != $checklicenseline &&
>       >                   $rawline =~ /\bSPDX-License-Identifier:/ &&
>       >                   substr($line, @-, @+ - @-) eq "$;" x (@+ - @-)) {
>       > --
>       > 2.25.1
>       >
>       >
> 
> 
> 

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees