[PULL 0/3] SPDX checkpatch

[PULL 0/3] SPDX checkpatch

[Daniel P. Berrangé]

The following changes since commit b69801dd6b1eb4d107f7c2f643adf0a4e3ec9124:

  Merge tag 'for_upstream' of https://git.kernel.org/pub/scm/virt/kvm/mst/qemu into staging (2025-02-22 05:06:39 +0800)

are available in the Git repository at:

  https://gitlab.com/berrange/qemu tags/spdx-check-pull-request

for you to fetch changes up to 6b7521818b26134726b3494cd06f04e30659ce2c:

  scripts: forbid use of arbitrary SPDX tags besides license identifiers (2025-02-28 15:37:36 +0000)

----------------------------------------------------------------

* Mandate use of SPDX-License-Identifier in new files
* Validate SPDX license choices
* Forbid other SPDX tags

----------------------------------------------------------------

Daniel P. Berrangé (3):
  scripts: mandate that new files have SPDX-License-Identifier
  scripts: validate SPDX license choices
  scripts: forbid use of arbitrary SPDX tags besides license identifiers

 scripts/checkpatch.pl | 111 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

-- 
2.48.1

[PULL 1/3] scripts: mandate that new files have SPDX-License-Identifier

[Daniel P. Berrangé]

Going forward we want all newly created source files to have an
SPDX-License-Identifier tag present.

Initially mandate this for C, Python, Perl, Shell source files,
as well as JSON (QAPI) and Makefiles, while encouraging users
to consider it for other file types.

Reviewed-by: Brian Cain <bcain@quicinc.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 06d07e6c22..01f25aa88d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1378,6 +1378,8 @@ sub process {
 	my $in_imported_file = 0;
 	my $in_no_imported_file = 0;
 	my $non_utf8_charset = 0;
+	my $expect_spdx = 0;
+	my $expect_spdx_file;
 
 	our @report = ();
 	our $cnt_lines = 0;
@@ -1615,6 +1617,34 @@ sub process {
 			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
 		}
 
+# All new files should have a SPDX-License-Identifier tag
+		if ($line =~ /^new file mode\s*\d+\s*$/) {
+		    if ($expect_spdx) {
+			if ($expect_spdx_file =~
+			    /\.(c|h|py|pl|sh|json|inc|Makefile)$/) {
+			    # source code files MUST have SPDX license declared
+			    ERROR("New file '$expect_spdx_file' requires " .
+				  "'SPDX-License-Identifer'");
+			} else {
+			    # Other files MAY have SPDX license if appropriate
+			    WARNING("Does new file '$expect_spdx_file' need " .
+				    "'SPDX-License-Identifer'?");
+			}
+		    }
+		    $expect_spdx = 1;
+		    $expect_spdx_file = undef;
+		} elsif ($expect_spdx) {
+		    $expect_spdx_file = $realfile unless
+			defined $expect_spdx_file;
+
+		    # SPDX tags may occurr in comments which were
+		    # stripped from '$line', so use '$rawline'
+		    if ($rawline =~ /SPDX-License-Identifier/) {
+			$expect_spdx = 0;
+			$expect_spdx_file = undef;
+		    }
+		}
+
 # Check for wrappage within a valid hunk of the file
 		if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) {
 			ERROR("patch seems to be corrupt (line wrapped?)\n" .
-- 
2.48.1

[PULL 2/3] scripts: validate SPDX license choices

[Daniel P. Berrangé]

We expect all new code to be contributed with the "GPL-2.0-or-later"
license tag. Divergence is permitted if the new file is derived from
pre-existing code under a different license, whether from elsewhere
in QEMU codebase, or outside.

Issue a warning if the declared license is not "GPL-2.0-or-later",
and an error if the license is not one of the handful of the
expected licenses to prevent unintended proliferation. The warning
asks users to explain their unusual choice of license in the commit
message.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 69 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 01f25aa88d..8995d2c391 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1353,6 +1353,70 @@ sub checkfilename {
 	}
 }
 
+sub checkspdx {
+    my ($file, $expr) = @_;
+
+    # Imported Linux headers probably have SPDX tags, but if they
+    # don't we're not requiring contributors to fix this, as these
+    # files are not expected to be modified locally in QEMU.
+    # Also don't accidentally detect own checking code.
+    if ($file =~ m,include/standard-headers, ||
+	$file =~ m,linux-headers, ||
+	$file =~ m,checkpatch.pl,) {
+	return;
+    }
+
+    my $origexpr = $expr;
+
+    # Flatten sub-expressions
+    $expr =~ s/\(|\)/ /g;
+    $expr =~ s/OR|AND/ /g;
+
+    # Merge WITH exceptions to the license
+    $expr =~ s/\s+WITH\s+/-WITH-/g;
+
+    # Cull more leading/trailing whitespace
+    $expr =~ s/^\s*//g;
+    $expr =~ s/\s*$//g;
+
+    my @bits = split / +/, $expr;
+
+    my $prefer = "GPL-2.0-or-later";
+    my @valid = qw(
+	GPL-2.0-only
+	LGPL-2.1-only
+	LGPL-2.1-or-later
+	BSD-2-Clause
+	BSD-3-Clause
+	MIT
+	);
+
+    my $nonpreferred = 0;
+    my @unknown = ();
+    foreach my $bit (@bits) {
+	if ($bit eq $prefer) {
+	    next;
+	}
+	if (grep /^$bit$/, @valid) {
+	    $nonpreferred = 1;
+	} else {
+	    push @unknown, $bit;
+	}
+    }
+    if (@unknown) {
+	ERROR("Saw unacceptable licenses '" . join(',', @unknown) .
+	      "', valid choices for QEMU are:\n" . join("\n", $prefer, @valid));
+    }
+
+    if ($nonpreferred) {
+	WARN("Saw acceptable license '$origexpr' but note '$prefer' is " .
+	     "preferred for new files unless the code is derived from a " .
+	     "source file with an existing declared license that must be " .
+	     "retained. Please explain the license choice in the commit " .
+	     "message.");
+    }
+}
+
 sub process {
 	my $filename = shift;
 
@@ -1645,6 +1709,11 @@ sub process {
 		    }
 		}
 
+# Check SPDX-License-Identifier references a permitted license
+		if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
+		    &checkspdx($realfile, $1);
+		}
+
 # Check for wrappage within a valid hunk of the file
 		if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) {
 			ERROR("patch seems to be corrupt (line wrapped?)\n" .
-- 
2.48.1

[PULL 3/3] scripts: forbid use of arbitrary SPDX tags besides license identifiers

[Daniel P. Berrangé]

While SPDX-License-Identifier is a well known SPDX tag, there are a
great many more besides that[1]. These are mostly focused on making
machine readable metadata available to the 'reuse' tool and similar.
They cover concepts like author names, copyright owners, and much
more. It is even possible to define source file line groups and apply
different SPDX tags to regions of code within a file.

At this time we're only interested in adopting SPDX for recording the
file global licensing info, so detect & reject any other SPDX metadata.
If we want to explicitly collect extra data in SPDX format, we can
evaluate each data item on its merits when someone wants to propose it
at a later date.

[1] https://spdx.github.io/spdx-spec/v2.2.2/file-tags/
    https://spdx.github.io/spdx-spec/v2.2.2/file-information/

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 8995d2c391..83b59fb443 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1714,6 +1714,18 @@ sub process {
 		    &checkspdx($realfile, $1);
 		}
 
+		if ($rawline =~ m,(SPDX-[a-zA-Z0-9-_]+):,) {
+		    my $tag = $1;
+		    my @permitted = qw(
+			SPDX-License-Identifier
+		    );
+
+		    unless (grep { /^$tag$/ } @permitted) {
+			ERROR("Tag $tag not permitted in QEMU code, valid " .
+			      "choices are: " . join(", ", @permitted));
+		    }
+		}
+
 # Check for wrappage within a valid hunk of the file
 		if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) {
 			ERROR("patch seems to be corrupt (line wrapped?)\n" .
-- 
2.48.1

Re: [PULL 0/3] SPDX checkpatch

[Stefan Hajnoczi]

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

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/10.0 for any user-visible changes.

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

Re: [PULL 1/3] scripts: mandate that new files have SPDX-License-Identifier

[Peter Maydell]

On Fri, 28 Feb 2025 at 15:45, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> Going forward we want all newly created source files to have an
> SPDX-License-Identifier tag present.
>
> Initially mandate this for C, Python, Perl, Shell source files,
> as well as JSON (QAPI) and Makefiles, while encouraging users
> to consider it for other file types.

I noticed recently that this check doesn't actually catch
all cases of new files without an SPDX tag. It looks to
me like the logic is wrong, unless I'm misreading it:



> +# All new files should have a SPDX-License-Identifier tag
> +               if ($line =~ /^new file mode\s*\d+\s*$/) {
> +                   if ($expect_spdx) {
> +                       if ($expect_spdx_file =~
> +                           /\.(c|h|py|pl|sh|json|inc|Makefile)$/) {
> +                           # source code files MUST have SPDX license declared
> +                           ERROR("New file '$expect_spdx_file' requires " .
> +                                 "'SPDX-License-Identifer'");
> +                       } else {
> +                           # Other files MAY have SPDX license if appropriate
> +                           WARNING("Does new file '$expect_spdx_file' need " .
> +                                   "'SPDX-License-Identifer'?");
> +                       }
> +                   }
> +                   $expect_spdx = 1;
> +                   $expect_spdx_file = undef;
> +               } elsif ($expect_spdx) {
> +                   $expect_spdx_file = $realfile unless
> +                       defined $expect_spdx_file;
> +
> +                   # SPDX tags may occurr in comments which were
> +                   # stripped from '$line', so use '$rawline'
> +                   if ($rawline =~ /SPDX-License-Identifier/) {
> +                       $expect_spdx = 0;
> +                       $expect_spdx_file = undef;
> +                   }
> +               }
> +

The logic goes:
 * when we see a "new file" line, we set expect_spdx to 1
 * when we see an SPDX tag we set expect_spdx to 0
 * at a later point, if we still have expect_spdx 1 we complain

The problem is that the "later point" here is "we saw another
'new file' line", not "we got to the end of all the parts of
the patch that touch this file". So if the patch adds two
new files then we'll warn for the first one (when we see the
"new file" line for the secand new file), but we won't warn
for a patch which adds only one new file (there's never a second
"new file" line) or for the last new file added in a patch
that adds multiple files.

I'm not sure where the "complain if expect_spdx is 1" check
should go, but I don't think it should be here...

thanks
-- PMM

Re: [PULL 1/3] scripts: mandate that new files have SPDX-License-Identifier

[Cédric Le Goater]

On 5/8/25 12:42, Peter Maydell wrote:
> On Fri, 28 Feb 2025 at 15:45, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>
>> Going forward we want all newly created source files to have an
>> SPDX-License-Identifier tag present.
>>
>> Initially mandate this for C, Python, Perl, Shell source files,
>> as well as JSON (QAPI) and Makefiles, while encouraging users
>> to consider it for other file types.
> 
> I noticed recently that this check doesn't actually catch
> all cases of new files without an SPDX tag. It looks to
> me like the logic is wrong, unless I'm misreading it:
> 
> 
> 
>> +# All new files should have a SPDX-License-Identifier tag
>> +               if ($line =~ /^new file mode\s*\d+\s*$/) {
>> +                   if ($expect_spdx) {
>> +                       if ($expect_spdx_file =~
>> +                           /\.(c|h|py|pl|sh|json|inc|Makefile)$/) {
>> +                           # source code files MUST have SPDX license declared
>> +                           ERROR("New file '$expect_spdx_file' requires " .
>> +                                 "'SPDX-License-Identifer'");
>> +                       } else {
>> +                           # Other files MAY have SPDX license if appropriate
>> +                           WARNING("Does new file '$expect_spdx_file' need " .
>> +                                   "'SPDX-License-Identifer'?");
>> +                       }
>> +                   }
>> +                   $expect_spdx = 1;
>> +                   $expect_spdx_file = undef;
>> +               } elsif ($expect_spdx) {
>> +                   $expect_spdx_file = $realfile unless
>> +                       defined $expect_spdx_file;
>> +
>> +                   # SPDX tags may occurr in comments which were
>> +                   # stripped from '$line', so use '$rawline'
>> +                   if ($rawline =~ /SPDX-License-Identifier/) {
>> +                       $expect_spdx = 0;
>> +                       $expect_spdx_file = undef;
>> +                   }
>> +               }
>> +
> 
> The logic goes:
>   * when we see a "new file" line, we set expect_spdx to 1
>   * when we see an SPDX tag we set expect_spdx to 0
>   * at a later point, if we still have expect_spdx 1 we complain
> 
> The problem is that the "later point" here is "we saw another
> 'new file' line", not "we got to the end of all the parts of
> the patch that touch this file". So if the patch adds two
> new files then we'll warn for the first one (when we see the
> "new file" line for the secand new file), but we won't warn
> for a patch which adds only one new file (there's never a second
> "new file" line) or for the last new file added in a patch
> that adds multiple files.
> 
> I'm not sure where the "complain if expect_spdx is 1" check
> should go, but I don't think it should be here...

yes. I just made the same observation on this patch :

    https://lore.kernel.org/qemu-devel/20250509163645.33050-7-rreyes@linux.ibm.com/

Thanks,

C.

Re: [PULL 1/3] scripts: mandate that new files have SPDX-License-Identifier

[Cédric Le Goater]

On 5/12/25 15:52, Cédric Le Goater wrote:
> On 5/8/25 12:42, Peter Maydell wrote:
>> On Fri, 28 Feb 2025 at 15:45, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>
>>> Going forward we want all newly created source files to have an
>>> SPDX-License-Identifier tag present.
>>>
>>> Initially mandate this for C, Python, Perl, Shell source files,
>>> as well as JSON (QAPI) and Makefiles, while encouraging users
>>> to consider it for other file types.
>>
>> I noticed recently that this check doesn't actually catch
>> all cases of new files without an SPDX tag. It looks to
>> me like the logic is wrong, unless I'm misreading it:
>>
>>
>>
>>> +# All new files should have a SPDX-License-Identifier tag
>>> +               if ($line =~ /^new file mode\s*\d+\s*$/) {
>>> +                   if ($expect_spdx) {
>>> +                       if ($expect_spdx_file =~
>>> +                           /\.(c|h|py|pl|sh|json|inc|Makefile)$/) {
>>> +                           # source code files MUST have SPDX license declared
>>> +                           ERROR("New file '$expect_spdx_file' requires " .
>>> +                                 "'SPDX-License-Identifer'");
>>> +                       } else {
>>> +                           # Other files MAY have SPDX license if appropriate
>>> +                           WARNING("Does new file '$expect_spdx_file' need " .
>>> +                                   "'SPDX-License-Identifer'?");
>>> +                       }
>>> +                   }
>>> +                   $expect_spdx = 1;
>>> +                   $expect_spdx_file = undef;
>>> +               } elsif ($expect_spdx) {
>>> +                   $expect_spdx_file = $realfile unless
>>> +                       defined $expect_spdx_file;
>>> +
>>> +                   # SPDX tags may occurr in comments which were
>>> +                   # stripped from '$line', so use '$rawline'
>>> +                   if ($rawline =~ /SPDX-License-Identifier/) {
>>> +                       $expect_spdx = 0;
>>> +                       $expect_spdx_file = undef;
>>> +                   }
>>> +               }
>>> +
>>
>> The logic goes:
>>   * when we see a "new file" line, we set expect_spdx to 1
>>   * when we see an SPDX tag we set expect_spdx to 0
>>   * at a later point, if we still have expect_spdx 1 we complain
>>
>> The problem is that the "later point" here is "we saw another
>> 'new file' line", not "we got to the end of all the parts of
>> the patch that touch this file". So if the patch adds two
>> new files then we'll warn for the first one (when we see the
>> "new file" line for the secand new file), but we won't warn
>> for a patch which adds only one new file (there's never a second
>> "new file" line) or for the last new file added in a patch
>> that adds multiple files.
>>
>> I'm not sure where the "complain if expect_spdx is 1" check
>> should go, but I don't think it should be here...
> 
> yes. I just made the same observation on this patch :
> 
>     https://lore.kernel.org/qemu-devel/20250509163645.33050-7-rreyes@linux.ibm.com/
> 

This seems to work well enough.

    # All new files should have a SPDX-License-Identifier tag
		if ($line =~ /^new file mode\s*\d+\s*$/) {
		    $expect_spdx = 1;
		    $expect_spdx_file = undef;
		} elsif ($expect_spdx) {
		    $expect_spdx_file = $realfile unless
			defined $expect_spdx_file;

		    # SPDX tags may occurr in comments which were
		    # stripped from '$line', so use '$rawline'
		    if ($rawline =~ /SPDX-License-Identifier/) {
			$expect_spdx = 0;
			$expect_spdx_file = undef;
		    }

		    if ($expect_spdx_file ne $realfile) {        <-- checks for a filename change
			if ($expect_spdx_file =~
			    /\.(c|h|py|pl|sh|json|inc|Makefile)$/) {
			    # source code files MUST have SPDX license declared
			    ERROR("New file '$expect_spdx_file' requires " .
				  "'SPDX-License-Identifier'");
			} else {
			    # Other files MAY have SPDX license if appropriate
			    WARN("Does new file '$expect_spdx_file' need " .
				 "'SPDX-License-Identifier'?");
			}
			$expect_spdx = 0;
			$expect_spdx_file = undef;			
		    }
		}


Thanks,

C.

Re: [PULL 1/3] scripts: mandate that new files have SPDX-License-Identifier

[Peter Maydell]

On Mon, 12 May 2025 at 16:18, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 5/12/25 15:52, Cédric Le Goater wrote:
> > On 5/8/25 12:42, Peter Maydell wrote:
> >> The logic goes:
> >>   * when we see a "new file" line, we set expect_spdx to 1
> >>   * when we see an SPDX tag we set expect_spdx to 0
> >>   * at a later point, if we still have expect_spdx 1 we complain
> >>
> >> The problem is that the "later point" here is "we saw another
> >> 'new file' line", not "we got to the end of all the parts of
> >> the patch that touch this file". So if the patch adds two
> >> new files then we'll warn for the first one (when we see the
> >> "new file" line for the secand new file), but we won't warn
> >> for a patch which adds only one new file (there's never a second
> >> "new file" line) or for the last new file added in a patch
> >> that adds multiple files.
> >>
> >> I'm not sure where the "complain if expect_spdx is 1" check
> >> should go, but I don't think it should be here...
> >
> > yes. I just made the same observation on this patch :
> >
> >     https://lore.kernel.org/qemu-devel/20250509163645.33050-7-rreyes@linux.ibm.com/
> >
>
> This seems to work well enough.

Dan sent out a patchset that overhauls the spdx detection
logic last week ("
scripts/checkpatch: fix
SPDX-License-Identifier
detection"):
https://www.mail-archive.com/qemu-devel@nongnu.org/msg1113638.html

(Unfortunately lore.kernel.org seems to be missing it,
and patchew was unsubscribed to qemu-devel emails over the
weekend so it doesn't have it either.)

thanks
-- PMM

Re: [PULL 1/3] scripts: mandate that new files have SPDX-License-Identifier

[Cédric Le Goater]

On 5/12/25 17:28, Peter Maydell wrote:
> On Mon, 12 May 2025 at 16:18, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 5/12/25 15:52, Cédric Le Goater wrote:
>>> On 5/8/25 12:42, Peter Maydell wrote:
>>>> The logic goes:
>>>>    * when we see a "new file" line, we set expect_spdx to 1
>>>>    * when we see an SPDX tag we set expect_spdx to 0
>>>>    * at a later point, if we still have expect_spdx 1 we complain
>>>>
>>>> The problem is that the "later point" here is "we saw another
>>>> 'new file' line", not "we got to the end of all the parts of
>>>> the patch that touch this file". So if the patch adds two
>>>> new files then we'll warn for the first one (when we see the
>>>> "new file" line for the secand new file), but we won't warn
>>>> for a patch which adds only one new file (there's never a second
>>>> "new file" line) or for the last new file added in a patch
>>>> that adds multiple files.
>>>>
>>>> I'm not sure where the "complain if expect_spdx is 1" check
>>>> should go, but I don't think it should be here...
>>>
>>> yes. I just made the same observation on this patch :
>>>
>>>      https://lore.kernel.org/qemu-devel/20250509163645.33050-7-rreyes@linux.ibm.com/
>>>
>>
>> This seems to work well enough.
> 
> Dan sent out a patchset that overhauls the spdx detection
> logic last week ("
> scripts/checkpatch: fix
> SPDX-License-Identifier
> detection"):
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg1113638.html

Oh ! good, I will wait for it.

Thanks,

C.