Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27b82dec27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if(FLB_SIMD_RISCV_COMPILER_FLAG) | ||
| set(FLB_SIMD_ENABLED Yes) | ||
| set(FLB_SIMD_RISCV_C_FLAGS "-march=rv64gcv_zba") |
There was a problem hiding this comment.
Don't auto-enable RVV from compiler support alone
On riscv64, Auto now flips on FLB_HAVE_SIMD whenever the compiler accepts -march=rv64gcv_zba. That only proves the toolchain can emit RVV instructions, not that the target CPU has RVV; include/fluent-bit/flb_simd.h:88-102 and the hot paths in src/flb_pack.c/src/flb_utils.c then compile unconditional RVV intrinsics. A generic riscv64 package built with a modern compiler will therefore SIGILL on the many riscv64 systems that lack the optional V extension, so Auto is no longer safe for that architecture.
Useful? React with 👍 / 👎.
| if(FLB_SIMD_MODE STREQUAL "ON") | ||
| message(FATAL_ERROR | ||
| "FLB_SIMD=On is only supported on x86_64, aarch64/arm64, and " | ||
| "riscv64 with RVV compiler support") |
There was a problem hiding this comment.
Preserve arm32
FLB_SIMD=On package builds
This new fatal-error branch turns every non-x86_64/aarch64/riscv64 -DFLB_SIMD=On build into a configure failure. That breaks an existing repo build path: packaging/distros/raspbian/Dockerfile:66-81 still passes -DFLB_SIMD=On, packaging/README.md:49-51 documents those targets as arm32v7, and .github/workflows/call-build-linux-packages.yaml:105-130 still runs the raspbian/* package jobs. Before this change those builds fell back to the scalar FLB_SIMD_NONE path in include/fluent-bit/flb_simd.h, so the packaging workflow now stops at CMake configure time.
Useful? React with 👍 / 👎.
|
CI errors are associated to TCP ports |
| if(NOT FLB_SIMD_MODE STREQUAL "OFF") | ||
| set(FLB_SIMD_SUPPORTED_ARCH No) | ||
|
|
||
| if(CMAKE_SYSTEM_PROCESSOR MATCHES "^(x86_64|AMD64|amd64)$") |
There was a problem hiding this comment.
This check seems to enable SIMD support (FLB_SIMD_ENABLED == Yes) for x86 32bit Windows build which breaks it. Refer to https://github.com/fluent/fluent-bit/actions/runs/23405338585/job/68083237733#logs:
flb_pack.c.obj : error LNK2019: unresolved external symbol _flb_vector8_eq referenced in function _is_float ..\bin\fluent-bit.dll : fatal error LNK1120: 1 unresolved externals
According to CMAKE_SYSTEM_PROCESSOR documentation:
When not cross-compiling, this variable has the same value as the CMAKE_HOST_SYSTEM_PROCESSOR variable.
It means that if CI uses x86 64bit hardware (which seems to be so), then x86 32bit Windows build gets SIMD enabled in all places which rely on FLB_HAVE_SIMD macro.
Maybe it can be fixed this way:
| if(CMAKE_SYSTEM_PROCESSOR MATCHES "^(x86_64|AMD64|amd64)$") | |
| if(CMAKE_SYSTEM_PROCESSOR MATCHES "^(x86_64|AMD64|amd64)$" AND (CMAKE_SIZEOF_VOID_P GREATER 4)) |
configure.
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
Release Notes