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

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

[Mrinal Pandey]

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

In all the script files(except yaml), SPDX license identifier is expected
on the second line, the first line being the shebang. Sometimes, the
diff content includes the SPDX licensing information but excludes the
shebang when a commit is made to a script file.

This can be seen in the commits like 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.

The existing logic looks for a shebang in the patch/file being checked
and if a shebang is encountered it directs checkpatch to expect
license on the second line by setting `$checklicenseline` to `2`.

However, this approach doesn't work when we don't have a shebang in the
patch and `$checklicensline` continues to be `1` in this case leading to
the unnecessary warning.

Fix this by setting `$checklicenseline` to `2` by checking if the patch
comes from a script for the cases when the shebang doesn't appear in the
patch.

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4c820607540b..80dfa83ed0fb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3167,7 +3167,7 @@ sub process {
 
 # check for using SPDX license tag at beginning of files
 		if ($realline == $checklicenseline) {
-			if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
+			if ($rawline =~ /^[ \+]\s*\#\!\s*\// || $realfile =~ /.*\.\(py\|sh\|pl\|awk\|tc\)/) {
 				$checklicenseline = 2;
 			} elsif ($rawline =~ /^\+/) {
 				my $comment = "";
-- 
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: Fix SPDX license check for script files

[Lukas Bulwahn]

On Sat, 25 Jul 2020, Mrinal Pandey wrote:

> In all the script files(except yaml), SPDX license identifier is expected

Why do believe a yaml file is a script file? Where did you read that?

To me, this reads like:

All fish(except zebras) live in water...

I think you can just drop that note on yaml.

> on the second line, the first line being the shebang. Sometimes, the
> diff content includes the SPDX licensing information but excludes the
> shebang when a commit is made to a script file.
> 

You can be more explicit here. Sometimes? You can exactly say when and how 
that happens.

> This can be seen in the commits like 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.
> 
> The existing logic looks for a shebang in the patch/file being checked
> and if a shebang is encountered it directs checkpatch to expect
> license on the second line by setting `$checklicenseline` to `2`.
>

You can drop those `quotes` around variable names and numbers.

This can be written shorter:
Currently, if checkpatch find a shebang in line 1, it expects the 
license identifier in line 2 instead of line 1.
 
> However, this approach doesn't work when we don't have a shebang in the
> patch and `$checklicensline` continues to be `1` in this case leading to
> the unnecessary warning.
> 

However, this only works when both line 1 and 2 are part of the patch.
 
> Fix this by setting `$checklicenseline` to `2` by checking if the patch
> comes from a script for the cases when the shebang doesn't appear in the
> patch.
>

You are not really fixing it. You are improving the heuristics.

So:

Reduce the false warnings for such patches on scripts, by checking file 
extension.

Alternatively, you could actually also check the file permissions of the 
file.



Improve the patch; if it looks good then, it could be suitable for the 
larger discussion on lkml.

Lukas

> Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4c820607540b..80dfa83ed0fb 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3167,7 +3167,7 @@ sub process {
>  
>  # check for using SPDX license tag at beginning of files
>  		if ($realline == $checklicenseline) {
> -			if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
> +			if ($rawline =~ /^[ \+]\s*\#\!\s*\// || $realfile =~ /.*\.\(py\|sh\|pl\|awk\|tc\)/) {
>  				$checklicenseline = 2;
>  			} elsif ($rawline =~ /^\+/) {
>  				my $comment = "";
> -- 
> 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: Fix SPDX license check for script files

[Mrinal Pandey]

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

On Sun, Jul 26, 2020 at 11:43 AM Lukas Bulwahn <lukas.bulwahn@gmail.com>
wrote:

>
>
> On Sat, 25 Jul 2020, Mrinal Pandey wrote:
>
> > In all the script files(except yaml), SPDX license identifier is expected
>
> Why do believe a yaml file is a script file? Where did you read that?
>
> To me, this reads like:
>
> All fish(except zebras) live in water...
>
> I think you can just drop that note on yaml.
>
> > on the second line, the first line being the shebang. Sometimes, the
> > diff content includes the SPDX licensing information but excludes the
> > shebang when a commit is made to a script file.
> >
>
> You can be more explicit here. Sometimes? You can exactly say when and how
> that happens.
>
> > This can be seen in the commits like 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.
> >
> > The existing logic looks for a shebang in the patch/file being checked
> > and if a shebang is encountered it directs checkpatch to expect
> > license on the second line by setting `$checklicenseline` to `2`.
> >
>
> You can drop those `quotes` around variable names and numbers.
>
> This can be written shorter:
> Currently, if checkpatch find a shebang in line 1, it expects the
> license identifier in line 2 instead of line 1.
>
> > However, this approach doesn't work when we don't have a shebang in the
> > patch and `$checklicensline` continues to be `1` in this case leading to
> > the unnecessary warning.
> >
>
> However, this only works when both line 1 and 2 are part of the patch.
>
> > Fix this by setting `$checklicenseline` to `2` by checking if the patch
> > comes from a script for the cases when the shebang doesn't appear in the
> > patch.
> >
>
> You are not really fixing it. You are improving the heuristics.
>
> So:
>
> Reduce the false warnings for such patches on scripts, by checking file
> extension.
>
> Alternatively, you could actually also check the file permissions of the
> file.
>
> Sir,

There are 577 executable files in the kernel, out of which 53 don't have a
shebang in the first line.

Moreover, in these 53 files, 8 should have definitely had it as they are
.py or .sh files.
The other files are .dts, .dtsi, .S, .h, .c files,
scripts/atomic/atomics.tbl file, Documentation/Changes file and all the
files
inside scripts/atomic/fallbacks that are without extension.
If I am correct none of these should have a shebang but I am not very sure
about files inside scripts/atomic/fallbacks.

If we check for file extension we warn wrongly for 120 files iff shebang
doesn't appear in the patch but if we check for
permissions we would warn wrongly for 53 files. I think this is better than
checking for extension.
Should we use this heuristic in the new patch I send?

Thank you.

>
>
> Improve the patch; if it looks good then, it could be suitable for the
> larger discussion on lkml.
>

> Lukas
>
> > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
> > ---
> >  scripts/checkpatch.pl | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 4c820607540b..80dfa83ed0fb 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3167,7 +3167,7 @@ sub process {
> >
> >  # check for using SPDX license tag at beginning of files
> >               if ($realline == $checklicenseline) {
> > -                     if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
> > +                     if ($rawline =~ /^[ \+]\s*\#\!\s*\// || $realfile
> =~ /.*\.\(py\|sh\|pl\|awk\|tc\)/) {
> >                               $checklicenseline = 2;
> >                       } elsif ($rawline =~ /^\+/) {
> >                               my $comment = "";
> > --
> > 2.25.1
> >
> >
>

[-- Attachment #1.2: Type: text/html, Size: 6065 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: Fix SPDX license check for script files

[Lukas Bulwahn]

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

On Sun, Jul 26, 2020 at 12:23 PM Mrinal Pandey <mrinalmni@gmail.com> wrote:

>
>
> On Sun, Jul 26, 2020 at 11:43 AM Lukas Bulwahn <lukas.bulwahn@gmail.com>
> wrote:
>
>>
>>
>> On Sat, 25 Jul 2020, Mrinal Pandey wrote:
>>
>> > In all the script files(except yaml), SPDX license identifier is
>> expected
>>
>> Why do believe a yaml file is a script file? Where did you read that?
>>
>> To me, this reads like:
>>
>> All fish(except zebras) live in water...
>>
>> I think you can just drop that note on yaml.
>>
>> > on the second line, the first line being the shebang. Sometimes, the
>> > diff content includes the SPDX licensing information but excludes the
>> > shebang when a commit is made to a script file.
>> >
>>
>> You can be more explicit here. Sometimes? You can exactly say when and
>> how
>> that happens.
>>
>> > This can be seen in the commits like 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.
>> >
>> > The existing logic looks for a shebang in the patch/file being checked
>> > and if a shebang is encountered it directs checkpatch to expect
>> > license on the second line by setting `$checklicenseline` to `2`.
>> >
>>
>> You can drop those `quotes` around variable names and numbers.
>>
>> This can be written shorter:
>> Currently, if checkpatch find a shebang in line 1, it expects the
>> license identifier in line 2 instead of line 1.
>>
>> > However, this approach doesn't work when we don't have a shebang in the
>> > patch and `$checklicensline` continues to be `1` in this case leading to
>> > the unnecessary warning.
>> >
>>
>> However, this only works when both line 1 and 2 are part of the patch.
>>
>> > Fix this by setting `$checklicenseline` to `2` by checking if the patch
>> > comes from a script for the cases when the shebang doesn't appear in the
>> > patch.
>> >
>>
>> You are not really fixing it. You are improving the heuristics.
>>
>> So:
>>
>> Reduce the false warnings for such patches on scripts, by checking file
>> extension.
>>
>> Alternatively, you could actually also check the file permissions of the
>> file.
>>
>> Sir,
>
> There are 577 executable files in the kernel, out of which 53 don't have a
> shebang in the first line.
>
> Moreover, in these 53 files, 8 should have definitely had it as they are
> .py or .sh files.
> The other files are .dts, .dtsi, .S, .h, .c files,
> scripts/atomic/atomics.tbl file, Documentation/Changes file and all the
> files
> inside scripts/atomic/fallbacks that are without extension.
> If I am correct none of these should have a shebang but I am not very sure
> about files inside scripts/atomic/fallbacks.
>
> If we check for file extension we warn wrongly for 120 files iff shebang
> doesn't appear in the patch but if we check for
> permissions we would warn wrongly for 53 files. I think this is better
> than checking for extension.
> Should we use this heuristic in the new patch I send?
>
> Thank you.
>
>>
>>
>> Improve the patch; if it looks good then, it could be suitable for the
>> larger discussion on lkml.
>>
>
>> Lukas
>>
>> > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
>> > ---
>> >  scripts/checkpatch.pl | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> > index 4c820607540b..80dfa83ed0fb 100755
>> > --- a/scripts/checkpatch.pl
>> > +++ b/scripts/checkpatch.pl
>> > @@ -3167,7 +3167,7 @@ sub process {
>> >
>> >  # check for using SPDX license tag at beginning of files
>> >               if ($realline == $checklicenseline) {
>> > -                     if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
>> > +                     if ($rawline =~ /^[ \+]\s*\#\!\s*\// || $realfile
>> =~ /.*\.\(py\|sh\|pl\|awk\|tc\)/) {
>> >                               $checklicenseline = 2;
>> >                       } elsif ($rawline =~ /^\+/) {
>> >                               my $comment = "";
>> > --
>> > 2.25.1
>> >
>> >
>>
>

[-- Attachment #1.2: Type: text/html, Size: 6443 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: Fix SPDX license check for script files

[Lukas Bulwahn]

[-- Attachment #1: Type: text/plain, Size: 5402 bytes --]



On Sun, 26 Jul 2020, Mrinal Pandey wrote:

> 
> 
> On Sun, Jul 26, 2020 at 11:43 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> 
> 
>       On Sat, 25 Jul 2020, Mrinal Pandey wrote:
> 
>       > In all the script files(except yaml), SPDX license identifier is expected
> 
>       Why do believe a yaml file is a script file? Where did you read that?
> 
>       To me, this reads like:
> 
>       All fish(except zebras) live in water...
> 
>       I think you can just drop that note on yaml.
> 
>       > on the second line, the first line being the shebang. Sometimes, the
>       > diff content includes the SPDX licensing information but excludes the
>       > shebang when a commit is made to a script file.
>       >
> 
>       You can be more explicit here. Sometimes? You can exactly say when and how
>       that happens.
> 
>       > This can be seen in the commits like 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.
>       >
>       > The existing logic looks for a shebang in the patch/file being checked
>       > and if a shebang is encountered it directs checkpatch to expect
>       > license on the second line by setting `$checklicenseline` to `2`.
>       >
> 
>       You can drop those `quotes` around variable names and numbers.
> 
>       This can be written shorter:
>       Currently, if checkpatch find a shebang in line 1, it expects the
>       license identifier in line 2 instead of line 1.
> 
>       > However, this approach doesn't work when we don't have a shebang in the
>       > patch and `$checklicensline` continues to be `1` in this case leading to
>       > the unnecessary warning.
>       >
> 
>       However, this only works when both line 1 and 2 are part of the patch.
> 
>       > Fix this by setting `$checklicenseline` to `2` by checking if the patch
>       > comes from a script for the cases when the shebang doesn't appear in the
>       > patch.
>       >
> 
>       You are not really fixing it. You are improving the heuristics.
> 
>       So:
> 
>       Reduce the false warnings for such patches on scripts, by checking file
>       extension.
> 
>       Alternatively, you could actually also check the file permissions of the
>       file.
> 

First, please fix your email client. It is broken. If you want to work in 
the kernel community, your email client needs to work as defined in the 
kernel documentation.

Read the kernel documentation on email clients and fix your client.

> 
> There are 577 executable files in the kernel, out of which 53 don't have a shebang in the first line.
> 
> Moreover, in these 53 files, 8 should have definitely had it as they are .py or .sh files.

How about fixing those eight?

> The other files are .dts, .dtsi, .S, .h, .c files, scripts/atomic/atomics.tbl file, Documentation/Changes file

It seems wrong that those have an executable flag in the first place.

That could be fixed as well.

> and all the files
> inside scripts/atomic/fallbacks that are without extension.
> If I am correct none of these should have a shebang but I am not very sure about files inside scripts/atomic/fallbacks.
> 

Probably that whole atomics stuff needs some documentation and cleanup.

> If we check for file extension we warn wrongly for 120 files iff shebang doesn't appear in the patch but if we check for
> permissions we would warn wrongly for 53 files. I think this is better than checking for extension.
> Should we use this heuristic in the new patch I send?
>

Yes, go ahead a rework your patch.


Lukas
 
> Thank you.
> 
> 
>       Improve the patch; if it looks good then, it could be suitable for the
>       larger discussion on lkml.
> 
> 
>       Lukas
> 
>       > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
>       > ---
>       >  scripts/checkpatch.pl | 2 +-
>       >  1 file changed, 1 insertion(+), 1 deletion(-)
>       >
>       > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>       > index 4c820607540b..80dfa83ed0fb 100755
>       > --- a/scripts/checkpatch.pl
>       > +++ b/scripts/checkpatch.pl
>       > @@ -3167,7 +3167,7 @@ sub process {
>       > 
>       >  # check for using SPDX license tag at beginning of files
>       >               if ($realline == $checklicenseline) {
>       > -                     if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
>       > +                     if ($rawline =~ /^[ \+]\s*\#\!\s*\// || $realfile =~ /.*\.\(py\|sh\|pl\|awk\|tc\)/) {
>       >                               $checklicenseline = 2;
>       >                       } elsif ($rawline =~ /^\+/) {
>       >                               my $comment = "";
>       > --
>       > 2.25.1
>       >
>       >
> 
> 
> 

[-- 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: Fix SPDX license check for script files

[Mrinal Pandey]

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

On 20/07/26 01:09PM, Lukas Bulwahn wrote:
> 
> 
> On Sun, 26 Jul 2020, Mrinal Pandey wrote:
> 
> > 
> > 
> > On Sun, Jul 26, 2020 at 11:43 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> > 
> > 
> >       On Sat, 25 Jul 2020, Mrinal Pandey wrote:
> > 
> >       > In all the script files(except yaml), SPDX license identifier is expected
> > 
> >       Why do believe a yaml file is a script file? Where did you read that?
> > 
> >       To me, this reads like:
> > 
> >       All fish(except zebras) live in water...
> > 
> >       I think you can just drop that note on yaml.
> > 
> >       > on the second line, the first line being the shebang. Sometimes, the
> >       > diff content includes the SPDX licensing information but excludes the
> >       > shebang when a commit is made to a script file.
> >       >
> > 
> >       You can be more explicit here. Sometimes? You can exactly say when and how
> >       that happens.
> > 
> >       > This can be seen in the commits like 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.
> >       >
> >       > The existing logic looks for a shebang in the patch/file being checked
> >       > and if a shebang is encountered it directs checkpatch to expect
> >       > license on the second line by setting `$checklicenseline` to `2`.
> >       >
> > 
> >       You can drop those `quotes` around variable names and numbers.
> > 
> >       This can be written shorter:
> >       Currently, if checkpatch find a shebang in line 1, it expects the
> >       license identifier in line 2 instead of line 1.
> > 
> >       > However, this approach doesn't work when we don't have a shebang in the
> >       > patch and `$checklicensline` continues to be `1` in this case leading to
> >       > the unnecessary warning.
> >       >
> > 
> >       However, this only works when both line 1 and 2 are part of the patch.
> > 
> >       > Fix this by setting `$checklicenseline` to `2` by checking if the patch
> >       > comes from a script for the cases when the shebang doesn't appear in the
> >       > patch.
> >       >
> > 
> >       You are not really fixing it. You are improving the heuristics.
> > 
> >       So:
> > 
> >       Reduce the false warnings for such patches on scripts, by checking file
> >       extension.
> > 
> >       Alternatively, you could actually also check the file permissions of the
> >       file.
> > 
> 
> First, please fix your email client. It is broken. If you want to work in 
> the kernel community, your email client needs to work as defined in the 
> kernel documentation.
> 
> Read the kernel documentation on email clients and fix your client.

Sir,

I apologize if this has been causing trouble lately.
I have been using neomutt for sending patches and Gmail for conversation.
Could you please let me know once the issues you have seen with my mailing
client so that I can address them?
> 
> > 
> > There are 577 executable files in the kernel, out of which 53 don't have a shebang in the first line.
> > 
> > Moreover, in these 53 files, 8 should have definitely had it as they are .py or .sh files.
> 
> How about fixing those eight?

Okay, I would send patches to fix this.

> 
> > The other files are .dts, .dtsi, .S, .h, .c files, scripts/atomic/atomics.tbl file, Documentation/Changes file
> 
> It seems wrong that those have an executable flag in the first place.
> 
> That could be fixed as well.

Is there anything I can do to fix this?
> 
> > and all the files
> > inside scripts/atomic/fallbacks that are without extension.
> > If I am correct none of these should have a shebang but I am not very sure about files inside scripts/atomic/fallbacks.
> > 
> 
> Probably that whole atomics stuff needs some documentation and cleanup.

It would be great if we could do this.

Thank you.
> 
> > If we check for file extension we warn wrongly for 120 files iff shebang doesn't appear in the patch but if we check for
> > permissions we would warn wrongly for 53 files. I think this is better than checking for extension.
> > Should we use this heuristic in the new patch I send?
> >
> 
> Yes, go ahead a rework your patch.
> 
> 
> Lukas
>  
> > Thank you.
> > 
> > 
> >       Improve the patch; if it looks good then, it could be suitable for the
> >       larger discussion on lkml.
> > 
> > 
> >       Lukas
> > 
> >       > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
> >       > ---
> >       >  scripts/checkpatch.pl | 2 +-
> >       >  1 file changed, 1 insertion(+), 1 deletion(-)
> >       >
> >       > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >       > index 4c820607540b..80dfa83ed0fb 100755
> >       > --- a/scripts/checkpatch.pl
> >       > +++ b/scripts/checkpatch.pl
> >       > @@ -3167,7 +3167,7 @@ sub process {
> >       > 
> >       >  # check for using SPDX license tag at beginning of files
> >       >               if ($realline == $checklicenseline) {
> >       > -                     if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
> >       > +                     if ($rawline =~ /^[ \+]\s*\#\!\s*\// || $realfile =~ /.*\.\(py\|sh\|pl\|awk\|tc\)/) {
> >       >                               $checklicenseline = 2;
> >       >                       } elsif ($rawline =~ /^\+/) {
> >       >                               my $comment = "";
> >       > --
> >       > 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: Fix SPDX license check for script files

[Lukas Bulwahn]

[-- Attachment #1: Type: text/plain, Size: 6526 bytes --]



On Sun, 26 Jul 2020, Mrinal Pandey wrote:

> On 20/07/26 01:09PM, Lukas Bulwahn wrote:
> > 
> > 
> > On Sun, 26 Jul 2020, Mrinal Pandey wrote:
> > 
> > > 
> > > 
> > > On Sun, Jul 26, 2020 at 11:43 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> > > 
> > > 
> > >       On Sat, 25 Jul 2020, Mrinal Pandey wrote:
> > > 
> > >       > In all the script files(except yaml), SPDX license identifier is expected
> > > 
> > >       Why do believe a yaml file is a script file? Where did you read that?
> > > 
> > >       To me, this reads like:
> > > 
> > >       All fish(except zebras) live in water...
> > > 
> > >       I think you can just drop that note on yaml.
> > > 
> > >       > on the second line, the first line being the shebang. Sometimes, the
> > >       > diff content includes the SPDX licensing information but excludes the
> > >       > shebang when a commit is made to a script file.
> > >       >
> > > 
> > >       You can be more explicit here. Sometimes? You can exactly say when and how
> > >       that happens.
> > > 
> > >       > This can be seen in the commits like 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.
> > >       >
> > >       > The existing logic looks for a shebang in the patch/file being checked
> > >       > and if a shebang is encountered it directs checkpatch to expect
> > >       > license on the second line by setting `$checklicenseline` to `2`.
> > >       >
> > > 
> > >       You can drop those `quotes` around variable names and numbers.
> > > 
> > >       This can be written shorter:
> > >       Currently, if checkpatch find a shebang in line 1, it expects the
> > >       license identifier in line 2 instead of line 1.
> > > 
> > >       > However, this approach doesn't work when we don't have a shebang in the
> > >       > patch and `$checklicensline` continues to be `1` in this case leading to
> > >       > the unnecessary warning.
> > >       >
> > > 
> > >       However, this only works when both line 1 and 2 are part of the patch.
> > > 
> > >       > Fix this by setting `$checklicenseline` to `2` by checking if the patch
> > >       > comes from a script for the cases when the shebang doesn't appear in the
> > >       > patch.
> > >       >
> > > 
> > >       You are not really fixing it. You are improving the heuristics.
> > > 
> > >       So:
> > > 
> > >       Reduce the false warnings for such patches on scripts, by checking file
> > >       extension.
> > > 
> > >       Alternatively, you could actually also check the file permissions of the
> > >       file.
> > > 
> > 
> > First, please fix your email client. It is broken. If you want to work in 
> > the kernel community, your email client needs to work as defined in the 
> > kernel documentation.
> > 
> > Read the kernel documentation on email clients and fix your client.
> 
> Sir,
> 
> I apologize if this has been causing trouble lately.
> I have been using neomutt for sending patches and Gmail for conversation.
> Could you please let me know once the issues you have seen with my mailing
> client so that I can address them?

It seems that Gmail does not work... look at the raw emails in email 
archives.

> > 
> > > 
> > > There are 577 executable files in the kernel, out of which 53 don't have a shebang in the first line.
> > > 
> > > Moreover, in these 53 files, 8 should have definitely had it as they are .py or .sh files.
> > 
> > How about fixing those eight?
> 
> Okay, I would send patches to fix this.
> 
> > 
> > > The other files are .dts, .dtsi, .S, .h, .c files, scripts/atomic/atomics.tbl file, Documentation/Changes file
> > 
> > It seems wrong that those have an executable flag in the first place.
> > 
> > That could be fixed as well.
> 
> Is there anything I can do to fix this?

Send patches for those changes.

> > 
> > > and all the files
> > > inside scripts/atomic/fallbacks that are without extension.
> > > If I am correct none of these should have a shebang but I am not very sure about files inside scripts/atomic/fallbacks.
> > > 
> > 
> > Probably that whole atomics stuff needs some documentation and cleanup.
> 
> It would be great if we could do this.
> 
> Thank you.
> > 
> > > If we check for file extension we warn wrongly for 120 files iff shebang doesn't appear in the patch but if we check for
> > > permissions we would warn wrongly for 53 files. I think this is better than checking for extension.
> > > Should we use this heuristic in the new patch I send?
> > >
> > 
> > Yes, go ahead a rework your patch.
> > 
> > 
> > Lukas
> >  
> > > Thank you.
> > > 
> > > 
> > >       Improve the patch; if it looks good then, it could be suitable for the
> > >       larger discussion on lkml.
> > > 
> > > 
> > >       Lukas
> > > 
> > >       > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
> > >       > ---
> > >       >  scripts/checkpatch.pl | 2 +-
> > >       >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >       >
> > >       > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > >       > index 4c820607540b..80dfa83ed0fb 100755
> > >       > --- a/scripts/checkpatch.pl
> > >       > +++ b/scripts/checkpatch.pl
> > >       > @@ -3167,7 +3167,7 @@ sub process {
> > >       > 
> > >       >  # check for using SPDX license tag at beginning of files
> > >       >               if ($realline == $checklicenseline) {
> > >       > -                     if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
> > >       > +                     if ($rawline =~ /^[ \+]\s*\#\!\s*\// || $realfile =~ /.*\.\(py\|sh\|pl\|awk\|tc\)/) {
> > >       >                               $checklicenseline = 2;
> > >       >                       } elsif ($rawline =~ /^\+/) {
> > >       >                               my $comment = "";
> > >       > --
> > >       > 2.25.1
> > >       >
> > >       >
> > > 
> > > 
> > > 
> 
> 

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