search

zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.88k stars 6.63k forks source link

Build failures with minimal libC for qemu_nios2 in main CI #75837

Closed aescolar closed 4 months ago

aescolar commented 4 months ago

Describe the bug

Fail to build in qemu_nios2

To Reproduce

Steps to reproduce the behavior:

  1. mkdir build; cd build
  2. cmake -GNinja -DBOARD=qemu_nios2 ../tests/kernel/common/ -DCONFIG_MINIMAL_LIBC=y
  3. ninja
  4. See error

OR

twister -p qemu_nios2 -T tests/kernel/common/ -s kernel.common.minimallibc

Expected behavior No build failures

Impact

Logs and console output https://github.com/zephyrproject-rtos/zephyr/actions/runs/9915851294/job/27397451863#step:11:2509

Environment (please complete the following information):

Additional context Introduced by 0155c6f87fca523e52b7578dfcf97cbaca0a0ad3

75775

aescolar commented 4 months ago

CC @nashif @ycsin

kartben commented 4 months ago

@nashif time to maybe pull the plug on Nios-II eh? While this obviously won't fix the bug in the module, I think excluding qemu_nios2 from these tests is probably an acceptable fix.

ycsin commented 4 months ago

No errors when:

$ west build -b qemu_nios2 -p auto \
    -T zephyr/samples/basic/hash_map/libraries.hash_map.minimal.open_addressing.djb2 \
        -- -DCONFIG_POSIX_API=y -DCONFIG_POSIX_TIMERS=y

75840

nashif commented 4 months ago

@nashif time to maybe pull the plug on Nios-II eh? While this obviously won't fix the bug in the module, I think excluding qemu_nios2 from these tests is probably an acceptable fix.

this is not related to nios2, this is a driver that is needed by others beside nios2 qemu platforms.

It just happens nios2 is part of default CI, now I am concerned about anyone else still including this header, our deprecation was soft and was just sending a message and not failing builds. A quick look at module code, we still have many places using these headers.

nashif commented 4 months ago

$ west build -b qemu_nios2 -p auto \ -T zephyr/samples/basic/hash_map/libraries.hash_map.minimal.open_addressing.djb2 \ -- -DCONFIG_POSIX_API=y -DCONFIG_POSIX_TIMERS=y

this is interesting, this basically means we force use of complete POSIX in cases where a type or a define is needed from a posix header. Any 3rd party library that has been using such headers in the past will now have to depend on CONFIG_POSIX_API and what comes with it just to include the headers and use something out of them? A lonely type or a define?

cfriedt commented 4 months ago

this is interesting, this basically means we force use of complete POSIX in cases where a type or a define is needed from a posix header. Any 3rd party library that has been using such headers in the past will now have to depend on CONFIG_POSIX_API and what comes with it just to include the headers and use something out of them? A lonely type or a define?

@nashif - your diagnosis / conclusion here was incorrect. Maybe next time just ask?

There is a very simple solution that should have been adopted over two and a half years ago when Google required use of the <zephyr/posix/..> prefix, which was also before I took on posix maintainership.

So actually, nothing new, but because we had a deprecated header in tree and because this hal is unmaintained, I guess nobody took notice.

https://github.com/zephyrproject-rtos/hal_altera/pull/4

But generally, it's a bad idea to try and use a feature that isn't enabled. Also, using posix in drivers is a bad idea because it creates a dependency cycle.

Those are the same two antipatterns that I pointed out a while ago in #75513

nashif commented 4 months ago

this is interesting, this basically means we force use of complete POSIX in cases where a type or a define is needed from a posix header. Any 3rd party library that has been using such headers in the past will now have to depend on CONFIG_POSIX_API and what comes with it just to include the headers and use something out of them? A lonely type or a define?

@nashif - your diagnosis / conclusion here was incorrect. Maybe next time just ask?

and what part of the above exactly is not a question? There are 2 question marks in the above paragraph.

The patch in this PR enables CONFIG_POSIX_API and CONFIG_POSIX_TIMERS and that is what I am commenting on. In https://github.com/zephyrproject-rtos/hal_altera/pull/4 (which changes a driver that is not used in Zephyr anymore) adds Zephyr specific header into 3rd party code, that is not going to always work, such HALs have users other than zephyr. Anyways,. the issue was resolved already, no need to do anything about it now.

Google required use of the <zephyr/posix/..> prefix

Hmm, what are you referring to here exactly? are you referring to the introduction of zephyr/ prefix to headers? Google as someone from google pushing for this change in zephyr?