fluent-bit icon indicating copy to clipboard operation
fluent-bit copied to clipboard

build: Tweak linker flags for zstd

Open cosmo0920 opened this issue 1 month ago • 11 comments

This is because vendored libzstd will cause symbol collisions. So, we need to conceal the exported symbols from zstd.

Related to https://github.com/fluent/fluent-bit/issues/11068.


Enter [N/A] in the box, if an item is not applicable to your change.

Testing Before we can approve your change; please submit the following in a comment:

  • [ ] Example configuration file for the change
  • [ ] Debug log output from testing the change
  • [ ] Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • [ ] Run local packaging test showing all targets (including any new ones) build.
  • [ ] Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • [ ] Documentation required for this feature

Backporting

  • [ ] Backport to latest stable release.

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

  • Chores
    • Added a build-time toggle to prefer the system zstd library, propagated across multi-stage distro builds (AlmaLinux, CentOS, Rocky Linux) and threaded into the build configuration.
    • Ensure zstd development packages are installed where needed and the build honors the chosen preference during packaging.
    • Build system now prefers a found system zstd, falls back to pkg-config, and otherwise builds a bundled zstd with appropriate visibility.

✏️ Tip: You can customize this high-level summary in your review settings.

cosmo0920 avatar Nov 05 '25 12:11 cosmo0920

[!NOTE]

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds a FLB_PREFER_SYSTEM_LIB_ZSTD build-time flag propagated through multi-stage Docker builds (AlmaLinux, CentOS, Rocky Linux) into the CMake configure step; CMake now prefers a discovered system ZSTD, falls back to pkg-config, and otherwise builds a local zstd with hidden visibility for the static library.

Changes

Cohort / File(s) Summary
Dockerfiles — AlmaLinux
packaging/distros/almalinux/Dockerfile
Add ARG/ENV FLB_PREFER_SYSTEM_LIB_ZSTD across base variants (8/9/10 arm64v8) and stages, propagate it through stages, pass to CMake via -DFLB_PREFER_SYSTEM_LIB_ZSTD, and add libzstd-devel where applicable.
Dockerfiles — CentOS
packaging/distros/centos/Dockerfile
Introduce ARG/ENV FLB_PREFER_SYSTEM_LIB_ZSTD in build stages, propagate via ENV, forward to CMake with -DFLB_PREFER_SYSTEM_LIB_ZSTD, and set variant-specific defaults.
Dockerfiles — Rocky Linux
packaging/distros/rockylinux/Dockerfile
Add ARG/ENV FLB_PREFER_SYSTEM_LIB_ZSTD across base stages (8/9/10), propagate through stages, forward to CMake, and include libzstd-devel in variants preferring system zstd.
CMake ZSTD discovery / local build
CMakeLists.txt, cmake/zstd.cmake
Prefer find_package(ZSTD 1.4.8 QUIET), then pkg-config fallback (create imported ZSTD::ZSTD target), else include cmake/zstd.cmake to build local libzstd_static. Apply hidden visibility to local static lib on non-MSVC and set LIBZSTD_LIBRARIES. Thread FLB_PREFER_SYSTEM_LIB_ZSTD into CMake options.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Docker as Docker (multi-stage)
    participant Builder as Builder stage
    participant CMake as CMake
    participant Sys as System / pkg-config
    participant Local as Local zstd build

    Docker->>Builder: pass ARG/ENV FLB_PREFER_SYSTEM_LIB_ZSTD
    Builder->>CMake: cmake -DFLB_PREFER_SYSTEM_LIB_ZSTD=...
    CMake->>Sys: find_package(ZSTD) (system)
    alt system ZSTD found
        Sys-->>CMake: ZSTD::ZSTD (system)
        CMake-->>Builder: link system ZSTD
    else pkg-config provides libs
        CMake->>Sys: pkg-config search -> create imported ZSTD::ZSTD
        Sys-->>CMake: imported ZSTD::ZSTD (pkg-config)
        CMake-->>Builder: link pkg-config ZSTD
    else none found
        CMake->>Local: include cmake/zstd.cmake -> build libzstd_static
        Local-->>CMake: libzstd_static (hidden visibility on non-MSVC)
        CMake-->>Builder: link local libzstd_static
    end
    Builder-->>Docker: produce final artifact with chosen ZSTD

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Verify ARG/ENV propagation and default values across all Docker stages/variants.
  • Confirm libzstd-devel is added only where intended.
  • Validate CMake branches: find_package, pkg-config imported-target creation, and fallback to cmake/zstd.cmake, including visibility flags and passing of FLB_PREFER_SYSTEM_LIB_ZSTD.

Suggested reviewers

  • patrick-stephens
  • fujimotos
  • niedbalski
  • celalettin1286

Poem

🐰 I hop through Dockerfiles and CMake lines,
I carry a flag so zstd knows the signs,
System then pkg, else compile with care,
Stages pass the token and artifacts share,
A crunchy carrot build — hop, build, prepare! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'build: Tweak linker flags for zstd' directly relates to the main changes in the changeset, which introduce FLB_PREFER_SYSTEM_LIB_ZSTD configuration to control zstd library preference across multiple Dockerfiles and CMakeLists.txt, fundamentally addressing linker flag adjustments for zstd symbol visibility.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch cosmo0920-tweak-linker-flags-for-zstd

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 05 '25 12:11 coderabbitai[bot]

Do we have a linked issue and how do we verify it? What's the actual problem?

patrick-stephens avatar Nov 05 '25 12:11 patrick-stephens

@cosmo0920 do we need to do this for Rocky Linux 10 as well and maybe Centos stream 10

patrick-stephens avatar Dec 10 '25 10:12 patrick-stephens

@cosmo0920 do we need to do this for Rocky Linux 10 as well and maybe Centos stream 10

Not sure. If those of distributions use libzstd 1.5.5 as a system zstd library, we need to change the behavior of package creations.

cosmo0920 avatar Dec 10 '25 12:12 cosmo0920

@cosmo0920 do we need to do this for Rocky Linux 10 as well and maybe Centos stream 10

Not sure. If those of distributions use libzstd 1.5.5 as a system zstd library, we need to change the behavior of package creations.

Almalinux 10 (for RHEL 10), centos stream10 and rockylinux 10 use and install libzstd 1.5.5. So, we also need to manage this.

cosmo0920 avatar Dec 11 '25 08:12 cosmo0920

@cosmo0920 is there a reason we do not just enable this always? What's the issue there? It may be that some folks can install the specific problematic version of the dependency in quite a few different ways even on other targets - including future ones.

patrick-stephens avatar Dec 11 '25 10:12 patrick-stephens

@cosmo0920 is there a reason we do not just enable this always? What's the issue there? It may be that some folks can install the specific problematic version of the dependency in quite a few different ways even on other targets - including future ones.

This is because there's no issue until RHEL10 causes symbol collisions on their environments.

cosmo0920 avatar Dec 11 '25 12:12 cosmo0920

@cosmo0920 is there a reason we do not just enable this always? What's the issue there? It may be that some folks can install the specific problematic version of the dependency in quite a few different ways even on other targets - including future ones.

This is because there's no issue until RHEL10 causes symbol collisions on their environments.

Yeah but if we enable it for everything then will it cause problems for legacy targets? I'm just thinking it is simpler to enable for all then to pick and choose targets, plus keep updating them as they update their dependencies (e.g. what if Rocky/Alma/Centos go with libzstd 1.5 in their repos for 9?)

patrick-stephens avatar Dec 11 '25 14:12 patrick-stephens

@codex review

edsiper avatar Dec 11 '25 15:12 edsiper

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

You don't have the ability to clone this repository.
ℹ️ 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".

@codex review

edsiper avatar Dec 11 '25 19:12 edsiper

@codex review

cosmo0920 avatar Dec 12 '25 13:12 cosmo0920

@codex review

edsiper avatar Dec 12 '25 16:12 edsiper