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

doc: kconfig: Explain why zephyr/.config assigns promptless symbols #22022

Merged

Conversation

ulfalizer
Copy link
Collaborator

I think a lot of the confusion with promptless symbols being assigned
might come from opening zephyr/.config, seeing that it assigns a bunch
of promptless symbols, and assuming that Kconfig must respect those
assignments, even though it doesn't.

Explain why zephyr/.config assigns promptless symbols (it's because it
doubles as configuration output). That should clarify things a bit.

Also mention what "invisible" means for symbols early on in the page.

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 17, 2020

@ulfalizer
Copy link
Collaborator Author

https://builds.zephyrproject.org/zephyrproject-rtos/zephyr/22022/docs/ doesn't work any more?

@dbkinder , help?

Think David is off Zephyr. Maybe @nashif knows.

In the meantime:

Skärmbild från 2020-01-17 20-47-35

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 17, 2020

Think David is off Zephyr.

I just got confirmation of that. But this is not Zephyr, it's Kconfiglib ;-)

n the meantime:

I'll read the source, thanks! The convenience of https://builds.zephyrproject.org/zephyrproject-rtos/zephyr/22022/docs/ was amazing but I'll survive without it.

Copy link
Collaborator

@marc-hb marc-hb 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 doing this! Really appreciated.

Shouldn't something like this be submitted to kconfig[lib] directly rather than Zephyr?


2. it holds configuration output. :file:`zephyr/.config` is parsed by the CMake
files to let them query configuration settings, for example.

Copy link
Collaborator

@marc-hb marc-hb Jan 17, 2020

Choose a reason for hiding this comment

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

How about:

It helps to realize that .config holds two different kinds of symbols that unfortunately do not appear different:

  1. It holds read-write symbols saved by Kconfig, and
  2. It holds write-only symbols that Kconfig never reads.

Really not a fan of the word "invisible" because it's invisible only in the interactive interfaces that some people do not use. In fact this entire clarification is needed only because some "non-interactive" users see these "invisible" symbols! Interactive users don't experience any issue in the first place and don't need this paragraph.

"Invisible" is like telling non-interactive users: "these are not the symbols you're looking for"... :-)

Besides "write-only" I can also think of "immutable" or simply "internal" as in "internal, do not touch". "unconfigurable" was also mentioned. This new section could use more than one term.

Copy link
Collaborator Author

@ulfalizer ulfalizer Jan 17, 2020

Choose a reason for hiding this comment

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

I prefer visible and invisible, because it's the terminology you'll see in other places that talk about Kconfig. There's also symbol visibility.

I added another explanation of what visible means at the top of the page in this commit btw. It mentions that promptless symbols are always invisible, and that symbols with prompts can be invisible too.

Really not a fan of the word "invisible" because it's invisible only in the interactive interfaces that some people do not use.

Yeah... I'm trying to encourage more people to use it. I know I keep nagging about it, but if people work with Kconfig without ever using the configuration interfaces, they're guaranteed to misunderstand and mess up a lot of things.

menuconfig should be front and center of people's minds, because it shows how Kconfig thinks. Enabling/disabling a symbol in a configuration file is just like toggling it on/off in menuconfig. If the symbol isn't visible, you can't toggle it on/off.

It's kinda inverted now, where people think menuconfig is this obscure thing that can safely be ignored most of the time, even though it's enormously important to get an understanding of how Kconfig works. :/

menuconfig is really important for checking changes too.

Copy link
Collaborator

@marc-hb marc-hb Jan 18, 2020

Choose a reason for hiding this comment

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

Yes, having used menuconfig is necessary to understand how Kconfig works. You can even stop at menuconfig if you're configuring your single, hobby system at home. And then you don't even know that some symbols are invisible because... you don't see them.

On the other hand, menuconfig is nowhere near enough to "scale up" to managing fine grained policies across various hardware, software versions and concurrent developers. For that you need text config files in source control as usual:
https://www.yoctoproject.org/docs/2.5/kernel-dev/kernel-dev.html#creating-config-fragments
https://www.chromium.org/chromium-os/how-tos-and-troubleshooting/kernel-configuration
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/271688
etc.

The serious usability issue with text files that this documentation PR is trying to mitigate is that "visible" and "invisible"... look exactly the same! Hilarious.

I prefer visible and invisible, because it's the terminology you'll see in other places that talk about Kconfig. There's also symbol visibility.

A good way to deal with terminology that bad in documentation is to treat as if it were "deprecated": mentioned for reference at the start of the section and then replaced in the rest.

"We've always done it this way"

PS: "available" is yet another, better alternative to "visible". There are many because the bar is so low.

Copy link
Collaborator Author

@ulfalizer ulfalizer Jan 18, 2020

Choose a reason for hiding this comment

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

Just saying that you should be running menuconfig after every non-trivial change you make, to check things. I'd probably mess up a lot of things if I didn't.

No such thing as something being broken just in menuconfig by the way, which I think is a common misunderstanding (in general, not saying you misunderstood it). It's just that issues are much easier to spot there.

I think visible and invisible is good terminology. Shows up in lots of other places too, so inventing our own terms might make things confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See "hidden" a lot too. I could sneak that in there if you want.

configuration output. Kconfig itself ignores assignments to invisible symbols
when calculating symbol values.

.. note::
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO, another very useful note would tell that olddefconfig sanitizes the .config file, by (among others) re-issuing internal symbols with their correct values. In my project I just added a sanity check that runs olddefconfig, compares with .config and stops the build if any difference. It was very easy.

Copy link
Collaborator Author

@ulfalizer ulfalizer Jan 17, 2020

Choose a reason for hiding this comment

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

olddefconfig isn't used by Zephyr, so wonder if it'd be confusing. 🤔

Copy link
Collaborator

@marc-hb marc-hb Jan 18, 2020

Choose a reason for hiding this comment

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

Why is olddefconfig removed by Zephyr? It doesn't seem like it would hurt...

I haven't look at it in detail, but again I feel like most of the extremely useful Kconfig documentation hosted by Zephyr is not specific to Zephyr and not getting the larger https://github.com/ulfalizer/Kconfiglib visibility (!) it deserves.

BTW I just quickly hacked a new genconfig.py --sane option, it was much easier than I thought it would be. In case anyone's interested.

This is based on kconfiglib HEAD faa1d2199801 + another, unrelated args.dot_config addition that should be easy to get rid of.

--- a/Kconfiglib-subrepo/genconfig.py
+++ b/Kconfiglib-subrepo/genconfig.py
@@ -68,6 +68,13 @@ deps/auto.conf instead. --config-out is meant for cases where incremental build
 information isn't needed.
 """)
 
+    parser.add_argument(
+        "--sane", action='store_true',
+        help="""
+Requires a --config-out parameter. Compares .config input and config-out output
+and fail if they differ.
+""") 
+
     parser.add_argument(
         "--sync-deps",
         metavar="OUTPUT_DIR",
@@ -114,11 +121,17 @@ only supported for backwards compatibility).
     kconf = kconfiglib.Kconfig(args.kconfig_filename)
     print(kconf.load_config(args.dot_config))
 
-    kconf.write_autoconf(args.header_path)
-
     if args.config_out is not None:
         kconf.write_config(args.config_out, save_old=False)
 
+    if args.sane:
+        sanitized = kconf._config_contents()
+        # _open_config() looks in several directories
+        orig = kconf._open_config(args.dot_config).read(len(sanitized)+1)
+        if not orig == sanitized:
+            sys.exit("{} and {} are different, try olddefconfig".format(
+                args.dot_config, args.config_out)
+            )
+
+    kconf.write_autoconf(args.header_path)
+
     if args.sync_deps is not None:
         kconf.sync_deps(args.sync_deps)
 
diff --git a/Kconfiglib-subrepo/kconfiglib.py b/Kconfiglib-subrepo/kconfiglib.py
index 37ba501afcb2..6a9680f487c1 100644
--- a/Kconfiglib-subrepo/kconfiglib.py
+++ b/Kconfiglib-subrepo/kconfiglib.py
@@ -1415,8 +1416,10 @@ class Kconfig(object):
 
         return "".join(chunks)
 
+    HEADER = "# Generated by Kconfiglib (https://github.com/ulfalizer/Kconfiglib)\n"
+
     def write_config(self, filename=None,
-                     header="# Generated by Kconfiglib (https://github.com/ulfalizer/Kconfiglib)\n",
+                     header=HEADER,
                      save_old=True, verbose=None):
         r"""
         Writes out symbol values in the .config format. The format matches the
@@ -1491,7 +1494,7 @@ class Kconfig(object):
 
         return "Configuration saved to '{}'".format(filename)
 
-    def _config_contents(self, header):
+    def _config_contents(self, header=HEADER):
         # write_config() helper. Returns the contents to write as a string,
         # with 'header' at the beginning.
         #

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why is olddefconfig removed by Zephyr? It doesn't seem like it would hurt...

Yeah... could maybe import it along with some other scripts, though things like all{yes,no}config.py is probably broken for Zephyr.

Note that the header will still be "as-if" you had run olddefconfig, because Kconfig never writes output that doesn't respect dependencies. It can be handy if you inspect .config manually though.

Re. --sane, I think many people rely on not having to run olddefconfig, and update the configuration file via e.g. menuconfig instead. Might be a bit too special-cased (though I know it wasn't meant as a PR :).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Re. --sane, I think many people rely on not having to run olddefconfig, and update the configuration file via e.g. menuconfig instead.

Two different tools for two different jobs. menuconfig is best for learning and exploration. olddefconfig is when you know exactly the one option you want to change and quickly [git] diff and see all the consequences. In that case it's much faster to use your favorite editor.

olddefconfig can also be scripted, menuconfig obviously not https://www.chromium.org/chromium-os/how-tos-and-troubleshooting/kernel-configuration#TOC-Generate-a-new-splitconfig-after-adding-new-Kconfig-options

Command line or web browser? Both of course.

(though I know it wasn't meant as a PR :).

Indeed it wasn't, however the more my build system uses it and the more I love it.

if it makes sense for the user to change its value.
the symbol's value in the ``menuconfig`` or ``guiconfig`` interface (see
:ref:`menuconfig`), or by manually editing configuration files. Therefore, only
put a prompt on a symbol if it makes sense for the user to change its value.
Copy link
Collaborator

@marc-hb marc-hb Jan 17, 2020

Choose a reason for hiding this comment

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

The second sentence can be dropped. This one is much more useful:

Conversely, a symbol without a prompt can never be changed by the user, not even by manually editing the configuration file.

This is a dead simple, binary situation so it's more useful to describe it as such very plainly and very early rather than getting into what "makes sense" or not. There are countless things that make sense to some people and not to others, this shouldn't be one of them.

Copy link
Collaborator Author

@ulfalizer ulfalizer Jan 17, 2020

Choose a reason for hiding this comment

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

I've seen lots and lots of symbols in Zephyr and elsewhere that had prompts even though changing them didn't make sense, so I think it's helpful to put it like this, even if it might seem "obvious" in retrospect.

I think it's more subtle than it might seem, especially when starting out with Kconfig.

(Getting back to menuconfig, it might be because people didn't realize they were creating a knob for people to change the value of the symbol there.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, then I would just add my sentence and not remove anything.

Copy link
Collaborator Author

@ulfalizer ulfalizer Jan 20, 2020

Choose a reason for hiding this comment

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

Added it. Check that it looks alright in context.

@ulfalizer
Copy link
Collaborator Author

Shouldn't something like this be submitted to kconfig[lib] directly rather than Zephyr?

The Kconfiglib README links to the tips & tricks page in a bunch of place. It's documented in the library documentation too, though that's probably not the best intro, because it has all the nitty-gritty details.

:ref:`menuconfig`), or by manually editing configuration files. Therefore, only
put a prompt on a symbol if it makes sense for the user to change its value.

Symbols without prompts are also called *invisible* symbols, because they don't
Copy link
Member

Choose a reason for hiding this comment

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

'hidden', is a better term, right? At least, this is what we have been using

Copy link
Collaborator Author

@ulfalizer ulfalizer Jan 20, 2020

Choose a reason for hiding this comment

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

Yeah, looking around a bit, it's more common. Now uses hidden to refer to promptless symbols, and just mentions invisible when explaining that symbols with prompts can be invisible too.

I think a lot of the confusion with promptless symbols being assigned
might come from opening zephyr/.config, seeing that it assigns a bunch
of promptless symbols, and assuming that Kconfig must respect those
assignments, even though it doesn't.

Explain why zephyr/.config assigns promptless symbols (it's because it
doubles as configuration output). That should clarify things a bit.

Also mention what "invisible" means for symbols early on in the page.

Signed-off-by: Ulf Magnusson <[email protected]>
@carlescufi carlescufi merged commit 31822c6 into zephyrproject-rtos:master Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants