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

[Mrinal Pandey <mrinalmni@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4590 bytes --]

On 20/08/01 08:04AM, Lukas Bulwahn wrote:
> 
> 
> On Thu, 30 Jul 2020, Mrinal Pandey wrote:
> 
> > In all the script files, SPDX license identifier is expected on the second
> > line, the first line being the shebang.
> > 
> > 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".
> > 
> > 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.
> > 
> > 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.
> >
> ----
> > Improve this by ensuring the patch to have originated from a script by
> > checking the extension. However, there are 120 files in the kernel source
> > that do not have an extension but have a shebang in line 1.
> >
> 
> Well, you are not doing that anymore. So the commit message is wrong.
> 
> Maybe, you simply say:
> 
>   - what is the problem?
>   - what are the alternatives considered?
>   - what did you evaluate on these two alternatives?
>   - why did you decide the one you chose?
> 
> If you structure it that way, it is easier to follow your thoughts.
> 
> > An alternate approach is to check for permissions of the file. There are
> > 53 files in kernel source that have executable flag set but don't have a
> > shebang in the first line. These files could be patched suitably so that
> > they don't issue false warnings. Hence, choose this approach.
> >
> 
> I would not mention the potential follow-up work in this commit.
> You can say that:
> 
> At first sight on these 53 files, it seems that these files have a wrong 
> file permission set or could be reasonably extended with a shebang and 
> license information.
> Hence, further clean-up in the repository would make this heuristics work 
> even more precisely.
> 
> > Reduce SPDX license false warnings on patches by checking the permissions
> > on the file.
> > 
> > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
> > ---
> >  scripts/checkpatch.pl | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 4c820607540b..c55595113499 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2368,6 +2368,7 @@ sub process {
> >  
> >  	# Trace the real file/line as we go.
> >  	my $realfile = '';
> > +	my $realfile_perms = '';
> >  	my $realline = 0;
> >  	my $realcnt = 0;
> >  	my $here = '';
> > @@ -2555,11 +2556,13 @@ sub process {
> >  		if ($line =~ /^diff --git.*?(\S+)$/) {
> >  			$realfile = $1;
> >  			$realfile =~ s@^([^/]*)/@@ if (!$file);
> > +			$realfile_perms = `stat -c "%a" $realfile`;
> 
> Again, this is totally wrong!
> 
> We already noted that you can only use the information provided in the 
> patch file.
> 
> Is that information on file permissions provided with a patch?
> Where is it provided? Find out and then parse that information.

Sir,

I tried to improve the patch and the commit message. Please let me know
if I can further improve on it.
> 
> >  			$in_commit_log = 0;
> >  			$found_file = 1;
> >  		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
> >  			$realfile = $1;
> >  			$realfile =~ s@^([^/]*)/@@ if (!$file);
> > +			$realfile_perms = `stat -c "%a" $realfile`;
> >  			$in_commit_log = 0;
> >  
> >  			$p1_prefix = $1;
> > @@ -3166,6 +3169,9 @@ sub process {
> >  		}
> >  
> >  # check for using SPDX license tag at beginning of files
> > +		if ($realfile_perms =~ /[7531]\d{0,2}/) {
> > +			$checklicenseline = 2;
> > +		}
> 
> That check looks good. I assume you copied this expression from another 
> place in checkpatch.pl.

Yes, I copied this check from line 2649 in checkpatch.pl.
> 
> 
> >  		if ($realline == $checklicenseline) {
> >  			if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
> >  				$checklicenseline = 2;
> > -- 
> > 2.25.1
> > 
> > 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

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