libpcap: expose the with-snf option for recipes #23361
Conversation
|
|
This comment has been minimized.
This comment has been minimized.
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.
@darakelian Thank you for your PR. It's an authentic case for adding this option. Let's simplify a bit, the default value is known, so adding auto would complicated more.
recipes/libpcap/all/conanfile.py
Outdated
| @@ -29,11 +29,13 @@ class LibPcapConan(ConanFile): | |||
| "shared": [True, False], | |||
| "fPIC": [True, False], | |||
| "enable_libusb": [True, False], | |||
| "with_snf": [None, True, False], | |||
There was a problem hiding this comment.
| "with_snf": [None, True, False], | |
| "with_snf": [True, False], |
Let's keep binary decision, no auto values. We can sure about its default value: https://github.com/the-tcpdump-group/libpcap/blob/master/configure.ac#L1515
recipes/libpcap/all/conanfile.py
Outdated
| } | ||
| default_options = { | ||
| "shared": False, | ||
| "fPIC": True, | ||
| "enable_libusb": False, | ||
| "with_snf": None, |
There was a problem hiding this comment.
| "with_snf": None, | |
| "with_snf": True, |
There was a problem hiding this comment.
After reading through the configure file, I think this should default to False (though maybe None still makes sense) since I believe the "yes, if present" means that --with-snf=yes and --with-snf are equivalent.
There was a problem hiding this comment.
Hi, I was reading through the libpcap stuff more and I do believe None is a valid option here, when --with-snf is not present, the autoconfigure stuff will attempt to see if the snf library is present at /opt/snf and if so link against it. This is different behavior than explicitly passing yes/no
There was a problem hiding this comment.
I am not sure how to best model this with conan2, if it is even possible? Maybe defaulting to false/true is sufficient for this use case?
|
I detected other pull requests that are modifying libpcap/all recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
|
You should also export a dependency on libsnf in if self.options.get_safe("with_snf"):
self.cpp_info.system_libs.append("snf") # the library name might be incorrect |
How would I go about confirming the correct name? I don't see a recipe for libsnf either. |
This comment has been minimized.
This comment has been minimized.
|
@darakelian You need to add it as a "system library" because it is not provided by this package or other Conan packages, but it's still required for linking. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…-center-index into libpcap/expose-with-snf
This comment has been minimized.
This comment has been minimized.
recipes/libpcap/all/conanfile.py
Outdated
| snf = self.options.get_safe("with_snf") | ||
| if snf is not None: | ||
| tc.configure_args.append(f"--with-snf={yes_no(snf)}") |
There was a problem hiding this comment.
| snf = self.options.get_safe("with_snf") | |
| if snf is not None: | |
| tc.configure_args.append(f"--with-snf={yes_no(snf)}") | |
| tc.configure_args.append(f"--with-snf={yes_no(self.options.with_snf)}") |
Always present don't need to check.
Conan v1 pipeline ✔️All green in build 11 (
Conan v2 pipeline ✔️
All green in build 11 ( |
| } | ||
| default_options = { | ||
| "shared": False, | ||
| "fPIC": True, | ||
| "enable_libusb": False, | ||
| "with_snf": False, |
There was a problem hiding this comment.
| "with_snf": False, | |
| "with_snf": False, |
https://github.com/the-tcpdump-group/libpcap/blob/libpcap-1.10.4/CMakeLists.txt#L521
It's enabled by default, but I'm fine accepting the opposite, because it's private product.
Specify library name and version: libpcap/*
Some of the build agents we use have myricom support. This causes libpcap to try and link against the myricom snf libraries (since with-snf=yes is the default) which causes problems when we use our internal libpcap pre-built package on a host that does not have myricom snf support. We had an internal fork of this recipe for conan 1 with this functionality supported but as we are migrating to conan 2, it would be easier if this could be mainlined and we can decom the fork of the recipe.