workflows: add sanity check for toolchains w/ CXX support #9277
Conversation
ThomasDevoogdt
requested review from
celalettin1286,
edsiper,
fujimotos,
koleini,
leonardo-albertovich,
niedbalski and
patrick-stephens
as code owners
ThomasDevoogdt
force-pushed
the
feature/enforce-no-cxx-support
branch
from
06b1ed2 to
7cb18ea
Compare
ThomasDevoogdt
changed the title
ThomasDevoogdt
force-pushed
the
feature/enforce-no-cxx-support
branch
3 times, most recently
from
ddf2522 to
a3c1402
Compare
|
Hi, we don't want to modify the vendored libraries by ourselves. |
Understand that, see my other PR/ remark #7765 (comment). There is an upstream PR: confluentinc/librdkafka#4366, but no reply in over a year. |
ThomasDevoogdt
force-pushed
the
feature/enforce-no-cxx-support
branch
from
a3c1402 to
d734a74
Compare
|
Hi @cosmo0920, is it possible to re-evaluate this? I don't get an answer from upstream, but the changes are relevant IMHO. So I think that we should go forward with it. I know that changes exists that this issue wrt librdkafka gets re-introduced when syncing upstream, but the added workflows will tell us about this at that time. |
ThomasDevoogdt
force-pushed
the
feature/enforce-no-cxx-support
branch
from
d734a74 to
6c93936
Compare
ThomasDevoogdt
force-pushed
the
feature/enforce-no-cxx-support
branch
from
6c93936 to
539bf0a
Compare
|
@edsiper It has been a while. Any chance that this gets approved? I try to reduce my own patch list. |
ThomasDevoogdt
force-pushed
the
feature/enforce-no-cxx-support
branch
from
539bf0a to
f4e72aa
Compare
|
@patrick-stephens @edsiper I see that 4.0.0 has landed, this is the ideal moment to consider this PR. |
ThomasDevoogdt
force-pushed
the
feature/enforce-no-cxx-support
branch
4 times, most recently
from
c56a9ce to
dfc566c
Compare
|
@leonardo-albertovich Ok, thanks for the clear explanation.
I have a PR open for all the dependencies, can you help to merge them?:
I will then rebase this branch. |
|
I don't know if @edsiper prefers to make the updates himself once the upstream PRs are merged or if he's ok with those PRs you've opened. I take that the PRs you're opening when the upstream PRs are merged are complete updates and not just what you changed right? Otherwise they obviously wouldn't be viable. |
Then I can wait for quite some time. Is it also requested to wait for the next upstream release tag?
The changes I propose are quite minimal, I don't see why it can't be taken along as is. Otherwise, I don't see this branch merged in the first year. Also, the workflow file will anyway assert if things get out of sync. |
|
That's up to @edsiper but please be mindful of the fact that even though fluent-bit might be a minor concern for you and you'll forget about it once you get your changes merged we will still be accountable for it so we want to ensure that things are done in a way that won't come back to bite us in the back in the future. TBH I could've seen these dependency patches merged much faster if you implemented them as patches that are applied during the build process but you didn't seem to care so there's not much I can do to help you. |
@leonardo-albertovich, is there such system in fluent-bit to apply patches in the build process? If yes, then I can for sure use that. Or do you expect that I create such patch system? It obviously would make sense to have that. And about caring, I do care about compatibility, before I started the Buildroot integration, a dozen of compilation targets were failing due to all kind of reasons. E.g. Libc usage (uClibc, musl, glibc), Linux versions, Builder back-ends (make, ninja), toolchains (GCC versions, with/without C++ support), and so on. I'm thankful that you guys accepted most of my PRs, this PR and a few others are some left overs. FYI, this the Buildroot package: https://github.com/buildroot/buildroot/tree/master/package/fluent-bit. |
|
There is no such system in fluent-bit and I just had to tell someone else about it in issue #10139 because we might need to patch the I think I'd do it within cmake and try to figure out something that doesn't require third party tools or something minimal because that could be a problem considering the platforms we support. |
ThomasDevoogdt
force-pushed
the
feature/enforce-no-cxx-support
branch
3 times, most recently
from
100d95a to
699cffb
Compare
ThomasDevoogdt
force-pushed
the
feature/enforce-no-cxx-support
branch
from
699cffb to
6e73c77
Compare
ThomasDevoogdt
force-pushed
the
feature/enforce-no-cxx-support
branch
from
6e73c77 to
a64d7ca
Compare
WalkthroughAdds a new GitHub Actions job Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Pull Request
participant GH as GitHub Actions
participant Runner as ubuntu-24.04 Runner
participant Setup as setup-cmake (jwlawson/actions-setup-cmake@v2)
participant Checkout as actions/checkout@v5
participant Env as Build Env
participant CMake as cmake
participant Make as make
Dev->>GH: Open/Update PR
GH->>Runner: Trigger job pr-compile-without-cxx
Runner->>Env: Install packages (bison, flex, gcc, libssl-dev, libyaml-dev, libzstd-dev, librdkafka-dev)
Runner->>Setup: Install cmake (matrix cmake_version: 3.31.6)
Runner->>Checkout: Checkout repo
Runner->>Env: Set CXX=/bin/false
Runner->>Env: Determine CPU count (cap at 8)
Runner->>CMake: Configure with flags
Note right of CMake: -DFLB_PREFER_SYSTEM_LIB_ZSTD=ON<br/>-DFLB_PREFER_SYSTEM_LIB_KAFKA=ON
CMake-->>Runner: Generate build system
Runner->>Make: make -j <cpus> (in build/)
Make-->>Runner: Compilation result
Runner-->>GH: Job status (success/failure)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-compile-check.yaml (1)
135-145: Build directory is never created; steps will fail with missing working-directoryYou set working-directory: build but don’t create it. The step will fail before cmake runs.
- name: Checkout Fluent Bit code uses: actions/checkout@v5 + + - name: Prepare build directory + run: mkdir -p build
🧹 Nitpick comments (4)
.github/workflows/pr-compile-check.yaml (4)
122-124: Correct wording: this job compiles without C++Comment and naming say “w/ CXX support” but the job enforces no C++ (CXX=/bin/false). Update to avoid confusion.
- # Sanity check for compilation w/ CXX support + # Sanity check for compilation without CXX support pr-compile-without-cxx:
129-134: Add pkg-config to ensure system Zstd detectionCMake find scripts and some FindZstd.cmake paths rely on pkg-config; without it you may silently fall back to bundled libs.
- sudo apt-get install -y bison cmake flex gcc libssl-dev libyaml-dev - sudo apt-get install -y libzstd-dev + sudo apt-get install -y bison cmake flex gcc libssl-dev libyaml-dev pkg-config libzstd-dev
135-137: Align checkout action with the rest of the workflowOther jobs use actions/checkout@v5. Keep versions consistent.
- - name: Checkout Fluent Bit code - uses: actions/checkout@v4 + - name: Checkout Fluent Bit code + uses: actions/checkout@v5
138-144: Rename step and harden the “no C++” guarantee
- Rename step to “without CXX”.
- Also pass CMAKE_CXX_COMPILER=/bin/false to cmake for robustness (env CXX is a hint; this makes it explicit).
- - name: Compile w/ CXX support + - name: Compile without CXX support run: | export CXX=/bin/false export nparallel=$(( $(getconf _NPROCESSORS_ONLN) > 8 ? 8 : $(getconf _NPROCESSORS_ONLN) )) - cmake -DPREFER_SYSTEM_LIB_ZSTD=ON -DFLB_KAFKA=OFF ../ + cmake -DCMAKE_CXX_COMPILER=/bin/false -DPREFER_SYSTEM_LIB_ZSTD=ON -DFLB_KAFKA=OFF ../ make -j $nparallel working-directory: build
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/pr-compile-check.yaml(1 hunks)
🔇 Additional comments (1)
.github/workflows/pr-compile-check.yaml (1)
142-142: Fix CMake Zstd flag
In .github/workflows/pr-compile-check.yaml (line 142), change-DPREFER_SYSTEM_LIB_ZSTD=ONto-DFLB_PREFER_SYSTEM_LIB_ZSTD=ON— the CMake option is defined as FLB_PREFER_SYSTEM_LIB_ZSTD.⛔ Skipped due to learnings
Learnt from: shadowshot-x PR: fluent/fluent-bit#10794 File: tests/internal/aws_compress.c:93-107 Timestamp: 2025-08-29T06:25:27.222Z Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.Learnt from: shadowshot-x PR: fluent/fluent-bit#10794 File: src/aws/flb_aws_compress.c:52-56 Timestamp: 2025-08-29T06:24:55.819Z Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.Learnt from: shadowshot-x PR: fluent/fluent-bit#10794 File: src/aws/flb_aws_compress.c:26-26 Timestamp: 2025-08-29T06:24:44.771Z Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.Learnt from: shadowshot-x PR: fluent/fluent-bit#10794 File: tests/internal/aws_compress.c:93-107 Timestamp: 2025-08-29T06:25:27.222Z Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.Learnt from: shadowshot-x PR: fluent/fluent-bit#10794 File: tests/internal/aws_compress.c:39-42 Timestamp: 2025-08-29T06:24:26.125Z Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.Learnt from: shadowshot-x PR: fluent/fluent-bit#10794 File: tests/internal/aws_compress.c:7-7 Timestamp: 2025-08-29T06:25:02.540Z Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
ThomasDevoogdt
force-pushed
the
feature/enforce-no-cxx-support
branch
from
a64d7ca to
389daf6
Compare
|
@leonardo-albertovich I updated this PR to exclude upstream packages libzstd and librdkafka. If the changes for these make their way upstream, then this workflow can be adapted. Can you have a look, I would appreciate if this can be merged. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-compile-check.yaml (1)
143-153: Create the build directory before using it as working-directory.This step uses working-directory: build but no prior step creates it in this job. Without the directory, the step will fail immediately.
- name: Checkout Fluent Bit code uses: actions/checkout@v5 + - name: Create build directory + run: mkdir -p build + - - name: Compile without CXX + - name: Compile without CXX run: | export CXX=/bin/false export nparallel=$(( $(getconf _NPROCESSORS_ONLN) > 8 ? 8 : $(getconf _NPROCESSORS_ONLN) )) - cmake -DPREFER_SYSTEM_LIB_ZSTD=ON -DFLB_PREFER_SYSTEM_LIB_KAFKA=ON ../ + cmake -DPREFER_SYSTEM_LIB_ZSTD=ON -DFLB_PREFER_SYSTEM_LIB_KAFKA=ON ../ make -j $nparallel working-directory: build
🧹 Nitpick comments (2)
.github/workflows/pr-compile-check.yaml (2)
122-122: Fix mislabeled comments/step: it's “without CXX,” not “with CXX”.Both the job comment and step name contradict the intent (CXX is disabled). Update the labels to avoid confusion when scanning CI logs.
- # Sanity check for compilation w/ CXX support + # Sanity check for compilation without CXX support ... - - name: Compile w/ CXX support + - name: Compile without CXXAlso applies to: 146-146
135-141: Avoid installing cmake via apt when using actions-setup-cmake.You already pin cmake with jwlawson/actions-setup-cmake@v2. Installing cmake from apt adds noise and can cause version confusion.
- sudo apt-get install -y bison cmake flex gcc libssl-dev libyaml-dev + sudo apt-get install -y bison flex gcc libssl-dev libyaml-dev
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/pr-compile-check.yaml(1 hunks)
Fluent-bit is written in C, so don't require CXX, also not from the libs. Signed-off-by: Thomas Devoogdt <[email protected]>
ThomasDevoogdt
force-pushed
the
feature/enforce-no-cxx-support
branch
from
389daf6 to
83bbde0
Compare
This is a temporary fix, until facebook/zstd@769723a makes its way to a release. Please revert this commit once we bump to the next libzstd version. Signed-off-by: Thomas Devoogdt <[email protected]>
Upstream is not really amused with the proposal to allow compilation without CXX support, see confluentinc/librdkafka#4366. So disable it from the check for now. Signed-off-by: Thomas Devoogdt <[email protected]>
ThomasDevoogdt
force-pushed
the
feature/enforce-no-cxx-support
branch
from
83bbde0 to
a3b205b
Compare
|
@edsiper @cosmo0920 @patrick-stephens @leonardo-albertovich can someone have a look? |
4 participants
Fluent-bit is written in C. There is no reason to have a CXX compiler. (Except for some libs which are all individually optional.) So add a check that enforces a C-only toolchain. E.g. useful for buildroot which does support platforms with only a C compiler.
Upstream status:
CMakeLists.txt: allow compilation without CXX support confluentinc/librdkafka#4366
[cmake] only require a CXX compiler when tests are build facebook/zstd#4357
CMakeLists.txt: only require a C compiler zhaozg/luajit-cmake#17
CMakeLists.txt: only require a C compiler richgel999/miniz#338
Summary by CodeRabbit