modules: drop Basis' own bundled Zstd in favor of the real library.
This basically means users now have to either install a system zstd package or add zstd as a CMake subproject, as described in the docs. If Zstd isn't found, Basis gets compiled without Zstd support, meaning only non-zstd-compressed KTX files will be supported for both import and image conversion.
Putting this aside until packages (such as vcpkg) are all updated to depend on external zstd to avoid temporarily breaking KTX+zstd support in Basis{Importer,ImageConverter} plugins.
Codecov Report
Patch and project coverage have no change.
Comparison is base (
d8eb82c) 97.06% compared to head (8b6c348) 97.06%.
:exclamation: Current head 8b6c348 differs from pull request most recent head 2183d47. Consider uploading reports for the commit 2183d47 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@
## master #115 +/- ##
=======================================
Coverage 97.06% 97.06%
=======================================
Files 143 143
Lines 16360 16360
=======================================
Hits 15880 15880
Misses 480 480
| Files Changed | Coverage Δ | |
|---|---|---|
| ...mPlugins/BasisImageConverter/BasisImageConverter.h | 100.00% <ø> (ø) |
|
| src/MagnumPlugins/BasisImporter/BasisImporter.h | 100.00% <ø> (ø) |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Ah well, realized that Basis includes Zstd as ../zstd/zstd.h, which is everything but replaceable with an external library. Sigh.
Not sure how I failed to spot this earlier.
Ah well, realized that Basis includes Zstd as
../zstd/zstd.h, which is everything but replaceable with an external library. Sigh.Not sure how I failed to spot this earlier.
That should still work with a system library, right? Basis only uses a minimal set of zstd functions: ZSTD_compressBound, ZSTD_compress (in the encoder), ZSTD_decompress (in the transcoder), ZSTD_isError (in both), and no structs. As long as the system zstd has those basic functions, it should link without issues even while using the bundled zstd.h.
The vcpkg port currently does it this way.
I mean, I have /usr/include/zstd.h, so there's no way I can convince #include "../zstd/zstd.h" to work with that...
I mean, I have
/usr/include/zstd.h, so there's no way I can convince#include "../zstd/zstd.h"to work with that...
The zstd.h will always be bundled with basis_universal, won't it? My point was that mixing bundled header and system .so should theoretically work since basis only uses a few minimal functions that will be in virtually any old and foreseeable future version of zstd. And the bundled zstd.h is not header-only ("stb-style" 🤡).
Or is there another issue here like visibility/dllimport mismatch?
The zstd.h will always be bundled with basis_universal, won't it?
I don't want that, that sounds like a dirty hack, especially regarding various baked-in defines for multithreading and whatnot. I want to use a header corresponding to the actual binary.
Blocked on https://github.com/BinomialLLC/basis_universal/pull/228 (December 2021), or whatever other solution gets integrated there, if at all.
Finally found a solution by creating the following include hierarchy:
put-this-on-include-path/ <- this is put into the include path
zstd/
zstd.h <- this is what Basis then gets from #include "../zstd/zstd.h"
The zstd.h file is then just the following, together with a sanity check after to make sure the real zstd.h is actually included. Important is the use of <> to make the compiler pick a header from the include path and not itself -- not even #include_next was needed to make this work. The include guards are there to avoid endless recursion in case there's actually no real zstd.h on include path and the file then attempts to include itself.
#ifndef this_is_not_the_real_zstd_h
#define this_is_not_the_real_zstd_h
#include <zstd.h>
#ifndef ZSTD_VERSION_MAJOR
#error expected to have a real zstd on the include path
#endif
#endif