flann-lib / flann

Fast Library for Approximate Nearest Neighbors
http://people.cs.ubc.ca/~mariusm/flann
Other
2.21k stars 646 forks source link

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

Open nh2 opened 2 years ago

nh2 commented 2 years ago

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 commented 2 years ago

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 commented 12 months ago

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

@longhuan2018 Done.

jspricke commented 11 months ago

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

nh2 commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

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.

nh2 commented 11 months ago

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.