-
Notifications
You must be signed in to change notification settings - Fork 7.5k
[dpdk] Disable optional dependencies. #51495
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| diff --git a/config/meson.build b/config/meson.build | ||
| index 34b85f10b5..5ed4625d9e 100644 | ||
| index bbdad71..a9c63b5 100644 | ||
| --- a/config/meson.build | ||
| +++ b/config/meson.build | ||
| @@ -238,12 +238,10 @@ if meson.is_cross_build() and not meson.get_external_property('numa', true) | ||
| @@ -238,17 +238,15 @@ if meson.is_cross_build() and not meson.get_external_property('numa', true) | ||
| find_libnuma = false | ||
| endif | ||
| if find_libnuma | ||
|
|
@@ -17,19 +17,59 @@ index 34b85f10b5..5ed4625d9e 100644 | |
| endif | ||
| endif | ||
|
|
||
| has_libfdt = false | ||
| -fdt_dep = cc.find_library('fdt', required: false) | ||
| +fdt_dep = disabler() | ||
| if fdt_dep.found() and cc.has_header('fdt.h') and cc.links(min_c_code, dependencies: fdt_dep) | ||
| dpdk_conf.set10('RTE_HAS_LIBFDT', true) | ||
| has_libfdt = true | ||
| @@ -269,28 +267,28 @@ if libarchive.found() | ||
| endif | ||
|
|
||
| # check for libbsd | ||
| -libbsd = dependency('libbsd', required: false, method: 'pkg-config') | ||
| +libbsd = disabler() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT, removing the dependency on |
||
| if libbsd.found() | ||
| dpdk_conf.set('RTE_USE_LIBBSD', 1) | ||
| endif | ||
|
|
||
| -jansson_dep = dependency('jansson', required: false, method: 'pkg-config') | ||
| +jansson_dep = disabler() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is that as currently set up it makes things path dependent. That is, gives a totally different result than which is why the most recent dpdk update PR landed fine but we have not seen a clean world rebuild since, because jansson happened to get installed first and trigger incompatibilities with MSVC. The question for purposes of this PR to restore nightly runs is not "would someone like when these optional dependencies are used", but "is the port completely broken for all purposes and we need to try to get vcpkg to provide them instead".
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIU the patch arranges for the package to not offer this capability. This is fine. No installation order dependency. |
||
| if jansson_dep.found() | ||
| dpdk_conf.set('RTE_HAS_JANSSON', 1) | ||
| endif | ||
|
|
||
| # check for OpenSSL | ||
| -openssl_dep = dependency('openssl', required: false, method: 'pkg-config') | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The correct change may be to add openssl as a dependency instead?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT, |
||
| +openssl_dep = disabler() | ||
| if openssl_dep.found() | ||
| dpdk_conf.set('RTE_HAS_OPENSSL', 1) | ||
| endif | ||
|
|
||
| # check for pcap | ||
| -pcap_dep = dependency('libpcap', required: false, method: 'pkg-config') | ||
| +pcap_dep = disabler() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT, |
||
| pcap_lib = is_windows ? 'wpcap' : 'pcap' | ||
| if not pcap_dep.found() | ||
| # pcap got a pkg-config file only in 1.9.0 | ||
| - pcap_dep = cc.find_library(pcap_lib, required: false) | ||
| + pcap_dep = disabler() | ||
|
Comment on lines
+66
to
+70
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would make the patch longer so I do not want to do that. |
||
| endif | ||
| if (pcap_dep.found() and cc.has_header('pcap.h', dependencies: pcap_dep) | ||
| and cc.links(min_c_code, dependencies: pcap_dep)) | ||
| diff --git a/lib/eal/linux/meson.build b/lib/eal/linux/meson.build | ||
| index e99ebed256..672c70547b 100644 | ||
| index 29ba313..5a173c4 100644 | ||
| --- a/lib/eal/linux/meson.build | ||
| +++ b/lib/eal/linux/meson.build | ||
| @@ -21,5 +21,6 @@ sources += files( | ||
| @@ -25,5 +25,6 @@ endif | ||
|
|
||
| deps += ['kvargs', 'telemetry'] | ||
| if has_libnuma | ||
| + ext_deps += numa_dep | ||
| dpdk_conf.set10('RTE_EAL_NUMA_AWARE_HUGEPAGES', true) | ||
| endif | ||
| diff --git a/lib/vhost/meson.build b/lib/vhost/meson.build | ||
| index 51bcf17244..1099a0232f 100644 | ||
| index 6a24981..5283e79 100644 | ||
| --- a/lib/vhost/meson.build | ||
| +++ b/lib/vhost/meson.build | ||
| @@ -6,6 +6,7 @@ if not is_linux | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,8 @@ if(VCPKG_TARGET_IS_LINUX AND VCPKG_HOST_IS_LINUX) | |
| endif() | ||
|
|
||
| # Add a leading zero to the minor version if it consists of only one digit, otherwise the regex does nothing | ||
| string(REGEX REPLACE "^([0-9]+)\\.([0-9])(\\..*)$" "\\1.0\\2\\3" VERSION_REF "${VERSION}") | ||
| # Match both `X.Y` and `X.Y.Z...` forms (optional remainder). | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That this was wrong and nobody noticed suggests that people don't actually care about this port :(
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this transformation even desirable? Would it not be preferable to store the canonical version, as defined by the dpdk project, within
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only added the regex in the last update, and during the last adjustment, I must have forgotten to make the group optional after I hadn't forced the period in the group at first ;-). So this issue is in this repo only since 5 days ;-)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Desirable, yes. However, there is no suitable version scheme in vcpkg which allows |
||
| string(REGEX REPLACE "^([0-9]+)\\.([0-9])(\\..*)?$" "\\1.0\\2\\3" VERSION_REF "${VERSION}") | ||
| vcpkg_from_github( | ||
| OUT_SOURCE_PATH SOURCE_PATH | ||
| REPO DPDK/dpdk | ||
|
|
||
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.
I am not sure about some of these but they're dependencies not in vcpkg.json.
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.
AFAICT,
libfdtis required to build the IFPGA Rawdev Driver, and the IPN3KE Poll Mode Driver. These drivers may well be important to a user of DPDK, depending on their use-case, and the devices they are looking to interact with.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.
Then the port needs to be fixed to arrange for those dependencies to be present.
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.
AFAIU the patch arranges for the package to not offer this capability. This is fine. No installation order dependency.
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.
It's fine from vcpkg's perspective but I'm not sure it's fine from dpdk users' perspective. For some of these vcpkg already has the dependency available and I could resolve it the other way (always turn the dependency on).