Skip to content

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

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

Closed
aescolar opened this issue Jul 13, 2024 · 7 comments · Fixed by #75842 · May be fixed by zephyrproject-rtos/hal_altera#4
Closed

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

aescolar opened this issue Jul 13, 2024 · 7 comments · Fixed by #75842 · May be fixed by zephyrproject-rtos/hal_altera#4
Labels
bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug Regression Something, which was working, does not anymore

Comments

@aescolar
Copy link
Member

aescolar commented Jul 13, 2024

Describe the bug

  • samples/basic/hash_map/libraries.hash_map.minimal.open_addressing.djb2
  • samples/basic/hash_map/libraries.hash_map.minimal.separate_chaining.djb2
  • tests/crypto/mbedtls/crypto.mbedtls
  • tests/kernel/common/kernel.common.minimallibc
  • tests/kernel/context/kernel.context.minimallibc
  • tests/kernel/device/kernel.device.minimallibc
  • tests/kernel/early_sleep/kernel.common.sleep.minimallibc
  • tests/kernel/obj_tracking/kernel.objects.tracking.minimallibc
  • tests/kernel/pending/kernel.objects.minimallibc
  • tests/kernel/poll/kernel.poll.minimallibc

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

  • Failing CI in main

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

Environment (please complete the following information):

  • OS: Linux Ubuntu 22.04 CI
  • Toolchain: 0.16.8
  • Commit SHA or Version used: Main as of now 2e01266

Additional context
Introduced by 0155c6f
#75775

@aescolar aescolar added bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug Regression Something, which was working, does not anymore labels Jul 13, 2024
@aescolar
Copy link
Member Author

CC @nashif @ycsin

@kartben
Copy link
Collaborator

kartben commented Jul 13, 2024

@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
Copy link
Member

ycsin commented Jul 13, 2024

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
Copy link
Member

nashif commented Jul 13, 2024

@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
Copy link
Member

nashif commented Jul 13, 2024

$ 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?

nashif added a commit to nashif/zephyr that referenced this issue Jul 13, 2024
Do not build HAL uart driver, use in-tree driver instead.

Fixes zephyrproject-rtos#75837

Signed-off-by: Anas Nashif <[email protected]>
nashif added a commit that referenced this issue Jul 13, 2024
Do not build HAL uart driver, use in-tree driver instead.

Fixes #75837

Signed-off-by: Anas Nashif <[email protected]>
@cfriedt
Copy link
Member

cfriedt commented Jul 13, 2024

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.

zephyrproject-rtos/hal_altera#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
Copy link
Member

nashif commented Jul 16, 2024

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 zephyrproject-rtos/hal_altera#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?

Chenhongren pushed a commit to Chenhongren/zephyr that referenced this issue Aug 26, 2024
Do not build HAL uart driver, use in-tree driver instead.

Fixes zephyrproject-rtos#75837

(cherry picked from commit 74a7502)

Original-Signed-off-by: Anas Nashif <[email protected]>
GitOrigin-RevId: 74a7502
Change-Id: I6b5c657ae9d94527b6045017dac4d4d88a93fc22
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5705804
Reviewed-by: Keith Short <[email protected]>
Commit-Queue: Keith Short <[email protected]>
Tested-by: ChromeOS Prod (Robot) <[email protected]>
Tested-by: Keith Short <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug Regression Something, which was working, does not anymore
Projects
None yet
5 participants