[PATCH 0/3] checkkconfigsymbols.py: Fix various bugs

[PATCH 0/3] checkkconfigsymbols.py: Fix various bugs

[Ariel Marcovitch]

Hi!

This series fixes some issues in the checkkconfigsymbols.py script.

The first patch fixes a bug in the --ignore option that makes the
script check only the files matching the pattern instead of ignoring
them.

The second patch fixes a parsing error in the Kconfig files parser
that makes it ignore 'if' statements after 'help' sections.

The third patch prevents the user from using 'HEAD' refs in the
 --commit option, because it doesn't really work.

Thanks!

-Ariel

Ariel Marcovitch (3):
  checkkconfigsymbols.py: Fix the '--ignore' option
  checkkconfigsymbols.py: Fix Kconfig parsing to find 'if' lines
  checkkconfigsymbols.py: Forbid passing 'HEAD' to --commit

 scripts/checkkconfigsymbols.py | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)


base-commit: 36a21d51725af2ce0700c6ebcb6b9594aac658a6
-- 
2.25.1

[PATCH 1/3] checkkconfigsymbols.py: Fix the '--ignore' option

[Ariel Marcovitch]

It seems like the implementation of the --ignore option is broken.

In check_symbols_helper, when going through the list of files, a file is
added to the list of source files to check if it matches the ignore
pattern. Instead, as stated in the comment below this condition, the
file should be added if it doesn't match the pattern.

This means that when providing an ignore pattern, the only files that
will be checked will be the ones we want the ignore, in addition to the
Kconfig files that don't match the pattern (the check in
parse_kconfig_files is done right)

Signed-off-by: Ariel Marcovitch <arielmarcovitch@gmail.com>
---
 scripts/checkkconfigsymbols.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkkconfigsymbols.py b/scripts/checkkconfigsymbols.py
index 1548f9ce4682..b9b0f15e5880 100755
--- a/scripts/checkkconfigsymbols.py
+++ b/scripts/checkkconfigsymbols.py
@@ -329,7 +329,7 @@ def check_symbols_helper(pool, ignore):
         if REGEX_FILE_KCONFIG.match(gitfile):
             kconfig_files.append(gitfile)
         else:
-            if ignore and not re.match(ignore, gitfile):
+            if ignore and re.match(ignore, gitfile):
                 continue
             # add source files that do not match the ignore pattern
             source_files.append(gitfile)
-- 
2.25.1

[PATCH 2/3] checkkconfigsymbols.py: Fix Kconfig parsing to find 'if' lines

[Ariel Marcovitch]

When parsing Kconfig files to find symbol definitions and references,
lines after a 'help' line are skipped until a new config definition
starts.

However, it is quite common to define a config and then make some other
configs depend on it by adding an 'if' line. This kind of kconfig
statement usually appears after a config definition which might contain
a 'help' section. The 'if' line is skipped in parse_kconfig_file()
because it is not a config definition.

This means that symbols referenced in this kind of statements are
ignored by this function and thus are not considered undefined
references in case the symbol is not defined.

The REGEX_KCONFIG_STMT regex can't be used because the other types of
statements can't break help lines.

Define a new regex for matching 'if' statements and stop the 'help'
skipping in case it is encountered.

Signed-off-by: Ariel Marcovitch <arielmarcovitch@gmail.com>
---
 scripts/checkkconfigsymbols.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/scripts/checkkconfigsymbols.py b/scripts/checkkconfigsymbols.py
index b9b0f15e5880..875e9a2c14b2 100755
--- a/scripts/checkkconfigsymbols.py
+++ b/scripts/checkkconfigsymbols.py
@@ -26,6 +26,7 @@ EXPR = r"(?:" + OPERATORS + r"|\s|" + SYMBOL + r")+"
 DEFAULT = r"default\s+.*?(?:if\s.+){,1}"
 STMT = r"^\s*(?:if|select|imply|depends\s+on|(?:" + DEFAULT + r"))\s+" + EXPR
 SOURCE_SYMBOL = r"(?:\W|\b)+[D]{,1}CONFIG_(" + SYMBOL + r")"
+IF_LINE = r"^\s*(?:if)\s+" + EXPR
 
 # regex objects
 REGEX_FILE_KCONFIG = re.compile(r".*Kconfig[\.\w+\-]*$")
@@ -35,11 +36,11 @@ REGEX_KCONFIG_DEF = re.compile(DEF)
 REGEX_KCONFIG_EXPR = re.compile(EXPR)
 REGEX_KCONFIG_STMT = re.compile(STMT)
 REGEX_KCONFIG_HELP = re.compile(r"^\s+help\s*$")
+REGEX_KCONFIG_IF_LINE = re.compile(IF_LINE)
 REGEX_FILTER_SYMBOLS = re.compile(r"[A-Za-z0-9]$")
 REGEX_NUMERIC = re.compile(r"0[xX][0-9a-fA-F]+|[0-9]+")
 REGEX_QUOTES = re.compile("(\"(.*?)\")")
 
-
 def parse_options():
     """The user interface of this module."""
     usage = "Run this tool to detect Kconfig symbols that are referenced but " \
@@ -445,6 +446,11 @@ def parse_kconfig_file(kfile):
         line = line.strip('\n')
         line = line.split("#")[0]  # ignore comments
 
+        # 'if EXPR' lines can be after help lines
+        # The if line itself is handled later
+        if REGEX_KCONFIG_IF_LINE.match(line):
+            skip = False
+
         if REGEX_KCONFIG_DEF.match(line):
             symbol_def = REGEX_KCONFIG_DEF.findall(line)
             defined.append(symbol_def[0])
-- 
2.25.1

[PATCH 3/3] checkkconfigsymbols.py: Forbid passing 'HEAD' to --commit

[Ariel Marcovitch]

As opposed to the --diff option, --commit can get ref names instead of
commit hashes.

When using the --commit option, the script resets the working directory
to the commit before the given ref, by adding '~' to the end of the ref.

However, the 'HEAD' ref is relative, and so when the working directory
is reset to 'HEAD~', 'HEAD' points to what was 'HEAD~'. Then when the
script resets to 'HEAD' it actually stays in the same commit. In this
case, the script won't report any cases because there is no diff between
the cases of the two refs.

Prevent the user from using HEAD refs.

A better solution might be to resolve the refs before doing the
reset, but for now just disallow such refs.

Signed-off-by: Ariel Marcovitch <arielmarcovitch@gmail.com>
---
 scripts/checkkconfigsymbols.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/checkkconfigsymbols.py b/scripts/checkkconfigsymbols.py
index 875e9a2c14b2..6259698e662d 100755
--- a/scripts/checkkconfigsymbols.py
+++ b/scripts/checkkconfigsymbols.py
@@ -103,6 +103,9 @@ def parse_options():
                      "continue.")
 
     if args.commit:
+        if args.commit.startswith('HEAD'):
+            sys.exit("The --commit option can't get use the HEAD ref")
+
         args.find = False
 
     if args.ignore:
-- 
2.25.1

Re: [PATCH 0/3] checkkconfigsymbols.py: Fix various bugs

[Masahiro Yamada]

On Mon, Aug 23, 2021 at 4:22 AM Ariel Marcovitch
<arielmarcovitch@gmail.com> wrote:
>
> Hi!
>
> This series fixes some issues in the checkkconfigsymbols.py script.
>
> The first patch fixes a bug in the --ignore option that makes the
> script check only the files matching the pattern instead of ignoring
> them.
>
> The second patch fixes a parsing error in the Kconfig files parser
> that makes it ignore 'if' statements after 'help' sections.
>
> The third patch prevents the user from using 'HEAD' refs in the
>  --commit option, because it doesn't really work.

Honestly, I didn't even know this script.

I added Valentin Rothberg, the main contributor
to this script.




> Thanks!
>
> -Ariel
>
> Ariel Marcovitch (3):
>   checkkconfigsymbols.py: Fix the '--ignore' option
>   checkkconfigsymbols.py: Fix Kconfig parsing to find 'if' lines
>   checkkconfigsymbols.py: Forbid passing 'HEAD' to --commit
>
>  scripts/checkkconfigsymbols.py | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
>
> base-commit: 36a21d51725af2ce0700c6ebcb6b9594aac658a6
> --
> 2.25.1
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/3] checkkconfigsymbols.py: Fix the '--ignore' option

[Masahiro Yamada]

On Mon, Aug 23, 2021 at 4:22 AM Ariel Marcovitch
<arielmarcovitch@gmail.com> wrote:
>
> It seems like the implementation of the --ignore option is broken.
>
> In check_symbols_helper, when going through the list of files, a file is
> added to the list of source files to check if it matches the ignore
> pattern. Instead, as stated in the comment below this condition, the
> file should be added if it doesn't match the pattern.
>
> This means that when providing an ignore pattern, the only files that
> will be checked will be the ones we want the ignore, in addition to the
> Kconfig files that don't match the pattern (the check in
> parse_kconfig_files is done right)
>
> Signed-off-by: Ariel Marcovitch <arielmarcovitch@gmail.com>
> ---
>  scripts/checkkconfigsymbols.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/checkkconfigsymbols.py b/scripts/checkkconfigsymbols.py
> index 1548f9ce4682..b9b0f15e5880 100755
> --- a/scripts/checkkconfigsymbols.py
> +++ b/scripts/checkkconfigsymbols.py
> @@ -329,7 +329,7 @@ def check_symbols_helper(pool, ignore):
>          if REGEX_FILE_KCONFIG.match(gitfile):
>              kconfig_files.append(gitfile)
>          else:
> -            if ignore and not re.match(ignore, gitfile):
> +            if ignore and re.match(ignore, gitfile):
>                  continue

This fix seems correct.
Applied to linux-kbuild.



>              # add source files that do not match the ignore pattern
>              source_files.append(gitfile)
> --
> 2.25.1
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/3] checkkconfigsymbols.py: Fix Kconfig parsing to find 'if' lines

[Masahiro Yamada]

On Mon, Aug 23, 2021 at 4:22 AM Ariel Marcovitch
<arielmarcovitch@gmail.com> wrote:
>
> When parsing Kconfig files to find symbol definitions and references,
> lines after a 'help' line are skipped until a new config definition
> starts.
>
> However, it is quite common to define a config and then make some other
> configs depend on it by adding an 'if' line. This kind of kconfig
> statement usually appears after a config definition which might contain
> a 'help' section. The 'if' line is skipped in parse_kconfig_file()
> because it is not a config definition.
>
> This means that symbols referenced in this kind of statements are
> ignored by this function and thus are not considered undefined
> references in case the symbol is not defined.
>
> The REGEX_KCONFIG_STMT regex can't be used because the other types of
> statements can't break help lines.
>
> Define a new regex for matching 'if' statements and stop the 'help'
> skipping in case it is encountered.
>
> Signed-off-by: Ariel Marcovitch <arielmarcovitch@gmail.com>
> ---
>  scripts/checkkconfigsymbols.py | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/checkkconfigsymbols.py b/scripts/checkkconfigsymbols.py
> index b9b0f15e5880..875e9a2c14b2 100755
> --- a/scripts/checkkconfigsymbols.py
> +++ b/scripts/checkkconfigsymbols.py
> @@ -26,6 +26,7 @@ EXPR = r"(?:" + OPERATORS + r"|\s|" + SYMBOL + r")+"
>  DEFAULT = r"default\s+.*?(?:if\s.+){,1}"
>  STMT = r"^\s*(?:if|select|imply|depends\s+on|(?:" + DEFAULT + r"))\s+" + EXPR
>  SOURCE_SYMBOL = r"(?:\W|\b)+[D]{,1}CONFIG_(" + SYMBOL + r")"
> +IF_LINE = r"^\s*(?:if)\s+" + EXPR


Why is it enclosed by "(?: )"   ?

"(?:if)"  seems to the same as "if"






>
>  # regex objects
>  REGEX_FILE_KCONFIG = re.compile(r".*Kconfig[\.\w+\-]*$")
> @@ -35,11 +36,11 @@ REGEX_KCONFIG_DEF = re.compile(DEF)
>  REGEX_KCONFIG_EXPR = re.compile(EXPR)
>  REGEX_KCONFIG_STMT = re.compile(STMT)
>  REGEX_KCONFIG_HELP = re.compile(r"^\s+help\s*$")
> +REGEX_KCONFIG_IF_LINE = re.compile(IF_LINE)
>  REGEX_FILTER_SYMBOLS = re.compile(r"[A-Za-z0-9]$")
>  REGEX_NUMERIC = re.compile(r"0[xX][0-9a-fA-F]+|[0-9]+")
>  REGEX_QUOTES = re.compile("(\"(.*?)\")")
>
> -
>  def parse_options():
>      """The user interface of this module."""
>      usage = "Run this tool to detect Kconfig symbols that are referenced but " \
> @@ -445,6 +446,11 @@ def parse_kconfig_file(kfile):
>          line = line.strip('\n')
>          line = line.split("#")[0]  # ignore comments
>
> +        # 'if EXPR' lines can be after help lines
> +        # The if line itself is handled later
> +        if REGEX_KCONFIG_IF_LINE.match(line):
> +            skip = False
> +


I do not think this is the right fix.
There are similar patterns where
config references are ignored.

For example, FOO and BAR are ignored
in the following cases.

ex1)

choice
          prompt "foo"
          default FOO



ex2)

menu "bar"
           depends on BAR




The help block ends with shallower indentation.




>          if REGEX_KCONFIG_DEF.match(line):
>              symbol_def = REGEX_KCONFIG_DEF.findall(line)
>              defined.append(symbol_def[0])
> --
> 2.25.1
>


--
Best Regards
Masahiro Yamada

Re: [PATCH 3/3] checkkconfigsymbols.py: Forbid passing 'HEAD' to --commit

[Masahiro Yamada]

On Mon, Aug 23, 2021 at 4:23 AM Ariel Marcovitch
<arielmarcovitch@gmail.com> wrote:
>
> As opposed to the --diff option, --commit can get ref names instead of
> commit hashes.
>
> When using the --commit option, the script resets the working directory
> to the commit before the given ref, by adding '~' to the end of the ref.
>
> However, the 'HEAD' ref is relative, and so when the working directory
> is reset to 'HEAD~', 'HEAD' points to what was 'HEAD~'. Then when the
> script resets to 'HEAD' it actually stays in the same commit. In this
> case, the script won't report any cases because there is no diff between
> the cases of the two refs.
>
> Prevent the user from using HEAD refs.
>
> A better solution might be to resolve the refs before doing the
> reset, but for now just disallow such refs.


Better than doing nothing.
So, applied to linux-kbuild.





> Signed-off-by: Ariel Marcovitch <arielmarcovitch@gmail.com>
> ---
>  scripts/checkkconfigsymbols.py | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/scripts/checkkconfigsymbols.py b/scripts/checkkconfigsymbols.py
> index 875e9a2c14b2..6259698e662d 100755
> --- a/scripts/checkkconfigsymbols.py
> +++ b/scripts/checkkconfigsymbols.py
> @@ -103,6 +103,9 @@ def parse_options():
>                       "continue.")
>
>      if args.commit:
> +        if args.commit.startswith('HEAD'):
> +            sys.exit("The --commit option can't get use the HEAD ref")
> +
>          args.find = False
>
>      if args.ignore:
> --
> 2.25.1
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/3] checkkconfigsymbols.py: Fix Kconfig parsing to find 'if' lines

[Ariel Marcovitch]

Hello again!

On 24/08/2021 16:30, Masahiro Yamada wrote:
 > On Mon, Aug 23, 2021 at 4:22 AM Ariel Marcovitch
 > <arielmarcovitch@gmail.com> wrote:
 >>
 >> When parsing Kconfig files to find symbol definitions and references,
 >> lines after a 'help' line are skipped until a new config definition
 >> starts.
 >>
 >> However, it is quite common to define a config and then make some other
 >> configs depend on it by adding an 'if' line. This kind of kconfig
 >> statement usually appears after a config definition which might contain
 >> a 'help' section. The 'if' line is skipped in parse_kconfig_file()
 >> because it is not a config definition.
 >>
 >> This means that symbols referenced in this kind of statements are
 >> ignored by this function and thus are not considered undefined
 >> references in case the symbol is not defined.
 >>
 >> The REGEX_KCONFIG_STMT regex can't be used because the other types of
 >> statements can't break help lines.
 >>
 >> Define a new regex for matching 'if' statements and stop the 'help'
 >> skipping in case it is encountered.
 >>
 >> Signed-off-by: Ariel Marcovitch <arielmarcovitch@gmail.com>
 >> ---
 >>  scripts/checkkconfigsymbols.py | 8 +++++++-
 >>  1 file changed, 7 insertions(+), 1 deletion(-)
 >>
 >> diff --git a/scripts/checkkconfigsymbols.py 
b/scripts/checkkconfigsymbols.py
 >> index b9b0f15e5880..875e9a2c14b2 100755
 >> --- a/scripts/checkkconfigsymbols.py
 >> +++ b/scripts/checkkconfigsymbols.py
 >> @@ -26,6 +26,7 @@ EXPR = r"(?:" + OPERATORS + r"|\s|" + SYMBOL + r")+"
 >>  DEFAULT = r"default\s+.*?(?:if\s.+){,1}"
 >>  STMT = r"^\s*(?:if|select|imply|depends\s+on|(?:" + DEFAULT + 
r"))\s+" + EXPR
 >>  SOURCE_SYMBOL = r"(?:\W|\b)+[D]{,1}CONFIG_(" + SYMBOL + r")"
 >> +IF_LINE = r"^\s*(?:if)\s+" + EXPR
 >
 >
 > Why is it enclosed by "(?: )"   ?
 >
 > "(?:if)"  seems to the same as "if"
Oh you are absolutely right.
I just mindlessly copied the STMT regex and removed the other types :)
 >
 >
 >
 >
 >
 >
 >>
 >>  # regex objects
 >>  REGEX_FILE_KCONFIG = re.compile(r".*Kconfig[\.\w+\-]*$")
 >> @@ -35,11 +36,11 @@ REGEX_KCONFIG_DEF = re.compile(DEF)
 >>  REGEX_KCONFIG_EXPR = re.compile(EXPR)
 >>  REGEX_KCONFIG_STMT = re.compile(STMT)
 >>  REGEX_KCONFIG_HELP = re.compile(r"^\s+help\s*$")
 >> +REGEX_KCONFIG_IF_LINE = re.compile(IF_LINE)
 >>  REGEX_FILTER_SYMBOLS = re.compile(r"[A-Za-z0-9]$")
 >>  REGEX_NUMERIC = re.compile(r"0[xX][0-9a-fA-F]+|[0-9]+")
 >>  REGEX_QUOTES = re.compile("(\"(.*?)\")")
 >>
 >> -
 >>  def parse_options():
 >>      """The user interface of this module."""
 >>      usage = "Run this tool to detect Kconfig symbols that are 
referenced but " \
 >> @@ -445,6 +446,11 @@ def parse_kconfig_file(kfile):
 >>          line = line.strip('\n')
 >>          line = line.split("#")[0]  # ignore comments
 >>
 >> +        # 'if EXPR' lines can be after help lines
 >> +        # The if line itself is handled later
 >> +        if REGEX_KCONFIG_IF_LINE.match(line):
 >> +            skip = False
 >> +
 >
 >
 > I do not think this is the right fix.
 > There are similar patterns where
 > config references are ignored.
 >
 > For example, FOO and BAR are ignored
 > in the following cases.
 >
 > ex1)
 >
 > choice
 >           prompt "foo"
 >           default FOO
 >
 >
 >
 > ex2)
 >
 > menu "bar"
 >            depends on BAR
 >
 >
 >
 >
 > The help block ends with shallower indentation.
So IIUC we need to measure the indentation when we encounter a help
statement and in the next lines look for a line with a different depth
(which is not an empty line because these are allowed).
 >
 >
 >
 >
 >>          if REGEX_KCONFIG_DEF.match(line):
 >>              symbol_def = REGEX_KCONFIG_DEF.findall(line)
 >>              defined.append(symbol_def[0])
 >> --
 >> 2.25.1
 >>
 >
 >
 > --
 > Best Regards
 > Masahiro Yamada

Thanks for your time!

Ariel Marcovitch

Re: [PATCH 3/3] checkkconfigsymbols.py: Forbid passing 'HEAD' to --commit

[Ariel Marcovitch]

Hello!

On 24/08/2021 16:31, Masahiro Yamada wrote:
> On Mon, Aug 23, 2021 at 4:23 AM Ariel Marcovitch
> <arielmarcovitch@gmail.com> wrote:
>> As opposed to the --diff option, --commit can get ref names instead of
>> commit hashes.
>>
>> When using the --commit option, the script resets the working directory
>> to the commit before the given ref, by adding '~' to the end of the ref.
>>
>> However, the 'HEAD' ref is relative, and so when the working directory
>> is reset to 'HEAD~', 'HEAD' points to what was 'HEAD~'. Then when the
>> script resets to 'HEAD' it actually stays in the same commit. In this
>> case, the script won't report any cases because there is no diff between
>> the cases of the two refs.
>>
>> Prevent the user from using HEAD refs.
>>
>> A better solution might be to resolve the refs before doing the
>> reset, but for now just disallow such refs.
>
> Better than doing nothing.
> So, applied to linux-kbuild.
>
>
>
>
>
>> Signed-off-by: Ariel Marcovitch <arielmarcovitch@gmail.com>
>> ---
>>   scripts/checkkconfigsymbols.py | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/scripts/checkkconfigsymbols.py b/scripts/checkkconfigsymbols.py
>> index 875e9a2c14b2..6259698e662d 100755
>> --- a/scripts/checkkconfigsymbols.py
>> +++ b/scripts/checkkconfigsymbols.py
>> @@ -103,6 +103,9 @@ def parse_options():
>>                        "continue.")
>>
>>       if args.commit:
>> +        if args.commit.startswith('HEAD'):
>> +            sys.exit("The --commit option can't get use the HEAD ref")

Just realized that the message says "can't get use" which doesn't make
much sense :)

Do you want me to send a new patch to fix it?

>> +
>>           args.find = False
>>
>>       if args.ignore:
>> --
>> 2.25.1
>>
>
Thanks for your time :)

Ariel Marcovitch

Re: [PATCH 3/3] checkkconfigsymbols.py: Forbid passing 'HEAD' to --commit

[Masahiro Yamada]

On Sun, Aug 29, 2021 at 10:23 PM Ariel Marcovitch <trhtkmarco@gmail.com> wrote:
>
> Hello!
>
> On 24/08/2021 16:31, Masahiro Yamada wrote:
> > On Mon, Aug 23, 2021 at 4:23 AM Ariel Marcovitch
> > <arielmarcovitch@gmail.com> wrote:
> >> As opposed to the --diff option, --commit can get ref names instead of
> >> commit hashes.
> >>
> >> When using the --commit option, the script resets the working directory
> >> to the commit before the given ref, by adding '~' to the end of the ref.
> >>
> >> However, the 'HEAD' ref is relative, and so when the working directory
> >> is reset to 'HEAD~', 'HEAD' points to what was 'HEAD~'. Then when the
> >> script resets to 'HEAD' it actually stays in the same commit. In this
> >> case, the script won't report any cases because there is no diff between
> >> the cases of the two refs.
> >>
> >> Prevent the user from using HEAD refs.
> >>
> >> A better solution might be to resolve the refs before doing the
> >> reset, but for now just disallow such refs.
> >
> > Better than doing nothing.
> > So, applied to linux-kbuild.
> >
> >
> >
> >
> >
> >> Signed-off-by: Ariel Marcovitch <arielmarcovitch@gmail.com>
> >> ---
> >>   scripts/checkkconfigsymbols.py | 3 +++
> >>   1 file changed, 3 insertions(+)
> >>
> >> diff --git a/scripts/checkkconfigsymbols.py b/scripts/checkkconfigsymbols.py
> >> index 875e9a2c14b2..6259698e662d 100755
> >> --- a/scripts/checkkconfigsymbols.py
> >> +++ b/scripts/checkkconfigsymbols.py
> >> @@ -103,6 +103,9 @@ def parse_options():
> >>                        "continue.")
> >>
> >>       if args.commit:
> >> +        if args.commit.startswith('HEAD'):
> >> +            sys.exit("The --commit option can't get use the HEAD ref")
>
> Just realized that the message says "can't get use" which doesn't make
> much sense :)
>
> Do you want me to send a new patch to fix it?

OK, I dropped 3/3 from my tree.

Please send v2.






> >> +
> >>           args.find = False
> >>
> >>       if args.ignore:
> >> --
> >> 2.25.1
> >>
> >
> Thanks for your time :)
>
> Ariel Marcovitch
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/3] checkkconfigsymbols.py: Fix Kconfig parsing to find 'if' lines

[Masahiro Yamada]

On Sun, Aug 29, 2021 at 10:18 PM Ariel Marcovitch
<arielmarcovitch@gmail.com> wrote:
>
> Hello again!
>
> On 24/08/2021 16:30, Masahiro Yamada wrote:
>  > On Mon, Aug 23, 2021 at 4:22 AM Ariel Marcovitch
>  > <arielmarcovitch@gmail.com> wrote:
>  >>
>  >> When parsing Kconfig files to find symbol definitions and references,
>  >> lines after a 'help' line are skipped until a new config definition
>  >> starts.
>  >>
>  >> However, it is quite common to define a config and then make some other
>  >> configs depend on it by adding an 'if' line. This kind of kconfig
>  >> statement usually appears after a config definition which might contain
>  >> a 'help' section. The 'if' line is skipped in parse_kconfig_file()
>  >> because it is not a config definition.
>  >>
>  >> This means that symbols referenced in this kind of statements are
>  >> ignored by this function and thus are not considered undefined
>  >> references in case the symbol is not defined.
>  >>
>  >> The REGEX_KCONFIG_STMT regex can't be used because the other types of
>  >> statements can't break help lines.
>  >>
>  >> Define a new regex for matching 'if' statements and stop the 'help'
>  >> skipping in case it is encountered.
>  >>
>  >> Signed-off-by: Ariel Marcovitch <arielmarcovitch@gmail.com>
>  >> ---
>  >>  scripts/checkkconfigsymbols.py | 8 +++++++-
>  >>  1 file changed, 7 insertions(+), 1 deletion(-)
>  >>
>  >> diff --git a/scripts/checkkconfigsymbols.py
> b/scripts/checkkconfigsymbols.py
>  >> index b9b0f15e5880..875e9a2c14b2 100755
>  >> --- a/scripts/checkkconfigsymbols.py
>  >> +++ b/scripts/checkkconfigsymbols.py
>  >> @@ -26,6 +26,7 @@ EXPR = r"(?:" + OPERATORS + r"|\s|" + SYMBOL + r")+"
>  >>  DEFAULT = r"default\s+.*?(?:if\s.+){,1}"
>  >>  STMT = r"^\s*(?:if|select|imply|depends\s+on|(?:" + DEFAULT +
> r"))\s+" + EXPR
>  >>  SOURCE_SYMBOL = r"(?:\W|\b)+[D]{,1}CONFIG_(" + SYMBOL + r")"
>  >> +IF_LINE = r"^\s*(?:if)\s+" + EXPR
>  >
>  >
>  > Why is it enclosed by "(?: )"   ?
>  >
>  > "(?:if)"  seems to the same as "if"
> Oh you are absolutely right.
> I just mindlessly copied the STMT regex and removed the other types :)
>  >
>  >
>  >
>  >
>  >
>  >
>  >>
>  >>  # regex objects
>  >>  REGEX_FILE_KCONFIG = re.compile(r".*Kconfig[\.\w+\-]*$")
>  >> @@ -35,11 +36,11 @@ REGEX_KCONFIG_DEF = re.compile(DEF)
>  >>  REGEX_KCONFIG_EXPR = re.compile(EXPR)
>  >>  REGEX_KCONFIG_STMT = re.compile(STMT)
>  >>  REGEX_KCONFIG_HELP = re.compile(r"^\s+help\s*$")
>  >> +REGEX_KCONFIG_IF_LINE = re.compile(IF_LINE)
>  >>  REGEX_FILTER_SYMBOLS = re.compile(r"[A-Za-z0-9]$")
>  >>  REGEX_NUMERIC = re.compile(r"0[xX][0-9a-fA-F]+|[0-9]+")
>  >>  REGEX_QUOTES = re.compile("(\"(.*?)\")")
>  >>
>  >> -
>  >>  def parse_options():
>  >>      """The user interface of this module."""
>  >>      usage = "Run this tool to detect Kconfig symbols that are
> referenced but " \
>  >> @@ -445,6 +446,11 @@ def parse_kconfig_file(kfile):
>  >>          line = line.strip('\n')
>  >>          line = line.split("#")[0]  # ignore comments
>  >>
>  >> +        # 'if EXPR' lines can be after help lines
>  >> +        # The if line itself is handled later
>  >> +        if REGEX_KCONFIG_IF_LINE.match(line):
>  >> +            skip = False
>  >> +
>  >
>  >
>  > I do not think this is the right fix.
>  > There are similar patterns where
>  > config references are ignored.
>  >
>  > For example, FOO and BAR are ignored
>  > in the following cases.
>  >
>  > ex1)
>  >
>  > choice
>  >           prompt "foo"
>  >           default FOO
>  >
>  >
>  >
>  > ex2)
>  >
>  > menu "bar"
>  >            depends on BAR
>  >
>  >
>  >
>  >
>  > The help block ends with shallower indentation.
> So IIUC we need to measure the indentation when we encounter a help
> statement and in the next lines look for a line with a different depth
> (which is not an empty line because these are allowed).



If you want to implement it precisely, yes.


Or, if you want to adopt a simpler
solution, detect the following keywords.

comment
if
menu
choice


This is not precise, but it will work
in most cases.



In the following example, the first 'menu'
is just a comment.
The second 'menu' is a keyword since it has
a shallower indentation.



    help
       blah blah
       menu blah blah
       blah blah
  menu "menu prompt"
     depends on FOO












--
Best Regards
Masahiro Yamada

Re: [PATCH 2/3] checkkconfigsymbols.py: Fix Kconfig parsing to find 'if' lines

[Ariel Marcovitch]

Hi again!


On 30/08/2021 2:41, Masahiro Yamada wrote:
 > On Sun, Aug 29, 2021 at 10:18 PM Ariel Marcovitch
 > <arielmarcovitch@gmail.com> wrote:
 >>
 >> Hello again!
 >>
 >> On 24/08/2021 16:30, Masahiro Yamada wrote:
 >>  > On Mon, Aug 23, 2021 at 4:22 AM Ariel Marcovitch
 >>  > <arielmarcovitch@gmail.com> wrote:
 >>  >>
 >>  >> When parsing Kconfig files to find symbol definitions and 
references,
 >>  >> lines after a 'help' line are skipped until a new config definition
 >>  >> starts.
 >>  >>
 >>  >> However, it is quite common to define a config and then make 
some other
 >>  >> configs depend on it by adding an 'if' line. This kind of kconfig
 >>  >> statement usually appears after a config definition which might 
contain
 >>  >> a 'help' section. The 'if' line is skipped in parse_kconfig_file()
 >>  >> because it is not a config definition.
 >>  >>
 >>  >> This means that symbols referenced in this kind of statements are
 >>  >> ignored by this function and thus are not considered undefined
 >>  >> references in case the symbol is not defined.
 >>  >>
 >>  >> The REGEX_KCONFIG_STMT regex can't be used because the other 
types of
 >>  >> statements can't break help lines.
 >>  >>
 >>  >> Define a new regex for matching 'if' statements and stop the 'help'
 >>  >> skipping in case it is encountered.
 >>  >>
 >>  >> Signed-off-by: Ariel Marcovitch <arielmarcovitch@gmail.com>
 >>  >> ---
 >>  >>  scripts/checkkconfigsymbols.py | 8 +++++++-
 >>  >>  1 file changed, 7 insertions(+), 1 deletion(-)
 >>  >>
 >>  >> diff --git a/scripts/checkkconfigsymbols.py
 >> b/scripts/checkkconfigsymbols.py
 >>  >> index b9b0f15e5880..875e9a2c14b2 100755
 >>  >> --- a/scripts/checkkconfigsymbols.py
 >>  >> +++ b/scripts/checkkconfigsymbols.py
 >>  >> @@ -26,6 +26,7 @@ EXPR = r"(?:" + OPERATORS + r"|\s|" + SYMBOL + 
r")+"
 >>  >>  DEFAULT = r"default\s+.*?(?:if\s.+){,1}"
 >>  >>  STMT = r"^\s*(?:if|select|imply|depends\s+on|(?:" + DEFAULT +
 >> r"))\s+" + EXPR
 >>  >>  SOURCE_SYMBOL = r"(?:\W|\b)+[D]{,1}CONFIG_(" + SYMBOL + r")"
 >>  >> +IF_LINE = r"^\s*(?:if)\s+" + EXPR
 >>  >
 >>  >
 >>  > Why is it enclosed by "(?: )"   ?
 >>  >
 >>  > "(?:if)"  seems to the same as "if"
 >> Oh you are absolutely right.
 >> I just mindlessly copied the STMT regex and removed the other types :)
 >>  >
 >>  >
 >>  >
 >>  >
 >>  >
 >>  >
 >>  >>
 >>  >>  # regex objects
 >>  >>  REGEX_FILE_KCONFIG = re.compile(r".*Kconfig[\.\w+\-]*$")
 >>  >> @@ -35,11 +36,11 @@ REGEX_KCONFIG_DEF = re.compile(DEF)
 >>  >>  REGEX_KCONFIG_EXPR = re.compile(EXPR)
 >>  >>  REGEX_KCONFIG_STMT = re.compile(STMT)
 >>  >>  REGEX_KCONFIG_HELP = re.compile(r"^\s+help\s*$")
 >>  >> +REGEX_KCONFIG_IF_LINE = re.compile(IF_LINE)
 >>  >>  REGEX_FILTER_SYMBOLS = re.compile(r"[A-Za-z0-9]$")
 >>  >>  REGEX_NUMERIC = re.compile(r"0[xX][0-9a-fA-F]+|[0-9]+")
 >>  >>  REGEX_QUOTES = re.compile("(\"(.*?)\")")
 >>  >>
 >>  >> -
 >>  >>  def parse_options():
 >>  >>      """The user interface of this module."""
 >>  >>      usage = "Run this tool to detect Kconfig symbols that are
 >> referenced but " \
 >>  >> @@ -445,6 +446,11 @@ def parse_kconfig_file(kfile):
 >>  >>          line = line.strip('\n')
 >>  >>          line = line.split("#")[0]  # ignore comments
 >>  >>
 >>  >> +        # 'if EXPR' lines can be after help lines
 >>  >> +        # The if line itself is handled later
 >>  >> +        if REGEX_KCONFIG_IF_LINE.match(line):
 >>  >> +            skip = False
 >>  >> +
 >>  >
 >>  >
 >>  > I do not think this is the right fix.
 >>  > There are similar patterns where
 >>  > config references are ignored.
 >>  >
 >>  > For example, FOO and BAR are ignored
 >>  > in the following cases.
 >>  >
 >>  > ex1)
 >>  >
 >>  > choice
 >>  >           prompt "foo"
 >>  >           default FOO
 >>  >
 >>  >
 >>  >
 >>  > ex2)
 >>  >
 >>  > menu "bar"
 >>  >            depends on BAR
 >>  >
 >>  >
 >>  >
 >>  >
 >>  > The help block ends with shallower indentation.
 >> So IIUC we need to measure the indentation when we encounter a help
 >> statement and in the next lines look for a line with a different depth
 >> (which is not an empty line because these are allowed).
 >
 >
 >
 > If you want to implement it precisely, yes.
 >
 >
 > Or, if you want to adopt a simpler
 > solution, detect the following keywords.
 >
 > comment
 > if
 > menu
 > choice

Actually, it seems that all statements are legal in this context.

So we can just use the STMT regex!

It does require reshuffling the logic there a little but this should
do.

 >
 >
 >
 > This is not precise, but it will work
 > in most cases.
 >
 >
 >
 > In the following example, the first 'menu'
 > is just a comment.
 > The second 'menu' is a keyword since it has
 > a shallower indentation.
 >
 >
 >
 >     help
 >        blah blah
 >        menu blah blah
 >        blah blah
 >   menu "menu prompt"
 >      depends on FOO
 >
 >

Yeah this will probably drive the parser crazy (even without my
changes, although my changes might make it worse).

But the indentation solution is kinda nasty.

Do you think the STMT regex check is enough or should I handle the
cases you mentioned with the indentation check?


 >
 >
 >
 >
 >
 >
 >
 >
 >
 >
 > --
 > Best Regards
 > Masahiro Yamada

Thanks again for your time :)
Ariel Marcovitch