<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 17, 2020 at 5:18 PM Lukas Bulwahn <<a href="mailto:lukas.bulwahn@gmail.com">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"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 17, 2020 at 11:54 AM Mrinal Pandey <<a href="mailto:mrinalmni@gmail.com" target="_blank">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="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jul 16, 2020, 11:01 Lukas Bulwahn <<a href="mailto:lukas.bulwahn@gmail.com" rel="noreferrer" 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"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jul 16, 2020 at 7:15 AM Mrinal Pandey <<a href="mailto:mrinalmni@gmail.com" rel="noreferrer noreferrer" target="_blank">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 Tue, Jul 14, 2020 at 11:33 AM Lukas Bulwahn <<a href="mailto:lukas.bulwahn@gmail.com" rel="noreferrer noreferrer" 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 Tue, 14 Jul 2020, Mrinal Pandey wrote:<br>
<br>
> <br>
> <br>
> On Tue, Jul 14, 2020 at 1:16 AM Lukas Bulwahn <<a href="mailto:lukas.bulwahn@gmail.com" rel="noreferrer noreferrer" target="_blank">lukas.bulwahn@gmail.com</a>> wrote:<br>
> <br>
> <br>
> On Mon, 13 Jul 2020, Mrinal Pandey wrote:<br>
> <br>
> > In all the scripts, the SPDX license should be on the second line,<br>
> > the first line being the "sh-bang", but checkpatch issues a warning<br>
> > "Misplaced SPDX-License-Identifier tag - use line 1 instead" for the<br>
> > scripts that have SPDX license in the second line.<br>
> ><br>
> > However, this warning is not issued when checkpatch is run on a file using<br>
> > `-f` option. The case for files has been handled gracefully by changing<br>
> > `$checklicenseline` to `2` but a corresponding check when running checkpatch<br>
> > on a commit hash is missing.<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 commits which modified<br>
> > a script file.<br>
> ><br>
> > This check is missing in checkpatch since commit a8da38a9cf0e<br>
> > ("checkpatch: add test for SPDX-License-Identifier on wrong line #")<br>
> > when the corresponding rule was first commited.<br>
> ><br>
> > Fix this by setting `$checklicenseline` to `2` when the diff content that<br>
> > is being checked originates from a script, thus, informing checkpatch that<br>
> > the SPDX license should be on the second line.<br>
> ><br>
> > Signed-off-by: Mrinal Pandey <<a href="mailto:mrinalmni@gmail.com" rel="noreferrer noreferrer" target="_blank">mrinalmni@gmail.com</a>><br>
> > ---<br>
> > scripts/<a href="http://checkpatch.pl" rel="noreferrer noreferrer noreferrer" target="_blank">checkpatch.pl</a> | 3 +++<br>
> > 1 file changed, 3 insertions(+)<br>
> ><br>
> > diff --git a/scripts/<a href="http://checkpatch.pl" rel="noreferrer noreferrer noreferrer" target="_blank">checkpatch.pl</a> b/scripts/<a href="http://checkpatch.pl" rel="noreferrer noreferrer noreferrer" target="_blank">checkpatch.pl</a><br>
> > index 4c820607540b..bbffd0c4449d 100755<br>
> > --- a/scripts/<a href="http://checkpatch.pl" rel="noreferrer noreferrer noreferrer" target="_blank">checkpatch.pl</a><br>
> > +++ b/scripts/<a href="http://checkpatch.pl" rel="noreferrer noreferrer noreferrer" target="_blank">checkpatch.pl</a><br>
> > @@ -3218,6 +3218,9 @@ sub process {<br>
> > next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);<br>
> > <br>
> > # check for using SPDX-License-Identifier on the wrong line number<br>
> > + if ($realfile =~ /^scripts/) {<br>
> > + $checklicenseline = 2;<br>
> > + }<br>
> <br>
> I think this is somehow wrong here. The check for checklicenseline = 2<br>
> looks very different above.<br>
> <br>
> Why does -f work and using a patch file not work?<br>
> <br>
> <br>
> Sir,<br>
> <br>
> I am going to explain my observation based on file `scripts/atomic/gen-atomic-fallback.sh` and<br>
> commit hash `37f8173dd849`.<br>
> <br>
> If we are checking against the file, `checklicenseline` is set to 1 and when `realline` is 1 the above<br>
> `if` block is triggered, then we check if this line is of the form `#!/` using the regular expression<br>
> `^[ \+]\s*\#\!\s*\/`. If this is the case we set `checklicenseline` to `2` informing checkpatch that it should<br>
> expect license on the second line and this works all fine for a file.<br>
> The `if` block below my proposed changes evaluates to false in this case and thus it emits no false warning.<br>
> <br>
> However, If we are checking a diff content, the above `if` block is not triggered at all. This is<br>
> because `realline` stores the actual line number of the line we are checking currently out of diff content.<br>
> This value is 2 because SPDX identifier is indeed at the second line in the file but `checklicenseline` is still<br>
> `1`.<br>
> `realline` will never become equal to 1 again and thus the above `if` condition will never be true in this case.<br>
> Even if the above `if` block is triggered it would not update `checklicenseline` to 2 as the regular expression<br>
> is not satisfied since we don't have sh-bang in diff content and just the SPDX tag.<br>
> If we don't do this, the `if` block below evaluates to true when `realline` is 2 and `checklicensline` is `1`<br>
> leading<br>
> to the emission of a false warning.<br>
> <br>
<br>
So, maybe this whole logic needs to be reworked. If you do not know the <br>
first line, you need to have a different criteria in the first place<br>
to determine if you expect the license tag in the first or the second, <br>
e.g., the file extension, and then checking line 1 for a shebang is just<br>
sanity checking. If it is of a specific file extension, you know line 1<br>
and it is not a shebang, that is probably worth noting as a different <br>
recommendation in <a href="http://checkpatch.pl" rel="noreferrer noreferrer noreferrer" target="_blank">checkpatch.pl</a> anyway.<br></blockquote><div><br></div><div>Sir,</div><div><br></div><div><div>When we know the first line, i.e. we are running checkpatch against a file, the existing logic</div><div>works fine. We probably don't want to induce any changes there.<br></div><div><br></div></div></div></div></blockquote><div><br></div><div>Why not? Do you think we would break things there? Then we should not touch the code at all.</div><div>Do you think we cannot test it properly after the change? Then we should think about how we make a proper regression test suite for that.</div></div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Sir,</div><div dir="auto"><br></div><div dir="auto">No, breaking code or not being able to test is not why I suggest this. I feel that the existing logic handles the case of <br></div><div dir="auto">"Improper or malformed SPDX tag" and "Misplaced SPDX tag" for files i.e. when the first line is known. Anyway, the logic</div><div>for "Misplaced SPDX tag" is written as a different rule. We just need to add in the logic for patches there. <br></div><div>I tried to do this by checking for the scripts directory which was wrong. If I check instead for the file being a script that would make much more sense.<br></div><div dir="auto">Please let me know if you suggest something else.</div></div></div></blockquote><div><br></div><div>Well, you are going to add a different way of checking as you suggested, right? So are you suggesting to have two duplicated ways of checking for the same thing? That seems strange to me.</div></div></div></blockquote><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 class="gmail_quote"><div><br></div><div>Go ahead, make a suitable first proposal, then we will see if and how to refactor.</div></div></div></blockquote><div><br></div><div>Sir,</div><div><br></div><div>No, I would not want to duplicate things. Yes, let me first send a patch to you first, and then we can refactor it if needed.<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 class="gmail_quote"><div> <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="auto"><div dir="auto"><div class="gmail_quote"><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 class="gmail_quote"><div><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 class="gmail_quote"><div>But when we don't know the first line, if am not wrong, it would go somewhat like:<br></div><div>if (the file is a script) {</div><div> if (the first line is shebang) {</div><div> if (the second line is SPDX) {</div><div> All good<br></div><div> } else {</div><div> Issue a misplaced or missing SPDX tag warning<br></div><div> }<br></div><div> } else {</div><div> Issue a missing shebang warning<br></div><div> }<br></div><div>} else {</div><div> if (the first line is SPDX) {</div><div> All good<br></div><div> } else {</div><div> Issue a misplaced or missing SPDX tag warning</div><div> }<br></div><div>}</div><div> <br></div></div></div></blockquote><div><br></div><div>Basically agree, but that logic applies when you know the first line as well (and only, right?). What if you do not know the first line, how would you check "the first line is shebang" if you do not know the first line?</div><div><br></div><div><br></div><div>The missing shebang warning probably needs to go elsewhere in the whole script.</div></div></div></blockquote><div><br></div><div>By not knowing the first line I mean to say that the first line doesn't show up in diff content of the patch but<br></div><div>what if we open the file at that point in the commit history and check for the first line to be a shebang?</div><div>Would it be okay to do that? Once we check the first line we can then continue as suggested.<br></div></div></div></div></div></blockquote><div><br></div><div>I think that is essentially against the general design decision of <a href="http://checkpatch.pl" target="_blank">checkpatch.pl</a>; <a href="http://checkpatch.pl" target="_blank">checkpatch.pl</a> takes the patch but it does not check anything in the current working tree, nor does it know what the "parent" of that patch in the git history really is.</div><div><br></div><div>Maybe Shuah can confirm? Otherwise, I suggest to look if you find checking a file beyond the patch happening anywhere in the current code of <a href="http://checkpatch.pl" target="_blank">checkpatch.pl</a> and documented in the sparse documentation on <a href="http://checkpatch.pl" target="_blank">checkpatch.pl</a>.</div><div><br></div><div>So, I believe you need to make it work without checking the first line (if that is not in the patch).</div></div></div></blockquote><div><br></div><div>Yes. We can't open a file for checking, you are correct here but we need to check for shebang to be on the first line only if it appears</div><div> in the diff content or when we check a complete file, otherwise, we should anyway not chek it since checkpatch only checks the patch or the complete file.</div><div>Am I correct here?<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 class="gmail_quote"><div> </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="auto"><div dir="auto"><div class="gmail_quote"><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 class="gmail_quote"><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 class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> So, what I did was to check if the diff content we are checking actually comes from a script, if yes, we can set<br>
> `checklicenseline` to `2` to avoid this confusion.<br>
><br>
<br>
Why would you think that scripts are only in scripts?<br>
<br>
How about first listing all files where the SPDX tag is in line 2 in the <br>
current repository, e.g., v5.8-rc5?<br>
<br>
Then, we look at that list and determine a suitable criteria for looking <br>
in line 2 for the SPDX tag.<br></blockquote><div><br></div><div>Yes, the scripts are not only in scripts. I have listed all the files where the SPDX tag should be</div><div>on the second line. I've attached the list for reference. We should probably be checking the file</div><div>extension to determine if the tag needs to be on the second line or not.</div><div>The documentation says the SPDX tag should be present in all source files. Do these source files include</div><div>Documentation files too?</div><div><br></div></div></div></blockquote><div><br></div><div>How did you create that list?</div><div>Agree (if the way you created that list makes sense). File extension seems to cover all cases, and checking for the directory 'scripts' does not.</div><div><br></div></div></div></blockquote><div>I issued the command `find . -regex ".*\.\(py\|sh\|pl\)"` to make this list. I should have included awk, YAML and tc files too since they are scripts too.</div><div><br></div></div></div></div></div></blockquote><div><br></div><div>Why not look for all files that have a shebang in the first line?</div></div></div></blockquote></div><div class="gmail_quote"><br></div><div class="gmail_quote">We could be checking a file or a patch. I suggest we take the name of the file we are checking(or the name of the file from which the diff content comes from)</div><div class="gmail_quote">and then run a regex, similar to above, on it to determine if it is a script. If we instead go for checking the first line to be shebang in the current file would it not require to cat or open</div><div class="gmail_quote">the file?</div><div class="gmail_quote"><br></div><div class="gmail_quote">Thank you.<br></div><div class="gmail_quote"><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 class="gmail_quote"><div><br></div><div>Lukas</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="auto"><div dir="auto"><div class="gmail_quote"><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 class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
</blockquote></div></div>
</blockquote></div></div></div>
</div>
</blockquote></div></div>
</blockquote></div></div>