Skip to content
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

[dpdk] Fix build, dependencies, usage #26188

Merged
merged 7 commits into from
Aug 19, 2022
Merged

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Aug 5, 2022

Alternative to #25899. CC @ljishen for testing.

  • What does your PR fix?

    Fixes building dpdk for x64-linux-dynamic. (To resolve various linking issues with minimal effort, the patch doesn't remove the static libs from the dynamic build, but just disables the installation. The build reuses the compiled objects. But the patch does remove the shared libs from the static build and fills the "shared" variables with the static libs instead.)
    Fixes issues with dependencies by using the pkg-config files to create proper dependencies.
    Drops the unoffical config for direct use of imported target from pkg_check_modules.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    unchanged, no

  • Does your PR follow the maintainer guide?

    yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    yes

github-actions[bot]
github-actions bot previously approved these changes Aug 5, 2022
@dg0yt dg0yt marked this pull request as ready for review August 5, 2022 14:37
@ljishen
Copy link
Contributor

ljishen commented Aug 6, 2022

I can test it sometime this weekend.

@LilyWangLL LilyWangLL added the category:port-bug The issue is with a library, which is something the port should already support label Aug 8, 2022
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Aug 8, 2022
@BillyONeal
Copy link
Member

Leaving this info:reviewed for now pending reply from @ljishen indicating that testing was OK.

@ljishen
Copy link
Contributor

ljishen commented Aug 9, 2022

If the machine has system-installed dpdk, I can see the following error during cmake:

-- Found PkgConfig: /usr/bin/pkg-config (found version "0.29.1") 
-- Checking for module 'libdpdk'
--   Found libdpdk, version 20.11.5.2.2
CMake Error at /usr/share/cmake-3.24/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find dpdk: Found unsuitable version "20.11.5.2.2", but required
  is at least "21.11.0" (found
  /opt/mellanox/dpdk/include/dpdk;/opt/mellanox/dpdk/include/dpdk/../x86_64-linux-gnu/dpdk;/opt/mellanox/dpdk/include/dpdk;/mnt/sda8/projects/bitar/build-x64/vcpkg_installed/x64-linux/debug/lib/pkgconfig/../../../include;/usr/include/libnl3)
Call Stack (most recent call first):
  /usr/share/cmake-3.24/Modules/FindPackageHandleStandardArgs.cmake:592 (_FPHSA_FAILURE_MESSAGE)
  cmake_modules/Finddpdk.cmake:52 (find_package_handle_standard_args)
  opt/x64/src/vcpkg/scripts/buildsystems/vcpkg.cmake:824 (_find_package)

In this case, the system has the dpdk 20.11.5.2.2 installed.

The reason is commented here:
https://github.com/microsoft/vcpkg/blob/master/ports/dpdk/unofficial-dpdk-config.cmake.in#L1-L5

@ljishen
Copy link
Contributor

ljishen commented Aug 9, 2022

Additionally, do we want to leave this problem to users?
image
from: https://doc.dpdk.org/guides/linux_gsg/sys_reqs.html#compilation-of-the-dpdk

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 9, 2022

If the machine has system-installed dpdk, I can see the following error during cmake:

-- Found PkgConfig: /usr/bin/pkg-config (found version "0.29.1") 
-- Checking for module 'libdpdk'
--   Found libdpdk, version 20.11.5.2.2
CMake Error at /usr/share/cmake-3.24/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find dpdk: Found unsuitable version "20.11.5.2.2", but required
  is at least "21.11.0" (found
  /opt/mellanox/dpdk/include/dpdk;/opt/mellanox/dpdk/include/dpdk/../x86_64-linux-gnu/dpdk;/opt/mellanox/dpdk/include/dpdk;/mnt/sda8/projects/bitar/build-x64/vcpkg_installed/x64-linux/debug/lib/pkgconfig/../../../include;/usr/include/libnl3)
Call Stack (most recent call first):
  /usr/share/cmake-3.24/Modules/FindPackageHandleStandardArgs.cmake:592 (_FPHSA_FAILURE_MESSAGE)
  cmake_modules/Finddpdk.cmake:52 (find_package_handle_standard_args)
  opt/x64/src/vcpkg/scripts/buildsystems/vcpkg.cmake:824 (_find_package)

In this case, the system has the dpdk 20.11.5.2.2 installed.

The reason is commented here: https://github.com/microsoft/vcpkg/blob/master/ports/dpdk/unofficial-dpdk-config.cmake.in#L1-L5

There is neither a FindModule from CMake nor official CMake config from upstream. It is not vcpkg's responsibility to introduce and maintain either. That's why the initial post explicitly states:

Drops the unoffical config for direct use of imported target from pkg_check_modules.

You should use the now-documented direct use of FindPkgConfig.cmke with upstream's pkg-config data.

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 9, 2022

Additionally, do we want to leave this problem to users?
image
from: https://doc.dpdk.org/guides/linux_gsg/sys_reqs.html#compilation-of-the-dpdk

I assume this problem doesn't exist with vcpkg. The post-processing of pkg-config files in CMake will merge Libs.privateinto Libs for static triplets, so that the --static parameter makes no difference at all.

(Note that the fake CMake config didn't resolve this problem either.)

@ljishen
Copy link
Contributor

ljishen commented Aug 9, 2022

There is neither a FindModule from CMake nor official CMake config from upstream. It is not vcpkg's responsibility to introduce and maintain either. That's why the initial post explicitly states:

I think people would expect to find the correct, vcpkg installed package even if the system already has the package installed somewhere else, just like for packages with cmake config files. I don't understand why vcpkg is responsible for changing the CMAKE_PREFIX_PATH while leaving the PKG_CONFIG_PATH for end users to figure out, given that vcpkg currently supports installing packages with cmake config files and pkg-config pc files.

I assume this problem doesn't exist with vcpkg. The post-processing of pkg-config files in CMake will merge Libs.privateinto Libs for static triplets, so that the --static parameter makes no difference at all.

(Note that the fake CMake config didn't resolve this problem either.)

This problem is that the lower version of pkg-config provides the wrong sequence of flags with -Wl,--whole-archive and -Wl,--no-whole-archive not wrapping the static libs, resulting in missing symbols in the produced executable. The unofficial cmake config warns the user of this problem, and won't let users experience unexpected behaviors.

I just want to know, do you think those unofficial cmake config files currently in vcpkg are not following vcpkg's responsibility?

@BillyONeal BillyONeal added requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. and removed info:reviewed Pull Request changes follow basic guidelines labels Aug 9, 2022
@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 9, 2022

Maybe different things are getting mixed up here.

There is neither a FindModule from CMake nor official CMake config from upstream. It is not vcpkg's responsibility to introduce and maintain either. That's why the initial post explicitly states:

I think people would expect to find the correct, vcpkg installed package even if the system already has the package installed somewhere else, just like for packages with cmake config files.

The point is that if the package is installed somewhere else, it doesn't come with CMake config or find module support. If vcpkg adds that, it creates a lock-in effect which isn't desirable. There is room for convenience config, but it should be marked as unofficial. It is more like a last resort, in particular in absence of other mechanisms.

I don't understand why vcpkg is responsible for changing the CMAKE_PREFIX_PATH while leaving the PKG_CONFIG_PATH for end users to figure out, given that vcpkg currently supports installing packages with cmake config files and pkg-config pc files.

IIUC there is only one more thing required on the user side since CMake 3.1: Set PKG_CONFIG_USE_CMAKE_PREFIX_PATH.

I assume this problem doesn't exist with vcpkg. The post-processing of pkg-config files in CMake will merge Libs.privateinto Libs for static triplets, so that the --static parameter makes no difference at all.
(Note that the fake CMake config didn't resolve this problem either.)

This problem is that the lower version of pkg-config provides the wrong sequence of flags with -Wl,--whole-archive and -Wl,--no-whole-archive not wrapping the static libs, resulting in missing symbols in the produced executable. The unofficial cmake config warns the user of this problem, and won't let users experience unexpected behaviors.

IIUC the CMake config doesn't solve this problem.
But does this problem really occur with vcpkg? Do we have a test program to verify or solve the issue?

I just want to know, do you think those unofficial cmake config files currently in vcpkg are not following vcpkg's responsibility?

I am convinced that they are not in the correct unofficial namespace.
And I'm particular concerned about unofficial hand-written config files for complex package if not provided by upstream.

@ljishen
Copy link
Contributor

ljishen commented Aug 10, 2022

I have no problems with this PR. For the pkg-config version problem, I can check it in my own problem. Thanks @dg0yt.

@BillyONeal BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Aug 11, 2022
@BillyONeal
Copy link
Member

When there are official bindings for CMake or pkgconfig, we do want to prefer whatever upstream does.

There is neither a FindModule from CMake nor official CMake config from upstream. It is not vcpkg's responsibility to introduce and maintain either. That's why the initial post explicitly states:

I think people would expect to find the correct, vcpkg installed package even if the system already has the package installed somewhere else, just like for packages with cmake config files. I don't understand why vcpkg is responsible for changing the CMAKE_PREFIX_PATH while leaving the PKG_CONFIG_PATH for end users to figure out, given that vcpkg currently supports installing packages with cmake config files and pkg-config pc files.

vcpkg.cmake doesn't need to do anything here because FindPkgConfig already does the right thing:

https://cmake.org/cmake/help/latest/module/FindPkgConfig.html

New in version 3.1: The CMAKE_PREFIX_PATH, CMAKE_FRAMEWORK_PATH, and CMAKE_APPBUNDLE_PATH cache and environment variables will be added to the pkg-config search path. The NO_CMAKE_PATH and NO_CMAKE_ENVIRONMENT_PATH arguments disable this behavior for the cache variables and environment variables respectively. The PKG_CONFIG_USE_CMAKE_PREFIX_PATH variable set to FALSE disables this behavior globally.

Quoth @ras0219-msft :

For manual integration, we document the paths to .pc files in https://github.com/microsoft/vcpkg/blob/master/docs/users/buildsystems/manual-integration.md. We don't own the user's shell and so cannot adjust the path for them.

@ljishen
Copy link
Contributor

ljishen commented Aug 11, 2022

Hopeful I can see it gets merged soon. Thanks for the work.

@ljishen
Copy link
Contributor

ljishen commented Aug 16, 2022

What are we waiting for with this?

@Cheney-W Cheney-W added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Aug 19, 2022
@JavierMatosD
Copy link
Contributor

Thanks!

@JavierMatosD JavierMatosD merged commit df75e76 into microsoft:master Aug 19, 2022
@dg0yt dg0yt deleted the dpdk branch August 20, 2022 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants