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

build: use the system provided LuaJIT if found

Open ThomasDevoogdt opened this issue 2 years ago • 15 comments

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-test label 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.

ThomasDevoogdt avatar Apr 28 '23 09:04 ThomasDevoogdt

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.

leonardo-albertovich avatar Apr 28 '23 10:04 leonardo-albertovich

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.

patrick-stephens avatar Apr 28 '23 10:04 patrick-stephens

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.

ThomasDevoogdt avatar Apr 29 '23 11:04 ThomasDevoogdt

Hmm…, it’s reasonable. Maybe using FLB_PREFER_SYSTEM_LIBS should be better to handle them.

cosmo0920 avatar Apr 30 '23 05:04 cosmo0920

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 ?

edsiper avatar May 06 '23 00:05 edsiper

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.

ThomasDevoogdt avatar May 06 '23 10:05 ThomasDevoogdt

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.

github-actions[bot] avatar Oct 09 '23 01:10 github-actions[bot]

Could you rebase this @ThomasDevoogdt ?

patrick-stephens avatar Oct 09 '23 09:10 patrick-stephens

Could you rebase this @ThomasDevoogdt ?

Sure, I've rebased the branch.

ThomasDevoogdt avatar Oct 09 '23 11:10 ThomasDevoogdt

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.

github-actions[bot] avatar Jan 14 '24 01:01 github-actions[bot]

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 = off
  • FLB_PREFER_SYSTEM_LIB_C_ARES = off
  • ..

edsiper avatar Jun 13 '24 04:06 edsiper

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

ThomasDevoogdt avatar Jun 13 '24 08:06 ThomasDevoogdt

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

ThomasDevoogdt avatar Jun 18 '24 07:06 ThomasDevoogdt

AppVeyor is being deprecated so I'd not worry too much about that for now.

patrick-stephens avatar Jun 18 '24 08:06 patrick-stephens

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.

cosmo0920 avatar Jun 18 '24 10:06 cosmo0920

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

rightblank avatar Jul 03 '24 13:07 rightblank

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

ScarletTanager avatar Jul 03 '24 14:07 ScarletTanager

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

cosmo0920 avatar Jul 04 '24 03:07 cosmo0920

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.

ThomasDevoogdt avatar Jul 04 '24 13:07 ThomasDevoogdt

@cosmo0920 I've rebased the PR, the checks are now green again.

ThomasDevoogdt avatar Jul 06 '24 09:07 ThomasDevoogdt

Hi, @edsiper @cosmo0920, @patrick-stephens, is there any further blocker for merging this PR? If not, would you please help to merge it?

rightblank avatar Jul 08 '24 12:07 rightblank

@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.

ThomasDevoogdt avatar Jul 09 '24 13:07 ThomasDevoogdt

@edsiper @cosmo0920 kind ping

ThomasDevoogdt avatar Jul 15 '24 08:07 ThomasDevoogdt

@edsiper kind remember

ThomasDevoogdt avatar Jul 31 '24 20:07 ThomasDevoogdt

@edsiper Can we include this PR in the v3.1.5 milestone?

cosmo0920 avatar Aug 01 '24 02:08 cosmo0920

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!

rightblank avatar Aug 06 '24 06:08 rightblank

@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 ?

edsiper avatar Aug 07 '24 02:08 edsiper

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?

rightblank avatar Aug 07 '24 03:08 rightblank