Re: [RFC PATCH v2] checkpatch: rewrite Kconfig parsing
From: Joe Perches
Date: Tue Dec 08 2020 - 15:19:09 EST
On Tue, 2020-12-08 at 18:18 +0100, Nicolai Fischer wrote:
> Checkpatch currently only warns if the help text is too short.
> To determine this the diff gets parsed for keywords starting
> a new entry, but several kinds of false positives can occur with
> the current implementation, especially when the config
> is not well formatted.
>
> This patch makes the parsing more robust and includes
> new warnings if:
> 1) the help attribute is not specified last
> 2) there is no blank line or endif before the next keyword
> 3) the help text is not indented 2 spaces more than
> the attribute itself.
>
> Signed-off-by: Nicolai Fischer <nicolai.fischer@xxxxxx>
> Co-developed-by: Johannes Czekay <johannes.czekay@xxxxxx>
> Signed-off-by: Johannes Czekay <johannes.czekay@xxxxxx>
> ---
>
> This patch rewrites most of the Kconfig parsing to address
> the issues mentioned in the first RFC:
>
> 1) search for 'help' instead of '---help---'
> > I believe all the '---help---' lines have been converted to just 'help'
> > so the '(?:---)?' bits here could be removed
>
> 2) create new warnings:
> > Perhaps it'd be better to create a new warning when the help text
> > block is not the last block of the config section. Maybe warn when
> > a blank line or endif is not the separator to the next keyword.
> > Maybe warn when the next line after help is not indented 2 more
> > spaces than the help line.
>
> 3) fix handling of blank lines and rely on keywords for end of help text
> > This doesn't allow blank lines for multi-paragraph help text either.
> >
> > I think keyword parsing is necessary and some false positives are
> > inevitable as the parsing logic in a line-by-line analyzer will
> > always be incomplete.
>
>
> It has occurred to us, that kconfig-language.rst does not explicitly
> specify that the help text should be the last attribute, although
> this is the defacto convention. Now that checkpatch actively checks
> for this, we should probably update the documentation accordingly.
Generally process is either to update documentation along with
with a checkpatch change or to update documentation first.
Also checkpatch isn't necessarily the best tool for this.
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> -# check for Kconfig help text having a real description
> +# Check if Kconfig is well formatted. Warn if help text:
> +# 1) is shorter than $min_conf_desc_length lines
> +# 2) is not specified last
> +# 3) and next keyword are not separated by a blank line or endif
> +# 4) is not indented correctly
> # 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/ &&
[]
> + my $l = $line;
> + $l =~ s/^$help_indent//;
> + if ($l =~ /^(?:bool|tristate|string|hex|int|prompt|default|
> + depends\ on|select|imply|visible\ if|range|option)\b/x) {
I think this is overly fragile.
These keywords are not required to be at the same indent as help.
Also as specified in scripts/kconfig/lexer.h, the kconfig specification
has more keywords than the list above.
In general, checkpatch does not have to be the tool of choice for
verifying everything.
For instance, checkpatch has a trivial check for MAINTAINERS entry
ordering, but there is a complete tool called parse-maintainers.pl
that verifies alphabetic section ordering.
I think most of what you seem to be attempting should be in a new
tool that completely understands Kconfig parsing.
I suggest instead of updating checkpatch, the scripts/kconfig/
content be updated to do these things.