lib: zstd: only require a C compiler#10316
lib: zstd: only require a C compiler#10316ThomasDevoogdt wants to merge 2 commits intofluent:masterfrom
Conversation
ThomasDevoogdt
requested review from
edsiper,
fujimotos,
koleini and
leonardo-albertovich
as code owners
ThomasDevoogdt
force-pushed
the
bugfix/lib-zstd-only-require-a-c-compiler
branch
from
a1b4202 to
1a33abe
Compare
|
(cc: @cosmo0920) the changes should not touch the bundled dependencies because when we upgrade those changes will be lost. I recommend trying to merge the changes in libzstd repo first |
|
@edsiper The changes are already upstream. See the git commit message. |
|
@edsiper @cosmo0920 this has been approved (and upstreamed) for quite some time, can this be merged? |
ThomasDevoogdt
force-pushed
the
bugfix/lib-zstd-only-require-a-c-compiler
branch
from
1a33abe to
2cc0058
Compare
|
@cosmo0920 @edsiper It's again a bit silent around this PR. Everything is approved and ready to be merged, on what do we exactly wait? Can you just merge this one? |
|
@cosmo0920 Can you merge this one? Or add milestone that might land anytime soon. I have the feeling that next means forgotten. |
|
We don't want to merge this PR without merging for upstream libzstd library itself. |
|
This commit facebook/zstd@769723a is not released as a stable version officially, so we need to wait for merging this. |
ThomasDevoogdt
force-pushed
the
bugfix/lib-zstd-only-require-a-c-compiler
branch
from
2cc0058 to
163b99c
Compare
ThomasDevoogdt
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemove system libzstd from CI and make zstd's C++ usage conditional: CMake no longer declares CXX by default, instead sets ZSTD_ENABLE_CXX from ZSTD_BUILD_TESTS, conditionally enables CXX, and refactors compilation flags to be parameter-driven per C/CXX/LD. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant CI as CI Workflow
participant CMake as CMake (configure)
participant Zstd as zstd CMakeLists
participant Flags as AddZstdCompilationFlags
Dev->>CI: Push PR
CI->>CI: Setup build env (omit system libzstd)
CI->>CMake: Run configure (no -DFLB_PREFER_SYSTEM_LIB_ZSTD)
CMake->>Zstd: Load CMakeLists
Zstd->>Zstd: set(ZSTD_ENABLE_CXX ${ZSTD_BUILD_TESTS})
alt ZSTD_ENABLE_CXX = ON
Zstd->>CMake: enable_language(CXX)
else ZSTD_ENABLE_CXX = OFF
Zstd->>CMake: skip CXX enablement
end
Zstd->>Flags: ADD_ZSTD_COMPILATION_FLAGS(ON, ZSTD_ENABLE_CXX, ON)
Flags-->>CMake: Apply per-language (C/CXX) and linker flags
CMake-->>CI: Generate build system
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 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: 0
🧹 Nitpick comments (1)
lib/zstd-1.5.7/build/cmake/CMakeLists.txt (1)
119-130: LGTM with maintenance concern: bundled dependency modification before upstream stable release.The conditional C++ enablement logic is technically correct:
- C++ is enabled only when tests are built (
ZSTD_ENABLE_CXXis tied toZSTD_BUILD_TESTS)- The macro call
ADD_ZSTD_COMPILATION_FLAGS(ON ZSTD_ENABLE_CXX ON)correctly passes C (always on), C++ (conditional), and linker (always on) flag enablement parametersHowever, per the PR discussion, modifying bundled dependencies (
lib/zstd-1.5.7/) before the upstream changes are released in a stable zstd version creates maintenance risk. When fluent-bit updates to the next stable zstd release, these local modifications will be overwritten, requiring re-application or coordination with upstream timing.Recommendation: Consider one of the following approaches to reduce maintenance burden:
- Wait for the upstream zstd stable release that includes commit 769723aee2540aaff8951ac432a1babed358aa71 before merging this PR
- If immediate merge is required, document this local modification in the fluent-bit repository (e.g., in a
ZSTD_PATCHES.mdfile) to ensure the changes are re-applied during the next zstd bundle update- Track the upstream zstd release schedule and coordinate the bundle update timing with this PR merge
Based on learnings from the PR discussion.
📜 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/zstd-1.5.7/build/cmake/CMakeLists.txt(1 hunks)lib/zstd-1.5.7/build/cmake/CMakeModules/AddZstdCompilationFlags.cmake(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-31T12:46:11.940Z
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 CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
.github/workflows/pr-compile-check.yaml
⏰ 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_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 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=Off, 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_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=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_JEMALLOC=Off, 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 (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=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_SANITIZE_MEMORY=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 (-DFLB_SMALL=On, 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=On, 3.31.6, clang, clang++)
🔇 Additional comments (5)
.github/workflows/pr-compile-check.yaml (2)
138-138: LGTM! Consistent with the goal of testing bundled zstd.Removing
libzstd-devinstallation from thepr-compile-without-cxxjob is correct since this job now tests the bundled zstd library (which no longer requires C++) rather than the system-provided one.
152-152: LGTM! Correctly uses bundled zstd.Removing the
-DFLB_PREFER_SYSTEM_LIB_ZSTD=ONflag ensures the job uses the bundled zstd library, which aligns with testing the C-only compilation capability.lib/zstd-1.5.7/build/cmake/CMakeModules/AddZstdCompilationFlags.cmake (2)
52-52: LGTM! Macro signature correctly extended for conditional flag control.The updated signature
ADD_ZSTD_COMPILATION_FLAGS(_C _CXX _LD)enables fine-grained control over which compiler flags to enable, supporting the conditional C++ enablement pattern introduced in the related CMakeLists.txt changes.
66-111: LGTM! All EnableCompilerFlag calls consistently updated.All calls to
EnableCompilerFlaghave been correctly updated to pass the new_C,_CXX, and_LDparameters, enabling conditional flag application based on language availability.lib/zstd-1.5.7/build/cmake/CMakeLists.txt (1)
36-40: LGTM! Correctly removes C++ from default languages.Removing
CXXfrom the initialLANGUAGESlist is appropriate for making zstd buildable with C-only toolchains. C++ is now enabled conditionally based onZSTD_BUILD_TESTS(see lines 119-124).
|
@cosmo0920 @edsiper I understand that this is not yet part of an upstream release, but the commit at least has been merged upstream, AND there are tests which are checking the functionality. So is there any reason to keep waiting for it and to not take this along? |
Upstream: facebook/zstd@769723a Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
This reverts commit e816c48. Not needed anymore since libzstd has been bumped. Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
ThomasDevoogdt
force-pushed
the
bugfix/lib-zstd-only-require-a-c-compiler
branch
from
163b99c to
b0d89f1
Compare
3 participants
Upstream: facebook/zstd@769723a
Summary by CodeRabbit
Chores
Refactor
Tests
No changes to user-facing functionality.