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

build: use the system provided packages if found

Open ThomasDevoogdt opened this issue 1 year ago • 10 comments

Some packages might already be present in the system, so use them if that is the case. E.g. in buildroot, it doesn't make sense that e.g. luajit is compiled twice.


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.

  • [ ] 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

  • [N/A] 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.

This overrules these PRs:

  • https://github.com/fluent/fluent-bit/pull/8929
  • https://github.com/fluent/fluent-bit/pull/7286

ThomasDevoogdt avatar Jun 08 '24 13:06 ThomasDevoogdt

I think it'll be reasonable. However, we shouldn't break the priority of building and including the bundled libraries. So, how about adding FLB_USE_SYSTEM_LIBRARIRS or similar flag to provide an option to prioritize the system installed libraries? This is because using system installed libraries by default diversifies the version of dependent libraries.

Hi, I once did that in https://github.com/fluent/fluent-bit/pull/7286, but that PR is somewhat ignored in the meantime. I kept it open for reference. Please consider that one first, if it gets merged, then I can rework this PR to do the same for some other packages.

The only difference in the other PR is that I've used FLB_PREFER_SYSTEM_LIBS iso FLB_USE_SYSTEM_LIBRARIRS.

ThomasDevoogdt avatar Jun 10 '24 09:06 ThomasDevoogdt

I found collisions for the storing values of pkg_config results. Maybe, we'd better to use FLB_ prefix to store the results of referring the system libraries?

Also, the searching mechanism should be follow the naming convension of FindXXX.cmake. But it's optional. I guess.

So, we'd better to use the following names:

  • FindCares.cmake
  • Find(Rd)Kadfa.cmake
  • FindLuaJIT.cmake
  • FindNGHttp2.cmake

Do you still expect a change here? Not sure if we have to add the FLB_ prefix, there was only one package for which there was a conflict.

About the FindXXX.cmake, what do you mean? Is pkg_config not fine?

ThomasDevoogdt avatar Jun 20 '24 18:06 ThomasDevoogdt

I found collisions for the storing values of pkg_config results. Maybe, we'd better to use FLB_ prefix to store the results of referring the system libraries? Also, the searching mechanism should be follow the naming convension of FindXXX.cmake. But it's optional. I guess. So, we'd better to use the following names:

  • FindCares.cmake
  • Find(Rd)Kadfa.cmake
  • FindLuaJIT.cmake
  • FindNGHttp2.cmake

Do you still expect a change here? Not sure if we have to add the FLB_ prefix, there was only one package for which there was a conflict.

About the FindXXX.cmake, what do you mean? Is pkg_config not fine?

No worries, just my two cents. It's good time to approve! :+1:

cosmo0920 avatar Jun 21 '24 02:06 cosmo0920

can you please add a simple workflow that builds Fluent Bit in the CI with all those libs in shared mode ?

edsiper avatar Jun 21 '24 12:06 edsiper

can you please add a simple workflow that builds Fluent Bit in the CI with all those libs in shared mode ?

I have no experience with GitHub workflows. So that would take me some time to get started with it. Also, not all packages are up-to-date in e.g. the ubuntu docker containers. I tested all of them by linking to /usr/local/lib installs. I know that the apple homebrew eco system is quite up to date, so perhaps, that can be a good start? But I would propose to not hamper this PR with this extra request.

ThomasDevoogdt avatar Jun 21 '24 12:06 ThomasDevoogdt

@edsiper, sorry that I don't find the time to have a look at these GitHub actions. But is there any reason to still block this PR for this? I would like to have these changes so that I can improve the support in Buildroot.

ThomasDevoogdt avatar Jun 27 '24 07:06 ThomasDevoogdt

What exactly has to be invoked to build this? I can potentially update CI to do that

patrick-stephens avatar Jun 27 '24 09:06 patrick-stephens

What exactly has to be invoked to build this? I can potentially update CI to do that

@patrick-stephens A test job that compiles fluent-bit against a set of native system libraries, where we do enable the FLB_PREFER_SYSTEM_<LIB> flags.

e.g. for ubuntu 2022.04:

sudo apt-get install libc-ares-dev libluajit-5.1-dev libnghttp2-dev libjemalloc-dev librdkafka-dev

and then use

cmake -GNinja -DFLB_PREFER_SYSTEM_LIB_CARES=Yes -DFLB_PREFER_SYSTEM_LIB_LUAJIT=Yes -DFLB_PREFER_SYSTEM_LIB_NGHTTP2=Yes -DFLB_PREFER_SYSTEM_LIB_JEMALLOC=Yes -DFLB_PREFER_SYSTEM_LIB_KAFKA=Yes ../

The only problem is that Ubuntu doesn't always ship the latest libraries. A very similar test can be made, using the homebrew environment. Homebrew typically is more up-to-date, and if I'm right, homebrew does allow the installation of a fixed version.

ThomasDevoogdt avatar Jun 27 '24 12:06 ThomasDevoogdt

What exactly has to be invoked to build this? I can potentially update CI to do that

@patrick-stephens A test job that compiles fluent-bit against a set of native system libraries, where we do enable the FLB_PREFER_SYSTEM_ flags.

e.g. for ubuntu 2022.04:

sudo apt-get install libc-ares-dev libluajit-5.1-dev libnghttp2-dev libjemalloc-dev librdkafka-dev

and then use

cmake -GNinja -DFLB_PREFER_SYSTEM_LIB_CARES=Yes -DFLB_PREFER_SYSTEM_LIB_LUAJIT=Yes -DFLB_PREFER_SYSTEM_LIB_NGHTTP2=Yes -DFLB_PREFER_SYSTEM_LIB_JEMALLOC=Yes -DFLB_PREFER_SYSTEM_LIB_KAFKA=Yes ../

The only problem is that Ubuntu doesn't always ship the latest libraries. A very similar test can be made, using the homebrew environment. Homebrew typically is more up-to-date, and if I'm right, homebrew does allow the installation of a fixed version.

Probably not homebrew - macOS runners cost a lot and we already have Linux based package tests in place for all targets via simple container builds. My preference would be just updating ./packaging/build.sh to support the new option to make it easy to then invoke all in one go via the existing test workflow which then means we do it for all targets and stays up-to-date plus gives users a simple approach too using our actual release tooling. I can probably do this after this PR is merged though, it likely just needs some extra packages adding to the container base images for builds then passing the options in.

patrick-stephens avatar Jun 27 '24 12:06 patrick-stephens

Another option is like using ArchLinux via docker containers. ArchLinux's packaging is blazingly fast and typically no patches are applied. If we need to test against the latest packages, we'll be able to consider this option. However, that distribution is sometimes too cutting edge. So, I feel that testing with system packages on Ubuntu 22.04 is enough for most cases. ;)

cosmo0920 avatar Jun 27 '24 14:06 cosmo0920

can you please add a simple workflow that builds Fluent Bit in the CI with all those libs in shared mode ?

In https://github.com/fluent/fluent-bit/pull/7286, we added simple workflow which uses system libluajit-5.1-dev and its associated libraries. So, once that PR is merged we can continue to add dependent libraries as apt packages.

cosmo0920 avatar Jul 05 '24 07:07 cosmo0920

@edsiper @cosmo0920 kind remember to take this up again

ThomasDevoogdt avatar Jul 31 '24 20:07 ThomasDevoogdt

@edsiper thank you for merging #7286, this is a follow up pull request on top of the previous one. Can you also have a look at this one? I rebased the branch on top of the master branch.

ThomasDevoogdt avatar Aug 07 '24 19:08 ThomasDevoogdt

@edsiper @cosmo0920 @patrick-stephens @leonardo-albertovich Hi, I rebased this PR to the current master. Is it possible to consider this PR?

ThomasDevoogdt avatar Aug 25 '24 14:08 ThomasDevoogdt

Hi, thanks for continuously rebasing on top of master. Currently, I added a milestone for v3.2.0 in this PR.

cosmo0920 avatar Aug 26 '24 06:08 cosmo0920