<div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Jul 26, 2020 at 12:23 PM Mrinal Pandey <<a href="mailto:mrinalmni@gmail.com">mrinalmni@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Jul 26, 2020 at 11:43 AM Lukas Bulwahn <<a href="mailto:lukas.bulwahn@gmail.com" target="_blank">lukas.bulwahn@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On Sat, 25 Jul 2020, Mrinal Pandey wrote:<br>
<br>
> In all the script files(except yaml), SPDX license identifier is expected<br>
<br>
Why do believe a yaml file is a script file? Where did you read that?<br>
<br>
To me, this reads like:<br>
<br>
All fish(except zebras) live in water...<br>
<br>
I think you can just drop that note on yaml.<br>
<br>
> on the second line, the first line being the shebang. Sometimes, the<br>
> diff content includes the SPDX licensing information but excludes the<br>
> shebang when a commit is made to a script file.<br>
> <br>
<br>
You can be more explicit here. Sometimes? You can exactly say when and how <br>
that happens.<br>
<br>
> This can be seen in the commits like commit 37f8173dd849 ("locking/atomics:<br>
> Flip fallbacks and instrumentation") and commit 075c8aa79d54 ("selftests:<br>
> forwarding: tc_actions.sh: add matchall mirror test"). In these cases<br>
> checkpatch issues a false positive warning:<br>
> "Misplaced SPDX-License-Identifier tag - use line 1 instead".<br>
> <br>
> I noticed this false positive, while running checkpatch on the set of<br>
> commits from v5.7 to v5.8-rc1 of the kernel, on the said commits.<br>
> This false positive exists in checkpatch since commit a8da38a9cf0e<br>
> ("checkpatch: add test for SPDX-License-Identifier on wrong line #")<br>
> when the corresponding rule was first added.<br>
> <br>
> The existing logic looks for a shebang in the patch/file being checked<br>
> and if a shebang is encountered it directs checkpatch to expect<br>
> license on the second line by setting `$checklicenseline` to `2`.<br>
><br>
<br>
You can drop those `quotes` around variable names and numbers.<br>
<br>
This can be written shorter:<br>
Currently, if checkpatch find a shebang in line 1, it expects the <br>
license identifier in line 2 instead of line 1.<br>
<br>
> However, this approach doesn't work when we don't have a shebang in the<br>
> patch and `$checklicensline` continues to be `1` in this case leading to<br>
> the unnecessary warning.<br>
> <br>
<br>
However, this only works when both line 1 and 2 are part of the patch.<br>
<br>
> Fix this by setting `$checklicenseline` to `2` by checking if the patch<br>
> comes from a script for the cases when the shebang doesn't appear in the<br>
> patch.<br>
><br>
<br>
You are not really fixing it. You are improving the heuristics.<br>
<br>
So:<br>
<br>
Reduce the false warnings for such patches on scripts, by checking file <br>
extension.<br>
<br>
Alternatively, you could actually also check the file permissions of the <br>
file.<br>
<br></blockquote><div>Sir,</div><div><br></div><div>There are 577 executable files in the kernel, out of which 53 don't have a shebang in the first line.</div><div><br></div><div>Moreover, in these 53 files, 8 should have definitely had it as they are .py or .sh files.</div><div>The other files are .dts, .dtsi, .S, .h, .c files, scripts/atomic/atomics.tbl file, Documentation/Changes file and all the files <br></div><div>inside scripts/atomic/fallbacks that are without extension.</div><div>If I am correct none of these should have a shebang but I am not very sure about files inside scripts/atomic/fallbacks.</div><div><br></div><div>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</div><div>permissions we would warn wrongly for 53 files. I think this is better than checking for extension.<br></div><div>Should we use this heuristic in the new patch I send?</div><div><br></div><div>Thank you.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
Improve the patch; if it looks good then, it could be suitable for the <br>
larger discussion on lkml.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Lukas<br>
<br>
> Signed-off-by: Mrinal Pandey <<a href="mailto:mrinalmni@gmail.com" target="_blank">mrinalmni@gmail.com</a>><br>
> ---<br>
>  scripts/<a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a> | 2 +-<br>
>  1 file changed, 1 insertion(+), 1 deletion(-)<br>
> <br>
> diff --git a/scripts/<a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a> b/scripts/<a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a><br>
> index 4c820607540b..80dfa83ed0fb 100755<br>
> --- a/scripts/<a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a><br>
> +++ b/scripts/<a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a><br>
> @@ -3167,7 +3167,7 @@ sub process {<br>
>  <br>
>  # check for using SPDX license tag at beginning of files<br>
>               if ($realline == $checklicenseline) {<br>
> -                     if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {<br>
> +                     if ($rawline =~ /^[ \+]\s*\#\!\s*\// || $realfile =~ /.*\.\(py\|sh\|pl\|awk\|tc\)/) {<br>
>                               $checklicenseline = 2;<br>
>                       } elsif ($rawline =~ /^\+/) {<br>
>                               my $comment = "";<br>
> -- <br>
> 2.25.1<br>
> <br>
> <br>
</blockquote></div></div>
</blockquote></div>