-
Notifications
You must be signed in to change notification settings - Fork 663
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
Generate and install pkgconfig files for the RDMA libraries #373
Conversation
7726a2d
to
dc0b275
Compare
These changes look good to me (haven't tested it). |
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.
Thanks for your PR
@@ -127,6 +127,8 @@ Architecture: linux-any | |||
Multi-Arch: same | |||
Depends: ibverbs-providers (= ${binary:Version}), | |||
libibverbs1 (= ${binary:Version}), | |||
libnl-3-dev, | |||
libnl-route-3-dev, |
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.
rdma-core section has already this dependency,
Isn't this enough? sorry for my dumb question, I'm not debian guru.
Source: rdma-core
Maintainer: Linux RDMA Mailing List linux-rdma@vger.kernel.org
Uploaders: Benjamin Drung benjamin.drung@profitbricks.com,
Talat Batheesh talatb@mellanox.com
Section: net
Priority: optional
Build-Depends: cmake (>= 2.8.11),
debhelper (>= 9),
debhelper (>= 9.20160709) | dh-systemd,
dpkg-dev (>= 1.17),
libnl-3-dev,
libnl-route-3-dev,
libudev-dev,
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.
Yes - in that section that's a build dependency for these packages. Here it's a runtime dependency of the dev library, and as the commit msg says it's needed if a third party wants to do static linking with ibverbs, as they'll need to install and static link to everything that ibverbs depends on, which in this case is nl3
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.
ok, thanks
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.
No, this is wrong, we don't support static linking in Debian packaging today so don't need the -dev packages here. If we add static libraries someday it would become necessary.
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.
The static library is built and shipped as you can see here:
https://packages.debian.org/buster/amd64/libibverbs-dev/filelist
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.
packages.debian.org is not working, it is showing you a file list from a pre rdma-core package, our packaging does not enable static libraries. download the .deb from buster and check yourself with dpkg-deb -c.
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.
Uh, hadn't noticed it was broken, interesting (and annoying). I'll revert this commit then, thanks for spotting it.
@rleon all done, thanks |
One latest change, please fix title from "Genearate" to be "Generate" in this PR and in first commit too. |
Add a template and generate a pc file for each shared library using CMake. Signed-off-by: Luca Boccassi <bluca@debian.org>
ibverbs uses NL3, which has to be present when doing statically linked builds, so add the dependencies on the libibverbs-dev library. Signed-off-by: Luca Boccassi <bluca@debian.org>
I always make that typo! @rleon fixed |
Thanks |
@@ -127,6 +127,8 @@ Architecture: linux-any | |||
Multi-Arch: same | |||
Depends: ibverbs-providers (= ${binary:Version}), | |||
libibverbs1 (= ${binary:Version}), | |||
libnl-3-dev, | |||
libnl-route-3-dev, |
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.
No, this is wrong, we don't support static linking in Debian packaging today so don't need the -dev packages here. If we add static libraries someday it would become necessary.
@@ -13,6 +13,7 @@ usr/include/infiniband/verbs_api.h | |||
usr/lib/*/libibverbs*.so | |||
usr/lib/*/libmlx4.so | |||
usr/lib/*/libmlx5.so | |||
usr/lib/*/pkgconfig/libibverbs.pc |
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.
Where are the pkgconfigs for libmlx5 and 4?
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.
They are not built for now. Where are those libs built from?
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.
providers/mlx5 and providers/mlx4, they are intended as application linkable libraries and as provider plugins, eg dpdk links to them.
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.
Ok, thanks, will add them too
set(PC_LIB_PRIVATE "${PC_LIB_PRIVATE}") | ||
set(PC_INCLUDE_PRIVATE "${PC_INCLUDE_PRIVATE}") | ||
set(PC_REQUIRES "${PC_REQUIRES}") | ||
configure_file(${CMAKE_SOURCE_DIR}/buildlib/template.pc.in ${CMAKE_CURRENT_BINARY_DIR}/lib${PC_LIB_NAME}.pc @ONLY) |
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.
Don't we need a build/lib/pkgconfig/ as well specifying options to 'use in place' ?
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.
Sorry, I'm not sure I follow? What do you mean?
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.
rdma-core is designed to be built an run 'in-place' when the -DIN_PLACE=1 is used. It sets up rpaths and other things to enable it.
If you want to compile another application against it using pkgconfig I would expect you'd configure that application telling pkgconfig to search rdma-core/build/lib/pkgconfig for the .pc files before searching system directories, and then those .pc files should mirror the IN_PLACE setup and include -I and -L directives pointing to the build directory, not to system paths.
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.
Ok, will do.
set(NL_CFLAGS ${NL3_CFLAGS}) | ||
set(NL_LDFLAGS ${NL3_LDFLAGS}) | ||
# CMake separates ldflags with a semicolon, cannot be used in a command | ||
string(REPLACE ";" " " NL_LDFLAGS "${NL_LDFLAGS}") |
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.
Why is this needed here but not below for NL1 ? Isn't this just list to string conversion?
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.
Because nl1 only adds one flag, so cmake doesn't add a semicolon.
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.
That is relying a bit too much on cmake internal behavior, if the flags are cmake lists then they should always be converted.. Probably the helper function should be doing this conversion to keep with cmake convention of using lists for arguments.
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.
Sure, I'll change it that way
Should we revert the PR?
…On Sun, Sep 2, 2018, 04:56 Jason Gunthorpe ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In CMakeLists.txt
<#373 (comment)>:
> @@ -368,6 +368,10 @@ if (ENABLE_RESOLVE_NEIGH)
set(NL_KIND 3)
set(NL_INCLUDE_DIRS ${NL3_INCLUDE_DIRS})
set(NL_LIBRARIES ${NL3_LIBRARIES})
+ set(NL_CFLAGS ${NL3_CFLAGS})
+ set(NL_LDFLAGS ${NL3_LDFLAGS})
+ # CMake separates ldflags with a semicolon, cannot be used in a command
+ string(REPLACE ";" " " NL_LDFLAGS "${NL_LDFLAGS}")
That is relying a bit too much on cmake internal behavior, if the flags
are cmake lists then they should always be converted.. Probably the helper
function should be doing this conversion to keep with cmake convention of
using lists for arguments.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#373 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAbhEE5ZJKJKaKfoBO0nbmwBcqBoNJEUks5uWzrDgaJpZM4WQWq4>
.
|
I'll send the requested changes later tonight |
Pkgconfig: followup changes from PR #373
It's very useful for applications and other projects to be able to use pkgconfig to build with external libraries. Generate and install pc files for the three RDMA libraries.
Here's how the files look after a Debian build: