lib: librdkafka: only require a C compiler #10341
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ThomasDevoogdt
requested review from
edsiper,
fujimotos,
koleini and
leonardo-albertovich
as code owners
ThomasDevoogdt
force-pushed
the
bugfix/lib-kafka-only-require-a-c-compiler
branch
2 times, most recently
from
e69a7aa to
d9a9108
Compare
Upstream: confluentinc/librdkafka#4366 Signed-off-by: Thomas Devoogdt <[email protected]>
This reverts commit 32fc9dd. Not needed anymore since librdkafka has been fixed. Signed-off-by: Thomas Devoogdt <[email protected]>
ThomasDevoogdt
force-pushed
the
bugfix/lib-kafka-only-require-a-c-compiler
branch
from
d9a9108 to
0a124d4
Compare
ThomasDevoogdt
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
WalkthroughThe workflow stops installing system librdkafka and removes the CMake flag preferring system Kafka. The vendored librdkafka CMake adds C/C++ language detection and conditionally builds C++ components and examples only when a C++ compiler is available. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer/CI
participant CMake as CMake Config
participant Toolchain as Toolchain
participant Targets as Build Targets
Dev->>CMake: Configure project
CMake->>Toolchain: Check C compiler
Toolchain-->>CMake: C found
CMake->>Toolchain: Check C++ compiler
alt C++ compiler available
Toolchain-->>CMake: C++ found
CMake->>Targets: Enable C and C++\nAdd src and src-cpp\nEnable C++ examples/tests
else No C++ compiler
Toolchain-->>CMake: C++ not found
CMake->>Targets: Enable C only\nAdd src\nSkip src-cpp/examples/tests (C++)
Note over CMake,Targets: C-only build proceeds
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
lib/librdkafka-2.10.1/CMakeLists.txt (1)
304-307: Also gate tests on actual C++ availabilityEven without the
option()call, a cache entry like-DRDKAFKA_BUILD_TESTS=ONkeeps this block true. On hosts lacking a usable C++ toolchain (the exact case this PR targets), we would still dive intoadd_subdirectory(tests)and the build fails. Please guard this block with the sameCMAKE_CXX_COMPILERcheck used forsrc-cpp, e.g.:-if(RDKAFKA_BUILD_TESTS) +if(CMAKE_CXX_COMPILER AND RDKAFKA_BUILD_TESTS) enable_testing() add_subdirectory(tests) endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pr-compile-check.yaml(2 hunks)lib/librdkafka-2.10.1/CMakeLists.txt(3 hunks)lib/librdkafka-2.10.1/examples/CMakeLists.txt(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ThomasDevoogdt
PR: fluent/fluent-bit#9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
Comment on lines
+9
to
+15
| include(CheckLanguage) | ||
| check_language(CXX) | ||
| if(CMAKE_CXX_COMPILER) | ||
| enable_language(CXX) | ||
| else() | ||
| message(STATUS "C++ compiler not found, skipping C++ support") | ||
| endif() |
There was a problem hiding this comment.
Handle bogus CXX values before calling enable_language
When CXX is preset to something non-working (e.g. our no-C++ workflow exports /bin/false), check_language(CXX) still leaves CMAKE_CXX_COMPILER pointing at that path instead of *-NOTFOUND. The current if (CMAKE_CXX_COMPILER) conditional therefore fires and enable_language(CXX) aborts configuration, defeating the “C-only” fallback we’re trying to enable. Please tighten the guard so we only enable C++ when CMAKE_CXX_COMPILER is both defined and not *-NOTFOUND. One way is:
-check_language(CXX)
-if(CMAKE_CXX_COMPILER)
+check_language(CXX)
+if(CMAKE_CXX_COMPILER AND NOT CMAKE_CXX_COMPILER MATCHES "-NOTFOUND$")
enable_language(CXX)
else()
message(STATUS "C++ compiler not found, skipping C++ support")
endif()🤖 Prompt for AI Agents
In lib/librdkafka-2.10.1/CMakeLists.txt around lines 9-15, the current guard
uses if(CMAKE_CXX_COMPILER) which can be true for bogus values like /bin/false
left by check_language(CXX), causing enable_language(CXX) to abort; change the
conditional to only enable C++ when the compiler is both defined and not a
NOTFOUND marker, e.g. use if(DEFINED CMAKE_CXX_COMPILER AND NOT
CMAKE_CXX_COMPILER MATCHES "-NOTFOUND$") then enable_language(CXX) else emit the
existing status message.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Upstream: confluentinc/librdkafka#4366
Summary by CodeRabbit
Bug Fixes
Chores