-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix dpdk support when using custom prefix #2542
base: master
Are you sure you want to change the base?
Conversation
e391bb3
to
9845e56
Compare
The first commit fixes a problem that may be interesting. Some variables have same names across different cmake files. This is problematic because the Seastar project recommends to not set initial value of cmake variables and rely on the default being empty. At least I got this feedback in some of my older PRs. This assumption does not hold if some other file sets the variable to non-empty value beforehand. Perhaps it makes sense to invert the policy. Setting variables to empty results in additional code, which is small, stable inefficiency. On the other hand, accidentally being hit by cmake variables being overridden in non-obvious ways may waste hours or days of time or even more, if the failure manifests in non-obvious way in different part of codebase. |
# there is no need to force it to be discoverable via find_package(). | ||
if (args_BUILD) | ||
list (APPEND _seastar_all_dependencies dpdk) | ||
endif() |
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'd like to understand your use case better. do you enable Seastar_DPDK
when building Seastar?
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 build and use Seastar using the following:
./3rdparty/seastar/configure.py --mode=release --enable-dpdk --prefix=$(pwd)/build-install
ninja -C ./3rdparty/seastar/build/release install
...
mkdir -p build
cmake -S . -B build -GNinja \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_PREFIX_PATH=$(pwd)/build-install \
-DCMAKE_INSTALL_PREFIX=$(pwd)/build-install
ninja -C build
I would say this is pretty unconventional setup except for the install prefix and that DPDK is enabled. Without this PR build will fail because there's no dpdk installed in build-install (which is expected)
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.
@p12tic how do you build DPDK? do you build a static build of DPDK using something like cooking.sh
? i cannot find --cook dpdk
in your command line arguments passed to configure.py
.
but in a more unconventional setup, user might want to build Seastar with a dynamic build of DPDK libraries. in that case, we would have to find dpdk when building seastar and when building the seastar application. in this case, we cannot skip the find_package(dpdk)
call when building seastar application, i think.
252f857
to
7847cb3
Compare
@tchaikov Thanks for review. I think I've addressed your comments. |
seastar currently includes the objects of the dpdk library to within its static library. As a result, dpdk library may as well not exist to outside users - it is effectively an implementation detail. Unfortunately it is exposed in the INTERFACE_LINK_LIBRARIES list of the installed Seastar::seastar target. This is a known problem in CMake https://gitlab.kitware.com/cmake/cmake/-/issues/15415 and the solution is to use BUILD_INTERFACE generator expression. Co-authored-by: Kefu Chai <[email protected]>
dpdk is included to within seastar, accordingly it should not be required to have dpdk package available when using seastar as dependency. This fixes build failures when attempting to use seastar from a custom install prefix.
7847cb3
to
1cf9758
Compare
"$<BUILD_INTERFACE:DPDK::dpdk>") | ||
get_target_property(DPDK_INTERFACE_LINK_LIBRARIES DPDK::dpdk INTERFACE_LINK_LIBRARIES) | ||
target_link_libraries (seastar | ||
PRIVATE "${DPDK_INTERFACE_LINK_LIBRARIES}") |
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.
could you use something like
Lines 867 to 875 in 665fed0
if (CMAKE_VERSION VERSION_GREATER_EQUAL 3.26) | |
target_link_libraries (seastar | |
PRIVATE | |
"$<BUILD_LOCAL_INTERFACE:Valgrind::valgrind>") | |
else () | |
target_link_libraries (seastar | |
PRIVATE | |
"$<BUILD_INTERFACE:Valgrind::valgrind>") | |
endif () |
so we don't forget dropping the workaround for older versions of CMake when we bump up the minimal required version of CMake ?
# there is no need to force it to be discoverable via find_package(). | ||
if (args_BUILD) | ||
list (APPEND _seastar_all_dependencies dpdk) | ||
endif() |
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.
@p12tic how do you build DPDK? do you build a static build of DPDK using something like cooking.sh
? i cannot find --cook dpdk
in your command line arguments passed to configure.py
.
but in a more unconventional setup, user might want to build Seastar with a dynamic build of DPDK libraries. in that case, we would have to find dpdk when building seastar and when building the seastar application. in this case, we cannot skip the find_package(dpdk)
call when building seastar application, i think.
Currently Seastar manages building of DPDK internally and includes the built DPDK library to within Seastar library. Unfortunately when Seastar is installed to a custom prefix, it starts to require an installation of DPDK to that prefix. This does not make sense, as Seastar shouldn't require anything related to DPDK after installation.
This PR addresses the problem. The root cause is that the fact that Seastar uses DPDK leaks to installed CMake targets. First, Seastar is fixed to not search for installed DPDK once Seastar has been installed itself. Second, the dependencies have been adjusted so that the dependency on DPDK library does not transfer to users of Seastar library.
This allows Seastar with DPDK enabled being installed in custom prefix.