Deprecate # CONFIG_ is not set

[SebastianBoe]

Some parts of Kconfig treat lines like

# CONFIG_FOO is not set

to have meaning, instead of just being commented out lines as one would expect.
All parts of kconfig that attribute meaning to this construct should be removed
and the use-case that it is enabling should be supported with a construct
that does not look like a comment.
Kconfig should generate "=n" instead of "is not set" whenever possible.

In short, don't unnecessarily inherit the Linux kernel's cruft.

Source:
http://lists.openembedded.org/pipermail/openembedded-core/2016-May/120914.html

[ulfalizer]

This behavior actually makes some sense once you know that .config files use Makefile syntax (which means they can be included to check configuration values).

It's motivated by two things:

1. You want n to correspond to unset, because that's easy to test for in make.

2. You still need to remember n user values, e.g. if a symbol that's default y is set to n.

The comment will be ignored by make, but Kconfig will will see it. That solves both problems.

Less useful if you're not using make, but there's some sense behind it at least.

[ulfalizer]

Closing for now. It won't change upstream in Kconfiglib, and I think maintaining a patch for it might be overkill.

Feel free to reopen.

[SebastianBoe]

It might be okay to close. But I would like to understand this better before deciding.

You want n to correspond to unset, because that's easy to test for in make.

Makes sense, CONFIG_F=n is easy for both humans, make, and Kconfig to understand.

You still need to remember n user values, e.g. if a symbol that's default y is set to n.

Can you elaborate on what this means? Does Kconfig generate "=n" when the user has input through a GUI, but generate "# is not set" when the negative is due to Kconfig evaluation?

Does Kconfig treat "=n" and "# is not set" differently?

[SebastianBoe]

Said differently, what kind of features or use-cases would we lose if we started to use "=n" everywhere.

[ulfalizer]

@SebastianBoe
The problem is that Make treats CONFIG_FOO=n and # CONFIG_FOO is not set differently. You can't test for the first one with ifdef CONFIG_FOO, and that's a nice thing to be able to do.

Kconfig never generates CONFIG_FOO=n, for that reason. Changing it would break the Linux kernel, so it would have to be a separate patch.

[ulfalizer]

Consider this for example:

config FOO
    bool "foo"
    default y

When FOO is set to n, you want ifdef CONFIG_FOO to be false in Make, so nothing besides a Make comment can be generated for FOO.

At the same time, Kconfig needs to remember that FOO was set to n (especially important in this case, as it defaults to y). To do that without being forced to set it in the .config file (which would break the handy ifdef Make test, as .config files are really Makefiles), it generates a # CONFIG_FOO is not set comment instead.

[ulfalizer]

It's still fine to manually put a CONFIG_FOO=n into a .config file though, if you want to.

[SebastianBoe]

Thank you, I understand the situation.

For Zephyr, I believe the cleanest solution is to have kconfiglib support a dialect of Kconfig that produced "=n" instead of "is not set". Then users will not be confused about comments having semantics and users will not be confused about there being 2 different syntaxes for the same thing.

Having a configuration system that behaves as-expected, and therefore requires as little documentation as possible is extremely important for adoption.

Comments having semantics, and multiple syntaxes for the same (in the Zephyr context) thing is not behaving-as-expected.

[ulfalizer]

@SebastianBoe
One drawback would be drifting further away from kconfig-language.txt, but maybe that's fine if we'll need separate documentation later anyway.

It's a trivial change on the Kconfiglib side at least. Think this should be enough:

diff --git a/kconfiglib.py b/kconfiglib.py
index 96c7d37..ac28131 100644
--- a/kconfiglib.py
+++ b/kconfiglib.py
@@ -3023,10 +3023,7 @@ class Symbol(object):
 
         if self.orig_type in (BOOL, TRISTATE):
             return "{}{}={}\n" \
-                   .format(self.kconfig.config_prefix, self.name, val) \
-                   if val != "n" else \
-                   "# {}{} is not set\n" \
-                   .format(self.kconfig.config_prefix, self.name)
+                   .format(self.kconfig.config_prefix, self.name, val)
 
         if self.orig_type in (INT, HEX):
             return "{}{}={}\n" \

It'd still handle # CONFIG_FOO is not set lines after that change. Just wouldn't generate them.

[SebastianBoe]

One drawback would be drifting further away from kconfig-language.txt

Yes. This is the main drawback. But IMHO it is worth it. It should be noted that all we are changing is how Kconfig integrates with it's build system. For KBuild it makes sense to generate "is not set", whereas for CMake it makes more sense to generate "=n".

This dialect change will not affect in any way any documentation about Kconfig ("is not set" is not actually documented), nor will it affect how input is given to Kconfig. All it changes is the format used to export to CMake.

So it is a much less invasive change than the "prefer later defaults" patch for instance.

[ulfalizer]

Heh, never noticed it wasn't documented. Always amazes me how people can feel comfortable adding stuff like that without thinking it might be a good idea to at least hint at why it's there.

Yeah, pretty minimal fallout in that case, and the patch should be simple to maintain, so I'm fine with it.

[SebastianBoe]

Hi, @ulfalizer , there is a bug in extensions.cmake:import_kconfig that would be resolved if this "enhancement" was implemented.

Would it be possible to get this prioritized?

Also, it might be easier to support this through a feature-flag, than an out-of-tree patch.

[ulfalizer]

@SebastianBoe
How hard would it be to implement this on the CMake side? I'm kinda reluctant to complicate Kconfiglib for a one-off like this, because in reality it often means you have to support stuff forever. It's nice to not have any custom patches floating around either.

The reason n gets written out as # X is not set is that .config files are valid Makefiles. It makes it so that n-valued symbols don't get defined at all in Make, which is handy when testing them there.

To implement it on the CMake side, you'd just have to treat # CONFIG_FOO is not set as a synonym for CONFIG_FOO=n. Looks like CMake has some regex matching stuff that might be handy: https://cmake.org/cmake/help/v3.12/command/string.html

[ulfalizer]

If there's some way to get the values into scripts/kconfig/kconfig.py, you could do kconf.syms[sym_name].set_value(sym_value) from there as well.

[SebastianBoe]

The aforementioned bug can be fixed in a straightforward way.

This is more about preventing similar issues popping up in the future.

I agree about not maintaining a custom patch, which is why I would recommend maintaining a feature-flag / dialect argument kind-of-mechanism.

[SebastianBoe]

I'm hoping the complexity will not be significant, e.g. something like:

def emit_negative_assignment(symbol):
  if args.use_comment_style_false_assignment:
   return "# {} is not set".format(symbol)
  else:
    return "{}=n".format(symbol)

[ulfalizer]

If it comes up many times in the future, I might be more willing to add it. The reason Kconfiglib has stayed relatively readable (to me anyway, dunno how it reads to others) is that I've tried hard to avoid stuff like dialect flags and one-off features. It tends to add up and come back and bite you, from experience with Kconfiglib 1.

[SebastianBoe]

I have had to explain to many users that

# CONFIG_FOO is not set
is not a comment, but a valid way to set CONFIG_FOO=n.

I don't see any reason to believe that this will not continue to pop up and confuse users.

[ulfalizer]

It's explained in the application development primer at least (better than in Linux, where AFAICT no one bothered to document why Kconfig does that at all).

[SebastianBoe]

It is better to design systems to be self-explanatory so they do not need documentation.

We have the opportunity, in this particular case, to achieve this.

[ulfalizer]

Can't get rid of the standard behavior, and don't like the tradeoff in this case.

[carlescufi]

@ulfalizer given that we already maintain our own copy of Kconfiglib in Zephyr, what is the downside of having this change in our scripts/kconfig/ copy? That way we don't have to hack CMake, the users get a consistent view, and Kconfig upstream is not polluted

[ulfalizer]

It's another custom patch to maintain.

To be fair, it could be implemented in kconfig.py as well, because things are flexible enough that custom output formats can be implemented outside kconfiglib instead. That'd instead make kconfig.py harder to read though, and it's nice to have it as understandable as possible, because it's the entry point into the whole Kconfig side of things.

Changing the CMake script so that it understands the standard Kconfig output format might be the least hacky solution in this case.

[carlescufi]

@ulfalizer as @SebastianBoe mentions the issue is also one of support and simplicity. Could you come up with a quick patch for kconfig.py (even better, no need to keep a patch in Kconfiglib!) and then we can take a look with the code in front of us? Thanks!

[ulfalizer]

This seems to work. It's copy-pasting Kconfig.write_config(), trimming some stuff kconfig.py doesn't need, and changing how n-valued bool symbols get written.

Get a call to the internal _save_old() function there now, which is icky. It implements the .config.old stuff. Could expose it, but feels risky (often gotta keep stuff you expose around forever, with the same interface).

diff --git a/scripts/kconfig/kconfig.py b/scripts/kconfig/kconfig.py
index 6af3edd2cf..32b34059ae 100755
--- a/scripts/kconfig/kconfig.py
+++ b/scripts/kconfig/kconfig.py
@@ -5,7 +5,28 @@ import os
 import sys
 import textwrap
 
-from kconfiglib import Kconfig, BOOL, TRISTATE, TRI_TO_STR
+from kconfiglib import Kconfig, Symbol, expr_value, _save_old, BOOL, \
+                       TRISTATE, TRI_TO_STR, MENU, COMMENT
+
+
+def write_custom_config(kconf, filename):
+    _save_old(filename)
+
+    with open(filename, "w", encoding="utf-8") as f:
+        for node in kconf.node_iter(unique_syms=True):
+            item = node.item
+
+            if isinstance(node.item, Symbol):
+                if item.orig_type in (BOOL, TRISTATE) and not item.tri_value:
+                    f.write("{}{}=n\n".format(kconf.config_prefix, item.name))
+                else:
+                    f.write(item.config_string)
+
+            elif expr_value(node.dep) and \
+                 ((item is MENU and expr_value(node.visibility)) or
+                   item is COMMENT):
+
+                f.write("\n#\n# {}\n#\n".format(node.prompt[0]))
 
 
 # Warnings that won't be turned into errors (but that will still be printed),
@@ -89,7 +110,7 @@ def main():
                 100) + "\n")
 
     # Write the merged configuration and the C header
-    kconf.write_config(args.dotconfig)
+    write_custom_config(kconf, args.dotconfig)
     print("Configuration written to '{}'".format(args.dotconfig))
     kconf.write_autoconf(args.autoconf)

I'd prefer to make the change in CMake for now.

[SebastianBoe]

LGTM

[ulfalizer]

Looks not so good to me. Another problem with it is that menuconfig.py won't use the same format (hadn't thought of that). Any other tools we add that write configuration files wouldn't either.

Could you write a CMake version for comparison?

[SebastianBoe]

There is no "CMake version for comparison" because this is not about fixing a particular instance of the bug but about fixing the whole class of bugs that arises from users assuming that "# CONFIG_FOO is not set" is a comment.

[SebastianBoe]

Another problem with it is that menuconfig.py won't use the same format (hadn't thought of that). Any other tools we add that write configuration files wouldn't either.

Menuconfig, and any future tools, will have to also be updated.

[SebastianBoe]

@ulfalizer : Can you update menuconfig as well?

[nashif]

-1 from my on this enhancement request. if we go this path then we could as well stop saying that we are using kconfig. It is what it is, if you do not like XML, you do not go and change the semantics...

[SebastianBoe]

-1 from my on this enhancement request. if we go this path then we could as well stop saying that we are using kconfig. It is what it is, if you do not like XML, you do not go and change the semantics...

We are not changing any documented behaviour from Kconfig. We are just making an implementation-defined change.

According to Kconfig

FOO=n

disables an option.

There is nothing documented about what

# CONFIG_FOO is not set

means. Therefore choosing to generate FOO=n, instead of

# CONFIG_FOO is not set

does not diverge us from the language. Although it does diverge us from the accidental
undocumented behaviour of the Linux kernel's Kconfig implementation.

In other words, since "# CONFIG_FOO is not set" is not documented, it is reasonable to
treat it as a bug that the kernel happens to rely on. Or, to use XML as an analogy, a bug in
a particular implementation of XML. Not copying another XML implementation's bug is reasonable.

[nashif]

In other words, since "# CONFIG_FOO is not set" is not documented

Maybe the solution is just document it?

[SebastianBoe]

No, because that does not resolve this issue.

People don't read documentation. But people do
rely on standards when reading code.

I don't understand the push-back here. We write / maintain
a handful of lines of code, and users no longer have to
be confused by:

# this is a comment.
# CONFIG_FOO is not set
# But the above line is not a comment. It is equivalent to
# the below line. 
CONFIG_FOO=n

This is a no-brainer in trade-off between maintainability and usability to me.

[jameswalmsley]

I would strongly advise not to do this. E.g. our internal firmware build system relies on the fact that Zephyr uses real Kconfig.

So we integrate make, and cmake and kconfig that supports a lot of complex configuration. I'm sure there are other users who do this.

I agree the syntax isnt nice, but it has a logical rationale behind it.

Better to just document that more clearly.

If there should be a patch to kconfiglib, maybe have it output a comment header in the generated .config files to state the # CONFIG_ ... is not set explicitly disables the option.

[galak]

@jameswalmsley I doubt anything is going to happen on this front as both Ulf and Sebastian are no longer active contributors.

[galak]

Closing this issue as we don't plan on making these changes. Can re-open if someone feels like they need this and plan on working on it.