Re: [PATCH] checkpatch.pl: Add SPDX license tag check
On Thu, Feb 1, 2018 at 10:14 PM, Rob Herring wrote: > Add SPDX license tag check based on the rules defined in > Documentation/process/license-rules.rst. To summarize, SPDX license tags > should be on the 1st line (or 2nd line in scripts) using the appropriate > comment style for the file type. > > Cc: Andy Whitcroft > Cc: Joe Perches > Cc: Greg Kroah-Hartman > Cc: Thomas Gleixner > Cc: Philippe Ombredanne > Cc: Andrew Morton > Signed-off-by: Rob Herring Acked-by: Philippe Ombredanne (Sorry for the late reply but I was busy with FOSDEM)
Re: [PATCH] checkpatch.pl: Add SPDX license tag check
On Thu, Feb 01, 2018 at 03:14:29PM -0600, Rob Herring wrote: > Add SPDX license tag check based on the rules defined in > Documentation/process/license-rules.rst. To summarize, SPDX license tags > should be on the 1st line (or 2nd line in scripts) using the appropriate > comment style for the file type. > > Cc: Andy Whitcroft > Cc: Joe Perches > Cc: Greg Kroah-Hartman > Cc: Thomas Gleixner > Cc: Philippe Ombredanne > Cc: Andrew Morton > Signed-off-by: Rob Herring Acked-by: Greg Kroah-Hartman
Re: [PATCH] checkpatch.pl: Add SPDX license tag check
On Thu, 2018-02-01 at 15:14 -0600, Rob Herring wrote: > Add SPDX license tag check based on the rules defined in > Documentation/process/license-rules.rst. To summarize, SPDX license tags > should be on the 1st line (or 2nd line in scripts) using the appropriate > comment style for the file type. > > Cc: Andy Whitcroft > Cc: Joe Perches > Cc: Greg Kroah-Hartman > Cc: Thomas Gleixner > Cc: Philippe Ombredanne > Cc: Andrew Morton > Signed-off-by: Rob Herring > --- > I didn't get around to resending once license-rules.rst landed in -next. > Hopefully, this can be picked up for 4.16 so folks can start using it. > SPDX tags have already become a frequent review comment. Seems sensible enough now. Here are some other suggestions. > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > @@ -2866,6 +2869,30 @@ sub process { > } > } > > +# check for using SPDX license tag at beginning of files > + if ($realline == $checklicenseline) { > + if ($realfile =~ /\.(?:sh|pl|py)/ && $rawline =~ /\[ > \+]\s*\!\#/) { There are many files with a #! shebang that do not use these filename types. $ git grep -P --name-only '^\s*\#\!\s*/(?:bin|usr)' | \ grep -vP "(?:txt|rst|py|sh|pl)$" | wc -l 158 i.e.: .tc and .awk files and ~100 files without extensions So I would add awk and tc to the $realfile test and perhaps extend this check to test if the file is not binary and executable. > + $checklicenseline = 2; > + } elsif ($rawline =~ /^\+/) { > + my $comment = ""; > + if ($realfile =~ /\.(h|s|S)$/) { > + $comment = '/*'; > + } elsif ($realfile =~ /\.(c|dts|dtsi)$/) { > + $comment = '//'; > + } elsif ($realfile =~ /\.(sh|pl|py)$/) { > + $comment = '#'; > + } elsif ($realfile =~ /\.rst$/) { > + $comment = '..'; > + } > + > + if ($comment !~ /^$/ && > + $rawline !~ /^\+\Q$comment\E > SPDX-License-Identifier: /) { > + WARN("SPDX_LICENSE_TAG", > + "Missing or malformed > SPDX-License-Identifier tag in 1st (or 2nd for scripts) line\n" . $herecurr); Perhaps 'Missing ... in line $checklicense\n"
Re: [PATCH] checkpatch.pl: Add SPDX license tag check
On Thu, 2017-11-09 at 12:12 -0600, Rob Herring wrote: > On Thu, Nov 9, 2017 at 9:39 AM, Joe Perches wrote: > > On Thu, 2017-11-09 at 07:47 -0600, Rob Herring wrote: > > > On Wed, Nov 8, 2017 at 8:10 PM, Joe Perches wrote: > > > > On Wed, 2017-11-08 at 19:10 -0600, Rob Herring wrote: > > > > > Add a check warning if SPDX-License-Identifier tags are not used in > > > > > newly added files. > > > > > > > > If this is to be done, and I think it's not a great idea, > > > > > > Which part? SPDX tags or checking new files or just using checkpatch for > > > this? > > > > SPDX tags in all files. Is having an SPDX tag in every file really desired? > > > > There's no real way to check a patch for this. > > > > You have to check the entire file. > > Changing existing files is a separate problem. There is a script for > that (though the data file is not public). I'm only worried with new > files here because that's what I review and have to tell folks to > replace their 2 pages of license text with SPDX tags. (It will be much > easier to just tell them to run checkpatch. ;) ). > > > checkpatch could, as you've done, scan for new files > > against /dev/null, but a single patch can add > > multiple files and each newly added file should have > > a missing SPDX indicator check. > > I was going with the easy route of just giving one warning per patch. > I'd hope that's enough info for folks to figure out what's needed from > there. However, it should be possible to make it per file. The main > complication is we need to look for either '^+++' or the end of the > patch which I didn't see an easy/clean way to do. EOF is easy. There already is a $realfile test for start of file. > > My concern is that there are ~50,000 files in the > > kernel source tree and, after that scripted patch > > adding the tags, only about a quarter of them have > > an SPDX tag. > > > > So which files actually _need_ a SPDX tag? > > > > files in -next with an SPDX tag: > > > > $ git grep --name-only -i -P "spdx-licen[cs]e-identifier" | \ > > while read file ; do basename $file ; done | \ > > sed -r -e 's/^.*(\..*)/\1/' | \ > > sort | uniq -c | sort -rn | head -10 > >7514 .h > >3435 .c > >1193 Makefile > > 486 .S > > 221 .dts > > 186 Kconfig > > 185 .dtsi > > 97 .sh > > 34 .tc > > 24 .debug > > > > vs all files in -next (not Documentation/) > > > > $ git ls-files | grep -v "^Documentation/" | \ > > while read file ; do basename $file ; done | \ > > sed -r -e 's/^.*(\..*)/\1/' | \ > > sort | uniq -c | sort -rn | head -10 > > 25946 .c > > 20360 .h > >2437 Makefile > >1454 .S > >1442 .dts > >1380 Kconfig > >1099 .dtsi > > 207 .json > > 204 .gitignore > > 194 .sh > >
Re: [PATCH] checkpatch.pl: Add SPDX license tag check
On Thu, Nov 9, 2017 at 9:39 AM, Joe Perches wrote: > On Thu, 2017-11-09 at 07:47 -0600, Rob Herring wrote: >> On Wed, Nov 8, 2017 at 8:10 PM, Joe Perches wrote: >> > On Wed, 2017-11-08 at 19:10 -0600, Rob Herring wrote: >> > > Add a check warning if SPDX-License-Identifier tags are not used in >> > > newly added files. >> > >> > If this is to be done, and I think it's not a great idea, >> >> Which part? SPDX tags or checking new files or just using checkpatch for >> this? > > SPDX tags in all files. > > There's no real way to check a patch for this. > > You have to check the entire file. Changing existing files is a separate problem. There is a script for that (though the data file is not public). I'm only worried with new files here because that's what I review and have to tell folks to replace their 2 pages of license text with SPDX tags. (It will be much easier to just tell them to run checkpatch. ;) ). > checkpatch could, as you've done, scan for new files > against /dev/null, but a single patch can add > multiple files and each newly added file should have > a missing SPDX indicator check. I was going with the easy route of just giving one warning per patch. I'd hope that's enough info for folks to figure out what's needed from there. However, it should be possible to make it per file. The main complication is we need to look for either '^+++' or the end of the patch which I didn't see an easy/clean way to do. > My concern is that there are ~50,000 files in the > kernel source tree and, after that scripted patch > adding the tags, only about a quarter of them have > an SPDX tag. > > So which files actually _need_ a SPDX tag? > > files in -next with an SPDX tag: > > $ git grep --name-only -i -P "spdx-licen[cs]e-identifier" | \ > while read file ; do basename $file ; done | \ > sed -r -e 's/^.*(\..*)/\1/' | \ > sort | uniq -c | sort -rn | head -10 >7514 .h >3435 .c >1193 Makefile > 486 .S > 221 .dts > 186 Kconfig > 185 .dtsi > 97 .sh > 34 .tc > 24 .debug > > vs all files in -next (not Documentation/) > > $ git ls-files | grep -v "^Documentation/" | \ > while read file ; do basename $file ; done | \ > sed -r -e 's/^.*(\..*)/\1/' | \ > sort | uniq -c | sort -rn | head -10 > 25946 .c > 20360 .h >2437 Makefile >1454 .S >1442 .dts >1380 Kconfig >1099 .dtsi > 207 .json > 204 .gitignore > 194 .sh >
Re: [PATCH] checkpatch.pl: Add SPDX license tag check
On Thu, 2017-11-09 at 07:47 -0600, Rob Herring wrote: > On Wed, Nov 8, 2017 at 8:10 PM, Joe Perches wrote: > > On Wed, 2017-11-08 at 19:10 -0600, Rob Herring wrote: > > > Add a check warning if SPDX-License-Identifier tags are not used in > > > newly added files. > > > > If this is to be done, and I think it's not a great idea, > > Which part? SPDX tags or checking new files or just using checkpatch for this? SPDX tags in all files. There's no real way to check a patch for this. You have to check the entire file. checkpatch could, as you've done, scan for new files against /dev/null, but a single patch can add multiple files and each newly added file should have a missing SPDX indicator check. My concern is that there are ~50,000 files in the kernel source tree and, after that scripted patch adding the tags, only about a quarter of them have an SPDX tag. So which files actually _need_ a SPDX tag? files in -next with an SPDX tag: $ git grep --name-only -i -P "spdx-licen[cs]e-identifier" | \ while read file ; do basename $file ; done | \ sed -r -e 's/^.*(\..*)/\1/' | \ sort | uniq -c | sort -rn | head -10 7514 .h 3435 .c 1193 Makefile 486 .S 221 .dts 186 Kconfig 185 .dtsi 97 .sh 34 .tc 24 .debug vs all files in -next (not Documentation/) $ git ls-files | grep -v "^Documentation/" | \ while read file ; do basename $file ; done | \ sed -r -e 's/^.*(\..*)/\1/' | \ sort | uniq -c | sort -rn | head -10 25946 .c 20360 .h 2437 Makefile 1454 .S 1442 .dts 1380 Kconfig 1099 .dtsi 207 .json 204 .gitignore 194 .sh
Re: [PATCH] checkpatch.pl: Add SPDX license tag check
On Thu, 2017-11-09 at 10:12 +0100, Greg Kroah-Hartman wrote: > On Wed, Nov 08, 2017 at 06:10:19PM -0800, Joe Perches wrote: > > On Wed, 2017-11-08 at 19:10 -0600, Rob Herring wrote: > > > Add a check warning if SPDX-License-Identifier tags are not used in > > > newly added files. > > > > If this is to be done, and I think it's not a great idea, > > there are better ways of doing this that emit this warning > > on a per-file basis instead of a per-patch. > > Any hints as to how to do that? :) Sure, but only after it's decided if adding spdx license tag to all compilable source files is appropriate. And if that's true, then that requirement also has to be added in the Documentation directory somewhere.
Re: [PATCH] checkpatch.pl: Add SPDX license tag check
On Wed, Nov 8, 2017 at 8:10 PM, Joe Perches wrote: > On Wed, 2017-11-08 at 19:10 -0600, Rob Herring wrote: >> Add a check warning if SPDX-License-Identifier tags are not used in >> newly added files. > > If this is to be done, and I think it's not a great idea, Which part? SPDX tags or checking new files or just using checkpatch for this? > there are better ways of doing this that emit this warning > on a per-file basis instead of a per-patch. You had mentioned using something like checkincludes.pl before. The problem I see with that is few people run those tools. Lots of people run checkpatch.pl. We want this to be correct when added, not after the fact by someone else who is not the author. It fits in the workflow, because if checkpatch doesn't catch it, then I have to in reviews. I do agree though that the implementation is a bit ugly given the line by line way checkpatch works. Rob
Re: [PATCH] checkpatch.pl: Add SPDX license tag check
On Wed, Nov 08, 2017 at 06:10:19PM -0800, Joe Perches wrote: > On Wed, 2017-11-08 at 19:10 -0600, Rob Herring wrote: > > Add a check warning if SPDX-License-Identifier tags are not used in > > newly added files. > > If this is to be done, and I think it's not a great idea, > there are better ways of doing this that emit this warning > on a per-file basis instead of a per-patch. Any hints as to how to do that? :) thanks, greg k-h
Re: [PATCH] checkpatch.pl: Add SPDX license tag check
On Wed, 2017-11-08 at 19:10 -0600, Rob Herring wrote: > Add a check warning if SPDX-License-Identifier tags are not used in > newly added files. If this is to be done, and I think it's not a great idea, there are better ways of doing this that emit this warning on a per-file basis instead of a per-patch.
Re: [PATCH] checkpatch.pl: Add SPDX license tag check for dts files and headers
On Wed, Jun 07, 2017 at 05:45:03PM +0900, Masahiro Yamada wrote: > Hi Rob, > > 2017-02-24 0:56 GMT+09:00 Rob Herring : > > Add a check for using SPDX-License-Identifier tags to define the license of > > .dts{i} and DT header files rather than using free form license text. This > > check looks for GPL, BSD, or X11(really incorrectly labeled MIT license) > > license text which are the commonly used DT licenses. > > > > Signed-off-by: Rob Herring > > Cc: Andy Whitcroft > > Cc: Joe Perches > > I have a question. > > Is SPDX encouraged only for DT? > > What should we do with C source files? Depends on the maintainer and your lawyer I guess. gregkh is supportive[1]. I wouldn't recommend changing existing license text though. Rob [1] https://lkml.org/lkml/2017/2/15/968
Re: [PATCH] checkpatch.pl: Add SPDX license tag check for dts files and headers
Hi Rob, 2017-02-24 0:56 GMT+09:00 Rob Herring : > Add a check for using SPDX-License-Identifier tags to define the license of > .dts{i} and DT header files rather than using free form license text. This > check looks for GPL, BSD, or X11(really incorrectly labeled MIT license) > license text which are the commonly used DT licenses. > > Signed-off-by: Rob Herring > Cc: Andy Whitcroft > Cc: Joe Perches I have a question. Is SPDX encouraged only for DT? What should we do with C source files? -- Best Regards Masahiro Yamada
Re: [PATCH] checkpatch.pl: Add SPDX license tag check for dts files and headers
On Thu, 2017-02-23 at 12:45 -0600, Rob Herring wrote: > On Thu, Feb 23, 2017 at 11:51 AM, Joe Perches wrote: > > On Thu, 2017-02-23 at 09:56 -0600, Rob Herring wrote: > > > Add a check for using SPDX-License-Identifier tags to define the license > > > of > > > .dts{i} and DT header files rather than using free form license text. This > > > check looks for GPL, BSD, or X11(really incorrectly labeled MIT license) > > > license text which are the commonly used DT licenses. > > > > Hi Rob. > > > > > Signed-off-by: Rob Herring > > > Cc: Andy Whitcroft > > > Cc: Joe Perches > > > --- > > > scripts/checkpatch.pl | 13 + > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > > index 982c52ca6473..ce802b3146e3 100755 > > > --- a/scripts/checkpatch.pl > > > +++ b/scripts/checkpatch.pl > > > @@ -2139,6 +2139,7 @@ sub process { > > > my $commit_log_has_diff = 0; > > > my $reported_maintainer_file = 0; > > > my $non_utf8_charset = 0; > > > + my $licensefile = ''; > > > > Maybe this should be $spdx_license_file > > but what's the actual reason to check if > > multiple license bits are in a single file? > > Yes, just to get a single warning per file since the license matching > can get multiple hits. > > Really what I'd like to do is warn if an SPDX tag is not present in > any added file. Having a check for something missing doesn't really > work well with checkpatch at least in a scalable way that I saw. That isn't really possible with checkpatch as I'm sure you know that it looks at patch contexts and it isn't really meant to scan entire files. You might be better off with a separate script like the checkincludes.pl one. You might possibly integrate that script into checkpatch by looking at the "new file mode" block, remember those and see if in the patch each appropriate file has one of the spdx tags.
Re: [PATCH] checkpatch.pl: Add SPDX license tag check for dts files and headers
On Thu, Feb 23, 2017 at 11:51 AM, Joe Perches wrote: > On Thu, 2017-02-23 at 09:56 -0600, Rob Herring wrote: >> Add a check for using SPDX-License-Identifier tags to define the license of >> .dts{i} and DT header files rather than using free form license text. This >> check looks for GPL, BSD, or X11(really incorrectly labeled MIT license) >> license text which are the commonly used DT licenses. > > Hi Rob. > >> Signed-off-by: Rob Herring >> Cc: Andy Whitcroft >> Cc: Joe Perches >> --- >> scripts/checkpatch.pl | 13 + >> 1 file changed, 13 insertions(+) >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index 982c52ca6473..ce802b3146e3 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -2139,6 +2139,7 @@ sub process { >> my $commit_log_has_diff = 0; >> my $reported_maintainer_file = 0; >> my $non_utf8_charset = 0; >> + my $licensefile = ''; > > Maybe this should be $spdx_license_file > but what's the actual reason to check if > multiple license bits are in a single file? Yes, just to get a single warning per file since the license matching can get multiple hits. Really what I'd like to do is warn if an SPDX tag is not present in any added file. Having a check for something missing doesn't really work well with checkpatch at least in a scalable way that I saw. Rob
Re: [PATCH] checkpatch.pl: Add SPDX license tag check for dts files and headers
On Thu, 2017-02-23 at 09:56 -0600, Rob Herring wrote: > Add a check for using SPDX-License-Identifier tags to define the license of > .dts{i} and DT header files rather than using free form license text. This > check looks for GPL, BSD, or X11(really incorrectly labeled MIT license) > license text which are the commonly used DT licenses. Hi Rob. > Signed-off-by: Rob Herring > Cc: Andy Whitcroft > Cc: Joe Perches > --- > scripts/checkpatch.pl | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 982c52ca6473..ce802b3146e3 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -2139,6 +2139,7 @@ sub process { > my $commit_log_has_diff = 0; > my $reported_maintainer_file = 0; > my $non_utf8_charset = 0; > + my $licensefile = ''; Maybe this should be $spdx_license_file but what's the actual reason to check if multiple license bits are in a single file? > +# check for using SPDX tag instead of free form license text in dts and > binding header files > + if ($licensefile ne $realfile && > + ($realfile =~ /\.dtsi?$/ || $realfile =~ > /dt-bindings\/.*\.h$/) && It's nicer to use a non / leading char like @ so you don't have to escape the /. maybe $realfile =~ m@/dt-bindings/.*\.h$@ > + $rawline !~ /\bSPDX-License-Identifier/ && > + ($rawline =~ /^\+.*\bGeneral\s+Public\s+License/i || > + $rawline =~ > /^\+.*\bTHE\s+SOFTWARE\s+IS\s+PROVIDED\s+\"AS\s+IS\"/i || > + $rawline =~ /^\+.*\b(GPL|BSD|X11)/)) { nicer to indent these last 2 lines one more space to keep alignment to open parenthesis. > + $licensefile = $realfile; > + WARN("SPDX_LICENSE_TAG", > + "Use SPDX-License-Identifier tags instead of full > license text\n" . $herecurr); > + } > + > # check we are in a valid source file if not then ignore this hunk > next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);