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 libmsgpack and libsqlite3 if found #9572

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

Conversation

ThomasDevoogdt
Copy link
Contributor

This is a follow-up on #8930.


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

@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/libmsgpack-system-lib branch 2 times, most recently from 2bcee5d to 5495518 Compare November 9, 2024 19:16
@ThomasDevoogdt ThomasDevoogdt changed the title Feature/libmsgpack system lib build: use the system provided libmsgpack if found Nov 9, 2024
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/libmsgpack-system-lib branch from 5495518 to f988490 Compare November 9, 2024 19:23
@ThomasDevoogdt ThomasDevoogdt changed the title build: use the system provided libmsgpack if found build: use the system provided libmsgpack & libsqlite3 if found Nov 10, 2024
@ThomasDevoogdt ThomasDevoogdt changed the title build: use the system provided libmsgpack & libsqlite3 if found build: use the system provided libmsgpack and libsqlite3 if found Nov 10, 2024
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/libmsgpack-system-lib branch from 2c45d7d to 15e39dc Compare November 17, 2024 07:59
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/libmsgpack-system-lib branch from 15e39dc to 63ea0e2 Compare December 15, 2024 14:18
@ThomasDevoogdt
Copy link
Contributor Author

@edsiper It has been a while. Any chance that this gets approved? I try to reduce my own patch list.

@Arctize
Copy link

Arctize commented Jan 9, 2025

We're using these patches also for the Yocto recipe, so I’m in support of this change.

Thank you for all the unbundling work @ThomasDevoogdt!

@ThomasDevoogdt
Copy link
Contributor Author

Thank you for all the unbundling work @ThomasDevoogdt!

Thx!

We're using these patches also for the Yocto recipe, so I’m in support of this change.

Then you might also need #9600, which I found out later when bringing it all to buildroot.

@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/libmsgpack-system-lib branch from 63ea0e2 to 77d67ab Compare January 10, 2025 07:15
@@ -48,7 +48,7 @@ jobs:
run: |
sudo apt-get update
sudo apt-get install -y curl gcc-7 g++-7 clang-6.0 libsystemd-dev gcovr libyaml-dev libluajit-5.1-dev \
libnghttp2-dev libjemalloc-dev
libnghttp2-dev libjemalloc-dev libsqlite3-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the whole intention of this PR. Allowing to link fluent-bit against system libraries instead of build-int libraries. So adding this to the apt-get hook, ensures we test the libsqlite3 linking.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure but where do we test it?

My main query was we added it here but not the actual packaging build and not any of the other libraries in this PR so wanted to check why. Can we add a comment to indicate this was added specifically to test the linkage? Otherwise someone will remove it later or wonder why (probably me!).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test here is about this
"# Sanity check for compilation using system libraries"

But it's perhaps better to effective assert if these libs are used.

See the ldd ./bin/fluent-bit which gives something like:
(See https://github.com/fluent/fluent-bit/actions/runs/12705012946/job/35415319384)

Run ldd ./bin/fluent-bit
  ldd ./bin/fluent-bit
  shell: /usr/bin/bash -e {0}
	linux-vdso.so.1 (0x00007ffdc73ee000)
	libjemalloc.so.[2](https://github.com/fluent/fluent-bit/actions/runs/12705012946/job/35415319384#step:5:2) => /lib/x86_64-linux-gnu/libjemalloc.so.2 (0x00007f4fa96fc000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f4fa96d9000)
	libluajit-5.1.so.2 => /lib/x86_64-linux-gnu/libluajit-5.1.so.2 (0x00007f4fa965e000)
	libsqlite[3](https://github.com/fluent/fluent-bit/actions/runs/12705012946/job/35415319384#step:5:3).so.0 => /lib/x86_64-linux-gnu/libsqlite3.so.0 (0x00007f[4](https://github.com/fluent/fluent-bit/actions/runs/12705012946/job/35415319384#step:5:5)fa9535000)
	libnghttp2.so.14 => /lib/x86_64-linux-gnu/libnghttp2.so.14 (0x00007f4fa9[5](https://github.com/fluent/fluent-bit/actions/runs/12705012946/job/35415319384#step:5:6)0b000)
	libyaml-0.so.2 => /lib/x86_64-linux-gnu/libyaml-0.so.2 (0x00007f4fa94e9000)
	libsystemd.so.0 => /lib/x8[6](https://github.com/fluent/fluent-bit/actions/runs/12705012946/job/35415319384#step:5:7)_64-linux-gnu/libsystemd.so.0 (0x00007f4fa9438000)
	libssl.so.1.1 => /lib/x86_64-linux-gnu/libssl.so.1.1 (0x0000[7](https://github.com/fluent/fluent-bit/actions/runs/12705012946/job/35415319384#step:5:8)f4fa93a5000)
	libcrypto.so.1.1 => /lib/x86_64-linux-gnu/libcrypto.so.1.1 (0x00007f4fa90ce000)
	libz.so.1 => /lib/x[8](https://github.com/fluent/fluent-bit/actions/runs/12705012946/job/35415319384#step:5:9)6_64-linux-gnu/libz.so.1 (0x00007f4fa90b2000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f4fa8f63000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f4fa8f5d000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f4fa8f40000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f4fa8d4e000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f4faab88000)
	libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f4fa8b6c000)
	librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f4fa8b62000)
	liblzma.so.5 => /lib/x86_64-linux-gnu/liblzma.so.5 (0x00007f4fa8b3[9](https://github.com/fluent/fluent-bit/actions/runs/12705012946/job/35415319384#step:5:10)000)
	liblz4.so.1 => /lib/x86_64-linux-gnu/liblz4.so.1 (0x00007f4fa8b18000)
	libgcrypt.so.20 => /lib/x86_64-linux-gnu/libgcrypt.so.20 (0x00007f4fa89f8000)
	libgpg-error.so.0 => /lib/x86_64-linux-gnu/libgpg-error.so.0 (0x00007f4fa89d5000)

So perhaps we should assert that libsqlite3 is in that list, together with the other tested libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a commit to test it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we can do that I think, sorry if I missed where it was being done but a comment saying "adding this to support using system libraries and verified here: xxx" would help.

We can then see why it should not be mirrored into the centos7 image used for building - which would not be obvious particularly next time we have to tweak deps on both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated that last commit, with an extra step to make it clear that these packages are part of the test.

- added a install system libraries step

To make it clear that this is part of the test.

- assert the linked system libs

Check if the system lib is effectively linked as expected.

Signed-off-by: Thomas Devoogdt <[email protected]>
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/libmsgpack-system-lib branch from 4f3fc26 to a7a4518 Compare January 10, 2025 14:32
Copy link
Contributor

@patrick-stephens patrick-stephens left a comment

Choose a reason for hiding this comment

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

Looks ok to me but will wait on anyone else wanting to comment too.

@ThomasDevoogdt
Copy link
Contributor Author

@patrick-stephens Can you also have a look at #9600, which relates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants