Repository navigation
Compile zstd into TileDB instead of loading it at run time - #10
Conversation
TileDB loaded libzstd.so.1 with dlopen the first time a zstd codec was created, and threw if it was missing, which through GenomicsDB's JNI layer aborted the JVM on machines without libzstd. zstd 1.5.7 is now downloaded at configure time, checked against its SHA-256, and compiled into TileDB's objects, the way LZ4 already is, with its symbols hidden so they neither clash with nor bind to another zstd in the same process. The codec uses zstd's own context types, so freeing a context with the wrong function no longer compiles. CI no longer installs zstd.
|
|
||
| # Install ZStd | ||
| sudo apt-get install -y zstd && | ||
| # ZStd is compiled into TileDB, so there is nothing to install |
There was a problem hiding this comment.
Lets remove the ENABLE_ZSTD macro altogether and just have it included like zlib
There was a problem hiding this comment.
Done in 64f0145: the ENABLE_ZSTD option and macro are gone, so zstd is always compiled in, the way LZ4 is, and always available, like zlib. It's still built from the pinned zstd 1.5.7 source rather than found on the system; say if you'd rather link against a system libzstd instead. I also removed cmake/Modules/FindZSTD.cmake, which nothing used.
There was a problem hiding this comment.
We could probably do something like https://github.com/facebook/rocksdb/blob/main/cmake/modules/Findzstd.cmake which would be on the lines of what we do for zlib. But I am fine with the pinned zstd for now.
There was a problem hiding this comment.
My 2c is that for dependencies like zlib that have been super-stable over many many years, and are on basically every system everywhere, dynamic linking works well. But for anything that is either on < 99.9% of common systems, or that's still evolving, I like to pin and link statically.
There was a problem hiding this comment.
Actually, I was thinking more like linking in the libzstd.a installed on a system statically, but finding an installed one instead of building it ourselves. But, I am fine with the pin too.
| set(TILEDB_OBJECTS $<TARGET_OBJECTS:TILEDB_CORE_OBJECTS>) | ||
|
|
||
| if(ENABLE_ZSTD) | ||
| # Build the zstd library the way its own build/cmake does, minus the |
There was a problem hiding this comment.
Has this been tested on both architectures?
There was a problem hiding this comment.
Yes. The only architecture-specific part is zstd's huf_decompress_amd64.S, which compiles to an empty object off x86-64. With 64f0145, clean builds pass TileDB's full test suite on macOS arm64 (Apple clang 21) and Linux aarch64 (GCC 15.2 and clang 21, merged with main and with #12, which those compilers need), and CI's Ubuntu x86-64 and macOS Intel test jobs all build zstd in and pass its tests. The same zstd build is also in our fork's GenomicsDB 1.6.0 release for Linux x86-64 and aarch64 and macOS arm64.
* Require CMake 3.19 FetchContent_MakeAvailable, used to build zstd in since #10, needs CMake 3.14, and CMake 4 warns that compatibility with versions older than 3.10 will be removed. 3.19 is the version GenomicsDB's prerequisite scripts install for its release builds, which include TileDB as a subdirectory. * Indent CMake files with spaces and remove trailing whitespace * Set the version to 0.6.0
TileDB loads
libzstd.so.1withdlopenthe first time a zstd codec is created, and throws if it isn't there. GenomicsDB always enables zstd, and through its JNI layer that exception aborted the JVM on machines without libzstd. This downloads zstd 1.5.7 at configure time, checks it against its SHA-256 and compiles it into TileDB's objects, the way LZ4 is already built in, with its symbols hidden so that they neither clash with nor bind to another zstd in the same process. The codec now uses zstd's own context types, so freeing a context with the wrong function (#2) no longer compiles, and CI no longer installs zstd.zstd is now always built in, so the
ENABLE_ZSTDoption and macro are gone, along with the unused FindZSTD.cmake. Every build downloads zstd at configure time, which needs CMake 3.14 or later, and attributes that use zstd compression are always compressed by the built-in zstd 1.5.7 rather than by whatever version the system has; other zstd versions can compress the same tile to different bytes, but each reads the other's output.