[PATCH v2 0/3] scripts: mandate use of SPDX-License-Identifier tags in new files

[PATCH v2 0/3] scripts: mandate use of SPDX-License-Identifier tags in new files

[Daniel P. Berrangé]

One of the items raised at the QEMU maintainers meeting at KVM Forum
2024 was adoption of SPDX-License-Identifier for licensing of newly
contributed source files, for which there were no dissenting voices.

Thus, this series proposes a way to put this into action by extending
checkpatch.pl to mandate SPDX-License-Identifier in all new files.

Furthermore, anytime it sees SPDX-License-Identifier in any patch,
whether a new file or pre-existing, it validates the declared license
name. If it is not one of the commonly used QEMU licenses (the GPL
variants, MIT, & a few BSD variants), it will report an error. To
encourage sticking with GPL-2.0-or-later by default, it will issue
a warning even if it is one of the common licenses, encouraging
the contributor to double check their choice. This will reduce
accidental license proliferation.

Finally, I've seen a few other random SPDX tags such as:

  * SPDX-FileCopyrightText  - replacing "Copyright ..."
  * SPDX-FileContributor - replacing "Authors: ..."
  * SPDX-URL - a link to the link license text
  * SPDX-sourceInfo - arbitrary free form text about the file

These may or may not be worth considering in QEMU, but this series
discourages their usage by raising an error in checkpatch for now.

If we feel we want to adopt any of these, I think it should be
through a concious decision applied universally. Inconsistent &
adhoc usage of other SPDX tags by a subset of contributors feels
like it doesn't seem to give a clear win, and could even be a
net loss through making practices inconsistent across the code.

Changed in v2:

 * Tweaks to the commit messages
 * Expand the message warning about non GPL-2.0-or-later
   usage, to request an explanation in the commit message
   for the unusual choice.

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 | 106 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

-- 
2.46.0

[PATCH v2 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 | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 06d07e6c22..d946121b8e 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,30 @@ 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.46.0

[PATCH v2 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. Divergance 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.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 68 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d946121b8e..b507da8e2b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1353,6 +1353,69 @@ 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
+    if ($file =~ m,include/standard-headers, ||
+	$file =~ m,linux-headers,) {
+	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(
+	LGPL-2.0-or-later
+	LGPL-2.1-or-later
+	GPL-2.0-only
+	LGPL-2.0-only
+	LGPL-2.0-only
+	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 with an " .
+	     "existed declared license that must be followed. Please explain " .
+	     "license choice in the commit message");
+    }
+}
+
 sub process {
 	my $filename = shift;
 
@@ -1641,6 +1704,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.46.0

[PATCH v2 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 b507da8e2b..888b670d43 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1709,6 +1709,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.46.0

Re: [PATCH v2 2/3] scripts: validate SPDX license choices

[Peter Maydell]

On Tue, 19 Nov 2024 at 11:29, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> We expect all new code to be contributed with the "GPL-2.0-or-later"
> license tag. Divergance is permitted if the new file is derived from

"divergence"

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

Should we update LICENSE (or something under docs/devel ?) to
state our policy ?

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/checkpatch.pl | 68 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d946121b8e..b507da8e2b 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1353,6 +1353,69 @@ 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
> +    if ($file =~ m,include/standard-headers, ||
> +       $file =~ m,linux-headers,) {
> +       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(
> +       LGPL-2.0-or-later
> +       LGPL-2.1-or-later
> +       GPL-2.0-only
> +       LGPL-2.0-only
> +       LGPL-2.0-only

Lists LGPL-2.0-only twice ? I'm guessing the second should be 2.1.

I'm not sure we really want to allow more LGPL-2.0-only
code...we don't have a reason like we do with GPL-2.0-only
where the reason is "code from the kernel", and I feel like
LGPL-2.0-only is quite rare anyway, and at least sometimes
a mistake where the author meant LGPL-2.1-only or GPL-2.0-only.
But maybe this list should be generous enough to only warn,
not error, for code copied within QEMU.

AFAICT the only code we have that is LGPL-2.0-only is
util/error.c. But that also refers to our COPYING.LIB,
which is LGPL2.1. In 2011, 12 years after the publication
of LGPL2.1, did Anthony Liguori *really* mean to use
LGPL2.0 only? Answers on a postcard :-)

> +       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 with an " .
> +            "existed declared license that must be followed. Please explain " .
> +            "license choice in the commit message");
> +    }
> +}
> +
>  sub process {
>         my $filename = shift;
>
> @@ -1641,6 +1704,11 @@ sub process {
>                     }
>                 }
>
> +# Check SPDX-License-Identifier references a permitted license
> +               if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
> +                   &checkspdx($realfile, $1);
> +               }
> +

The code changes look OK to me.

thanks
-- PMM

Re: [PATCH v2 2/3] scripts: validate SPDX license choices

[Daniel P. Berrangé]

On Mon, Dec 02, 2024 at 04:41:48PM +0000, Peter Maydell wrote:
> On Tue, 19 Nov 2024 at 11:29, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > We expect all new code to be contributed with the "GPL-2.0-or-later"
> > license tag. Divergance is permitted if the new file is derived from
> 
> "divergence"
> 
> > 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.
> 
> Should we update LICENSE (or something under docs/devel ?) to
> state our policy ?

Yeah, we really ought to, i'll have a look at it.

> 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  scripts/checkpatch.pl | 68 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 68 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index d946121b8e..b507da8e2b 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1353,6 +1353,69 @@ 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
> > +    if ($file =~ m,include/standard-headers, ||
> > +       $file =~ m,linux-headers,) {
> > +       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(
> > +       LGPL-2.0-or-later
> > +       LGPL-2.1-or-later
> > +       GPL-2.0-only
> > +       LGPL-2.0-only
> > +       LGPL-2.0-only
> 
> Lists LGPL-2.0-only twice ? I'm guessing the second should be 2.1.

Opps, indeed 2.1

> I'm not sure we really want to allow more LGPL-2.0-only
> code...we don't have a reason like we do with GPL-2.0-only
> where the reason is "code from the kernel", and I feel like
> LGPL-2.0-only is quite rare anyway, and at least sometimes
> a mistake where the author meant LGPL-2.1-only or GPL-2.0-only.
> But maybe this list should be generous enough to only warn,
> not error, for code copied within QEMU.

Reliably identifying that a patch is merely "copying code within
QEMU" is a non-trivial task. I'm not sure its worth the effort,
given that we always have the option of ignoring the script's
advice if a human knows better.

> AFAICT the only code we have that is LGPL-2.0-only is
> util/error.c. But that also refers to our COPYING.LIB,
> which is LGPL2.1. In 2011, 12 years after the publication
> of LGPL2.1, did Anthony Liguori *really* mean to use
> LGPL2.0 only? Answers on a postcard :-)

I'm fine dropping LGPL2.0-or-later and LGPL2.0-only,
for the very reasons you state.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 2/3] scripts: validate SPDX license choices

[Philippe Mathieu-Daudé]

Hi Daniel, Peter,

On 2/12/24 17:54, Daniel P. Berrangé wrote:
> On Mon, Dec 02, 2024 at 04:41:48PM +0000, Peter Maydell wrote:
>> On Tue, 19 Nov 2024 at 11:29, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>
>>> We expect all new code to be contributed with the "GPL-2.0-or-later"
>>> license tag. Divergance is permitted if the new file is derived from
>>
>> "divergence"
>>
>>> 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.
>>
>> Should we update LICENSE (or something under docs/devel ?) to
>> state our policy ?
> 
> Yeah, we really ought to, i'll have a look at it.

Could we merge the previous (reviewed) patch (1/3) without having to
wait for a v3?

> 
>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>   scripts/checkpatch.pl | 68 +++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 68 insertions(+)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index d946121b8e..b507da8e2b 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -1353,6 +1353,69 @@ 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
>>> +    if ($file =~ m,include/standard-headers, ||
>>> +       $file =~ m,linux-headers,) {
>>> +       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(
>>> +       LGPL-2.0-or-later
>>> +       LGPL-2.1-or-later
>>> +       GPL-2.0-only
>>> +       LGPL-2.0-only
>>> +       LGPL-2.0-only
>>
>> Lists LGPL-2.0-only twice ? I'm guessing the second should be 2.1.
> 
> Opps, indeed 2.1
> 
>> I'm not sure we really want to allow more LGPL-2.0-only
>> code...we don't have a reason like we do with GPL-2.0-only
>> where the reason is "code from the kernel", and I feel like
>> LGPL-2.0-only is quite rare anyway, and at least sometimes
>> a mistake where the author meant LGPL-2.1-only or GPL-2.0-only.
>> But maybe this list should be generous enough to only warn,
>> not error, for code copied within QEMU.
> 
> Reliably identifying that a patch is merely "copying code within
> QEMU" is a non-trivial task. I'm not sure its worth the effort,
> given that we always have the option of ignoring the script's
> advice if a human knows better.
> 
>> AFAICT the only code we have that is LGPL-2.0-only is
>> util/error.c. But that also refers to our COPYING.LIB,
>> which is LGPL2.1. In 2011, 12 years after the publication
>> of LGPL2.1, did Anthony Liguori *really* mean to use
>> LGPL2.0 only? Answers on a postcard :-)
> 
> I'm fine dropping LGPL2.0-or-later and LGPL2.0-only,
> for the very reasons you state.
> 
> 
> With regards,
> Daniel