Skip to content

build: Detect msgpack-c and msgpack-c to be valid msgpack library #10387

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

Merged
merged 3 commits into from
May 29, 2025

Conversation

arianvp
Copy link
Contributor

@arianvp arianvp commented May 26, 2025

Follow-up on #9572 as I noticed msgpack-c system library was not being picked up.

the PkgConfig file is called msgpack-c not msgpack https://github.com/msgpack/msgpack-c/blob/c_master/msgpack-c.pc.in

Before:

-- Checking for module 'msgpack>=4.0.0'
--   No package 'msgpack' found

After:

-- Checking for module 'msgpack-c>=4.0.0'
--   Found msgpack-c, version 6.1.0

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

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

@arianvp arianvp marked this pull request as ready for review May 26, 2025 18:58
@arianvp
Copy link
Contributor Author

arianvp commented May 26, 2025

Should I update the pr-compile-check.yml too?

@arianvp arianvp force-pushed the patch-1 branch 2 times, most recently from 0eb6b5f to e06abfe Compare May 26, 2025 19:49
@cosmo0920
Copy link
Contributor

Hi, thanks for your work. It seems that the last commit didn't include signed-off line. So, could you amend and commit with signed-off line again?

@cosmo0920
Copy link
Contributor

Plus, in Ubuntu environment, libmsgpack is provided as libmsgpack and pkg-config picks up that library as msgpack:

% pkg-config --modversion msgpack
4.0.0

Which Linux distributions do you use?

@arianvp
Copy link
Contributor Author

arianvp commented May 27, 2025

So it seems that starting 4.0.0. msgpack was split into two divergent branches and versions and codebases. a header-only msgpack for CXX and msgpack-c for C (https://github.com/msgpack/msgpack-c/releases/tag/cpp-4.0.0)

And distros seem to be handling it differently.

NixOS and ArchLinux have separate msgpack-cxx and msgpack-c packages and the pkgconfig file for the C library is called msgpack-c.pc

However ubuntu and Debian has a libmsgpack-cxx-dev package that contains just the C++headers and a libmsgpack-dev that seems to combine both C and C++ header files into one and renames the pkg-config file to msgpack.pc . I'm not sure what they're trying to do with Ubuntu here. Maybe some weird attempt at trying to keep things backwards-compatible? It's very strange.

I think the fix here is to have CMake look for both msgpack-c and msgpack? annoying.

Fedora is still in 3.x so is still pre-split and will use the vendored version of msgpack

@arianvp
Copy link
Contributor Author

arianvp commented May 27, 2025

Hmm seems version is too old in CI. Shall I remove the check again?:

-- Checking for module 'msgpack>=4.0.0'
--   Package dependency requirement 'msgpack >= 4.0.0' could not be satisfied.
Package 'msgpack' has version '3.3.0', required version is '>= 4.0.0'
-- Checking for module 'msgpack-c>=4.0.0'
--   Package 'msgpack-c', required by 'virtual:world', not found

@cosmo0920
Copy link
Contributor

cosmo0920 commented May 27, 2025

Hmm seems version is too old in CI. Shall I remove the check again?:

No, we need to do this.
I created a canary PR to process linkage of libmsgpack here:
59a6fb6

Plus, could you split your commit into 2 commits?

One is:
build: Detect libmsgpack as msgpack-c
The other is:
workflows: Confirm libmsgpack linkage
It would be great if my canary PR's changes into the last one.

@cosmo0920 cosmo0920 changed the title msgpack -> msgpack-c build: Detect msgpack-c and msgpack-c to be valid msgpack library May 27, 2025
arianvp added 2 commits May 27, 2025 13:52
The pc file is called differently on Ubuntu than on Arch Linux

Signed-off-by: Arian van Putten <[email protected]>
@cosmo0920
Copy link
Contributor

cosmo0920 commented May 27, 2025

@arianvp Could you add another commit 59a6fb6 in your PR? This should solve your question.

@cosmo0920 cosmo0920 added this to the Fluent bit v4.0.3 milestone May 29, 2025
@edsiper
Copy link
Member

edsiper commented May 29, 2025

since this touch workflows, adding @patrick-stephens for visibility

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 lovely from the CI side so all good as long as it's green

@edsiper edsiper merged commit 1c80c1a into fluent:master May 29, 2025
43 checks passed
@edsiper edsiper removed this from the Fluent Bit Next milestone May 29, 2025
@ThomasDevoogdt
Copy link
Contributor

Hi all,

Thanks for applying this patch. I also saw the same problem in Buildroot, but didn't send a patch yet. Also nice to see that it is getting used.

Kr,

Thomas

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.

5 participants