magnum-plugins icon indicating copy to clipboard operation
magnum-plugins copied to clipboard

modules: drop Basis' own bundled Zstd in favor of the real library.

Open mosra opened this issue 4 years ago • 7 comments

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.

mosra avatar Nov 13 '21 16:11 mosra

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.

codecov[bot] avatar Nov 13 '21 18:11 codecov[bot]

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.

mosra avatar Dec 20 '21 20:12 mosra

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.

pezcode avatar Dec 20 '21 20:12 pezcode

I mean, I have /usr/include/zstd.h, so there's no way I can convince #include "../zstd/zstd.h" to work with that...

mosra avatar Dec 20 '21 21:12 mosra

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?

pezcode avatar Dec 20 '21 21:12 pezcode

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.

mosra avatar Dec 20 '21 23:12 mosra

Blocked on https://github.com/BinomialLLC/basis_universal/pull/228 (December 2021), or whatever other solution gets integrated there, if at all.

mosra avatar Sep 18 '22 22:09 mosra

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

mosra avatar Sep 04 '23 18:09 mosra