-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
libpcap: add support for dbus, libnl and rdma #23383
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that this took so long to get reviewed, looks good to me! :)
Thanks a lot for also including a full compilation log, truly appreciated!
@valgur there are some conflicts, please take a look so we can merge this! |
# Conflicts: # recipes/libpcap/all/conanfile.py
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking for further review. Thank you for your update! libpcap is a powerful library!
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@valgur I just tried to build locally, but using the official Docker image libpcap-1.10.4-linux-all-options.log The error occurred because libpcap was not capable to find those dependencies via pkg-config. The PkgConfigDeps is missing in the recipe. Furthermore, there is a new bug in Conan client that does not allow us renaming the .pc for The commit 9271f24 fixes the situation by add PkgConfigDeps and renaming the generated .pc file, so we do not patch pcap directly (much cheaper). libpcap-1.10.4-linux-pkgconfdeps.log Please, consider using those docker images in the future, when providing the build lob. It did not fail for you, as you system is well configured, having all dependencies installed already. |
UPDATE: I just checked AUR and indeed the expected .pc file for nl-genl is https://archlinux.org/packages/core/x86_64/libnl/ I'll open a PR fixing it, so we do not need this current workaround by renaming the .pc file here. |
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Depends on #25380 first. It will fix the correct pkg-config missing for libnl when active with libpcap. |
This comment has been minimized.
This comment has been minimized.
The PR #25380 has been merged. I just rebased this PR. |
This comment has been minimized.
This comment has been minimized.
Need extra PR: #25386 |
LGTM. Thank you for the libnl fix, @uilianries! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Now libpcap is really consuming Conan dependencies for those new options:
lipcap-1.10.4-linux-shared.log
lipcap-1.10.4-linux-static.log
Conan v1 pipeline ✔️All green in build 10 ( |
Build log with all options enabled on Linux: https://gist.github.com/valgur/bb1ff3fd0ca5045a378ddd228c5588b3