-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
cmake: Add support for global overlay config file #15669
Conversation
If OVERLAY_CONFIG_DIRS environment variable is specified, then search it if the file in OVERLAY_CONFIG is not found. Example: $ cd samples/net/ipv4_autoconf $ OVERLAY_CONFIG_DIRS=../sockets/echo_server \ cmake -B build -DBOARD=qemu_x86 -DOVERLAY_CONFIG=overlay-e1000.conf . Zephyr version: 1.14.99 -- Selected BOARD qemu_x86 -- Found west: west (found version "0.5.6", minimum required is "0.5.6") -- Loading zephyr/boards/x86/qemu_x86/qemu_x86.dts as base -- Overlaying zephyr/dts/common/common.dts -- Overlaying ../sockets/echo_server/overlay-e1000.conf Parsing Kconfig tree in zephyr/Kconfig So in this example the samples/net/ipv4_autoconf directory does not contain overlay-e1000.conf, but it is found in ../sockets/echo_server/ directory which is mentioned in OVERLAY_CONFIG_DIRS variable. Fixes zephyrproject-rtos#14740 Signed-off-by: Jukka Rissanen <[email protected]>
If this is acceptable, then we can define where the global config overlays could be placed and I can do more updates to documentation etc. |
Should we have this behave and be named, like BOARD_ROOT, and DTS_BINDINGS_ROOT? So we have a list that contains an in-tree directory, and also any other directories that are appended by the user. |
@@ -104,6 +104,38 @@ if(EXTRA_KCONFIG_OPTIONS) | |||
) | |||
endif() | |||
|
|||
# Set overlay config files list using a list of directories where the | |||
# overlay files could be found. | |||
if(DEFINED ENV{OVERLAY_CONFIG_DIRS}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we support env vars, we should also support setting this through a CMake var.
endforeach() | ||
else() | ||
set(overlay_config_files ${OVERLAY_CONFIG_AS_LIST}) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The application docs need updating.
https://docs.zephyrproject.org/latest/application/index.html
|
||
# We add current directory first so that it is seached first | ||
set(overlay_config_dir_list ".:${OVERLAY_CONFIG_DIRS}") | ||
string(REPLACE ":" ";" overlay_config_dirs "${overlay_config_dir_list}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
: is used in Windows (C:), use '|' instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe ' ' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was following the way directories are separated in PATH in unix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but we need to also support Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we should just figure out what separator to use. Probably not space as that could be part of file or directory. Perhaps ";" could be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to use what we are already using, and that is whitespace.
We can't have every user-settable list use a different separator.
else() | ||
set(found 0) | ||
foreach(d ${overlay_config_dirs}) | ||
if(EXISTS ${d}/${f} AND NOT IS_DIRECTORY ${d}/${f}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it a user-error for d/f to be a directory?
Can we ignore or error-out on this case instead?
set(found 0) | ||
foreach(d ${overlay_config_dirs}) | ||
if(EXISTS ${d}/${f} AND NOT IS_DIRECTORY ${d}/${f}) | ||
message(STATUS "Overlaying ${d}/${f}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't kconfig.py spit this out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not see it doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loading /home/sebo/ncs/zephyr/boards/arm/nrf9160_pca10090/nrf9160_pca10090_defconfig as base
Merging /home/sebo/ncs/zephyr/samples/hello_world/prj.conf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The system definitely does not print anything for the overlay config file atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is strange, can you find out why?
Perhaps it is being omitted.
endif() | ||
endforeach() | ||
if(NOT found) | ||
# Let the file checker below to report error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what this means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant by this comment, is that in about 20-30 lines below in this file, there is a checker that makes sure that the config fragment file exists (search message(FATAL_ERROR "File not found: ${f}")
). So we do not bail out here if the file is not found as that is checked later in this file anyway.
endif() | ||
endif() | ||
endforeach() | ||
else() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this if-else?
Wouldn't technically . be the highest priority, and therefore the files be appended correctly to overlay_config_files ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that else() could be removed if needed. Just tried to optimize this a bit so that the default case (no OVERLAY_CONFIG_DIRS defined) would work just like now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get rid of the if-else, this would be consistent with BOARD_ROOT and DTS_BINDINGS_ROOT.
An in-tree common directory of Kconfig fragments sounds sane. |
Having to set an env var seems a bit tedious. Were we planning on having a default setting / location? |
It would be nice if not all default overlay config files are in same directory. What I mean by this is that for example in networking case, it would make lot of sense if we could place network related overlays in network specific directory etc. |
Wouldn't this be solved by marti's suggestion; of defaulting to including zephyr/kconfig/common, and then doing
? foo.conf would be in zephyr/kconfig/common/subsys/net/ieee/foo.conf |
Do I understand correctly, that with the current approach we apply either local overlay config or the global one, with the location specified in OVERLAY_CONFIG_DIRS, not both? I'm asking, because looking at various overlay files, it's quite common to see that they contain a common part and sample-specific tweaks. For instance, most of the |
Closing this as I do not have time to work on this. Feel free to use the code and ideas as needed. |
If OVERLAY_CONFIG_DIRS environment variable is specified, then
search it if the file in OVERLAY_CONFIG is not found.
Example:
$ cd samples/net/ipv4_autoconf
$ OVERLAY_CONFIG_DIRS=../sockets/echo_server
cmake -B build -DBOARD=qemu_x86 -DOVERLAY_CONFIG=overlay-e1000.conf .
Zephyr version: 1.14.99
-- Selected BOARD qemu_x86
-- Found west: west (found version "0.5.6", minimum required is "0.5.6")
-- Loading zephyr/boards/x86/qemu_x86/qemu_x86.dts as base
-- Overlaying zephyr/dts/common/common.dts
-- Overlaying ../sockets/echo_server/overlay-e1000.conf
Parsing Kconfig tree in zephyr/Kconfig
So in this example the samples/net/ipv4_autoconf directory does
not contain overlay-e1000.conf, but it is found in
../sockets/echo_server/ directory which is mentioned in
OVERLAY_CONFIG_DIRS variable.
Fixes #14740
Signed-off-by: Jukka Rissanen [email protected]