fluent-bit
fluent-bit copied to clipboard
build: use the system provided LuaJIT if found
e.g. buildroot has logic to build luajit, so if pkg_check_modules can find a suitable version, then use that one
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:
- [N/A] Example configuration file for the change
- [N/A] Debug log output from testing the change
- [N/A] 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.
- [N/A] Run local packaging test showing all targets (including any new ones) build.
- [ ] Set
ok-package-testlabel to test for all targets (requires maintainer to do).
Documentation
- [N/A] 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.
I'm not entirely sure if this is something we want to do as it could introduce some variations which could make troubleshooting harder in the future.
Additionally, I think the people who have been involved in LuaJIT related issues or PRs should weigh in because I remember there were some things related to arm windows, macos, risc linux and ppcle and I don't know if all those are closed cases and what actions we took on each case so it'd be good to double check so we don't end up causing unnecessary issues.
Once I approved this change but I wanted to ask you about the possibility to handle an additional variable to make opt-in for this feature. This is because once this feature is merged, cmake of fluent-bit always refers to the system LuaJIT if found. This is unconsciously breaking changes for used LuaJIT libraries whether from a system or bundled one.
That's a good point actually @cosmo0920 and we should add that to ensure backwards compatibility - we had similar breaking changes recently with some systemd configuration.
I'm still wondering if it's not better to have a more generic flag which encourages for every library to use pkg_config one. This would reduce the complexity for buildroot quite a bit since most of the used libraries already have their custom handling.
Hmm…, it’s reasonable.
Maybe using FLB_PREFER_SYSTEM_LIBS should be better to handle them.
if we have a couple of dependencies that are provided as built-in libraries (such as now), how "FLB_PREFER_SYSTEM_LIBS" provides the flexibility to the user to choose to which ones must be used from the system instead of the bundled ones ?
if we have a couple of dependencies that are provided as built-in libraries (such as now), how "FLB_PREFER_SYSTEM_LIBS" provides the flexibility to the user to choose to which ones must be used from the system instead of the bundled ones ?
Luajit has quite some specialties related to architecture support. This is already handled in buildroot by the Luajit package: https://git.buildroot.net/buildroot/tree/package/luajit/Config.in
So it's far easier to link against the provided version than re-doing it here. Also the install target is not completely correct since it's overriding the buildroot install. So the "FLB_PREFER_SYSTEM_LIBS" flag indicates that cmake should search for system installed libraries, and if not found, then the static version is compiled anyway. This commit starts with Luajit, but other libraries might follow.
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.
Could you rebase this @ThomasDevoogdt ?
Could you rebase this @ThomasDevoogdt ?
Sure, I've rebased the branch.
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.
While I agree this works for LuaJIT I still think we need to provide granular control per lib, starting with Luajit is a good start, then FLB_PREFER_SYSTEM_LIBS can force all those libraries to use the version provided by the system.
ref: https://github.com/fluent/fluent-bit/pull/7286#issuecomment-1536937724
e.g:
FLB_PREFER_SYSTEM_LIBS= off (turn off all system libs option)FLB_PREFER_SYSTEM_LIB_LUAJIT= offFLB_PREFER_SYSTEM_LIB_C_ARES= off- ..
Hi @edsiper,
I will have a look to add this FLB_PREFER_SYSTEM_LIB_LUAJIT flag. I have another PR open (https://github.com/fluent/fluent-bit/pull/8930), which already does the same for e.g. c-ares. I will first update this one, and then update the other. Will ping you if I have an update.
Kr,
Thomas
Hi @edsiper, @cosmo0920, @patrick-stephens,
I've updated this PR according to the given remark. All build checks seems to be fine. My other PR (https://github.com/fluent/fluent-bit/pull/8930) which builds further on this one also got an update, but, there, I get one (unclear) build failure with AppVeyor. Any ideas? Perhaps its better to discuss about that one in https://github.com/fluent/fluent-bit/pull/8930.
Kr,
Thomas
AppVeyor is being deprecated so I'd not worry too much about that for now.
Currently, AppVeyor CI is unstable for uploading artefacts into the remote artifacts server. I'm also suffering this type of failures. Yes, we can ignore the failure of AppVeyor.
Hi, @cosmo0920, I have tested this PR on s390x platform, it works well, will you please help to merge this PR? https://github.com/fluent/fluent-bit/pull/8903#issuecomment-2206090890
Hi @cosmo0920 , this PR is really important for us to leverage fluentbit on zLinux (s390x), as we make extensive use of Lua in our configurations.
/cc @edsiper @celalettin1286 @fujimotos @koleini @leonardo-albertovich @niedbalski @patrick-stephens @agup006
Hi, any chance to add the following workflows in https://github.com/fluent/fluent-bit/blob/master/.github/workflows/pr-compile-check.yaml?
# Sanity check for compilation using system libraries
pr-compile-system-libs:
runs-on: ubuntu-20.04
timeout-minutes: 60
strategy:
fail-fast: false
matrix:
flb_option:
- "-DFLB_PREFER_SYSTEM_LIBS=On"
compiler:
- gcc
- clang
steps:
- name: Setup environment
run: |
sudo apt-get update
sudo apt-get install -y gcc-7 g++-7 clang-6.0 libsystemd-dev gcovr libyaml-dev libluajit-5.1-dev
sudo ln -s /usr/bin/llvm-symbolizer-6.0 /usr/bin/llvm-symbolizer || true
- name: Checkout Fluent Bit code
uses: actions/checkout@v4
- name: ${{ matrix.compiler }} - ${{ matrix.flb_option }}
run: |
export nparallel=$(( $(getconf _NPROCESSORS_ONLN) > 8 ? 8 : $(getconf _NPROCESSORS_ONLN) ))
echo "CC = $CC, CXX = $CXX, FLB_OPT = $FLB_OPT"
sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-7 90
sudo update-alternatives --install /usr/bin/g++ g++ /usr/bin/g++-7 90
sudo update-alternatives --install /usr/bin/clang clang /usr/bin/clang-6.0 90
cmake $GLOBAL_OPTS $FLB_OPT ../
make -j $nparallel
working-directory: build
env:
CC: ${{ matrix.compiler }}
CXX: ${{ matrix.compiler }}
FLB_OPT: ${{ matrix.flb_option }}
GLOBAL_OPTS: "-DFLB_BACKTRACE=Off -DFLB_SHARED_LIB=Off -DFLB_DEBUG=On -DFLB_ALL=On -DFLB_EXAMPLES=Off"
- name: Display dependencies w/ ldd
run: |
ldd ./bin/fluent-bit
working-directory: build
This would be good to add to verify every time whether buildable or not with prefer system libs flag.
I tested the above workflow in my fluent-bit fork and works fine:
Run ldd ./bin/fluent-bit
linux-vdso.so.1 (0x00007ffc[4](https://github.com/cosmo0920/fluent-bit/actions/runs/9788101299/job/27025620765#step:5:5)b1f1000)
libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007ffa7869c000)
librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007ffa78692000)
libluajit-[5](https://github.com/cosmo0920/fluent-bit/actions/runs/9788101299/job/27025620765#step:5:6).1.so.2 => /lib/x86_64-linux-gnu/libluajit-5.1.so.2 (0x00007ffa78617000)
libyaml-0.so.2 => /lib/x8[6](https://github.com/cosmo0920/fluent-bit/actions/runs/9788101299/job/27025620765#step:5:7)_64-linux-gnu/libyaml-0.so.2 (0x00007ffa785f5000)
libsystemd.so.0 => /lib/x86_64-linux-gnu/libsystemd.so.0 (0x0000[7](https://github.com/cosmo0920/fluent-bit/actions/runs/9788101299/job/27025620765#step:5:8)ffa78546000)
libssl.so.1.1 => /lib/x86_64-linux-gnu/libssl.so.1.1 (0x00007ffa784b3000)
libcrypto.so.1.1 => /lib/x[8](https://github.com/cosmo0920/fluent-bit/actions/runs/9788101299/job/27025620765#step:5:9)6_64-linux-gnu/libcrypto.so.1.1 (0x00007ffa781da000)
libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007ffa781be000)
libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007ffa7806f000)
libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007ffa7806[9](https://github.com/cosmo0920/fluent-bit/actions/runs/9788101299/job/27025620765#step:5:10)000)
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007ffa77e77000)
/lib64/ld-linux-x86-64.so.2 (0x00007ffa786d[10](https://github.com/cosmo0920/fluent-bit/actions/runs/9788101299/job/27025620765#step:5:11)00)
libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007ffa77e5c000)
liblzma.so.5 => /lib/x86_64-linux-gnu/liblzma.so.5 (0x00007ffa77e31000)
liblz4.so.1 => /lib/x86_64-linux-gnu/liblz4.so.1 (0x00007ffa77e10000)
libgcrypt.so.20 => /lib/x86_64-linux-gnu/libgcrypt.so.20 (0x00007ffa77cf2000)
libgpg-error.so.0 => /lib/x86_64-linux-gnu/libgpg-error.so.0 (0x00007ffa77ccf000)
ref: https://github.com/cosmo0920/fluent-bit/actions/runs/9788101299/job/27025620765
Hi, any chance to add the following workflows in https://github.com/fluent/fluent-bit/blob/master/.github/workflows/pr-compile-check.yaml?
I've pushed a commit with your commit proposal.
@cosmo0920 I've rebased the PR, the checks are now green again.
Hi, @edsiper @cosmo0920, @patrick-stephens, is there any further blocker for merging this PR? If not, would you please help to merge it?
@cosmo0920, @patrick-stephens, is there any further blocker for merging this PR? If not, would you please help to merge it?
I guess it's @edsiper you have to ping.
@edsiper @cosmo0920 kind ping
@edsiper kind remember
@edsiper Can we include this PR in the v3.1.5 milestone?
Hi, @ThomasDevoogdt, would you please help to merge https://github.com/ThomasDevoogdt/fluent-bit/pull/13 so that the feature in this PR can be enabled on the s390x architecture? Thanks!
@ThomasDevoogdt thanks for the contribution!. Would you please submit a PR for fluent-bit-docs so we can reflect these new feature at build time ?
Hi, @edsiper , would you please help to review https://github.com/fluent/fluent-bit/pull/9172 which depends on this PR and further enable it on s390x?