Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools/kconfig2html.c: Add missing parse logic #3611

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

patacongo
Copy link
Contributor

@patacongo patacongo commented Apr 26, 2021

Summary

I tried running tools/mkconfigars.sh for the first time in a long time and it failed due to some missing support in the parser: The following was supported

config FOO
  int "Enable foo"

But not this equivalent form:

config FOO
  int
  prompt "Enable foo"

Also, conditional comments of the following form are now supported:

 comment "Isn't foo great?"
      depends on FOO

And support was added for comments following dependencies like:

      depends on FOO # We all depend on FOO

Impact

Should have no impact since this is not a critical build tool.

NOTE: There is one other, unrelated issues:

$ tools/mkconfigvars.sh
Unrecognized garbage after default value

This is due to missing logic to handle default values that are expressions. Specifically, the problem is caused by this in graphics/Kconfig:

    config NX_UPDATE
      bool "Display update hooks"
      default FB_UPDATE && !NX_LCDDRIVER

This is not currently addressed by this PR.

Testing

Tested using tools/mkconfigvars.sh

@patacongo patacongo marked this pull request as draft April 26, 2021 18:06
@patacongo patacongo changed the title tools/kconfig2html.c: Add missing parse logic [DRAFT]tools/kconfig2html.c: Add missing parse logic Apr 26, 2021
@patacongo
Copy link
Contributor Author

patacongo commented Apr 26, 2021

This is marked a draft because additional effort will be required to get this through nxstyle, mostly because there are many long lines reported. This PR does at least provide a fix to the problem if this ever becomes an issue for anyone who is more motivated than I to deal with the nxstyle complaints.

@patacongo patacongo changed the title [DRAFT]tools/kconfig2html.c: Add missing parse logic tools/kconfig2html.c: Add missing parse logic Apr 26, 2021
@gustavonihei
Copy link
Contributor

File ./arch/risc-v/src/esp32c3/Kconfig Unhandled token: depends

This error is related to this comment command:

comment "Selected ESP32-C3 chip without embedded Flash, an external Flash memory is required."
	depends on ARCH_CHIP_ESP32C3X

arch/xtensa/src/esp32/Kconfig contains a similar comment to let the user know why some options are hidden.

@patacongo
Copy link
Contributor Author

File ./arch/risc-v/src/esp32c3/Kconfig Unhandled token: depends

This error is related to this comment command:

comment "Selected ESP32-C3 chip without embedded Flash, an external Flash memory is required."
	depends on ARCH_CHIP_ESP32C3X

arch/xtensa/src/esp32/Kconfig contains a similar comment to let the user know why some options are hidden.

Thanks. I didn't look at all of the of depends on in the file. When I new which one it was the fix was easy. We talked about this in the past and decided not to support condition comments of that form, that is why it is not support. The ones for the Expressif parts are inconsistent and the only cases where that form is used. Those comments probably should be removed (I know the Linux community encourages that usage, but it has been discourage in NuttX for many years so any usage is inconsistent).

BTW: The other

depends on ARCH_CHIP_ESP32C3

are unnecessary and redundant since the Kconfig file begins with:

if ARCH_CHIP_ESP32C3

Everything from there to the matching endif has the that dependency.

@patacongo
Copy link
Contributor Author

I have not looked into the other reported error, but here is a little more information from running within debug output enabled:

      <li><a href="#CONFIG_ESP32C3_UART1_RXPIN">1.2.717.4 <code>CONFIG_ESP32C3_UART1_RXPIN</code>: UART1 RX Pin</a></li>
      <li><a href="#CONFIG_ESP32C3_PARTITION">1.2.717.5 <code>CONFIG_ESP32C3_PARTITION</code>: ESP32-C3 Partition</a></li>
    </ul>
    Menu: Terminating token: config
    <li><a name="menu_365_toc"><a href="#menu_365">1.2.718 Menu: Real-Time Timer</a></a></li>
<a href="#menu_365_toc" onclick="toggle('toggle_364', this)">Expand</a>
    <ul id="toggle_364"  style="display:none">
    process_menu: TOKEN_MENU
      kconfigdir:  ./arch/risc-v/src/esp32c3
      kconfigname: Kconfig
      level:       4
    CONFIG_ESP32C3_RT_TIMER_TASK_NAME: Terminating token: config
    <li><a href="#CONFIG_ESP32C3_RT_TIMER_TASK_NAME">1.2.718.1 <code>CONFIG_ESP32C3_RT_TIMER_TASK_NAME</code>: Timer task name</a></li>
    Unrecognized garbage after default value

It is probably complaining about this:

  default 223 # Lower than high priority workqueue

@gustavonihei
Copy link
Contributor

Thanks. I didn't look at all of the of depends on in the file. When I new which one it was the fix was easy. We talked about this in the past and decided not to support condition comments of that form, that is why it is not support. The ones for the Expressif parts are inconsistent and the only cases where that form is used. Those comments probably should be removed (I know the Linux community encourages that usage, but it has been discourage in NuttX for many years so any usage is inconsistent).

No problem, I can submit a PR to remove the comments. I was used to working with Buildroot and I thought it could be interesting to add.

BTW: The other

depends on ARCH_CHIP_ESP32C3

are unnecessary and redundant since the Kconfig file begins with:

if ARCH_CHIP_ESP32C3

Everything from there to the matching endif has the that dependency.

Thanks for the tip, I'll address that as well.

The following was supported

    config FOO
      int "Enable foo"

But not this equivalent form:

    config FOO
      int
      prompt "Enable foo"

Also, conditional comments of the following form are now supported:

    comment "Isn't foo great?"
      depends on FOO

And support was added for comments following dependencies like:

      depends on FOO # We all depend on FOO

Should have not impact since this is not a critical build tool.

NOTE:  There is one other, unrelated issues:

    $ tools/mkconfigvars.sh
    Unrecognized garbage after default value

This is due to missing logic to handle default values that are expressions.  Speifically, the problem is caused by this in graphics/Kconfig:

    config NX_UPDATE
      bool "Display update hooks"
      default FB_UPDATE && !NX_LCDDRIVER

This is not currently addressed by this PR.

Tested using tools/mkconfigvars.sh
@patacongo
Copy link
Contributor Author

patacongo commented Apr 27, 2021

No problem, I can submit a PR to remove the comments. I was used to working with Buildroot and I thought it could be interesting to add.

No problem. I updated the tool skip over # comments at the end of the line.

Thanks for the tip, I'll address that as well.

There are other issues as well. For example, people have begun using default values that are expressions of other configuration settings. The tool would not need to parse the expressions other than to know when they end.

But clearly this tools has not been used for many years and it would take more effort to get it back "up to snuff" and passing nxstyle issues. If the tools is truly unused, then should just be deleted rather than supported and fixed.

All of the other documentation has moved away from HTML.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants