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

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

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@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?

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