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(-)
--
2.14.1
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.
Signed-off-by: Ulf Magnusson <[email protected]>
---
scripts/checkpatch.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3d4040322ae1..2b404317daea 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -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;
--
2.14.1
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.
Signed-off-by: Ulf Magnusson <[email protected]>
---
scripts/checkpatch.pl | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2b404317daea..54b782fab4fd 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -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;
}
--
2.14.1
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.
Signed-off-by: Ulf Magnusson <[email protected]>
---
scripts/checkpatch.pl | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 54b782fab4fd..2784f6ab309f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -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;
}
--
2.14.1
On Fri, 2018-02-16 at 21:22 +0100, Ulf Magnusson wrote:
> Hello,
>
> This patchset contains some improvements for the Kconfig help text check in
> scripts/checkconfig.pl:
Seems sensible enough to me.
Signed-off-by: Joe Perches <[email protected]>
> - 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. :)
Everyone is. Seems fine.
> 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(-)
>
On Fri, Feb 16, 2018 at 10:14 PM, Joe Perches <[email protected]> wrote:
> On Fri, 2018-02-16 at 21:22 +0100, Ulf Magnusson wrote:
>> Hello,
>>
>> This patchset contains some improvements for the Kconfig help text check in
>> scripts/checkconfig.pl:
>
> Seems sensible enough to me.
> Signed-off-by: Joe Perches <[email protected]>
Will you be taking this in yourself?
(Adding Masahiro on CC -- forgot when I sent the patchset.)
Cheers,
Ulf
2018-02-23 10:30 GMT+09:00 Ulf Magnusson <[email protected]>:
> On Fri, Feb 16, 2018 at 10:14 PM, Joe Perches <[email protected]> wrote:
>> On Fri, 2018-02-16 at 21:22 +0100, Ulf Magnusson wrote:
>>> Hello,
>>>
>>> This patchset contains some improvements for the Kconfig help text check in
>>> scripts/checkconfig.pl:
>>
>> Seems sensible enough to me.
>> Signed-off-by: Joe Perches <[email protected]>
>
> Will you be taking this in yourself?
>
> (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!
--
Best Regards
Masahiro Yamada
On Sat, Feb 24, 2018 at 2:53 PM, Masahiro Yamada
<[email protected]> wrote:
> 2018-02-23 10:30 GMT+09:00 Ulf Magnusson <[email protected]>:
>> On Fri, Feb 16, 2018 at 10:14 PM, Joe Perches <[email protected]> wrote:
>>> On Fri, 2018-02-16 at 21:22 +0100, Ulf Magnusson wrote:
>>>> Hello,
>>>>
>>>> This patchset contains some improvements for the Kconfig help text check in
>>>> scripts/checkconfig.pl:
>>>
>>> Seems sensible enough to me.
>>> Signed-off-by: Joe Perches <[email protected]>
>>
>> Will you be taking this in yourself?
>>
>> (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!
>
>
>
>
> --
> Best Regards
> Masahiro Yamada
*Bump*
(+To: Andrew)
2018-03-06 13:52 GMT+09:00 Ulf Magnusson <[email protected]>:
> On Sat, Feb 24, 2018 at 2:53 PM, Masahiro Yamada
> <[email protected]> wrote:
>> 2018-02-23 10:30 GMT+09:00 Ulf Magnusson <[email protected]>:
>>> On Fri, Feb 16, 2018 at 10:14 PM, Joe Perches <[email protected]> wrote:
>>>> On Fri, 2018-02-16 at 21:22 +0100, Ulf Magnusson wrote:
>>>>> Hello,
>>>>>
>>>>> This patchset contains some improvements for the Kconfig help text check in
>>>>> scripts/checkconfig.pl:
>>>>
>>>> Seems sensible enough to me.
>>>> Signed-off-by: Joe Perches <[email protected]>
>>>
>>> Will you be taking this in yourself?
>>>
>>> (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!
>>
>>
>>
>>
>> --
>> 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]>
--
Best Regards
Masahiro Yamada
2018-03-06 14:13 GMT+09:00 Masahiro Yamada <[email protected]>:
> (+To: Andrew)
>
> 2018-03-06 13:52 GMT+09:00 Ulf Magnusson <[email protected]>:
>> On Sat, Feb 24, 2018 at 2:53 PM, Masahiro Yamada
>> <[email protected]> wrote:
>>> 2018-02-23 10:30 GMT+09:00 Ulf Magnusson <[email protected]>:
>>>> On Fri, Feb 16, 2018 at 10:14 PM, Joe Perches <[email protected]> wrote:
>>>>> On Fri, 2018-02-16 at 21:22 +0100, Ulf Magnusson wrote:
>>>>>> Hello,
>>>>>>
>>>>>> This patchset contains some improvements for the Kconfig help text check in
>>>>>> scripts/checkconfig.pl:
>>>>>
>>>>> Seems sensible enough to me.
>>>>> Signed-off-by: Joe Perches <[email protected]>
>>>>
>>>> Will you be taking this in yourself?
>>>>
>>>> (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!
>>>
>>>
>>>
>>>
>>> --
>>> 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]>
>
>
>
>
> --
> 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.
--
Best Regards
Masahiro Yamada
2018-02-17 5:22 GMT+09:00 Ulf Magnusson <[email protected]>:
> 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.
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.
--
Best Regards
Masahiro Yamada
2018-02-17 5:22 GMT+09:00 Ulf Magnusson <[email protected]>:
> 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.
>
> Signed-off-by: Ulf Magnusson <[email protected]>
> ---
> scripts/checkpatch.pl | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 54b782fab4fd..2784f6ab309f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -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;
> }
>
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.
--
Best Regards
Masahiro Yamada