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

[Lukas Bulwahn]

On Sun, Jul 26, 2020 at 12:23 PM Mrinal Pandey <mrinalmni at gmail.com> 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.
>>
>> 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/da08e35a/attachment.html>