Skip to content
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

posix: standardize kconfig options #73047

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented May 21, 2024

This change (outlined as part of #51211) deprecates several POSIX Kconfig variables (linked below) in favour of their more normative replacements defined by IEEE 1003.1-2017.

Release Notes

Doc Preview

Out-of-tree users of the POSIX API should use the provided script to automatically update to the new Kconfig options.

$ python $ZEPHYR_BASE/scripts/utils/migrate_posix_kconfigs.py -r root_path

See also
Migration Guide

  • DEPRECATED added to older symbols following policy
  • add section / script in migration guide
  • email sent to zephyr-devel

Fixes #66132

@cfriedt cfriedt requested a review from ycsin May 21, 2024 04:25
@cfriedt cfriedt changed the title Posix deprecate non standard config options posix: timers: deprecate the non-standard option POSIX_CLOCK May 21, 2024
tests/posix/headers/src/dirent_h.c Show resolved Hide resolved
lib/posix/options/Kconfig.signal Show resolved Hide resolved
lib/posix/options/signal.c Outdated Show resolved Hide resolved
include/zephyr/posix/posix_features.h Show resolved Hide resolved
lib/posix/options/Kconfig.timer Outdated Show resolved Hide resolved
@cfriedt cfriedt force-pushed the posix-deprecate-non-standard-config-options branch 3 times, most recently from b0ed626 to 2da46dc Compare May 21, 2024 22:16
@cfriedt
Copy link
Member Author

cfriedt commented May 21, 2024

Just kicking CI again. It looks fine locally for me.

@cfriedt cfriedt force-pushed the posix-deprecate-non-standard-config-options branch 7 times, most recently from c48f5af to 071c1ce Compare May 23, 2024 04:02
@cfriedt cfriedt changed the title posix: timers: deprecate the non-standard option POSIX_CLOCK posix: deprecate non-standard- kconfig options May 23, 2024
Copy link
Member

@ycsin ycsin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a much needed clean up, thanks for this, some more comments.

tests/posix/headers/src/unistd_h.c Show resolved Hide resolved
lib/posix/options/sysconf.c Show resolved Hide resolved
lib/posix/options/sysconf.c Show resolved Hide resolved
lib/os/fdtable.c Show resolved Hide resolved
lib/posix/options/Kconfig.barrier Outdated Show resolved Hide resolved
include/zephyr/posix/sys/utsname.h Show resolved Hide resolved
lib/posix/options/Kconfig.deprecated Show resolved Hide resolved
@cfriedt cfriedt force-pushed the posix-deprecate-non-standard-config-options branch 4 times, most recently from 3939558 to 3e8cd31 Compare May 24, 2024 19:17
@cfriedt cfriedt changed the title posix: deprecate non-standard- kconfig options posix: standardize kconfig options May 24, 2024
@cfriedt cfriedt force-pushed the posix-deprecate-non-standard-config-options branch 9 times, most recently from 432df23 to c5c834a Compare May 27, 2024 13:29
@cfriedt
Copy link
Member Author

cfriedt commented Jun 1, 2024

rebased after #73604 was merged

@cfriedt
Copy link
Member Author

cfriedt commented Jun 1, 2024

@aescolar - your change requests have been made. Is there anything else?

Copy link
Member

@ycsin ycsin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refresh +1

@cfriedt
Copy link
Member Author

cfriedt commented Jun 2, 2024

Friendly ping to @peter-mitsis @andyross @jukkar . Can I solicit more refresh +1s?

@aescolar aescolar dismissed their stale review June 3, 2024 08:04

Addressed

Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the standard names. Not a big fan of renaming churn in general (makes bisection across long history a high impedance process because you need to remember what the conventions were at each point), but if we're going to do it "all in a single PR" is for sure the best way.

@dleach02 dleach02 merged commit fe1e7f8 into zephyrproject-rtos:main Jun 4, 2024
25 checks passed
@SeppoTakalo
Copy link
Collaborator

The Zephyr 3.7 migration guide still has this:

  • The DNS resolver and mDNS/LLMNR responders are converted to use socket service API.
    This means that the number of pollable sockets in the system might need to be increased.
    Please check that the values of :kconfig:option:CONFIG_NET_SOCKETS_POLL_MAX and
    :kconfig:option:CONFIG_POSIX_MAX_FDS are high enough. Unfortunately no exact values
    for these can be given as it depends on application needs and usage. (:github:72834)

should it be refactored so you don't need to recursively read the migration guide?

#define PTHREAD_RWLOCK_INITIALIZER (-1)
#define POSIX_RWLOCK_INITIALIZER (-1)
Copy link
Member

@fabiobaltieri fabiobaltieri Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @cfriedt is this correct? I bumped into a library that was using PTHREAD_RWLOCK_INITIALIZER (boringssl) and now does not build anymore, I see that /usr/include/pthread.h on my system defines PTHREAD_RWLOCK_INITIALIZER as well

If the change is intentional does this need a migration guide entry?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabiobaltieri - good find. PTHREAD_RWLOCK_INITIALIZER is the standard macro name. Are you good to submit a fix?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I can do that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there you go #73808

@fabiobaltieri
Copy link
Member

Another issue I bumped into this: the sysconfig change somehow collides with a macro in the stm32 hals, try:

west build -p -b nucleo_f410rb samples/posix/uname/ -DCONFIG_POSIX_SYSCONF_IMPL_FULL=y

/home/fabiobaltieri/zephyrproject/modules/hal/stm32/stm32cube/stm32f4xx/drivers/include/Legacy/stm32_hal_legacy.h:475:39: error: 'FLASH_PAGE_SIZE' undeclared (first use in this function); did you mean 'FLASH_CR_PSIZE'?
  475 | #define PAGESIZE                      FLASH_PAGE_SIZE

I think there's a macro collision for PAGESIZE, not sure what's the proper fix though

@cfriedt @erwango

@cfriedt
Copy link
Member Author

cfriedt commented Jun 5, 2024

Another issue I bumped into this: the sysconfig change somehow collides with a macro in the stm32 hals, try:

@fabiobaltieri - I believe this was fixed a while ago by Erwan. Did you west update?

@cfriedt
Copy link
Member Author

cfriedt commented Jun 5, 2024

The Zephyr 3.7 migration guide still has this:

  • The DNS resolver and mDNS/LLMNR responders are converted to use socket service API.
    This means that the number of pollable sockets in the system might need to be increased.
    Please check that the values of :kconfig:option:CONFIG_NET_SOCKETS_POLL_MAX and
    :kconfig:option:CONFIG_POSIX_MAX_FDS are high enough. Unfortunately no exact values
    for these can be given as it depends on application needs and usage. (:github:72834)

should it be refactored so you don't need to recursively read the migration guide?

Probably, yea. @SeppoTakalo are you good to submit a fix?

@fabiobaltieri
Copy link
Member

@fabiobaltieri - I believe this was fixed a while ago by Erwan. Did you west update?

~/zephyrproject/zephyr$ git fetch
~/zephyrproject/zephyr$ git checkout --detach main
HEAD is now at d3081e2f30f net: lwm2m: On write, use server selected block size
~/zephyrproject/zephyr$ west update
~/zephyrproject/zephyr$ west build -p -b nucleo_f410rb samples/posix/uname/ -DCONFIG_POSIX_SYSCONF_IMPL_FULL=y
<broken>

Looks like it's still a problem, maybe manifest is out of date?

@cfriedt
Copy link
Member Author

cfriedt commented Jun 5, 2024

Looks like it's still a problem, maybe manifest is out of date?

Need to update the manifest with zephyrproject-rtos/hal_stm32#209

@cfriedt cfriedt deleted the posix-deprecate-non-standard-config-options branch June 5, 2024 18:33
@fabiobaltieri
Copy link
Member

fabiobaltieri commented Jun 5, 2024

Need to update the manifest with zephyrproject-rtos/hal_stm32#209

Ah, I do have that commit, but that just removes the path from the include path list, the header is still being included:

In file included from /home/fabiobaltieri/zephyrproject/modules/hal/stm32/stm32cube/stm32f4xx/drivers/include/stm32f4xx_hal_def.h:30,
                 from /home/fabiobaltieri/zephyrproject/modules/hal/stm32/stm32cube/stm32f4xx/drivers/include/stm32f4xx_hal_rcc.h:27,
                 from /home/fabiobaltieri/zephyrproject/modules/hal/stm32/stm32cube/stm32f4xx/drivers/include/stm32f4xx_hal_conf.h:280,
                 from /home/fabiobaltieri/zephyrproject/modules/hal/stm32/stm32cube/stm32f4xx/drivers/include/stm32f4xx_hal.h:29,
                 from /home/fabiobaltieri/zephyrproject/modules/hal/stm32/stm32cube/stm32f4xx/soc/stm32f4xx.h:287,
                 from /home/fabiobaltieri/zephyrproject/zephyr/soc/st/stm32/stm32f4x/./soc.h:23,
                 from /home/fabiobaltieri/zephyrproject/zephyr/modules/cmsis/./cmsis_core_m.h:24,
                 from /home/fabiobaltieri/zephyrproject/zephyr/modules/cmsis/./cmsis_core.h:10,
                 from /home/fabiobaltieri/zephyrproject/zephyr/include/zephyr/arch/arm/asm_inline_gcc.h:24,
                 from /home/fabiobaltieri/zephyrproject/zephyr/include/zephyr/arch/arm/asm_inline.h:18,
                 from /home/fabiobaltieri/zephyrproject/zephyr/include/zephyr/arch/arm/arch.h:33,
                 from /home/fabiobaltieri/zephyrproject/zephyr/include/zephyr/arch/cpu.h:19,
                 from /home/fabiobaltieri/zephyrproject/zephyr/include/zephyr/kernel_includes.h:36,
                 from /home/fabiobaltieri/zephyrproject/zephyr/include/zephyr/kernel.h:17,
                 from /home/fabiobaltieri/zephyrproject/zephyr/arch/arm/core/offsets/offsets_aarch32.c:28,
                 from /home/fabiobaltieri/zephyrproject/zephyr/arch/arm/core/offsets/offsets.c:9:

@erwango could you look into it? There's quite a few explicit #include "Legacy/stm32_hal_legacy.h" in the hal

coreboot-org-bot pushed a commit to coreboot/chrome-ec that referenced this pull request Jun 6, 2024
Upstream migrated to a new set of posix options in
zephyrproject-rtos/zephyr#73047.

Change the fpmcu options to match the new ones, get rid of few
deprecation messages:

warning: Deprecated symbol GETENTROPY is enabled.
warning: Deprecated symbol POSIX_CLOCK is enabled.
warning: Deprecated symbol PTHREAD is enabled.
warning: Deprecated symbol PTHREAD_IPC is enabled.
warning: Deprecated symbol PTHREAD_KEY is enabled.
warning: Deprecated symbol PTHREAD_MUTEX is enabled.

BUG=none
TEST=cq dry run

Change-Id: I092bc50c37611023eff394478470e9a2af6e2059
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/5604463
Code-Coverage: Zoss <zoss-cl-coverage@prod.google.com>
Tested-by: Fabio Baltieri <fabiobaltieri@google.com>
Commit-Queue: Fabio Baltieri <fabiobaltieri@google.com>
Reviewed-by: Patryk Duda <patrykd@google.com>
Fymyte added a commit to Fymyte/zephyr that referenced this pull request Jun 19, 2024
With the update of the Kconfig symbols to new naming scheme,
CONFIG_PTHREAD does not exist anymore.
Use the new CONFIG_POSIX_THREADS instead

Follow-up: zephyrproject-rtos#73047
Signed-off-by: Pierrick Guillaume <pierguill@gmail.com>
nashif pushed a commit that referenced this pull request Jun 20, 2024
With the update of the Kconfig symbols to new naming scheme,
CONFIG_PTHREAD does not exist anymore.
Use the new CONFIG_POSIX_THREADS instead

Follow-up: #73047
Signed-off-by: Pierrick Guillaume <pierguill@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Documentation area: POSIX POSIX API Library Release Notes To be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

undefined reference to _open when using newlib with Zephyr3.3.99 and above
9 participants