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

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

[Mrinal Pandey]

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

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.

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.

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`;
 			$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;
+		}
 		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

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

[Lukas Bulwahn]

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.

>  			$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.


>  		if ($realline == $checklicenseline) {
>  			if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
>  				$checklicenseline = 2;
> -- 
> 2.25.1
> 
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Mrinal Pandey]

[-- 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

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

[Lukas Bulwahn]

On Mon, 3 Aug 2020, Mrinal Pandey wrote:

> 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.

Just a hint:

A line number has no meaning if you do not say the commit sha of the 
repository you are looking at.

I simply ignored your sentence anyway and just read yes; so, it does not 
matter. I assume you know what you are doing.

Lukas

> > 
> > 
> > >  		if ($realline == $checklicenseline) {
> > >  			if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
> > >  				$checklicenseline = 2;
> > > -- 
> > > 2.25.1
> > > 
> > > 
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Mrinal Pandey]

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

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.

Reduce the number of false warnings for such patches on scripts by
checking the permissions of the file.

Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
---
 scripts/checkpatch.pl | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4c820607540b..695300839f48 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3166,6 +3166,10 @@ sub process {
 		}
 
 # check for using SPDX license tag at beginning of files
+		my $filepermissions = `stat -c "%a" $realfile`;
+		if (int(substr($filepermissions, 0, 1)) % 2) {
+			$checklicenseline = 2;
+		}
 		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

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

[Lukas Bulwahn]

On Mon, 27 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.
> 
> Reduce the number of false warnings for such patches on scripts by
> checking the permissions of the file.
>

I would certainly explain the different options you considered and why you 
finally decided for the solution you propose.
 
> Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
> ---
>  scripts/checkpatch.pl | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4c820607540b..695300839f48 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3166,6 +3166,10 @@ sub process {
>  		}
>  
>  # check for using SPDX license tag at beginning of files
> +		my $filepermissions = `stat -c "%a" $realfile`;

I fear this is now placed somewhere, so you are running stat on every line 
again and again and ...

How about determining permissions when the realfile is set?

I bet that is already done anyway by getting the information from the 
patch, as you cannot assume the $realfile to exist.

You only can use the information in the patch file.

Read more code in checkpatch.pl.

> +		if (int(substr($filepermissions, 0, 1)) % 2) {

That is a really complicated way to express it. I will bet you will find a 
better way already implemented in checkpatch.pl.

> +			$checklicenseline = 2;
> +		}
>  		if ($realline == $checklicenseline) {
>  			if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
>  				$checklicenseline = 2;
> -- 
> 2.25.1


I guess we will need another iteration of this patch.

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