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

Fix lz4 missing from flann.pc Requires: line. #481

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nh2
Copy link

@nh2 nh2 commented Nov 1, 2021

lz4 is an unconditional dependency of flann (see #399),
but until now was not correctly generated into the
Requires: lz4 line of flann.pc,
because the PKG_EXTERNAL_DEPS variable used in flann.pc.in
was not defined at all.

This fixes build error lz4.h: No such file or directory
for properly sandboxed builds, in which undeclared dependencies
are not made available.

@longhuan2018
Copy link

Can you add HDF5's dependency at the same time?
Modify CMakeLists.txt

find_hdf5()
if (NOT HDF5_FOUND)
	message(WARNING "hdf5 library not found, some tests will not be run")
else()
    include_directories(${HDF5_INCLUDE_DIR})
    list(APPEND PKGCONFIG_EXTERNAL_DEPS_LIST "hdf5")
endif()

@nh2
Copy link
Author

nh2 commented Sep 8, 2023

Can you add HDF5's dependency at the same time?

@longhuan2018 Done.

@jspricke
Copy link
Contributor

Can you drop @LZ4_STATIC_LDFLAGS@ from cmake/flann.pc.in? That should fix #480.

@nh2
Copy link
Author

nh2 commented Sep 26, 2023

Can you drop @LZ4_STATIC_LDFLAGS@ from cmake/flann.pc.in? That should fix #480.

@jspricke Done.

But why does flann.pc.in list these depenencies (hdf5 and liblz4) in Requires instead of Requires.private?

According to https://people.freedesktop.org/~dbn/pkg-config-guide.html, section "Writing pkg-config files",

Requires and Requires.private define other modules needed by the library.
It is usually preferred to use the private variant of Requires to avoid exposing unnecessary libraries to the program that is linking with your library.
If the program will not be using the symbols of the required library, it should not be linking directly to that library. See the discussion of overlinking for a more thorough explanation.

@nh2
Copy link
Author

nh2 commented Sep 26, 2023

@longhuan2018 Should HDF5 this really be in the .pc file?

It says

hdf5 library not found, some tests will not be run

So it sounds like it is only needed for tests, should it then be used for pkg-config?
I noticed that the libflann*.so files are not linked against hdf5 so files.

@jspricke
Copy link
Contributor

But why does flann.pc.in list these depenencies (hdf5 and liblz4) in Requires instead of Requires.private?

According to https://people.freedesktop.org/~dbn/pkg-config-guide.html, section "Writing pkg-config files",

Requires and Requires.private define other modules needed by the library.
It is usually preferred to use the private variant of Requires to avoid exposing unnecessary libraries to the program that is linking with your library.
If the program will not be using the symbols of the required library, it should not be linking directly to that library. See the discussion of overlinking for a more thorough explanation.

flann is exposing lz4 headers in util/serialization.h so to my understanding Requires is correct.

lz4 is an unconditional dependency of flann (see flann-lib#399),
but until now was not correctly generated into the
`Requires: lz4` line of `flann.pc`,
because the `PKG_EXTERNAL_DEPS` variable used in `flann.pc.in`
was not defined at all.

This fixes build error `lz4.h: No such file or directory`
for properly sandboxed builds, in which undeclared dependencies
are not made available.

Same thing for HDF5, but conditionally.

For lz4, also remove the hardcode of `@LZ4_STATIC_LDFLAGS@`
from `flann.pc.in`, as this is no longer necessary.
That fixes an incorrect `-L` flag being generated in there, e.g.
`-L/usr/lib;-llz4`. Thus fixes flann-lib#480.
@nh2
Copy link
Author

nh2 commented Sep 27, 2023

flann is exposing lz4 headers in util/serialization.h so to my understanding Requires is correct.

Yes, then it is correct. I've added that info as a CMake comment.

Then I only need the answer from @longhuan2018 regarding HDF5.

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.

3 participants