posix: deprecate CONFIG_POSIX_API and remove zephyr/posix include prefix #97855
Conversation
cfriedt force-pushed the deprecate-config-posix-api branch from d6f7dd9 to c82f505 Compare 
zephyrbot requested review from aescolar, andyross, carlescufi, dcpleung, jakub-uC, jukkar, keith-packard, nashif, pdgendt, peter-mitsis, rerickson1, rlubos, ssharks, stephanosio and ycsin cfriedt force-pushed the deprecate-config-posix-api branch from c82f505 to 21ab57c Compare 
| The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
github-actions bot added manifest manifest-hostap DNM (manifest) 
cfriedt force-pushed the deprecate-config-posix-api branch 2 times, most recently from b9dac21 to dc6abc8 Compare 
|
cfriedt force-pushed the deprecate-config-posix-api branch from 9fa199e to 79415fb Compare | Ok, I ran through testing with all of the change requests that @aescolar made and @nordicjm. Here is the status:
And follow-up items
|
Previously, `lib/posix/options/CMakeLists.txt` defined the feature test macros `_POSIX_C_SOURCE` and `_XOPEN_SOURCE` globally via `zephyr_compile_definitions()`, to mitigate build and runtime errors. However, it is preferable to not define those globally. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
cfriedt force-pushed the deprecate-config-posix-api branch from 79415fb to cc8fa8e Compare 
|
| Looks like |
this is not a functional flag, is it? we do not build anything in zephyr with -pedantic, this is probably needed upstream in this library, but here in zephyr it wont be adding much value.
wonder how this was not caught before, fork should be blacklisted here scripts/coccinelle/symbols.txt
agree, we often have this type of issues with those arches, just open a bug and link it in there so we can back to it and have some context. |
Not a functional flag - it would be nice to actually capture
It's not something declared in the zephyr unistd.h because why would we, of course. Only really became obvious when we switched to using libc headers. I'll make a pr to add it to coccinelle.
I can do that 👍 |
Add posix feature test macro _POSIX_C_SOURCE=200809L to the list of compilation defines when building and remove the `zephyr/posix/` prefix from standard header paths. This is required for upstream PR zephyrproject-rtos/zephyr#97855 Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Add posix feature test macro _POSIX_C_SOURCE=200809L to the list of compilation defines when building and remove the `zephyr/posix/` prefix from standard header paths. This is required for upstream PR zephyrproject-rtos/zephyr#97855 Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
| * dependency loop should be fixed with #97050 or adjacent PRs. | ||
| */ | ||
| #undef _POSIX_C_SOURCE | ||
| #define _POSIX_C_SOURCE 200809L |
There was a problem hiding this comment.
a) Major: Headers cannot define these macros as they affect what other headers define & expose.
This is for the application to define, and must be defined before including any header.
(Defining it in a Zephyr header means the macro can/will change from not being defined to being defined causing it to not be consistent in different included headers and therefore compilation problems)
b) I would not now include a tangential change like this (not defining globally these 2 feature test macros) in this PR. This PR is already too big. Meaning, I would drop this new commit from this PR.
There was a problem hiding this comment.
a) I would say more major is that there is a dependency cycle and think we should clean them up together. These will be removed when the dependency cycle itself is removed. Otherwise, this is required here to ensure that expected functionality is not broken.
b) I think dropping the last commit would mean that @nordicjm 's earlier change request is not met. As it is, these macros are there to make the required symbols visible, and that's all, and then they are undefined.
@jukkar - do you have a preference?
There was a problem hiding this comment.
Let's focus on b)
@nordicjm 's request to not define feature test macros globally (_POSIX_C_SOURCE and _XOPEN_SOURCE) is fair, but: that is a much more complex change; not something directly related to this PR; and something for which there is no evident clean fix.
So as such I would recommend to remove this last commit which tries to fix this _POSIX_C_SOURCE and _XOPEN_SOURCE issue out of this PR, and for a later one.
(But please leave the fix for the unistd.h constants in, that is, _POSIX_ASYNCHRONOUS_IO and so forth)
There was a problem hiding this comment.
Getting rid of most global defines is a good start, if these 2 need a longer time to work out how to properly get rid of them then they can stay, would rather have them moved and working properly with everyone happy than rushed and it done in a way that is causing issues. If they can be limited using zephyr_library_compile_definitions() then that would be nice, but not essential
| | ||
| mqd = mq_open(queue, O_WRONLY); | ||
| clock_gettime(CLOCK_MONOTONIC, &curtime); | ||
| zassert_ok(clock_gettime(CLOCK_REALTIME, &curtime)); |
There was a problem hiding this comment.
This change seems unrelated to this commit.
There was a problem hiding this comment.
It's pretty related - especially if you want to ensure that things aren't failing silently ;-)
| | ||
| uint32_t timespec_to_timeoutms(int clock_id, const struct timespec *abstime); | ||
| | ||
| /* Convert a POSIX clock (cast to int) to a sys_clock identifier */ |
There was a problem hiding this comment.
Moving this function seems unrelated to this commit
There was a problem hiding this comment.
It's also related to the last commit and was necessary for the changes requested by @nordicjm to not break builds.
| select POSIX_SYSTEM_INTERFACES | ||
| select POSIX_BASE_DEFINITIONS | ||
| select POSIX_AEP_REALTIME_MINIMAL | ||
| select POSIX_AEP_REALTIME_CONTROLLER |
There was a problem hiding this comment.
Selecting this feature seems unrelated to this commit
There was a problem hiding this comment.
Also, directly related to the commit.
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| | ||
| #ifndef INCLUDE_ZEPHYR_POSIX_POSIX_FEATURES_H_ |
There was a problem hiding this comment.
a) Please apply the part of this commit which is fixing up "posix: remove zephyr/posix in header prefixes" directly in that commit as a fix up. (Otherwise this is just undoing part of that commit). The rest of this commit can either be a commit before that one, or (if you want to save time) be squashed into "posix: remove zephyr/posix in header prefixes".
b) Otherwise, if you dont do a) , fix further the commit msg:
Minor:
posix: ensure that unistd.h declares version and symbolic constants The specification requires that unistd.h (or a file that is included via unistd.h) declare version macros and symbolic constants. Feature test macros declarations are also removed from `lib/posix/options/CMakeLists.txt`. =>
posix: ensure that unistd.h declares version and symbolic constants The specification requires that unistd.h (or a file that is included via unistd.h) declare version macros and symbolic constants. | zephyr_compile_definitions(-D_POSIX_VERSION=${POSIX_VERSION}) | ||
| zephyr_compile_definitions(-D_POSIX_C_SOURCE=${POSIX_VERSION}) | ||
| zephyr_compile_definitions(-D_POSIX2_C_BIND=${POSIX_VERSION}) | ||
| zephyr_compile_options("SHELL: $<$<COMPILE_LANGUAGE:C>:$<TARGET_PROPERTY:compiler,imacros> ${ZEPHYR_BASE}/include/zephyr/posix/posix_features.h>") |
There was a problem hiding this comment.
a) Major: We cannot include with -imacros a header whose definitions are ifdefed on other macros the .c files will define (headers added with -imacros are parsed before any content of the .c files, and therefore the value of these macros will not match).
b) A request for clarification:
Adding any header globally, with -imacros header is risky. This is something I would have tried to avoid. What is the motivation? (what test failed if you simply ensured include/zephyr/posix/posix_features.h was included from unistd.h?).
If you do this, you would need to be very careful about what you add inside this header (and given a) it already is not ok)
There was a problem hiding this comment.
a) Major: We cannot include with
-imacrosa header whose definitions areifdefed on other macros the .c files will define (headers added with-imacrosare parsed before any content of the .c files, and therefore the value of these macros will not match).
Can you be more specific?
We should be using _POSIX_C_SOURCE and CONFIG_POSIX_SYSTEM_INTERFACES in order to gate definitions in include/zephyr/posix/posix_features.h.
_POSIX_C_SOURCE is used when a C file or compile option is defined before <unistd.h> is included.
CONFIG_POSIX_SYSTEM_INTERFACES (defined in autoconf.h, before posix_features.h) is used otherwise.
Both are necessary.
b) A request for clarification: Adding any header globally, with
-imacrosheader is risky. This is something I would have tried to avoid. What is the motivation? (what test failed if you simply ensuredinclude/zephyr/posix/posix_features.hwas included from unistd.h?). If you do this, you would need to be very careful about what you add inside this header (and givena)it already is not ok)
Feel free to clone the branch and try running through CI yourself in a separate PR. I unfortunately cannot dedicate as much time as I did on the day of feature freeze to addressing your review comments.
There was a problem hiding this comment.
Can you be more specific?
The result of including posix_features.h depends on other macros. Some of these macros (_POSIX_C_SOURCE) can be defined by the application in its source (.c) files before including other headers.
When we include a header with -imacros the compiler will include it before parsing those .c files contents (and therefore their #defines). Which means the ifdef's in posix_features.h won't be based on the right conditions.
So doing it this way will only work while _POSIX_C_SOURCE remains defined from the command line which is not a constraint we would not like to impose on others.
The order of inclusion of autoconf.h and posix_features.h depends on the cmake scripts. If that needs to be a given order we are setting a dependency to the cmake scripts.
Meaning, the best solution for the problem of "how to ensure posix/posix_features.h is included is to ensure it is included from all system headers which need it.
But if that is too tricky to do in this PR, and given that posix_features.h is, after this PR, only defining macros which are not too likely to be used by the application itself (and therefore it being incorrectly parsed is not that likely to be a problem), I guess we could go with the -imacros solution, but:
a) I would create an enhancement issue to indicate this should be fixed eventually + b) I would clearly document in the header that it is being included with imacros as a provisional solution, and that therefore one must be very careful with what is defined on it.
There was a problem hiding this comment.
If you would like to create an enhancement issue, please do.
There was a problem hiding this comment.
What is the motivation? (what test failed if you simply ensured
include/zephyr/posix/posix_features.hwas included from unistd.h?).
To clarify a bit on this, newlib and picolibc both pull in <sys/features.h> with every compile, regardless of whether <unistd.h> is included.
I did try to override <sys/features.h> for both of those but it still would result in build failures.
The reason that <sys/features.h> is included as part of every compile is that a lot of the POSIX spec does not say to include <unistd.h> before including e.g. <time.h> to get the correct visibility of structures and functions in <time.h> and therefore many symbolic constants are implicit.
-imacros resulted in fewer errors than -include as well, which was surprising.
The thing to keep in mind is that we aren't how the symbolic constants are provided just that they are, so it should be possible to change how if needed.
| #include <errno.h> | ||
| /* required for FNM_LEADING_DIR */ | ||
| #define _GNU_SOURCE | ||
| #include <fnmatch.h> | ||
| #undef _GNU_SOURCE |
There was a problem hiding this comment.
| #include <errno.h> | |
| /* required for FNM_LEADING_DIR */ | |
| #define _GNU_SOURCE | |
| #include <fnmatch.h> | |
| #undef _GNU_SOURCE | |
| /* required for FNM_LEADING_DIR */ | |
| #define _GNU_SOURCE | |
| #include <errno.h> | |
| #include <fnmatch.h> |
_GNU_SOURCE needs to be defined before any header, and cannot be undefined after.
There was a problem hiding this comment.
It's not causing any issues, to define it exclusively for <fnmatch.h>. @jukkar - do you have any concerns?
There was a problem hiding this comment.
It will cause issues depending on the exact C library and which headers are included.
The requirement to define it before including any header is clearly stated:
https://ftp.gnu.org/old-gnu/Manuals/glibc-2.2.3/html_node/libc_13.html
"These directives must come before any #include of a system header file."
https://pubs.opengroup.org/onlinepubs/007904875/functions/xsh_chap02_02.html
"In the compilation of an application that #defines a feature test macro specified by IEEE Std 1003.1-2001, no header defined by IEEE Std 1003.1-2001 shall be included prior to the definition of the feature test macro. "
There was a problem hiding this comment.
I'll make the change. It's fixed in staging
The minimal libc does not include `fnmatch.h` in its default search path. This causes an issue when running coverage in CI as shown here. https://github.com/zephyrproject-rtos/zephyr/actions/runs/18839500380/\ job/53748166725 That would technically be fixed with zephyrproject-rtos#97855 as well. Testing Done: ``` west twister -c -p qemu_x86/atom -s kernel.pending.minimallibc --coverage\ --coverage-tool gcovr ``` Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
The minimal libc does not include `fnmatch.h` in its default search path. This causes an issue when running coverage in CI as shown here. https://github.com/zephyrproject-rtos/zephyr/actions/runs/18839500380/\ job/53748166725 That would technically be fixed with #97855 as well. Testing Done: ``` west twister -c -p qemu_x86/atom -s kernel.pending.minimallibc --coverage\ --coverage-tool gcovr ``` Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Add posix feature test macro _POSIX_C_SOURCE=200809L to the list of compilation defines when building and remove the `zephyr/posix/` prefix from standard header paths. This is required for upstream PR zephyrproject-rtos/zephyr#97855 Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Add posix feature test macro _POSIX_C_SOURCE=200809L to the list of compilation defines when building and remove the `zephyr/posix/` prefix from standard header paths. This is required for upstream PR zephyrproject-rtos/zephyr#97855 Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
9 participants
Deprecation of
CONFIG_POSIX_APIDeprecate
CONFIG_POSIX_API.User may choose one of the pre-defined POSIX subprofiles, for simplicity.
Libraries should depend on CONFIG_POSIX_SYSTEM_INTERFACES as well as other POSIX Option Groups, as needed.
Custom Configurations
Of course, instead of, or in addition to, using a pre-defined POSIX subprofile, users can still fully customize their POSIX configuration via Kconfig.
Zephyr's POSIX Kconfig option names correspond 1:1 with official POSIX Option Group names. Simply add a
CONFIG_at the beginning of an Option Group to get the corresponding Kconfig option.For more information on POSIX Subprofiling Option Groups, please see
https://pubs.opengroup.org/onlinepubs/9799919799/xrat/V4_subprofiles.html
Removal of
zephyr/posix/prefix for standard includesThe majority of the changes in this category (~156 files) are mechanical and are contained in the commit posix: remove zephyr/posix in header prefixes. They are approximately equivalent to running this command (with some adjustments).
Originally from #97152, but included here for simplicity, these changes mean that we no longer need to have
include/zephyr/posixin the library search path for C libraries that have (at least partially) conformant POSIX headers. In other words, we use the C library's POSIX headers.Originally added to mitigate C-library conflicts, the
<zephyr/posix/...h>prefix on standard includes has been a thorn in our sides for several years. Since these changes remove the conflicts, opting to use C-library headers (if they exist), we no longer need to namespace those in Zephyr.Depends on
Note
Compliance issues are false positives
Fixes #97949