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

allow one to use system ZSTD #131

Merged
merged 2 commits into from
Nov 11, 2024
Merged

allow one to use system ZSTD #131

merged 2 commits into from
Nov 11, 2024

Conversation

a-detiste
Copy link
Contributor

Hi, I used this to de-vendor ZSTD

@rtissera
Copy link
Owner

Looks good thanks a lot

@bsdcode
Copy link

bsdcode commented Nov 9, 2024

The proposed change doesn't work on FreeBSD 14.1:

$ cmake .. -DWITH_SYSTEM_ZSTD=ON
-- The C compiler identification is Clang 18.1.5
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Looking for sys/types.h
-- Looking for sys/types.h - found
-- Looking for stdint.h
-- Looking for stdint.h - found
-- Looking for stddef.h
-- Looking for stddef.h - found
-- Check size of off64_t
-- Check size of off64_t - done
-- Looking for fseeko
-- Looking for fseeko - found
-- Looking for unistd.h
-- Looking for unistd.h - found
-- Renaming
--     /tmp/libchdr/deps/zlib-1.3.1/zconf.h
-- to 'zconf.h.included' because this file is included with zlib
-- but CMake generates it automatically in the build directory.
-- Configuring done (0.6s)
-- Generating done (0.0s)
-- Build files have been written to: /tmp/libchdr/build
$ cmake --build .
/tmp/libchdr/build> cmake --build .
[  2%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/adler32.c.o
[  4%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/compress.c.o
[  7%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/crc32.c.o
[  9%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/deflate.c.o
[ 11%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/gzclose.c.o
[ 14%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/gzlib.c.o
[ 16%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/gzread.c.o
[ 19%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/gzwrite.c.o
[ 21%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/inflate.c.o
[ 23%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/infback.c.o
[ 26%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/inftrees.c.o
[ 28%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/inffast.c.o
[ 30%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/trees.c.o
[ 33%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/uncompr.c.o
[ 35%] Building C object deps/zlib-1.3.1/CMakeFiles/zlibstatic.dir/zutil.c.o
[ 38%] Linking C static library libz.a
[ 38%] Built target zlibstatic
[ 40%] Building C object deps/lzma-24.05/CMakeFiles/lzma.dir/src/Alloc.c.o
[ 42%] Building C object deps/lzma-24.05/CMakeFiles/lzma.dir/src/Bra.c.o
[ 45%] Building C object deps/lzma-24.05/CMakeFiles/lzma.dir/src/Bra86.c.o
[ 47%] Building C object deps/lzma-24.05/CMakeFiles/lzma.dir/src/BraIA64.c.o
[ 50%] Building C object deps/lzma-24.05/CMakeFiles/lzma.dir/src/CpuArch.c.o
[ 52%] Building C object deps/lzma-24.05/CMakeFiles/lzma.dir/src/Delta.c.o
[ 54%] Building C object deps/lzma-24.05/CMakeFiles/lzma.dir/src/LzFind.c.o
[ 57%] Building C object deps/lzma-24.05/CMakeFiles/lzma.dir/src/Lzma86Dec.c.o
[ 59%] Building C object deps/lzma-24.05/CMakeFiles/lzma.dir/src/LzmaDec.c.o
[ 61%] Building C object deps/lzma-24.05/CMakeFiles/lzma.dir/src/LzmaEnc.c.o
[ 64%] Building C object deps/lzma-24.05/CMakeFiles/lzma.dir/src/Sort.c.o
[ 66%] Linking C static library liblzma.a
[ 66%] Built target lzma
[ 69%] Building C object CMakeFiles/chdr-static.dir/src/libchdr_bitstream.c.o
[ 71%] Building C object CMakeFiles/chdr-static.dir/src/libchdr_cdrom.c.o
[ 73%] Building C object CMakeFiles/chdr-static.dir/src/libchdr_chd.c.o
/tmp/libchdr/src/libchdr_chd.c:50:10: fatal error: 'zstd.h' file not found
   50 | #include <zstd.h>
      |          ^~~~~~~~
1 error generated.

I need to add list(APPEND CHDR_INCLUDES ${ZSTD_INCLUDE_DIR}). After this I get a linker error:

ld: error: unable to find library -lzstd

Instead of appending zstd to CHDR_LIBS, we should append ${ZSTD_LIBRARIES} to PLATFORM_LIBS (PLATFORM_LIBS seems more correct here). So this works for me:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index a9625d4..b92fba5 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -7,6 +7,7 @@ if(CMAKE_PROJECT_NAME STREQUAL "chdr")
 endif()
 option(INSTALL_STATIC_LIBS "Install static libraries" OFF)
 option(WITH_SYSTEM_ZLIB "Use system provided zlib library" OFF)
+option(WITH_SYSTEM_ZSTD "Use system provided zstd library" OFF)

 option(BUILD_LTO "Compile libchdr with link-time optimization if supported" OFF)
 if(BUILD_LTO)
@@ -41,11 +42,18 @@ else()
 endif()

 # zstd
-option(ZSTD_BUILD_SHARED "BUILD SHARED LIBRARIES" OFF)
-option(ZSTD_BUILD_PROGRAMS "BUILD PROGRAMS" OFF)
-option(ZSTD_LEGACY_SUPPORT "LEGACY SUPPORT" OFF)
-add_subdirectory(deps/zstd-1.5.6/build/cmake EXCLUDE_FROM_ALL)
-list(APPEND CHDR_LIBS libzstd_static)
+if (WITH_SYSTEM_ZSTD)
+  find_path(ZSTD_INCLUDE_DIR NAMES zstd.h)
+  find_library(ZSTD_LIBRARIES NAMES zstd HINTS ${ZSTD_ROOT_DIR}/lib)
+  list(APPEND CHDR_INCLUDES ${ZSTD_INCLUDE_DIR})
+  list(APPEND PLATFORM_LIBS ${ZSTD_LIBRARIES})
+else()
+  option(ZSTD_BUILD_SHARED "BUILD SHARED LIBRARIES" OFF)
+  option(ZSTD_BUILD_PROGRAMS "BUILD PROGRAMS" OFF)
+  option(ZSTD_LEGACY_SUPPORT "LEGACY SUPPORT" OFF)
+  add_subdirectory(deps/zstd-1.5.6/build/cmake EXCLUDE_FROM_ALL)
+  list(APPEND CHDR_LIBS libzstd_static)
+endif()
 #--------------------------------------------------
 # chdr
 #--------------------------------------------------

Now, CMake should also install .cmake modules for zstd, on FreeBSD they are located at /usr/local/lib/cmake/zstd. This makes it also possible to use find_package(zstd REQUIRED) and the zstd::libzstd target:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index a9625d4..65dcaf6 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -7,6 +7,7 @@ if(CMAKE_PROJECT_NAME STREQUAL "chdr")
 endif()
 option(INSTALL_STATIC_LIBS "Install static libraries" OFF)
 option(WITH_SYSTEM_ZLIB "Use system provided zlib library" OFF)
+option(WITH_SYSTEM_ZSTD "Use system provided zstd library" OFF)

 option(BUILD_LTO "Compile libchdr with link-time optimization if supported" OFF)
 if(BUILD_LTO)
@@ -41,11 +42,16 @@ else()
 endif()

 # zstd
-option(ZSTD_BUILD_SHARED "BUILD SHARED LIBRARIES" OFF)
-option(ZSTD_BUILD_PROGRAMS "BUILD PROGRAMS" OFF)
-option(ZSTD_LEGACY_SUPPORT "LEGACY SUPPORT" OFF)
-add_subdirectory(deps/zstd-1.5.6/build/cmake EXCLUDE_FROM_ALL)
-list(APPEND CHDR_LIBS libzstd_static)
+if (WITH_SYSTEM_ZSTD)
+  find_package(zstd REQUIRED)
+  list(APPEND PLATFORM_LIBS zstd::libzstd)
+else()
+  option(ZSTD_BUILD_SHARED "BUILD SHARED LIBRARIES" OFF)
+  option(ZSTD_BUILD_PROGRAMS "BUILD PROGRAMS" OFF)
+  option(ZSTD_LEGACY_SUPPORT "LEGACY SUPPORT" OFF)
+  add_subdirectory(deps/zstd-1.5.6/build/cmake EXCLUDE_FROM_ALL)
+  list(APPEND CHDR_LIBS libzstd_static)
+endif()
 #--------------------------------------------------
 # chdr
 #--------------------------------------------------

@a-detiste: can you test if this also works for you? I think this would be a more proper way to handle this, but I'm no CMake expert.

@bsdcode
Copy link

bsdcode commented Nov 9, 2024

Actually it should be zstd::libzstd_shared in order to link against the shared object libzstd.so. zstd::libzstd links the static library libzstd.a in.

@a-detiste
Copy link
Contributor Author

Hi, my CMake patch was the result of long & hesitant fumbling.

Version 2 of yours is much better.
I launched some CI build right now but it already built fine localy.
https://salsa.debian.org/games-team/libchdr/-/pipelines/760243

@bsdcode
Copy link

bsdcode commented Nov 9, 2024

Thanks!

I see you used zstd::libzstd. This links the static library /usr/lib/x86_64-linux-gnu/libzstd.a in. For consistent behaviour in using the system libraries (like what WITH_SYSTEM_ZLIB does), you might probably use zstd:libzstd_shared instead to link against the shared library:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index a9625d4..65dcaf6 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -7,6 +7,7 @@ if(CMAKE_PROJECT_NAME STREQUAL "chdr")
 endif()
 option(INSTALL_STATIC_LIBS "Install static libraries" OFF)
 option(WITH_SYSTEM_ZLIB "Use system provided zlib library" OFF)
+option(WITH_SYSTEM_ZSTD "Use system provided zstd library" OFF)

 option(BUILD_LTO "Compile libchdr with link-time optimization if supported" OFF)
 if(BUILD_LTO)
@@ -41,11 +42,16 @@ else()
 endif()

 # zstd
-option(ZSTD_BUILD_SHARED "BUILD SHARED LIBRARIES" OFF)
-option(ZSTD_BUILD_PROGRAMS "BUILD PROGRAMS" OFF)
-option(ZSTD_LEGACY_SUPPORT "LEGACY SUPPORT" OFF)
-add_subdirectory(deps/zstd-1.5.6/build/cmake EXCLUDE_FROM_ALL)
-list(APPEND CHDR_LIBS libzstd_static)
+if (WITH_SYSTEM_ZSTD)
+  find_package(zstd REQUIRED)
+  list(APPEND PLATFORM_LIBS zstd::libzstd_shared)
+else()
+  option(ZSTD_BUILD_SHARED "BUILD SHARED LIBRARIES" OFF)
+  option(ZSTD_BUILD_PROGRAMS "BUILD PROGRAMS" OFF)
+  option(ZSTD_LEGACY_SUPPORT "LEGACY SUPPORT" OFF)
+  add_subdirectory(deps/zstd-1.5.6/build/cmake EXCLUDE_FROM_ALL)
+  list(APPEND CHDR_LIBS libzstd_static)
+endif()
 #--------------------------------------------------
 # chdr
 #--------------------------------------------------

I think this is also what this PR should use after you have confirmed that this works for you.

@rtissera
Copy link
Owner

Let’s run CI and merge if okay

@rtissera rtissera merged commit b397465 into rtissera:master Nov 11, 2024
11 checks passed
@bsdcode
Copy link

bsdcode commented Nov 11, 2024

Thanks!

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