Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: use the system provided packages if found #8930

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ThomasDevoogdt
Copy link
Contributor

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:

@ThomasDevoogdt ThomasDevoogdt changed the title Feature/use system provided packages build: use the system provided packages if found Jun 8, 2024
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/use-system-provided-packages branch from 6093bda to 4c6da55 Compare June 8, 2024 14:02
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/use-system-provided-packages branch 10 times, most recently from 95225eb to 169ee05 Compare June 9, 2024 14:10
Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ThomasDevoogdt
Copy link
Contributor Author

ThomasDevoogdt commented Jun 10, 2024

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 #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 ThomasDevoogdt force-pushed the feature/use-system-provided-packages branch from 169ee05 to 95ccc2d Compare June 10, 2024 18:50
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/use-system-provided-packages branch from 95ccc2d to 70c5a0e Compare June 15, 2024 15:56
@patrick-stephens patrick-stephens added the ok-package-test Run PR packaging tests label Jun 17, 2024
Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction of this PR is all good. But I found a concern to use system/bundled cares library case. Could you take a look?

CMakeLists.txt Outdated Show resolved Hide resolved
@cosmo0920
Copy link
Contributor

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! 👍

cosmo0920
cosmo0920 previously approved these changes Jun 21, 2024
@edsiper
Copy link
Member

edsiper commented Jun 21, 2024

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

@ThomasDevoogdt
Copy link
Contributor Author

ThomasDevoogdt commented Jun 21, 2024

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
Copy link
Contributor Author

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

@patrick-stephens
Copy link
Contributor

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

@ThomasDevoogdt
Copy link
Contributor Author

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.

@patrick-stephens
Copy link
Contributor

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.

@cosmo0920
Copy link
Contributor

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
Copy link
Contributor

cosmo0920 commented Jul 5, 2024

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

In #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.

@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/use-system-provided-packages branch 3 times, most recently from 1cfe759 to beae70e Compare July 7, 2024 06:02
@ThomasDevoogdt
Copy link
Contributor Author

@edsiper @cosmo0920 kind remember to take this up again

@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/use-system-provided-packages branch from beae70e to ef73d77 Compare August 7, 2024 17:28
@ThomasDevoogdt
Copy link
Contributor Author

ThomasDevoogdt commented Aug 7, 2024

@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 ThomasDevoogdt force-pushed the feature/use-system-provided-packages branch from ef73d77 to 4bdf7a6 Compare August 16, 2024 12:03
e.g. buildroot has logic to build libnghttp2,
so if pkg_check_modules can find a suitable version,
then use that one if -DFLB_PREFER_SYSTEM_LIB_NGHTTP2=Yes.

Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
e.g. buildroot has logic to build backtrace,
so if find_path/find_library can find a suitable version,
then use that one if -DFLB_PREFER_SYSTEM_LIB_BACKTRACE=Yes.

Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
e.g. buildroot has logic to build jemalloc,
so if pkg_check_modules can find a suitable version,
then use that one if -DFLB_PREFER_SYSTEM_LIB_JEMALLOC=Yes.

Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
e.g. buildroot has logic to build c-ares,
so if pkg_check_modules can find a suitable version,
then use that one if -DFLB_PREFER_SYSTEM_LIB_CARES=Yes.

Note that the CheckIncludeFiles include for wasm is added,
since that was done by c-ares otherwise.

Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
e.g. buildroot has logic to build librdkafka,
so if pkg_check_modules can find a suitable version,
then use that one if -DFLB_PREFER_SYSTEM_LIB_KAFKA=Yes.

Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/use-system-provided-packages branch from 4bdf7a6 to a24eac7 Compare August 25, 2024 13:21
@ThomasDevoogdt
Copy link
Contributor Author

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

@cosmo0920 cosmo0920 added this to the Fluent Bit v3.2.0 milestone Aug 26, 2024
@cosmo0920
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-required ok-package-test Run PR packaging tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants