Skip to contentSkip to conversation

Improve and extend checkpatch.pl Kconfig help text checks

PATCH In review Ulf Magnusson 3 participants 12 messages Feb 16, 2018 – Mar 22, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover Improve and extend checkpatch.pl Kconfig help text checks

Hello,

This patchset contains some improvements for the Kconfig help text check in scripts/checkconfig.pl:

- Commits 1 and 2 make the check more robust by checking menuconfig symbols and choices as well as symbols defined with 'config', and by making the detection of definition boundaries more reliable.

- Commit 3 adds a check for new '---help---'s being introduced. IMO, 'help' should be encouraged in new code.

All three commits can be applied independently.

The existing code is a bit weird in that it doesn't require symbols with "long" definitions (e.g., many selects) to have help texts. Fixing that is outside the scope of this patchset. I couldn't tell if it was deliberate.

I'm a Perl noob, so check for bad practices. :)

The changes were tested by running 'checkpatch.pl -f' on some large existing Kconfig files in the kernel and looking for false positives (e.g. arch/{x86,arm}/Kconfig).

This test file was also used, which contains some cases that confused the old code:

	config BAD_1
		bool "bad 1"
	
	config BAD_2
		bool 'bad 2'
	
	config BAD_3
		bool "bad 3"
		help
		  1
		  2
		  3
	
	menuconfig BAD_4
		bool "bad 4"
		help
		  1
		  2
		  3
	
	config BAD_5
		bool
		prompt "bad 5"
		help
		  1
		  2
		  3
	
	config BAD_6
		bool "bad 6"
		help
		  1
		  2
		  3
	
	if FOO
	
	config BAD_7
		bool "bad 7"
		help
		  1
		  2
		  3
	
	endif
	
	config BAD_8
		bool "bad 8"
		help
		  1
		  2
		  3
	
	source "foo"
	
	config BAD_9
		bool "bad 9"
		---help---
		  1
		  2
		  3
		  4
	
	choice
		bool "bad choice"
		help
		  1
		  2
		  3
	
	endchoice
	
	config OK_1
		bool
	
	config OK_2
		bool "ok 2"
		help
		  1
		  2
		  3
		  4
	
	config OK_3
		tristate "ok 3"
		help
		  1
		  2
		  3
		  4
	
	config OK_4
		tristate
		prompt "ok 4"
		help
		  1
		  2
		  3
		  4
	
	choice
		bool "ok choice"
		help
		  1
		  2
		  3
		  4
	
	endchoice

This now produces the following warnings:

WARNING: please write a paragraph that describes the config symbol fully #9: FILE: Kconfig.test_help_check:9:
+config BAD_1

WARNING: please write a paragraph that describes the config symbol fully #12: FILE: Kconfig.test_help_check:12:
+config BAD_2

WARNING: please write a paragraph that describes the config symbol fully #15: FILE: Kconfig.test_help_check:15:
+config BAD_3

WARNING: please write a paragraph that describes the config symbol fully #22: FILE: Kconfig.test_help_check:22:
+menuconfig BAD_4

WARNING: please write a paragraph that describes the config symbol fully #29: FILE: Kconfig.test_help_check:29:
+config BAD_5

WARNING: please write a paragraph that describes the config symbol fully #37: FILE: Kconfig.test_help_check:37:
+config BAD_6

WARNING: please write a paragraph that describes the config symbol fully #46: FILE: Kconfig.test_help_check:46:
+config BAD_7

WARNING: please write a paragraph that describes the config symbol fully #55: FILE: Kconfig.test_help_check:55:
+config BAD_8

WARNING: prefer 'help' over '---help---' for new help texts #64: FILE: Kconfig.test_help_check:64:
+config BAD_9

WARNING: please write a paragraph that describes the config symbol fully #72: FILE: Kconfig.test_help_check:72:
+choice

total: 0 errors, 10 warnings, 117 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.

Kconfig.test_help_check has style problems, please review.

NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

Cheers,
Ulf

Ulf Magnusson (3):
checkpatch: kconfig: recognize more prompts when checking help texts
checkpatch: kconfig: check help texts for menuconfig and choice
checkpatch: kconfig: prefer 'help' over '---help---'

 scripts/checkpatch.pl | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
Signature · 1 line
2.14.1
#1 lore raw

PATCH 1/3 checkpatch: kconfig: recognize more prompts when checking help texts

The check for a missing or short help text only considers symbols with a prompt, but doesn't recognize any of the following as a prompt:

bool 'foo'
tristate 'foo'
prompt "foo"
prompt 'foo'

Make the check recognize those too.

---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
scripts/checkpatch.pl +1 -1
index 3d4040322ae1..2b404317daea 100755
@@ -2812,7 +2812,7 @@ sub process {
 				next if ($f =~ /^-/);
 				last if (!$file && $f =~ /^\@\@/);
 
-				if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate)\s*\"/) {
+				if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) {
 					$is_start = 1;
 				} elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) {
 					$length = -1;
Signature · 1 line
2.14.1
#2 lore raw

PATCH 2/3 checkpatch: kconfig: check help texts for menuconfig and choice

In reply to Ulf Magnusson

Currently, only Kconfig symbols are checked for a missing or short help text, and are only checked if they are defined with the 'config' keyword.

To make the check more general, extend it to also check help texts for choices and for symbols defined with the 'menuconfig' keyword.

This increases the accuracy of the check for symbols that would already have been checked as well, since e.g. a 'menuconfig' symbol after a help text will be recognized as ending the preceding symbol/choice definition.

To increase the accuracy of the check further, also recognize 'if', 'endif', 'menu', 'endmenu', 'endchoice', and 'source' as ending a symbol/choice definition.

---
 scripts/checkpatch.pl | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
scripts/checkpatch.pl +11 -2
index 2b404317daea..54b782fab4fd 100755
@@ -2797,7 +2797,10 @@ sub process {
 # Only applies when adding the entry originally, after that we do not have
 # sufficient context to determine whether it is indeed long enough.
 		if ($realfile =~ /Kconfig/ &&
-		    $line =~ /^\+\s*config\s+/) {
+		    # 'choice' is usually the last thing on the line (though
+		    # Kconfig supports named choices), so use a word boundary
+		    # (\b) rather than a whitespace character (\s)
+		    $line =~ /^\+\s*(?:config|menuconfig|choice)\b/) {
 			my $length = 0;
 			my $cnt = $realcnt;
 			my $ln = $linenr + 1;
@@ -2822,7 +2825,13 @@ sub process {
 				$f =~ s/#.*//;
 				$f =~ s/^\s+//;
 				next if ($f =~ /^$/);
-				if ($f =~ /^\s*config\s/) {
+
+				# This only checks context lines in the patch
+				# and so hopefully shouldn't trigger false
+				# positives, even though some of these are
+				# common words in help texts
+				if ($f =~ /^\s*(?:config|menuconfig|choice|endchoice|
+						  if|endif|menu|endmenu|source)\b/x) {
 					$is_end = 1;
 					last;
 				}
Signature · 1 line
2.14.1
#3 lore raw

Re: [PATCH 2/3] checkpatch: kconfig: check help texts for menuconfig and choice

2018-02-17 5:22 GMT+09:00 Ulf Magnusson <[email protected]>:

Quoted 7 lines

Currently, only Kconfig symbols are checked for a missing or short help text, and are only checked if they are defined with the 'config' keyword.

To make the check more general, extend it to also check help texts for choices and for symbols defined with the 'menuconfig' keyword.

Quoted 8 lines

This increases the accuracy of the check for symbols that would already have been checked as well, since e.g. a 'menuconfig' symbol after a help text will be recognized as ending the preceding symbol/choice definition.

To increase the accuracy of the check further, also recognize 'if', 'endif', 'menu', 'endmenu', 'endchoice', and 'source' as ending a symbol/choice definition.

Currently, this is not checked for the last symbol in a file. Perhaps, EOF could be also an ending of symbol definition.

This patch is a good improvement enough,
so I queued it up.

If you have an idea for further improvement,
v2 is welcome, of course.

Signature · 2 lines
Best Regards
Masahiro Yamada
#4 lore raw

PATCH 3/3 checkpatch: kconfig: prefer 'help' over '---help---'

In reply to Ulf Magnusson

IMO, we should discourage '---help---' for new help texts, even in cases where it would be consistent with other help texts in the file. This will help if we ever want to get rid of '---help---' in the future.

Also simplify the code to only check for exactly '---help---'. Since commit c2264564df3d ("kconfig: warn of unhandled characters in Kconfig commands"), '---help---' is a proper keyword and can only appear in that form. Prior to that commit, '---help---' working was more of a syntactic quirk.

---
 scripts/checkpatch.pl | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
scripts/checkpatch.pl +5 -1
index 54b782fab4fd..2784f6ab309f 100755
@@ -2817,7 +2817,11 @@ sub process {
 
 				if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) {
 					$is_start = 1;
-				} elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) {
+				} elsif ($lines[$ln - 1] =~ /^\+\s*(?:help|---help---)\s*$/) {
+					if ($lines[$ln - 1] =~ "---help---") {
+						WARN("CONFIG_DESCRIPTION",
+						     "prefer 'help' over '---help---' for new help texts\n" . $herecurr);
+					}
 					$length = -1;
 				}
 
Signature · 1 line
2.14.1
#5 lore raw

Re: [PATCH 3/3] checkpatch: kconfig: prefer 'help' over '---help---'

2018-02-17 5:22 GMT+09:00 Ulf Magnusson <[email protected]>:

Quoted 22 lines

IMO, we should discourage '---help---' for new help texts, even in cases where it would be consistent with other help texts in the file. This will help if we ever want to get rid of '---help---' in the future.

Also simplify the code to only check for exactly '---help---'. Since commit c2264564df3d ("kconfig: warn of unhandled characters in Kconfig commands"), '---help---' is a proper keyword and can only appear in that form. Prior to that commit, '---help---' working was more of a syntactic quirk.

---
 scripts/checkpatch.pl | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
scripts/checkpatch.pl +0 -0
index 54b782fab4fd..2784f6ab309f 100755
@@ -2817,7 +2817,11 @@ sub process {
 
                                if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) {

scripts/checkpatch.pl sub process { · line 2827

Quoted 10 lines
scripts/checkpatch.pl +5 -1
                                        $is_start = 1;
-                               } elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) {
+                               } elsif ($lines[$ln - 1] =~ /^\+\s*(?:help|---help---)\s*$/) {
+                                       if ($lines[$ln - 1] =~ "---help---") {
+                                               WARN("CONFIG_DESCRIPTION",
+                                                    "prefer 'help' over '---help---' for new help texts\n" . $herecurr);
+                                       }
                                        $length = -1;
                                }
 

I applied this, but perhaps the line number for the offending part could be improved.

The warning looks like this.

WARNING: prefer 'help' over '---help---' for new help texts #1109: FILE: init/Kconfig:1109:
+config SGETMASK_SYSCALL

The #1109 points to the line of the symbol start. IMHO, it is even better to point to the '---help---' line.

Signature · 2 lines
Best Regards
Masahiro Yamada
maintainer
#6 lore raw
In reply to Ulf Magnusson
Quoted 4 lines · Ulf Magnusson

Hello,

This patchset contains some improvements for the Kconfig help text check in scripts/checkconfig.pl:

Seems sensible enough to me.

Quoted 7 lines

- Commits 1 and 2 make the check more robust by checking menuconfig symbols and choices as well as symbols defined with 'config', and by making the detection of definition boundaries more reliable.

- Commit 3 adds a check for new '---help---'s being introduced. IMO, 'help' should be encouraged in new code.

Quoted 7 lines

All three commits can be applied independently.

The existing code is a bit weird in that it doesn't require symbols with "long" definitions (e.g., many selects) to have help texts. Fixing that is outside the scope of this patchset. I couldn't tell if it was deliberate.

I'm a Perl noob, so check for bad practices. :)

Everyone is. Seems fine.

Quoted 182 lines

The changes were tested by running 'checkpatch.pl -f' on some large existing Kconfig files in the kernel and looking for false positives (e.g. arch/{x86,arm}/Kconfig).

This test file was also used, which contains some cases that confused the old code:

	config BAD_1
		bool "bad 1"
	
	config BAD_2
		bool 'bad 2'
	
	config BAD_3
		bool "bad 3"
		help
		  1
		  2
		  3
	
	menuconfig BAD_4
		bool "bad 4"
		help
		  1
		  2
		  3
	
	config BAD_5
		bool
		prompt "bad 5"
		help
		  1
		  2
		  3
	
	config BAD_6
		bool "bad 6"
		help
		  1
		  2
		  3
	
	if FOO
	
	config BAD_7
		bool "bad 7"
		help
		  1
		  2
		  3
	
	endif
	
	config BAD_8
		bool "bad 8"
		help
		  1
		  2
		  3
	
	source "foo"
	
	config BAD_9
		bool "bad 9"
		---help---
		  1
		  2
		  3
		  4
	
	choice
		bool "bad choice"
		help
		  1
		  2
		  3
	
	endchoice
	
	config OK_1
		bool
	
	config OK_2
		bool "ok 2"
		help
		  1
		  2
		  3
		  4
	
	config OK_3
		tristate "ok 3"
		help
		  1
		  2
		  3
		  4
	
	config OK_4
		tristate
		prompt "ok 4"
		help
		  1
		  2
		  3
		  4
	
	choice
		bool "ok choice"
		help
		  1
		  2
		  3
		  4
	
	endchoice

This now produces the following warnings:

WARNING: please write a paragraph that describes the config symbol fully #9: FILE: Kconfig.test_help_check:9:
+config BAD_1

WARNING: please write a paragraph that describes the config symbol fully #12: FILE: Kconfig.test_help_check:12:
+config BAD_2

WARNING: please write a paragraph that describes the config symbol fully #15: FILE: Kconfig.test_help_check:15:
+config BAD_3

WARNING: please write a paragraph that describes the config symbol fully #22: FILE: Kconfig.test_help_check:22:
+menuconfig BAD_4

WARNING: please write a paragraph that describes the config symbol fully #29: FILE: Kconfig.test_help_check:29:
+config BAD_5

WARNING: please write a paragraph that describes the config symbol fully #37: FILE: Kconfig.test_help_check:37:
+config BAD_6

WARNING: please write a paragraph that describes the config symbol fully #46: FILE: Kconfig.test_help_check:46:
+config BAD_7

WARNING: please write a paragraph that describes the config symbol fully #55: FILE: Kconfig.test_help_check:55:
+config BAD_8

WARNING: prefer 'help' over '---help---' for new help texts #64: FILE: Kconfig.test_help_check:64:
+config BAD_9

WARNING: please write a paragraph that describes the config symbol fully #72: FILE: Kconfig.test_help_check:72:
+choice

total: 0 errors, 10 warnings, 117 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.

Kconfig.test_help_check has style problems, please review.

NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

Cheers,
Ulf

Ulf Magnusson (3):
checkpatch: kconfig: recognize more prompts when checking help texts
checkpatch: kconfig: check help texts for menuconfig and choice
checkpatch: kconfig: prefer 'help' over '---help---'

 scripts/checkpatch.pl | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
#7 lore raw
Quoted 8 lines · Joe Perches
Quoted 4 lines · Ulf Magnusson

Hello,

This patchset contains some improvements for the Kconfig help text check in scripts/checkconfig.pl:

Seems sensible enough to me.

Will you be taking this in yourself?

(Adding Masahiro on CC -- forgot when I sent the patchset.)

Cheers,
Ulf

#8 lore raw

2018-02-23 10:30 GMT+09:00 Ulf Magnusson <[email protected]>:

Quoted 12 lines · Joe Perches
Quoted 8 lines · Ulf Magnusson
Quoted 4 lines

Hello,

This patchset contains some improvements for the Kconfig help text check in scripts/checkconfig.pl:

Seems sensible enough to me.

Will you be taking this in yourself?

Quoted 8 lines

(Adding Masahiro on CC -- forgot when I sent the patchset.)

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I am not a perl expert, but I have no objection for this series.

Thanks!

Signature · 2 lines
Best Regards
Masahiro Yamada
#9 lore raw
Quoted 26 lines · Masahiro Yamada

2018-02-23 10:30 GMT+09:00 Ulf Magnusson <[email protected]>:

Quoted 12 lines · Joe Perches
Quoted 8 lines · Ulf Magnusson
Quoted 4 lines

Hello,

This patchset contains some improvements for the Kconfig help text check in scripts/checkconfig.pl:

Seems sensible enough to me.

Will you be taking this in yourself?

Quoted 8 lines

(Adding Masahiro on CC -- forgot when I sent the patchset.)

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I am not a perl expert, but I have no objection for this series.

Quoted 8 lines

Thanks!

--
Best Regards
Masahiro Yamada

*Bump*

#10 lore raw

(+To: Andrew)

2018-03-06 13:52 GMT+09:00 Ulf Magnusson <[email protected]>:

Quoted 33 lines
Quoted 31 lines · Masahiro Yamada

2018-02-23 10:30 GMT+09:00 Ulf Magnusson <[email protected]>:

Quoted 12 lines · Joe Perches
Quoted 8 lines · Ulf Magnusson
Quoted 4 lines

Hello,

This patchset contains some improvements for the Kconfig help text check in scripts/checkconfig.pl:

Seems sensible enough to me.

Will you be taking this in yourself?

Quoted 8 lines

(Adding Masahiro on CC -- forgot when I sent the patchset.)

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I am not a perl expert, but I have no objection for this series.

Thanks!

Quoted 5 lines
Quoted 3 lines

--
Best Regards
Masahiro Yamada

*Bump*

Who is addressed by "*Bump*" ?

I think patches for checkpatch.pl
are supposed to be taken care of by Andrew.

He forwards patches to Linus.

$ git log --no-merges --pretty=fuller scripts/checkpatch.pl  | grep
'Commit:' | sort | uniq -c | sort -nr
    555 Commit:     Linus Torvalds <[email protected]>
     16 Commit:     Linus Torvalds <[email protected]>
      4 Commit:     Paul E. McKenney <[email protected]>
      4 Commit:     Michael S. Tsirkin <[email protected]>
      2 Commit:     Thomas Gleixner <[email protected]>
      2 Commit:     Ingo Molnar <[email protected]>
      2 Commit:     Greg Kroah-Hartman <[email protected]>
      1 Commit:     Tobin C. Harding <[email protected]>
      1 Commit:     Rob Herring <[email protected]>
      1 Commit:     Petr Mladek <[email protected]>
      1 Commit:     Michal Marek <[email protected]>
      1 Commit:     Mauro Carvalho Chehab <[email protected]>
      1 Commit:     Masahiro Yamada <[email protected]>
      1 Commit:     Lucas De Marchi <[email protected]>
      1 Commit:     Jiri Kosina <[email protected]>
      1 Commit:     Dan Williams <[email protected]>
      1 Commit:     Bjorn Helgaas <[email protected]>
Signature · 2 lines
Best Regards
Masahiro Yamada
#11 lore raw

2018-03-06 14:13 GMT+09:00 Masahiro Yamada <[email protected]>:

Quoted 76 lines

(+To: Andrew)

2018-03-06 13:52 GMT+09:00 Ulf Magnusson <[email protected]>:

Quoted 33 lines
Quoted 31 lines · Masahiro Yamada

2018-02-23 10:30 GMT+09:00 Ulf Magnusson <[email protected]>:

Quoted 12 lines · Joe Perches
Quoted 8 lines · Ulf Magnusson
Quoted 4 lines

Hello,

This patchset contains some improvements for the Kconfig help text check in scripts/checkconfig.pl:

Seems sensible enough to me.

Will you be taking this in yourself?

Quoted 8 lines

(Adding Masahiro on CC -- forgot when I sent the patchset.)

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I am not a perl expert, but I have no objection for this series.

Thanks!

Quoted 5 lines
Quoted 3 lines

--
Best Regards
Masahiro Yamada

*Bump*

Who is addressed by "*Bump*" ?

I think patches for checkpatch.pl
are supposed to be taken care of by Andrew.

He forwards patches to Linus.

$ git log --no-merges --pretty=fuller scripts/checkpatch.pl  | grep
'Commit:' | sort | uniq -c | sort -nr
    555 Commit:     Linus Torvalds <[email protected]>
     16 Commit:     Linus Torvalds <[email protected]>
      4 Commit:     Paul E. McKenney <[email protected]>
      4 Commit:     Michael S. Tsirkin <[email protected]>
      2 Commit:     Thomas Gleixner <[email protected]>
      2 Commit:     Ingo Molnar <[email protected]>
      2 Commit:     Greg Kroah-Hartman <[email protected]>
      1 Commit:     Tobin C. Harding <[email protected]>
      1 Commit:     Rob Herring <[email protected]>
      1 Commit:     Petr Mladek <[email protected]>
      1 Commit:     Michal Marek <[email protected]>
      1 Commit:     Mauro Carvalho Chehab <[email protected]>
      1 Commit:     Masahiro Yamada <[email protected]>
      1 Commit:     Lucas De Marchi <[email protected]>
      1 Commit:     Jiri Kosina <[email protected]>
      1 Commit:     Dan Williams <[email protected]>
      1 Commit:     Bjorn Helgaas <[email protected]>
Quoted 3 lines

--
Best Regards
Masahiro Yamada

I thought Andrew would pick it up, but
nobody is taking care of this after all.

I queued it up now.

Signature · 2 lines
Best Regards
Masahiro Yamada