[kconfig] Allow user-input when new options are detected

[SebastianBoe]

When Kbuild detected that new kconfig options were introduced to an existing project it would prompt the user for what he wanted to do with these new options.

CMake does not support this, new options will result in Kconfig aborting.

sebo@mach:~/zephyr/tests/crypto/mbedtls/b$ make rebuild_cache
Running CMake to regenerate build system...
-- Selected BOARD nrf52_pca10040
Zephyr version: 1.10.99
-- Generating zephyr/include/generated/autoconf.h
*
* Restart config...
*
*
* mbedTLS Support
*
mbedTLS Support (MBEDTLS) [Y/n/?] y
  Enable mbedTLS integrated sources (MBEDTLS_BUILTIN) [Y/n/?] y
    mbed TLS configuration file (MBEDTLS_CFG_FILE) [config-threadnet.h] config-threadnet.h
    mbed TLS debug activation (MBEDTLS_DEBUG) [N/y/?] n
    Compile internal self test functions (MBEDTLS_TEST) [Y/n/?] y
  Enable mbedTLS external library (MBEDTLS_LIBRARY) [N/y/?] n
  Enable global heap for mbed TLS (MBEDTLS_ENABLE_HEAP) [N/y/?] n
  Link 'app' with MBEDTLS (APP_LINK_WITH_MBEDTLS) [Y/n/?] (NEW) aborted!

Console input/output is redirected. Run 'make oldconfig' to update configuration.

-- Generating zephyr/include/generated/generated_dts_board.h
-- Configuring done
-- Generating done
-- Build files have been written to: /home/sebo/zephyr/tests/crypto/mbedtls/b

CMake needs to deal with this use-case in a more graceful way. It is not clear how though,
the straightforward solution would be to mimic Kbuild and accept user-input when new options
are detected. But it is not clear if Kbuild's behaviour is the desired behaviour.

[carlescufi]

This is no longer the case with kconfig.py. In fact now one would get an empty default value like:

NEW_OPTION=

[carlescufi]

Right now our script ignores the current .config and generates a new one every time
@ulfalizer is there a way for Kconfiglib to parse the current .config and ask the user for input when new options without a default value are defined?

[ulfalizer]

@carlescufi
After loading any .config files that should be loaded, oldconfig-like behavior could be implemented e.g. as follows. The way oldconfig works is that it prompts the user for the value of each visible symbol that wasn't set in the .config file. Those are the "new" symbols.

There's some usability issues here in that you can't give a string value the empty string as the value unless that's already the default, but hopefully it should get the idea across at least. I might add a oldconfig.py example later that tightens things up a bit. :)

I'm thinking of allowing "n", "m", "y" as aliases for 0, 1, 2 in set_value() for bool and tristate symbols. All that conversion is a bit annoying...

diff --git a/scripts/kconfig/kconfig.py b/scripts/kconfig/kconfig.py
index de8316556..4a020c5a1 100755
--- a/scripts/kconfig/kconfig.py
+++ b/scripts/kconfig/kconfig.py
@@ -1,6 +1,7 @@
 #!/usr/bin/env python3
 # Modified from: https://github.com/ulfalizer/Kconfiglib/blob/master/examples/merge_config.py
-from kconfiglib import Kconfig, Symbol, BOOL, STRING, TRISTATE, TRI_TO_STR
+from kconfiglib import Kconfig, Symbol, BOOL, STRING, TRISTATE, TRI_TO_STR, \
+                       STR_TO_STRI
 import sys
 
 if len(sys.argv) < 5:
@@ -25,6 +26,40 @@ for config in sys.argv[5:]:
 for config in sys.argv[4:]:
     kconf.load_config(config, replace=False)
 
+# Look for new modifiable symbols and prompt the user for a value for them,
+# oldconfig-style
+for sym in kconf.defined_syms:
+    if sym.user_value is None and sym.visibility:
+        if len(sym.assignable) == 1:
+            # Skip bool/tristate symbols that are already locked to a
+            # particular value through a select
+            continue
+
+        # Loop until the user enters a valid value or a blank string (for the
+        # default value)
+        while True:
+            prompt = "Value for new symbol {} (".format(sym.name)
+            if sym.type in (BOOL, TRISTATE):
+                prompt += "available: {}, ".format(
+                    ", ".join([TRI_TO_STR[val] for val in sym.assignable]))
+            prompt += "default '{}'): ".format(sym.str_value)
+
+            # If the user enters a blank value, use the default value
+            val_str = input(prompt) or sym.str_value
+
+            if sym.type in (BOOL, TRISTATE):
+                if val_str in STR_TO_TRI and \
+                   sym.set_value(STR_TO_TRI[val_str]):
+                    # Valid value input
+                    break
+            else:
+                if sym.set_value(val_str):
+                    # Valid value input
+                    break
+
+            # Invalid value input, loop
+            print("Invalid value -- try again")
+
 # Write the merged configuration
 kconf.write_config(sys.argv[2])
 

[carlescufi]

@ulfalizer thanks for the input and code.

The way oldconfig works is that it prompts the user for the value of each visible symbol that wasn't set in the .config file.

Understood, but for that kconfig.py should actually load an existing .config instead of simply overwriting it shouldn't it? The current implementation simply calls kconf.write_config(<path to .config>), which does not load the existing .config if any.
I wonder if we need a new line:

kconf.load_config(sys.argv[2], replace=False)

first to load that .config. Right now we only load sys.argv[4:], and the path to .config is argument 2.

[ulfalizer]

@carlescufi To check for and load an existing .config if it exists instead of the fragments, you could do something like

if os.path.exists(".config"):
    kconf.load_config(".config")
else:
    # Load fragments...

Maybe I'm misunderstanding the model you have in mind though.

[carlescufi]

@ulfalizer No, I don't think you're misunderstanding, it's just that before switching to Python we seem to have been using the oldconfig model which requires loading the .config into memory. During the transition this was lost and the existing .config if any is completely ignored and overwritten. Your snippet is what I meant was missing to complete the puzzle, which is actually 2 different issues:

1. Load and use .config if it exists
2. Prompt user for new values if no default exists

[ulfalizer]

@carlescufi Yep, with that change, it should respect an existing .config. Since the script started out as a merge_config.sh-alike, I had assumed it was fine to ignore it. :)

[carlescufi]

@ulfalizer I tried running your patch on a completely clean build but I got asked about values that should default to "n" automatically (no default implies "n" AFAIK), such as this which should simply default to "n".
EDIT: Note that this was your patch alone, with no loading of the .config added.

[ulfalizer]

@carlescufi That matches the make oldconfig behavior. Usually, you'd run it when you already have a .config that is somewhat filled-in from earlier, so that you only get prompted for a few new visible symbols (and can [Enter] your way through them quickly if they should get the default value).

[ulfalizer]

There's also olddefconfig (same as oldnoconfig), which just fills in new symbols with their default value. The old version of kconfig.py already implicitly implements that one though.

[carlescufi]

There's also olddefconfig (same as oldnoconfig), which just fills in new symbols with their default value.

Right, but what I see now is that if it's say an int with no default value called FOO then my .config ends up containing

FOO=

instead of prompting the user. So I guess what we would need is something aking to olddefconfig but with a prompt for values that cannot default to anything reasonable?

EDIT: in fact I just checked and before switching to Python we used to invoke conf with olddefconfig:
d92769b#diff-3a17f68c7317a0565fa6d10ecdd67c05L127

[ulfalizer]

Hmm... that shouldn't happen. Here's a test Kconfig:

config INT
	int "int"

When I run the script against that with an empty .config, I get the following:

Value for new symbol INT (default ''): 
warning: the value '' is invalid for INT (defined at Oldkconfig:1), which has type int. Assignment ignored.
Invalid value -- try again
Value for new symbol INT (default ''): foo
warning: the value 'foo' is invalid for INT (defined at Oldkconfig:1), which has type int. Assignment ignored.
Invalid value -- try again
Value for new symbol INT (default ''): 123

The final .config has this:

CONFIG_INT=123

int and hex values do default to the empty string (unless you give your own default), but you won't be able to assign that as a user value.

[carlescufi]

@ulfalizer I meant it happens in the current vanilla Zephyr tree without the snippet here. Maybe it's not quite clear, but what I'm trying to replicate here is the old behavior where:

1. If there's no .config then all values take defaults
2. If a .config exists then the user is prompted for any new Kconfig options that were not present in the existing .config

I assume the description above fits our previous conf --olddefconfig which used to prompt us like described. I might be wrong though, I will try to replicate it reverting back to to using conf and comparing

[ulfalizer]

Getting pretty deep in Kconfig arcana, but you can end up with CONFIG_INT= still in a .config file if you don't assign a user value to INT and simply let it get its default value. That assignment will simply be ignored when loading the .config file back in though.

When writing out a new .config, you might still get CONFIG_INT= in it again, from the default value, even though the user value was ignored.

Kconfiglib works exactly like the C implementation when it comes to these things. Need that to generate the same .config files for all those corner cases.

[ulfalizer]

@carlescufi This should work in order to get that behavior:

if os.path.exists(".config"):
    kconf.load_config(".config")
    # Do the new oldconfig stuff
    ...
else:
    # Load fragments... (this implicitly works like olddefconfig due to normal Kconfig semantics)

make olddefconfig never prompts the user for values btw. It's like if you ran make oldconfig and held down the [Enter] key to assign each new symbol its default.

[ulfalizer]

alldefconfig should've been olddefconfig -- sorry about that typo. They sound too similar...

[ulfalizer]

To clarify, when writing out a .config file with write_config(), all the visible symbols that didn't get assigned a user value (from a .config) automatically get their default value. This is required in order to generate a valid .config file, and is also what happens in e.g. the menuconfig interface when you run it without a .config.

So simply doing load_config() + write_config() automatically implements olddefconfig. oldconfig just adds some prompting on top of that, for visible symbols that weren't mentioned in the .config file.

[ulfalizer]

@carlescufi And yeah, running the oldconfig logic in the fragment merging case doesn't make much sense of course, because the fragments are minimal and will leave lots of stuff unspecified. Braino in my initial patch. I need my coffee fix. :)

[ulfalizer]

Hello,

I spent some time implementing a more robust oldconfig script that also handles choices properly (and looks more similar to the standard make oldconfig): https://github.com/ulfalizer/Kconfiglib/blob/master/examples/oldconfig.py

You might want to drop the script alongside kconfig.py I guess, since it's pretty large with all the checking and stuff. To do the oldconfig, you could then do:

from oldconfig import do_oldconfig
...
do_oldconfig(kconf)

I added a small bonus feature as well: Inputting '??' as the value displays the Kconfig help text of the item. Don't think that's a value that's very likely to come up in practice...

[carlescufi]

@ulfalizer wow that is a lot of info to digest and try :) I will do that and submit a PR to Zephyr with all this in mind. I think we had a strange hybrid of olddefconfig and oldconfig before switching to Kconfiglib and people had gotten used to that. I will however see what makes sense in the current context. Thank you again for all the comments and code!

[ulfalizer]

I just made set_value() accept "n"/"m"/"y" as aliases for 0/1/2 btw: ulfalizer/Kconfiglib@f66cd71

I also simplified oldconfig.py by making use of that, so you will need to add that Kconfiglib commit for the latest version of oldconfig.py to work.

No other example-breaking updates planned for now. :)

On an unrelated note, I updated merge_config.py so that it prints the symbol location(s) in the warning about the assigned value not matching the final value.

[carlescufi]

@ulfalizer thanks! Now that we have an "official" fork under the zephyrproject-rtos user it makes it easier for me to rebase. Will do that and then continue with related oldconfig work.

[carlescufi]

Closing this since in Zephyr we have settled on considering .config to be a build system artifact. Please reopen or open a new issue if required.