kconfig: add support for warnings when using experimental features #33566

tejlmand · 2021-03-22T13:41:34Z

This PR allows a user to have a warning printed if any experimental features are used in Zephyr.

As example, building samples/hello_world with THREAD_MONITOR which is experimental will look as:

$ cmake -GNinja -DBOARD=nrf52840dk_nrf52840 -Bbuild samples/hello_world/ -DCONFIG_THREAD_MONITOR=y
Parsing /projects/github/ncs/zephyr/Kconfig
Loaded configuration '/projects/github/ncs/zephyr/boards/arm/nrf52840dk_nrf52840/nrf52840dk_nrf52840_defconfig'
Merged configuration '/projects/github/ncs/zephyr/samples/hello_world/prj.conf'
Merged configuration '/projects/github/ncs/zephyr/build/zephyr/misc/generated/extra_kconfig_options.conf'
Configuration saved to '/projects/github/ncs/zephyr/build/zephyr/.config'
Kconfig header saved to '/projects/github/ncs/zephyr/build/zephyr/include/generated/autoconf.h'
-- The C compiler identification is GNU 7.3.1
....

Enabling this new warning together with THREAD_MONITOR, will give:

$ cmake -GNinja -DBOARD=nrf52840dk_nrf52840 -Bbuild samples/hello_world/ -DCONFIG_WARN_EXPERIMENTAL=y -DCONFIG_THREAD_MONITOR=y
Parsing /projects/github/ncs/zephyr/Kconfig
Loaded configuration '/projects/github/ncs/zephyr/boards/arm/nrf52840dk_nrf52840/nrf52840dk_nrf52840_defconfig'
Merged configuration '/projects/github/ncs/zephyr/samples/hello_world/prj.conf'
Merged configuration '/projects/github/ncs/zephyr/build/zephyr/misc/generated/extra_kconfig_options.conf'
Configuration saved to '/projects/github/ncs/zephyr/build/zephyr/.config'
Kconfig header saved to '/projects/github/ncs/zephyr/build/zephyr/include/generated/autoconf.h'

warning: Experimental symbol THREAD_MONITOR is enabled.

.....

This adds a new setting which allows a user to print a warning when
a feature that is marked [EXPERIMENTAL] is enabled.

Signed-off-by: Torsten Rasmussen [email protected]

galak

I like the idea, but I don't think we should be modifying scripts/kconfig/kconfig.py to do this. Maybe a separate script similar to scripts/kconfig/hardenconfig.py?

tejlmand · 2021-03-22T14:12:24Z

I like the idea, but I don't think we should be modifying scripts/kconfig/kconfig.py to do this. Maybe a separate script similar to scripts/kconfig/hardenconfig.py?

Except that hardenconfig.py is a build target that users can call with ninja hardenconfig, similar to menuconfig and other extra targets, and thus only take resources when it's called.

The CONFIG_WARN_EXPERIMENTAL=y allows you to get a warning if experimental features are enabled as part of the CMake invocation, similar to other build warnings.

The motivation for updating kconfig.py is because it has the parsed kconfig tree available, and thus no overhead of re-invoking Kconfig parsing at CMake time.

tejlmand · 2021-03-22T14:18:22Z

@galak also notice, kconfig.py already does various checking of the Kconfig tree today:

zephyr/scripts/kconfig/kconfig.py

Line 57 in 16d2ab7

check_no_promptless_assign(kconf)

zephyr/scripts/kconfig/kconfig.py

Line 62 in 16d2ab7

check_assigned_sym_values(kconf)

zephyr/scripts/kconfig/kconfig.py

Line 63 in 16d2ab7

check_assigned_choice_values(kconf)

rugeGerritsen · 2021-03-22T16:10:17Z

In a very limited set of places we have constructs like:

config BT_CTLR_ADV_EXT
	bool "LE Advertising Extensions" if !BT_LL_SW_SPLIT

config BT_CTLR_ADV_EXT
	bool "LE Advertising Extensions [EXPERIMENTAL]" if BT_LL_SW_SPLIT

I would assume that we need to rewrite those to something similar to

config BT_CTLR_ADV_EXT
	bool "LE Advertising Extensions"

config BT_CTLR_ADV_EXT_SW_LL_SPLIT
	bool "LE Advertising Extensions [EXPERIMENTAL]"
	default y if BT_LL_SW_SPLIT && BT_CTLR_ADV_EXT

Otherwise warning: Experimental symbol BT_CTLR_ADV_EXT will be printed even if BT_LL_SW_SPLIT is not selected. See https://github.com/zephyrproject-rtos/zephyr/blob/master/subsys/bluetooth/controller/Kconfig#L460

Would it be possible to make a similar construct that fails the configuration, as opposed to printing a warning, if an experimental feature is selected?

carlescufi · 2021-03-22T16:32:09Z

I like the idea, but I don't think we should be modifying scripts/kconfig/kconfig.py to do this. Maybe a separate script similar to scripts/kconfig/hardenconfig.py?

We discussed this, but I actually think this might well belong in this file. It is, after all, our bridge to Kconfig, and this is pretty much an extension to Kconfig that we are adding.

tejlmand · 2021-03-22T21:04:46Z

I would assume that we need to rewrite those to something similar to

Let me take a second look.
Right now a warning is printed if symbol is enabled, but I did consider to only print the warning for enabled symbols if they were also visible.
I assume that would help in this particular case.

The disadvantage is of course that a symbol marked [EXPERIMENTAL] will not be printed if it is not visible, but I don't know if we have any such cases where an experimental symbol can be enable but not shown, where we would like the warning.

Main point: we need to consider best user experience on this.

Would it be possible to make a similar construct that fails the configuration, as opposed to printing a warning, if an experimental feature is selected?

It would, but then you should take this issue into consideration #9573
meaning that failing the configuration currently prevents the user from entering menuconfig to fix the reason.

tejlmand · 2021-03-22T21:12:11Z

@rugeGerritsen you probably need to take a look at that Kconfig snippet anyway, cause if BT_LL_SW_SPLIT=n you'll see this is menuconfig today, which I assume is not your intention:

nashif · 2021-03-22T21:39:53Z

we could revert back to the original experimental support in Kconfig where each experimental Kconfig would select EXPERIMENTAL and then just print something out if any experimental features are enabled....

tejlmand · 2021-03-22T21:53:16Z

we could revert back to the original experimental support in Kconfig where each experimental Kconfig would select EXPERIMENTAL and then just print something out if any experimental features are enabled....

if we did have something like this back in time, then it might be worth knowing why it was removed.
Did users forget to make a select EXPERIMENTAL or was it insufficient to just print experimental in use, and not know which feature that was the root cause ?

With this PR, users would be informed about the symbol

warning: Experimental symbol THREAD_MONITOR is enabled.

This adds two new Kconfig settings. The first setting `EXPERIMENTAL` which is a promptless symbol. This symbol must be selected by any setting which is used to enable an experimental feature. The second setting is `WARN_EXPERIMENTAL` which is a user controlled setting that configures the build system to print a warning when experimental features are enabled. Signed-off-by: Torsten Rasmussen <[email protected]>

stale

nashif

need to see some documentation about this and how this can be used. We do not want people depending on this kconfig and this is not documented anywhere, this can have negative results.
I remember seeing a change that had all experimental features converted to use this, we need to have at least few places use this, consider it a test and a reference to be added in the docs.

carlescufi · 2021-10-21T15:09:02Z

2. I remember seeing a change that had all experimental features converted to use this, we need to have at least few places use this, consider it a test and a reference to be added in the docs.

Do you mean this one @nashif ? #39473

With the introduction of `EXPERIMENTAL` and `WARN_EXPERIMENTAL` in Zephyr all subsys/bluetooth and drivers/bluetooth/hci settings having `[EXPERIMENTAL]` in their prompt has has been updated to include `select EXPERIMENTAL` so that developers can enable warnings when experimental features are enabled. Signed-off-by: Torsten Rasmussen <[email protected]>

With the introduction of `EXPERIMENTAL` and `WARN_EXPERIMENTAL` in Zephyr all subsys/canbus, subsys/net/l2/canbus, and drivers/can settings having `[EXPERIMENTAL]` in their prompt has has been updated to include `select EXPERIMENTAL` so that developers can enable warnings when experimental features are enabled. Signed-off-by: Torsten Rasmussen <[email protected]>

This commit adds description of the `WARN_EXPERIMENTAL` Kconfig setting to the application development guide. It describes the use of `CONFIG_WARN_EXPERIMENTAL` along with an example of the warning output when experimental features are enabled. Signed-off-by: Torsten Rasmussen <[email protected]>

tejlmand · 2021-10-22T07:32:52Z

1. need to see some documentation about this and how this can be used. We do not want people depending on this kconfig and this is not documented anywhere, this can have negative results.

Done, see commit: 05c22eb

2. I remember seeing a change that had all experimental features converted to use this, we need to have at least few places use this, consider it a test and a reference to be added in the docs.

Done, I have taken bluetooth and canbus that uses select EXPERIMENTAL from #39473 into this PR.
Still waiting for confirmation on the remaining settings marked EXPERIMENTAL that they are still considered at an experimental state.

nashif · 2021-10-22T10:49:02Z

Do you mean this one @nashif ? #39473

ah, yes. sorry :-)

tejlmand requested review from nashif and carlescufi March 22, 2021 13:41

tejlmand requested a review from ulfalizer as a code owner March 22, 2021 13:41

github-actions bot added the area: Kconfig label Mar 22, 2021

galak previously requested changes Mar 22, 2021

View reviewed changes

tejlmand force-pushed the warning_experimental branch from fd94886 to b2e34ff Compare March 22, 2021 14:14

tejlmand force-pushed the warning_experimental branch from b2e34ff to 865598a Compare March 22, 2021 21:55

tejlmand requested review from anangl, andyross, aunsbjerg, ceolin, cvinayak, dcpleung, ioannisg, jhedberg, joerchan, jukkar, kruithofa, lemrey, MaureenHelm, mbolivar-nordic and microbuilder as code owners March 22, 2021 21:55

tejlmand force-pushed the warning_experimental branch from 6de6c22 to 95eba76 Compare October 15, 2021 14:10

rugeGerritsen approved these changes Oct 15, 2021

View reviewed changes

tejlmand force-pushed the warning_experimental branch from 95eba76 to 8807006 Compare October 18, 2021 11:41

dcpleung approved these changes Oct 18, 2021

View reviewed changes

tejlmand mentioned this pull request Oct 19, 2021

samples: use kconfig to include child images on other core on nrf53 nrfconnect/sdk-nrf#5052

Merged

nashif requested changes Oct 21, 2021

View reviewed changes

tejlmand added 3 commits October 22, 2021 08:38

tejlmand requested review from alexanderwachter, asbjornsabo and Thalley as code owners October 22, 2021 07:29

github-actions bot added area: Bluetooth area: Bluetooth Mesh area: Bluetooth Controller area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth Audio area: CAN area: Documentation area: Networking labels Oct 22, 2021

joerchan requested a review from alwa-nordic October 22, 2021 07:46

carlescufi approved these changes Oct 22, 2021

View reviewed changes

tejlmand removed the request for review from ulfalizer October 22, 2021 09:02

nashif approved these changes Oct 22, 2021

View reviewed changes

carlescufi merged commit f17630b into zephyrproject-rtos:main Oct 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kconfig: add support for warnings when using experimental features #33566

kconfig: add support for warnings when using experimental features #33566

tejlmand commented Mar 22, 2021

galak left a comment

tejlmand commented Mar 22, 2021

tejlmand commented Mar 22, 2021

rugeGerritsen commented Mar 22, 2021

carlescufi commented Mar 22, 2021

tejlmand commented Mar 22, 2021

tejlmand commented Mar 22, 2021

nashif commented Mar 22, 2021

tejlmand commented Mar 22, 2021

nashif left a comment

carlescufi commented Oct 21, 2021

tejlmand commented Oct 22, 2021

nashif commented Oct 22, 2021

kconfig: add support for warnings when using experimental features #33566

kconfig: add support for warnings when using experimental features #33566

Conversation

tejlmand commented Mar 22, 2021

galak left a comment

Choose a reason for hiding this comment

tejlmand commented Mar 22, 2021

tejlmand commented Mar 22, 2021

rugeGerritsen commented Mar 22, 2021

carlescufi commented Mar 22, 2021

tejlmand commented Mar 22, 2021

tejlmand commented Mar 22, 2021

nashif commented Mar 22, 2021

tejlmand commented Mar 22, 2021

nashif left a comment

Choose a reason for hiding this comment

carlescufi commented Oct 21, 2021

tejlmand commented Oct 22, 2021

nashif commented Oct 22, 2021