CMake update to find liblz4 using pkg-config instead of cmake.

[mholtrop]

This is a request from Chris Dilks to improve the way the liblz4 library is found by the CMake scheme.
There is also a minor update of the Readme for the RDataFrame component.

[c-dilks]

Suggested change

set(ENV{PKG_CONFIG_PATH} "${PKG_CONFIG_PATH}:${CMAKE_INSTALL_PREFIX}/lib/pkgconfig")

Is this needed? I think CMake's PkgConfig module will search CMAKE_PREFIX_PATH's lib/pkgconfig subdirectories by default, so you just need to make sure LZ4's top-level installation prefix is in CMAKE_PREFIX_PATH (and if LZ4 is installed at the system level, then it's not necessary to add the system directories since they will already be searched).

[mholtrop]

Well, "needed" it is not. If lz4 is properly installed it all works. However if the user installs lz4 from the directory included with hipo, then this line avoids the confusion with cmake not finding that install unless you also set the CMAKE_PREFIX_PATH. I would think that "expected behavior" is that cmake just finds the package files, as it would if we used a cmake install instead of pkgconfig. It seems a simple fix to me, but if anyone objects I am fine without it.

[c-dilks]

Sounds good to me

[c-dilks]

I'm not sure if this works, since include_directories documentation states directory parameters, not targets; however, the Linux CI tests pass, so maybe it's okay.

The macOS tests still fail though:

/Users/runner/work/hipo/hipo/hipo4/recordbuilder.cpp:11:10: fatal error: 'lz4.h' file not found
#include <lz4.h>
         ^~~~~~~
1 error generated.

I wonder if you'd be better off adding PkgConfig::LZ4 to each of the relevant target_include_directories calls.

[mholtrop]

Yes, that may work better.
I am testing this on an older Intel Mac right now. It seems that the "include_directory" was not even needed on my current system.

[mholtrop]

On the machine in question, I would like to see the output of the command: "pkg-config --debug --cflags liblz4", and then an "ls" of the directory the result points to.
On my Intel Mac, I was able to re-create the error seen here because the pkg-config had a file in the /usr/local/lib/pkgconfig directory that pointed to a non-existing directory for liblz4. In other words, liblz4 was moved in some update, but the pkgconfig was not updated accordingly.
This seems to be a weakness of pkgconfig, that the config files can get out of sync with the reality on the hard drive.

[c-dilks]

Sounds like its pkg-config file may be non-relocatable, which I think is common when the assumption is that installed files won't be moved.

You can add whatever commands you want to the CI config (it will run every time you push a new commit). Either add them just before the cmake call or put them in a separate step:

hipo/.github/workflows/ci.yml

Lines 29 to 31 in dfacdce

	cmake -S . -B build -DCMAKE_INSTALL_PREFIX=install
	cmake --build build -j2
	cmake --install build

Maybe mark this PR as draft so @gavalian doesn't merge this prematurely.

[c-dilks]

You can see my (unsuccessful) attempts here: c-dilks#2

I was doing some sanity checks to see if anything is out of date on the runner, but all the versions seem consistent with the Linux runner.

[c-dilks]

well.. I managed to get past the missing header file issue, but now the linker is complaining that it can't find liblz4:

https://github.com/c-dilks/hipo/actions/runs/7846867991/job/21414552371?pr=2

I'll be back online later this afternoon, but you can diff my branch with yours to see what I was trying.

[mholtrop]

ARGH! Okay, spend way too much time digging into the obscurities of cmake pkg-config implementation and how it is supposed to work. I still do not understand it quite, and why some things work on Intel Mac but not on Arm Mac and vise versa. I updated the code and now it works on both. I hope the tests pass now.

[c-dilks]

Got it to work on my fork. Here's the patch:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2ce2c85..f6aaac0 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -75,7 +75,7 @@ if(NOT LZ4_FOUND)
 endif()

 add_compile_definitions(__LZ4__)
-include_directories(PkgConfig::LZ4)
+include_directories(${LZ4_INCLUDE_DIRS})

 set(DATAFRAME_IN_MAIN TRUE)
 add_subdirectory(extensions/dataframes)
@@ -126,7 +126,7 @@ set_target_properties(hipo4_static PROPERTIES OUTPUT_NAME hipo4)   # So that the

 # Required on Unix OS family to be able to be linked into shared libraries.
 set_target_properties(hipo4_objs PROPERTIES POSITION_INDEPENDENT_CODE ON)
-add_dependencies(hipo4_objs LZ4)
+add_dependencies(hipo4_objs PkgConfig::LZ4)

 # Include the macros for creating export package.
 include(CMakePackageConfigHelpers)
@@ -141,8 +141,8 @@ target_include_directories(hipo4 PUBLIC
                            $<INSTALL_INTERFACE:include/hipo4>
                            )

-target_link_libraries(hipo4 PUBLIC  ${LZ4_LIBRARIES} )
-target_link_libraries(hipo4_static PUBLIC ${LZ4_LIBRARIES})
+target_link_libraries(hipo4 PUBLIC PkgConfig::LZ4)
+target_link_libraries(hipo4_static PUBLIC PkgConfig::LZ4)

 install(TARGETS hipo4
         EXPORT hipo4-export
diff --git a/cmake/LZ4Config.cmake b/cmake/LZ4Config.cmake
index 943bf59..ed710de 100644
--- a/cmake/LZ4Config.cmake
+++ b/cmake/LZ4Config.cmake
@@ -3,13 +3,14 @@
 #
 set(ENV{PKG_CONFIG_PATH} "${PKG_CONFIG_PATH}:${CMAKE_INSTALL_PREFIX}/lib/pkgconfig")
 find_package(PkgConfig REQUIRED)
-pkg_check_modules(LZ4 liblz4)
+pkg_check_modules(LZ4 IMPORTED_TARGET liblz4)

 if (NOT LZ4_FOUND)
     message(STATUS "No system version of LZ4 found.")
 else()
     add_library(LZ4 INTERFACE IMPORTED)
     set_target_properties(LZ4 PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${LZ4_INCLUDE_DIR}")
-    message(STATUS "Found LZ4: ${LZ4_LIBRARIES}")
+    message(STATUS "Found LZ4 libraries: ${LZ4_LIBRARIES}")
+    message(STATUS "Found LZ4 headers: ${LZ4_INCLUDE_DIRS}")
     mark_as_advanced(LZ4_INCLUDE_DIRS LZ4_INCLUDE_DIR LZ4_LIBRARIES LZ4_LIBRARY)
 endif (NOT LZ4_FOUND)

[c-dilks]

Looks good to me! CI tests pass both here and in iguana. Thank you very much for making this work.

@gavalian I believe this is ready for your review and merging.

[gavalian]

Looks good