-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
lib: libc: Kconfig: Correct dependencies for MINIMAL_LIBC_SUPPORTED #81410
base: main
Are you sure you want to change the base?
lib: libc: Kconfig: Correct dependencies for MINIMAL_LIBC_SUPPORTED #81410
Conversation
The MINIMAL_LIBC_SUPPORTED option is supposed to indicate that the minimal C library implementation can be chosen and many tests use this option as a filter for scenarios where the MINIMAL_LIBC option is enabled. On the other hand, the REQUIRES_FULL_LIBC option tells that the minimal C library is insufficient and it prevents the MINIMAL_LIBC option from being enabled. But it should also cause that MINIMAL_LIBC_SUPPORTED is no longer enabled, otherwise for targets requiring a complete C library implementation, tests using the filtering mentioned above will not be actually performed with the minimal C library. This commit removes the above discrepancy by moving the dependency on !REQUIRES_FULL_LIBC from MINIMAL_LIBC to MINIMAL_LIBC_SUPPORTED. Signed-off-by: Andrzej Głąbek <[email protected]>
@@ -28,6 +28,7 @@ config FULL_LIBC_SUPPORTED | |||
config MINIMAL_LIBC_SUPPORTED | |||
bool | |||
depends on !NATIVE_APPLICATION | |||
depends on !REQUIRES_FULL_LIBC |
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 think this is correct. MINIMAL_LIBC_SUPPORTED indicates the target itself supports the library.
REQUIRES_FULL_LIBC indicates an application or feature requires a full libc. Which has nothing to do with the target supporting or not minimallibc.
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.
Well, nrf9131ek/nrf9131/ns
is an example of a target that selects REQUIRES_FULL_LIBC
(because of NPM1300_CHARGER
) and this generates a build warning when the libraries.libc.common.minimal
scenario is executed for the tests/lib/c_lib/common
test.
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.
should that board kconfig.defconfig have a
config MINIMAL_LIBC_SUPPORTED
default n
?
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.
This would only solve the problem for this one board, but similar scenario may occur for others. I wanted to provide a generic solution and I thought that such dependency would be fine, as I saw that PICOLIBC_SUPPORTED
has dependency on !REQUIRES_FULL_LIBCPP
:
Lines 45 to 48 in 5f418f5
config PICOLIBC_SUPPORTED | |
bool | |
depends on !NATIVE_APPLICATION | |
depends on ("$(TOOLCHAIN_HAS_PICOLIBC)" = "y") || (ZEPHYR_PICOLIBC_MODULE && !REQUIRES_FULL_LIBCPP) |
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 pico one is different, as you can't build from the source module today if you need the c++ std lib.
Anyhow, to me it seems we should just set MINIMAL_LIBC_SUPPORTED to n for that board, and others which may just not be able to be built with minimal.
But if others think otherwise I'm not going to oppose this change.
The MINIMAL_LIBC_SUPPORTED option is supposed to indicate that the minimal C library implementation can be chosen and many tests use this option as a filter for scenarios where the MINIMAL_LIBC option is enabled.
On the other hand, the REQUIRES_FULL_LIBC option tells that the minimal C library is insufficient and it prevents the MINIMAL_LIBC option from being enabled. But it should also cause that MINIMAL_LIBC_SUPPORTED is no longer enabled, otherwise for targets requiring a complete C library implementation, tests using the filtering mentioned above will not be actually performed with the minimal C library.
This commit removes the above discrepancy by moving the dependency on !REQUIRES_FULL_LIBC from MINIMAL_LIBC to MINIMAL_LIBC_SUPPORTED.