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

Mrinal Pandey mrinalmni at gmail.com
Tue Aug 4 15:56:40 UTC 2020


On 20/08/03 12:59PM, Lukas Bulwahn wrote:
> 
> 
> On Mon, 3 Aug 2020, Mrinal Pandey wrote:
> 
> > The diff content includes the SPDX licensing information but excludes the
> > shebang when a change is made to a script file in 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".
> > 
> > Currently, if checkpatch finds a shebang in line 1, it expects the
> > license identifier in line 2. However, this doesn't work when a shebang
> > isn't found on the line 1.
> 
> It does not work when the diff does not contain line 1, but only line 2,
> because then the shebang check for line 1 cannot work.
> 
> > 
> > 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 alternatives considered to improve this check were looking the file
> > to be a script by either examining the file extension or file permissions.
> >
> 
> Make this sentence shorter. Try.
>  
> > The evaluation on former option resulted in 120 files which had a shebang
> > in the first line but no file extension. This didn't look like a promising
> > result and hence I dropped the idea of using this approach.
> > 
> > The evaluation on the latter approach shows that there are 53 files in the
> > kernel which have an executable bit set but don't have a shebang in the
> > first line.
> > 
> > At the first sight on these 53 files, it seems that they either have a
> > wrong file permission set or could be reasonably extended with a shebang
> > and SPDX license information. Thus, further cleanup in the repository
> > would make the latter approach to work even more precisely.
> > 
> > Hence, I chose to check the file permissions to determine if the file is a
> > script and notify checkpatch to expect SPDX on second line for such files.
> >
> 
> There is no notification here. Think about better wording.
>  
> > Signed-off-by: Mrinal Pandey <mrinalmni at gmail.com>
> > ---
> >  scripts/checkpatch.pl | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 4c820607540b..bae1dd824518 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3166,6 +3166,9 @@ sub process {
> >  		}
> >  
> >  # check for using SPDX license tag at beginning of files
> > +		if ($line =~ /^index\ .*\.\..*\ .*[7531]\d{0,2}$/) {
> > +			$checklicenseline = 2;
> > +		}
> 
> That check looks good now.
> 
> >  		if ($realline == $checklicenseline) {
> >  			if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
> >  				$checklicenseline = 2;
> 
> This is probably broken now. It should check for shebang in line 1 and 
> then set checklicenseline to line 2, right?

Sir,

Should we remove this check? Earlier when I checked for file extension
we had 120 cases where this check was also needed but now we have a
better heuristic which is going to work for all cases where license
should be on line 2 irrespective of the fact that we know the first line
or not.

If I am missing out on something and we should not be removing this check,
then I suggest placing the new heuristics below this block so that it doesn't
interfere with the existing logic.

Please let me know which path should I go about and then I shall resend
the patch with the modified commit message.

Thank you.
> 
> > -- 
> > 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/20200804/b258ae0b/attachment-0001.sig>


More information about the Linux-kernel-mentees mailing list