-
Notifications
You must be signed in to change notification settings - Fork 59
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
modules: drop Basis' own bundled Zstd in favor of the real library. #115
Conversation
Codecov ReportPatch and project coverage have no change.
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
☔ View full report in Codecov by Sentry. |
Ah well, realized that Basis includes Zstd as 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: The vcpkg port currently does it this way. |
I mean, I have |
The zstd.h will always be bundled with basis_universal, won't it? My point was that mixing bundled header and system Or is there another issue here like visibility/dllimport mismatch? |
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 BinomialLLC/basis_universal#228 (December 2021), or whatever other solution gets integrated there, if at all. |
8b6c348
to
2183d47
Compare
Finally found a solution by creating the following include hierarchy:
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 #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 |
2183d47
to
4ab02eb
Compare
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. However, Basis complicates all this by doing the following insanity: #include "../zstd/zstd.h" Fortunately, after patiently waiting since December 2021 for at least one of the three fix PRs to get emrged there, I realized this could be fixed by providing a fake include directory that would delegate to the real zstd.h. And it seems to work, even without needing #include_next. Ultimately updating also dev PKGBUILDs and all CIs to use this, since it just makes no sense anymore to use the bundled version. It's slower than the current one, and it doesn't contain any security updates.
4ab02eb
to
7f29410
Compare
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 inBasis{Importer,ImageConverter}
plugins.