kconfiglib improvements

[sjg20]

Add a few things to deal with the Kconfig used by Zephyr

• use of environment vars/macros to obtain Kconfig files
• use of modules containing Kconfig files in other directories

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[sjg20]

Hi, please can someone look at this one?

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[carlescufi]

@sjg20 sorry, @tejlmand who is the maintainer, was away. I expect him to be able to take a look now.

[tejlmand]

I think I need a better understanding of the benefit of this PR.

Besides the introduced risk of folder name collisions, which would make it hard
for developers to trace down, then this makes it also makes it unnecessary hard
to know where to find a given file.

Today, if I see:

source "motion/Kconfig"

I know I should start looking directly in $(ZEPHYR_BASE)/motion/Kconfig.
With this PR, I basically don't know where to start looking.

Today, all modules can be referred using: $(ZEPHYR_<name>_MODULE_DIR)
This means that if I want to source motion/Kconfig from the ec module, I can do

source "$(ZEPHYR_EC_MODULE_DIR)/motion/Kconfig"

No risk of naming collisions.
Clear to all developers from where motion/Kconfig is sourced.
So I really need very good reasons why the existing feature is not sufficient.

For the second commit, you write:

When parsing Kconfig which include macros it is currently necessary to
provide a value for all macros in advance.

Before fixing something, could you please open an issue describing what is wrong ?

The only way I can trigger this error, is by creating configs that are purely
defined using macros, like this:

config $(EXPANDS_TO_EMPTY)

as soon as I do,

config $(EXPANDS_TO_EMPTY)_FIXED_PART

the error is gone, cause the complete setting's name is no longer empty.

Afaik Zephyr doesn't create settings purely based on macros, they usually have a
fixed part, like this:

zephyr/subsys/logging/Kconfig.template.log_config

Line 3 in dcfb98a

choice "$(module)_LOG_LEVEL_CHOICE"

So far i'm not convinced about the value of this PR, so please provide more
information on the exact problem / limitation of the build system.

Which use-cases, with examples, is blocking you.

[sjg20]

Let's take them one at a time. For the first one, Zephyr does not require that Kconfig options have

source "$(ZEPHYR_EC_MODULE_DIR)/motion/Kconfig"

Just

source "motion/Kconfig"

is sufficient. After all, how can the core Zephyr Kconfig reference a module in that way? It would cause an error if the module were not present. Modules can be added as needed, but a module that is required by Zephyr to build, is not really a module.

As to the use case for this one, we want to check the Kconfig to make sure that people don't add #define CONFIG options when they should have a Kconfig too.

https://chromium.googlesource.com/chromiumos/platform/ec/+/HEAD/util/kconfig_check.py

We obviously need to load the Kconfig to make this work, and things like the log level are not relevant for this check. Without this commit, we get errors trying to load the Kconfig.

[tejlmand]

For the first one, Zephyr does not require that Kconfig options have

Today, Zephyr sources Kconfig from modules based on their setting in zephyr/module.yml.
It does so by generating a Kconfig.modules file in the build folder, looking like this:

menu "canopennode (/projects/github/ncs/modules/lib/canopennode)"
osource "$(ZEPHYR_CANOPENNODE_KCONFIG)"
config ZEPHYR_CANOPENNODE_MODULE
        bool
        default y
endmenu

If you have additional Kconfig files you want to source from a Zephyr module you can use the approach I described earlier, for example as seen here in a project that uses Zephyr:
https://github.com/nrfconnect/sdk-nrf/blob/c0bd1e1e48f395f06732d66b6ed692de5e5176be/tests/lib/nrf_modem_lib/nrf_modem_lib_trace/Kconfig#L3

source "$(ZEPHYR_NRF_MODULE_DIR)/lib/nrf_modem_lib/Kconfig.modemlib"

This is available in zephyr/main already today.

And my question is, why is this existing possibility not good enough ?
Why is a new approach needed ?

Just

source "motion/Kconfig"

with this PR, correct.
But i was referring to what exists in zephyr/main already.

I would like to understand why the new approach is needed, considering its short comings such as collision risk and easiness for developers to directly see location from where a Kconfig is sourced.
Why is the existing solution not sufficient ?

It would cause an error if the module were not present.

osource ?

Modules can be added as needed, but a module that is required by Zephyr to build, is not really a module.

hopefully there are no modules that are mandatory for Zephyr.
There might be modules that are required if you want to build for a specific target.
Like if building for a Nordic nRF SoC you need the nordic hal module, but building for SoCs from other vendors, you can drop this module.

Similar for features, a specific feature might require a specific module.

But there should not be a module that are always required.
So building tests for native_posix, and basic samples like hello_world for native posix should work without any modules.

Without this commit, we get errors trying to load the Kconfig.

What kind of errors ?
Cause in your commit I see your changing this line:

zephyr/scripts/kconfig/kconfiglib.py

Lines 2717 to 2720 in 26c5579

	# Avoid creating a Kconfig symbol with a blank name. It's almost
	# guaranteed to be an error.
	if not self.allow_empty_macros:
	self._parse_error("macro expanded to blank string")

and i'm only able to trigger this error in a very special case where I actually would like to see such error.
So could you please provide a clear example on when this happens so that it's easier to understand which use-case you try to support.

I ask for this, because I don't like to deviate from the upstream Kconfiglib.py unless we really have a good reason for doing so.
Remember Kconfiglib.py is a Python implementation of Kconfig as it's stated here:

Kconfiglib is a Kconfig implementation in Python 2/3. It started out as a helper library, but now has a enough functionality to also work well as a standalone Kconfig implementation

and would that allow for empty named config settings ?

[sjg20]

Firstly I should say that I did send this upstream, here:

ulfalizer/Kconfiglib#112

I mentioned in this in the second commit:

This corresponds to upstream PR:

ulfalizer/Kconfiglib#112

but it looks like the project might be dead as there is no activity in
18 months

It's great that you have a way to generate a Kconfig.modules with magic tooling. But that is in the Zephyr build system. We cannot run a build just to be able to parse the Kconfig. We need to be able to parse it as is, actually from outside the Zephyr build system.

If you want to reproduce this you can follow these steps. Sorry for any minor errors:

mkdir /tmp/xx
cd /tmp/xx
git clone https://chromium.googlesource.com/chromiumos/platform/ec
cd ec

put the path to your zephyr tree here

ZEPHYR_DIR=~/cosarm/src/third_party/zephyr/main

get my new kconfiglib

cp $ZEPHYR_DIR/scripts/kconfig/kconfiglib.py util

manually hack the test file util/test_kconfig_check.py:

#zephyr_path = pathlib.Path(

"../../../src/third_party/zephyr/main"

replace that with the path to your Zephyr tree

run the tests

python3 util/test_kconfig_check.py

Those tests check the various cases.

If you want to try this without the new features and see what happens, hack util/kconfig_check.py to change scan_kconfigs():

        kconf = kconfiglib.Kconfig(
            "Kconfig",
            warn=False,
            search_paths=search_paths,   # drop this param and the next
            allow_empty_macros=True,
        )

In my case this does:

python3 util/test_kconfig_check.py
......E...

ERROR: test_real_kconfig (main.KconfigCheck)
Same Kconfig should be returned for kconfiglib / adhoc

kconfiglib.KconfigError: emul/tcpc/Kconfig:20: 'subsys/logging/Kconfig.template.log_config' not found (in 'source "subsys/logging/Kconfig.template.log_config"'). Check that environment variables are set correctly (e.g. $srctree, which is set to 'zephyr'). Also note that unset environment variables expand to the empty string.

As you can imagine, that is only the first error that comes up. There are plenty of others.

I hope this helps you to understand the problem and that you can approve this PR quickly.

[sjg20]

@tejlmand does this make sense to you?

[tejlmand]

I hope this helps you to understand the problem and that you can approve this PR quickly.

@sjg20 I now had the time to fetch the referenced repo and try it out.
I don't think I fully understand your problem, and the repo link really doesn't help me much.

I'm still not quite sure exactly what your problem is.
To me it seems that your main goal is to be able to use

source "<path>/Kconfig"

in your module instead of an rsource.
But at the same time, the Kconfig files in your module might use / depend upon symbols in Zephyr Kconfig tree.

This means the module in itself is useless without the Zephyr tree.
Because of this, then I still fail to see why this patch is truly needed.

Somewhere in your Kconfig tree, you must load the Zephyr Kconfig tree.
If your module is the starting point, and ZEPHYR_BASE is srctree when calling kconfiglib.py, then you simply include the top-level Kconfig.zephyr as source "Kconfig.zephyr", from somewhere in your modules Kconfig.
This is something I assume you already know.

But if I understand your problem correctly, then your module is not including Kconfig.zephyr because Zephyr itself will be using your module in a real Zephyr build.

So this means Kconfig.zephyr is actually your starting point, and you want Zephyr to include it without generating a complete build.

I see that your test_kconfig_check.py imports kconfig_check.py which is the real user kconfiglib.py.

Let me explain how to get your ec module magically into the Zephyr tree.

For this, I need a module Kconfig file, but I didn't find any Kconfig files at the ec-repo top-level, but <ec-repo>/zephyr/app/Kconfig seems a good starting point, so i'll use that for exemplifying.

Considering <ec-repo>/zephyr/app/Kconfig as a module to be loaded by Kconfig.zephyr tree, then all you need to do is generate as single Kconfig.modules file somewhere, like this.

# <some-path>/Kconfig.modules
source "<ec-repo-path>/zephyr/app/Kconfig"

and then simply export the folder where Kconfig.modules is generated to kconfiglib.py through the environment.

You can test this in a terminal like this:

$ echo "source \"<ec-repo-path>/zephyr/app/Kconfig\"" > /tmp/Kconfig.modules
$ export KCONFIG_BINARY_DIR=/tmp

# run kconfiglib.py, either directly or through your kconfig wrapper check 

This will load <ec-repo-path>/zephyr/app/Kconfig as part of the Zephyr Kconfig tree.

Your Kconfig check should already know the module to check, so it should be very simple to generate and include above snippet through above method before running the check.

[tejlmand]

For reference, the generated Kconfig.modules is included in the tree here:

zephyr/modules/Kconfig

Line 13 in 3f62860

osource "$(KCONFIG_BINARY_DIR)/Kconfig.modules"

And I noticed, when playing with the ec repo, that you do generate files, cause I discovered that your specifying the srctree to be, as example: /tmp/tmpa3ijezsm

So why not set the srctree to ZEPHYR_BASE as it is intended, and then when anyway generating files in /tmp/tmpa3ijezsm or similar, then create the Kconfig.modules in addition.

[tejlmand]

not convinced until now that this should be accepted, main reasons:

• It's deviating the general Kconfig of a single tree starting point, something not possible either in regular Kconfig as used by Linux kernel, et. al.
• Risk of path collision, making it unpredictable for end-users whcih tree will be included, not to mention hard to debug when name collides.
• It's not providing any needed feature to Zephyr
• As the feature is not used by Zephyr itself, and used only be single downstream user for a very specific test use-case, the risk that this bit rots becomes high.
  Downstream user could even decide a new path in future, resulting in this code just become dead.
• A similar functionality can be achieved using the module approach described.

Feel free to continue the discussion and argue your case if you believe this is the only way forward, but for my understanding of your need, I fail to see why existing support cannot accomplish you need.

[sjg20]

I am quite surprised at the push-back on this and the overwhelming amount of text above.

The key point is that modules effectively merge with the Zephyr tree, so far as the build system is concerned. We don't have to explicitly add full paths to things in our cmake files or device tree. It just works.

With that in mind, we need Kconfig to work the same way. I cannot add relative/absolute paths to a completely different project. If I do that in platform/ec it is ignored when parsing zephyr/main Kconfig. If I do it in zephyr/main Kconfig then it breaks when the EC is at a different path or not present.

Chromium OS is using upstream Zephyr. We pull down new patches every day. We cannot add 'rsource /...' into the Zephyr tree. How then would someone build Zephyr without the EC?

This just seems to be completely at odds with the way Zephyr modules work.

As to the empty macros, have you tried parsing the zephyr Kconfig? I think I provided the information you asked for.

These commits add useful kconfiglib features along with unit tests to prevent bit rot. I respectfully request you to take another look.

[tejlmand]

I am quite surprised at the push-back on this and the overwhelming amount of text above.

The main reason was to show it was not a single reason, but a combination of the above.
Sometimes when giving feedback the author may just think there is only a single concern to be addressed, and when that is addressed the PR can go in.

I simply wanted to avoid that, and be clear on my concerns.

The key point is that modules effectively merge with the Zephyr tree, so far as the build system is concerned. We don't have to explicitly add full paths to things in our cmake files or device tree. It just works.

Devicetree uses regular #include <file.h>" syntax and is processed by the C pre-processor.
And with that, users are expected to be familiar on how it works, as well as know how to easily debug, like the -E or -M -MF flag, to see what is getting included.

For CMake, i'm unsure what you refer to.
Is it because your module is included automatically when you write find_package(Zephyr) ?
Because when you do that, you're actually running Zephyr CMake build system code which generates the needed inclusion for you. If you don't have find_package(Zephyr) in your code, then I believe things don't work.
For CMake I assume you're relying on the Zephyr build system / module feature to handle things for you, and that's why it works.

But for Kconfig, you're invoking Kconfiglib completely decoupled from the Zephyr creation of Kconfig files.
How can you compare that to running Zephyr CMake package ?

Did you at all look at the description I provided to you #42233 (comment) ?

Also, when using CMake, the add_subdirectory(source_dir [binary_dir]) is always using a relative to current source dir or absolute path, so more alike rsource and source <abs-path>.
There is no such thing as a Kconfig source concept which includes CMake file relative to a pre-defined location, like ZEPHYR_BASE.

Besides older posts then it would also be valuable to take a look here:
https://github.com/zephyrproject-rtos/zephyr/blob/main/scripts/zephyr_module.py
which can generate your Kconfig module file to include, or even the CMake package helper:
https://github.com/zephyrproject-rtos/zephyr/blob/main/cmake/package_helper.cmake

Chromium OS is using upstream Zephyr. We pull down new patches every day. We cannot add 'rsource /...' into the Zephyr tree.

and I don't believe I ever requested you to do that.
Already today it appears you integrate EC nicely with Zephyr for building, including the EC Kconfig tree.

You create your custom logic for doing extra testing, which is very valid.
But why don't you want to ensure that your tests integrates Kconfig code in the same way as Zephyr build system (CMake / Kconfig) integrates that very same Kconfig in a build ?

That was the feedback I tried to give you, and was very detailed on how it may be achieved: #42233 (comment)

These commits add useful kconfiglib features along with unit tests to prevent bit rot.

but notice, Kconfig language is not defined by Zephyr, nor by Kconfiglib project.
It's a Python implementation of Kconfig language

Yes, kconfiglib / Zephyr has a number of extensions, but those are extensions, not changing fundamentally how a command like source works, which is what you're asking for.
And it's described here:

zephyr/scripts/kconfig/kconfiglib.py

Lines 892 to 893 in 2636bb4

	If $srctree is set, 'filename' will be looked up relative to it.
	$srctree is also used to look up source'd files within Kconfig files.

To me, what you're asking, is to change the #include <file> behavior of pre-processor.

As to the empty macros, have you tried parsing the zephyr Kconfig?

I tried to follow your description, and spent time on looking into it, but please notice, I (and others) have many other things to do, so don't expect me to spend hours in understanding your downstream issue.
I noticed a lot of errors, not being able to identify which of those are related to this particular issue, and which are not.
Instead I actually spent time on looking into your repo and trying to understand your need, and gave you very detailed proposal on what I believe will address your issue.

If you expect me to look even further into this, then please provide a very easy and clear description on how to experience the particular issue you're facing, and how it manifest itself when not having this PR in the Zephyr tree.
Not something that appears to have insight knowledge of how a test is written or expected to be parsed by CI.

As for the Kconfig macro itself, you're always welcome to open a dedicated PR for that.
As I believe those to issues are orthogonal to each other, and having them in same PR can create noise during discussions.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[sjg20]

Reopening as I would still like this in

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.