Skip to content

tools/kconfiglib: Add riot_kconfig to override default behaviours#13916

Merged
aabadie merged 2 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/kconfiglib/parse_help
Apr 27, 2020
Merged

tools/kconfiglib: Add riot_kconfig to override default behaviours#13916
aabadie merged 2 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/kconfiglib/parse_help

Conversation

@leandrolanzieri
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri commented Apr 21, 2020

Contribution description

This PR adds a RiotKconfig class which can be used to override behaviours from the base Kconfig. It also patches menuconfig.py to use the new class. For now it implements a version of _parse_help which removes Doxygen markers.

standard_riot_kconfig is taken from the original standard_kconfig.

Testing procedure

  • Check that make menuconfig still works as usual
  • Check that the help string of a symbol with Doxygen markers (e.g. GNRC_NETIF_IPV6_ADDRS_NUMOF) does not show them.

Issues/PRs references

Solves issue presented here #13227 (comment)

@leandrolanzieri leandrolanzieri added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: tools Area: Supplementary tools Area: Kconfig Area: Kconfig integration labels Apr 21, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.07 milestone Apr 21, 2020
@leandrolanzieri leandrolanzieri force-pushed the pr/kconfiglib/parse_help branch from 841e735 to f246a8c Compare April 21, 2020 14:37
@bergzand
Copy link
Copy Markdown
Member

bergzand commented Apr 21, 2020

@aabadie Do you mind give this your usual python quality check? :)

@bergzand bergzand requested a review from aabadie April 21, 2020 15:00
aabadie
aabadie requested changes Apr 21, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor comments for the moment.

Do you think it would be possible to monkey patch the _main function of the menuconfig.py module in order to use standard_riot_kconfig instead of patching the file itself ?

In fact, I'm wondering why add kconfiglib as a package instead of asking it to be installed using pip (a bit off-topic in this PR) ?

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Apr 21, 2020

Just minor comments for the moment.

Do you think it would be possible to monkey patch the _main function of the menuconfig.py module in order to use standard_riot_kconfig instead of patching the file itself ?

Thanks for the suggestion. I added a riot_menuconfig.py which calls the menuconfig function now with the RIOT-specific class.

In fact, I'm wondering why add kconfiglib as a package instead of asking it to be installed using pip (a bit off-topic in this PR) ?

I added it as a package because it appeared to me as a ready-to-run solution, instead of burdening the user with installing an extra tool and adding another dependency.

@bergzand bergzand mentioned this pull request Apr 24, 2020
Merged
@leandrolanzieri leandrolanzieri force-pushed the pr/kconfiglib/parse_help branch from 81ad7de to 80ef3b8 Compare April 24, 2020 09:05
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Apr 24, 2020

I added a riot_menuconfig.py which calls the menuconfig function now with the RIOT-specific class.

Thanks, I prefer this approach!

I added it as a package because it appeared to me as a ready-to-run solution, instead of burdening the user with installing an extra tool and adding another dependency.

Ok

@aabadie aabadie self-assigned this Apr 24, 2020
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Apr 27, 2020

I'm testing this PR but I don't understand what I'm supposed to get with "Check that the help string of a symbol with Doxygen markers (e.g. GNRC_NETIF_IPV6_ADDRS_NUMOF) does not show them."

Any hint @leandrolanzieri ?

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Apr 27, 2020 •
edited
Loading

I'm testing this PR but I don't understand what I'm supposed to get with "Check that the help string of a symbol with Doxygen markers (e.g. GNRC_NETIF_IPV6_ADDRS_NUMOF) does not show them."

Any hint @leandrolanzieri ?

Yeah sorry. What I mean is that in master you should see in the menuconfig interface the help strings with Doxygen markers, and with this PR those should not appear. The help string can be displayed either toggling the 'show-help' mode (F) or showing the symbol info (?) when the symbol is highlighted.

  • For GNRC_NETIF_IPV6_ADDRS_NUMOF:
    master:
Name: GNRC_NETIF_IPV6_ADDRS_NUMOF                                                                                                                               
Prompt: Maximum number of unicast and anycast addresses per interface                                                                                           
Type: int                                                                                                                                                       
Value:                                                                                                                                                          
                                                                                                                                                                
Help:                                                                                                                                                           
                                                                                                                                                                
  If you change this, please make sure that                                                                                                                     
  @ref CONFIG_GNRC_NETIF_IPV6_GROUPS_NUMOF is also large enough to fit the                                                                                      
  addresses' solicited nodes multicast addresses.                                                                                                               
  Default: 2 (1 link-local + 1 global address).

This PR

Name: GNRC_NETIF_IPV6_ADDRS_NUMOF                                                                                                                               
Prompt: Maximum number of unicast and anycast addresses per interface                                                                                           
Type: int                                                                                                                                                       
Value:                                                                                                                                                          
                                                                                                                                                                
Help:                                                                                                                                                           
                                                                                                                                                                
  If you change this, please make sure that                                                                                                                     
  CONFIG_GNRC_NETIF_IPV6_GROUPS_NUMOF is also large enough to fit the                                                                                           
  addresses' solicited nodes multicast addresses.                                                                                                               
  Default: 2 (1 link-local + 1 global address).

Here @ref is gone.

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Apr 27, 2020

@aabadie you can search for a symbol using / and entering the name

aabadie
aabadie approved these changes Apr 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for providing more details on the testing procedure.

I could finally confirm that this PR is removing the doxygen markers from the detail info in menuconfig.

Code changes are also good. Let's go here!

ACK

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Apr 27, 2020

And please squash!

@leandrolanzieri
dist/tools/kconfiglib: Add riot_kconfig to override default behaviours
8023aa3 
This also patches menuconfig.py to use the new class. For now it
implements a version of _parse_help which removes Doxygen markers.
@leandrolanzieri
makefiles/kconfiglib: Use RIOT specific menuconfig script
ed38bd8
@leandrolanzieri leandrolanzieri force-pushed the pr/kconfiglib/parse_help branch from 80ef3b8 to ed38bd8 Compare April 27, 2020 10:30
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Apr 27, 2020

@aabadie squashed. Thanks for reviewing!

@leandrolanzieri leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 27, 2020
@aabadie aabadie merged commit e57b132 into RIOT-OS:master Apr 27, 2020
@leandrolanzieri leandrolanzieri deleted the pr/kconfiglib/parse_help branch April 27, 2020 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aabadie aabadie aabadie approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

@bergzand bergzand Awaiting requested review from bergzand

Assignees

@aabadie aabadie

Labels

Area: Kconfig Area: Kconfig integration Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2020.07

Development

Successfully merging this pull request may close these issues.

3 participants

@leandrolanzieri @bergzand @aabadie