Skip to content

[dpdk] Disable optional dependencies.#51495

Open
BillyONeal wants to merge 2 commits intomicrosoft:masterfrom
BillyONeal:fix-dpdk
Open

[dpdk] Disable optional dependencies.#51495
BillyONeal wants to merge 2 commits intomicrosoft:masterfrom
BillyONeal:fix-dpdk

Conversation

@BillyONeal
Copy link
Copy Markdown
Member

This fixes the CI failures https://dev.azure.com/vcpkg/public/_build/results?buildId=130806&view=results caused by optionally grabbing jansson which exposes unsupported C99 VLAs to MSVC.

Also regenerated all patches where the line numbers changed.

This fixes the CI failures https://dev.azure.com/vcpkg/public/_build/results?buildId=130806&view=results caused by optionally grabbing jansson which exposes unsupported C99 VLAs to MSVC.

Also regenerated all patches where the line numbers changed.

Co-authored-by: Copilot <copilot@github.com>
endif

has_libfdt = false
-fdt_dep = cc.find_library('fdt', required: false)
Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AFAICT, libfdt is 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

AFAICT, libfdt is 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.

Then the port needs to be fixed to arrange for those dependencies to be present.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member Author

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

endif

# check for OpenSSL
-openssl_dep = dependency('openssl', required: false, method: 'pkg-config')
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The correct change may be to add openssl as a dependency instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AFAICT, openssl is used by the OpenSSL Crypto Poll Mode Driver, and within examples.

@BillyONeal
Copy link
Copy Markdown
Member Author

@kreuzerkrieg @Rastaban @vicroms @ras0219-msft @ljishen
@chris1786 @autoantwort @dg0yt @bansan85 @SunBlack Tagging you in case you want to review because you have touched this port before.

Comment thread ports/dpdk/portfile.cmake

# 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).
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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 :(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 dpdk.json?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 ;-)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 dpdk.json?

Desirable, yes. However, there is no suitable version scheme in vcpkg which allows 26.04 without loosing comparing for <.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the dpdk vcpkg port to avoid non-deterministic optional dependency discovery (notably jansson) that breaks MSVC CI, and bumps the port version accordingly.

Changes:

  • Bump dpdk to port-version: 1 and update the versions database/baseline.
  • Adjust the tag REF formatting logic in portfile.cmake to handle both X.Y and X.Y.Z... version forms.
  • Regenerate/update the DPDK patch set (including disabling several optional Meson dependencies to prevent accidental pickup from the environment).

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
versions/d-/dpdk.json Adds the new 26.3#1 git-tree entry.
versions/baseline.json Bumps the dpdk baseline port-version to 1.
ports/dpdk/vcpkg.json Bumps port-version to 1.
ports/dpdk/portfile.cmake Updates version tag normalization regex used for vcpkg_from_github(REF ...).
ports/dpdk/0001-enable-either-static-or-shared-build.patch Patch regenerated (offset/index updates).
ports/dpdk/0002-fix-dependencies.patch Disables optional Meson deps (e.g., jansson) to avoid unintended discovery; other adjustments retained.
ports/dpdk/0003-remove-examples-src-from-datadir.patch Patch regenerated (offset/index updates).
ports/dpdk/0004-stop-building-apps.patch Patch regenerated (offset/index updates).
ports/dpdk/0005-no-absolute-driver-path.patch Patch regenerated (offset/index updates).
ports/dpdk/0006-rename-sched.h.patch Patch regenerated (offset/index updates).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +52 to +56
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()
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@chris1786
Copy link
Copy Markdown
Contributor

@kreuzerkrieg @Rastaban @vicroms @ras0219-msft @ljishen @chris1786 @autoantwort @dg0yt @bansan85 @SunBlack Tagging you in case you want to review because you have touched this port before.

I've added several comments, which hopefully add some context, although I will caveat this by saying that I am far from a dpdk expert. My experience of using this port, is that it currently pulls many of the dependencies it requires to build its drivers implicitly from the system on which it is built. This doesn't seem ideal, but probably requires substantial effort to resolve. I'd be wary of disabling dependencies entirely; this is likely to limit the utility of this port, as having the correct drivers available is key to being able to use dpdk effectively.

Comment thread ports/dpdk/portfile.cmake

# 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).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 dpdk.json?

endif

has_libfdt = false
-fdt_dep = cc.find_library('fdt', required: false)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AFAICT, libfdt is 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.


# check for libbsd
-libbsd = dependency('libbsd', required: false, method: 'pkg-config')
+libbsd = disabler()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AFAICT, removing the dependency on libbsd won't disable any functionality.

endif

-jansson_dep = dependency('jansson', required: false, method: 'pkg-config')
+jansson_dep = disabler()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

jansson appears to be used to support telemetry, and enables optional functionality in a number of drivers.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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,

vcpkg install jansson
vcpkg install dpdk

gives a totally different result than

vcpkg install dpdk
vcpkg install jansson

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

Copy link
Copy Markdown
Contributor

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.

endif

# check for OpenSSL
-openssl_dep = dependency('openssl', required: false, method: 'pkg-config')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AFAICT, openssl is used by the OpenSSL Crypto Poll Mode Driver, and within examples.


# check for pcap
-pcap_dep = dependency('libpcap', required: false, method: 'pkg-config')
+pcap_dep = disabler()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AFAICT, libpcap is required by the Pcap Poll Mode Driver, and to enable several other pieces of optional functionality

@chris1786
Copy link
Copy Markdown
Contributor

It's worth noting that this port also has several implicit uncontrolled dependencies specified within the drivers directory. Three library dependencies that I'm aware of are libibverbs, libmlx4, and libmlx5. These are provided by rdma-core, which I've created a port for in this PR.

There are undoubtedly other uncontrolled dependencies.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants