The contents of .config should not be able to prevent users from running menuconfig. #9573
Comments
SebastianBoe
changed the title
|
"When enabling nRF5 SPI drivers, invalid values are permitted" I'm not sure if permitted is the right word here, Kconfig is rightly disallowing the invalid value '' to be assigned to "SPI_0_NRF_SCK_PIN". Perhaps "invalid values are defaulted to" would be more correct to say? I see three options here; default the pins to a nonsensical, but Kconfig-valid value, e.g. all pins are set to pin 0 when the SPI instance is enabled. Or do nothing, and continue to let Kconfig error out on the user with this error message:
which pretty clearly IMHO explains that there is more configuration that needs to be done by the user. Or finally, the best solution is to either resolve or apply one of the workarounds of #7302. E.g. have the boards duplicate their DT information about default pin values in Kconfig. |
|
@SebastianBoe @ulfalizer the problem is, I can not get back into menuconfig to correct the values. |
|
Yeah... A quickfix is to manually edit the values in The simplest quickfix at the Kconfig level is to just give all This is really "just" a warning, but Could have made If needed, I could tweak the warning whitelist to not turn that warning into an error. That warning might be nice to have as an error in other cases though. |
|
|
I would say that this is the root cause, and what must be fixed. Fair enough that 0 might not make Producing defaults that make sense is another issue.
+1 |
One (dubious) upside is that you get notified if you forget to set the value of an And yeah, it's a bit wonky that you can write a I'm always a bit reluctant to break compatibility with the C tools though, because it's often handy to have the output match 100% (e.g., during testing). It could still be a custom patch though, if we can't live with the behavior. I could try to get it changed in the C tools as well, but as it "just" turns into an easily-fixed warning there (nothing promotes it to an error), that might provide useful information in rare cases, people might be reluctant. |
|
I want to turn this into a feature request. The reporter wants this issue to be about the fact that if you have a corrupted .config, for whatever reason, then it is not possible to do "ninja menuconfig". This is not possible because CMake uses the corrupted config and behaves in a corrupted/undefined way which could include doing anything, including crashing before Generation time, and thereby not creating a build system that can invoke menuconfig to rectify the corrupted config. This seems like a critical issue, but in practice, this has proven to not be a big problem. This is because of several things:
@cvinayak : Is it OK by you to change the description to: "The contents of .config should not be able to prevent users from running menuconfig." |
carlescufi
changed the title
carlescufi
added
Enhancement
|
@SebastianBoe I thought the problem was that If it's just |
No. But it does parse the .config before running menuconfig.py. So if menuconfig.py creates a corrupted .config file, this .config file will prevent menuconfig from being run again. turning off -Werror is not sufficient to resolve this issue. |
|
@SebastianBoe Don't know how hard that is to do in CMake though. |
…s were not found. Fixed by adding to zephyr.cmake
|
@SebastianBoe and @ulfalizer This issue is old now and "I" dont seem to have lately faced any issues related to this issue. Can we agree to close this issue as "won't fix"? |
|
This is still valid. |
@24601 Which is very unfortunately. One thing is clear, it should definitely be possible to run If a value has changed the definition of what a valid value means (and current value is outside that range), then it might be possible to direct the user to the setting, but we need to prefill Kconfig with a valid value (which might be different from what the user wants). One idea could be to introduce a I have some more ideas, just lack the time to work on them. Milestoned 2.5.0 to put a light pressure on myself. |
|
@tejlmand - Thank you for the full reply and consideration. I understand the difficult situation you're in, and think you have a good approach to the first set of things to address this. I think after reading your discussion of it, the only thing I can add (you are way better versed in this than I am no doubt), is considering how menuconfig was even allowed to write an invalid config to begin with. I see the entry to this bug/problem as possible to view in two different lights:
In the case of 1, if the user or an external tool left that file in an invalid state through an external edit, I still think "failing gracefully" is a good way to try to get to, even if it involves a "we noticed you had an invalid config, we've traversed that tree and disabled the given options and are starting menuconfig in safe mode to let you resolve them", but even this is a luxury/kindness to the user, because they brought the bad data/input. But, I think what we're seeing (or at least I am) is a situation of 2, where we are seeing a file that was actually written by menuconfig that, without doing anything but saving and opening menuconfig again, we're seeing this failure. Is it possible or advisable, even as a stop-gap until we have a broader fix strategy here, to "dry run" before a save where this validation error would be caught and the invalid file would never be written period? That could be a good way to stop at least some of the ways of invalid .config even happening to begin with, and the "nice to have" "menuconfig is more robust at eating/handling bad data" can come later, but I think we could start with "let's not write out knowingly bad data from menuconfig that menuconfig itself cannot open" and it maybe it is as easy enough as before save writing the config out to a temp file and attempting to parse it, and if it fails, don't even allow saving or make the user force such a save?
|
|
@24601 Thank you for the follow up. The more input, the better we can address this issue.
Yes, this is a user error, but we should still try to help and assist people
If this happens, that's a plain bug, and feel free to report that as such. I have a third experience, that you haven't described. That is also a case that must be considered, cause that is a bad user experience imho. |
|
@tejlmand - my pleasure, as we become more engaged with Zephyr we look forward to contributing more and becoming more productive members. I believe we have a situation with the MODEM_UBLOX_SARA that creates the situation I mentioned above: enabling MODEM_UBLOX_SARA on an otherwise fine config results in a .config that is broken such that menuconfig cannot open it even though it wrote the file. I will work on a demonstration repo and file an appropriate issue with a properly documented issue and reference herein. |
Several driver libraries uses: ``` zephyr_sources_ifdef() ``` instead of ``` zephyr_library() zephyr_library_sources_ifdef() ``` This results in a messy Zephyr lib as described in: zephyrproject-rtos#8825 One reason for drivers to use the first approach is because an empty Zephyr library results in a CMake build failure which again leads to twister issues when just enabling a driver and build for all known boards and then process the DTS result. Secondly, a CMake build failure prevents the user from launching menuconfig and disable the driver that creates the empty library. See zephyrproject-rtos#9573 for extra details. This commit verifies all Zephyr libraries, and if a library is found to have no source files it will be excluded from the build and a warning will be printed to the user. Printing a warning instead of a hard failure ensures that: - menuconfig can still be opened - CMake does not fail which allows twister to advance in the test case - Makes it possible to cleanup CMakeLists driver files Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
Several driver libraries uses: ``` zephyr_sources_ifdef() ``` instead of ``` zephyr_library() zephyr_library_sources_ifdef() ``` This results in a messy Zephyr lib as described in: #8825 One reason for drivers to use the first approach is because an empty Zephyr library results in a CMake build failure which again leads to twister issues when just enabling a driver and build for all known boards and then process the DTS result. Secondly, a CMake build failure prevents the user from launching menuconfig and disable the driver that creates the empty library. See #9573 for extra details. This commit verifies all Zephyr libraries, and if a library is found to have no source files it will be excluded from the build and a warning will be printed to the user. Printing a warning instead of a hard failure ensures that: - menuconfig can still be opened - CMake does not fail which allows twister to advance in the test case - Makes it possible to cleanup CMakeLists driver files Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
Fixes: zephyrproject-rtos#9573 Create a dedicated CMake project for menuconfig and related targets. This allows the Xephyr project to have menuconfig and related targets available even if CMake later fails due to invalid .config settings or other CMake errors. The build files (Makefile / ninja) or copied from the menuconfig project to the main project. This means that in the situation where CMake fails and no build files are generated for the main project, then the menuconfig targets will be available. When CMake succeeds for the main project, the temporary menuconfig build files are overwritten, and thus the projects build files, including menuconfig are available to the user. Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
|
and with sysbuild introduced, this even prevents running |
|
Hi @tejlmand, This issue, marked as an Enhancement, was opened a while ago and did not get any traction. Please confirm the issue is correctly assigned and re-assign it otherwise. Please take a moment to review if the issue is still relevant to the project. If it is, please provide feedback and direction on how to move forward. If it is not, has already been addressed, is a duplicate, or is no longer relevant, please close it with a short comment explaining the reason. @cvinayak you are also encouraged to help moving this issue forward by providing additional information and confirming this request/issue is still relevant to you. Thanks! |
When enabling nRF5 SPI drivers, invalid values are permitted, but next time opening menuconfig fails.
make menuconfig
-- Selected BOARD nrf52840_pca10056
Zephyr version: 1.12.99
Parsing Kconfig tree in /Users/hemal/workspace/zephyr/Kconfig
Using /Users/hemal/workspace/zephyr/build/peripheral/zephyr/.config as base
warning: the value '' is invalid for SPI_0_NRF_SCK_PIN (defined at drivers/spi/Kconfig.nrfx:55), which has type int -- assignment ignored
warning: the value '' is invalid for SPI_0_NRF_MOSI_PIN (defined at drivers/spi/Kconfig.nrfx:62), which has type int -- assignment ignored
warning: the value '' is invalid for SPI_0_NRF_MISO_PIN (defined at drivers/spi/Kconfig.nrfx:69), which has type int -- assignment ignored
Error: Aborting due to non-whitelisted Kconfig warning 'warning: the value '' is invalid for SPI_0_NRF_SCK_PIN (defined at drivers/spi/Kconfig.nrfx:55), which has type int -- assignment ignored'.
Note: If this warning doesn't point to an actual problem, you can add it to the whitelist at the top of /Users/hemal/workspace/zephyr/scripts/kconfig/kconfig.py.
CMake Error at /Users/hemal/workspace/zephyr/cmake/kconfig.cmake:149 (message):
command failed with return code: 1
Call Stack (most recent call first):
/Users/hemal/workspace/zephyr/cmake/app/boilerplate.cmake:251 (include)
CMakeLists.txt:2 (include)
-- Configuring incomplete, errors occurred!
See also "/Users/hemal/workspace/zephyr/build/peripheral/CMakeFiles/CMakeOutput.log".
See also "/Users/hemal/workspace/zephyr/build/peripheral/CMakeFiles/CMakeError.log".
make: *** [cmake_check_build_system] Error 1
The text was updated successfully, but these errors were encountered: