laudrup / lz4_stream

A C++ stream using LZ4 (de)compression
BSD 3-Clause "New" or "Revised" License
39 stars 13 forks source link

typo in CMakeLists.txt LZ4_INCLUDEDIR #3

Open kervel opened 5 years ago

kervel commented 5 years ago

hello,

In (master) CMakeLists you use LZ4_INCLUDE_DIR but pkg_config defines LZ4_INCLUDEDIR ... this breaks the case in which you don't want to use the supplied lz4.

in another note: i'm trying to crosscompile, and pkg_config also gives a library directory which is not being taken into account, but that's not so simple to fix as target_link_directories only exists in very recent versions of CMake. i work around by symlinking liblz4.a.

Frank

laudrup commented 5 years ago

Hi Frank,

Thanks a lot for reporting this issue. It's really nice to know that someone is actually using this.

The typo is easy enough to fix, but as you mention it's probably not that easy to fix having the actual lz4 library in a non standard location.

I will try to add another target to the automated builds (eg. travis) that installs lz4 to another location (eg. /usr/local) once I've come up with a solution.

Thanks for the hint on target_link_directories. You are right that it only seems to be supported on very recent versions of CMake and although I haven't really made up my mind on which versions of compilers and tools to support, I would like to avoid such as requirement.

I'll try to find a solution ASAP. Please let me know if you figure something out (or possibly open a PR).

Thanks.

/Kasper

kervel commented 5 years ago

hello,

none of my machines runs a recent enough cmake for target_link_directories. so i guess that rules it out for me now. For the time being we switched back to your old CMakeLists which didn't use pkg_config to find lz4. i have the impression that using pkg_config and crosscompiling now is not really supported well.

i needed the non-bundled approach because i'm crosscompiling, otherwise i would go for the bundled approach too.

Frank

kervel commented 5 years ago

And adding... a solution that would be working for us would be bundling a findLZ4.cmake Something like this (untested, if it looks workable i could test):

find_library(LZ4_LIBRARY
    PATHS ${LZ4_ROOT}
    NAMES liblz4.a
)

find_path(LZ4_INCLUDE_DIR
  lz4frame.h
  PATHS ${LZ4_ROOT}
  NO_DEFAULT_PATH)
include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(lz4 DEFAULT_MSG
  LZ4_INCLUDE_DIR LZ4_LIBRARY)

if(lz4_FOUND)
    add_library(lz4::lz4 INTERFACE IMPORTED)
    set_target_properties(lz4::lz4 PROPERTIES
        INTERFACE_INCLUDE_DIRECTORIES "${LZ4_INCLUDE_DIR}"
    )
    set_target_properties(lz4::lz4 PROPERTIES
        INTERFACE_LINK_LIBRARIES "${LZ4_LIBRARY}"
    )
endif()
laudrup commented 5 years ago

Hi Frank,

I'm not really sure why I chose to use pkg_config in the first place, since I used to bundle a FindLZ4.cmake, so it might be a good idea to reintroduce that.

If you could wrap this into a pull request I will definitely have a look at it, so please consider that.

Thanks and kind regards,

/Kasper