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

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

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

mosra
Copy link
Owner

@mosra mosra commented Nov 13, 2021

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 mosra added this to the 2021.0a milestone Nov 13, 2021
@codecov
Copy link

codecov bot commented Nov 13, 2021

Codecov Report

Patch and project coverage have no change.

Comparison is base (d8eb82c) 97.06% compared to head (8b6c348) 97.06%.

❗ 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% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mosra
Copy link
Owner Author

mosra commented Dec 20, 2021

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.

@pezcode
Copy link
Contributor

pezcode commented Dec 20, 2021

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.

@mosra
Copy link
Owner Author

mosra commented Dec 20, 2021

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

@pezcode
Copy link
Contributor

pezcode commented Dec 20, 2021

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?

@mosra
Copy link
Owner Author

mosra commented Dec 20, 2021

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 mosra removed this from the 2022.0a milestone Sep 18, 2022
@mosra
Copy link
Owner Author

mosra commented Sep 18, 2022

Blocked on BinomialLLC/basis_universal#228 (December 2021), or whatever other solution gets integrated there, if at all.

@mosra mosra added this to the 2023.0a milestone Sep 3, 2023
@mosra mosra force-pushed the basis-no-bundled-zstd branch 6 times, most recently from 8b6c348 to 2183d47 Compare September 4, 2023 18:12
@mosra mosra marked this pull request as ready for review September 4, 2023 18:47
@mosra
Copy link
Owner Author

mosra commented Sep 4, 2023

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

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.
@mosra mosra force-pushed the basis-no-bundled-zstd branch from 4ab02eb to 7f29410 Compare September 4, 2023 20:49
@mosra mosra merged commit 7f29410 into master Sep 4, 2023
@mosra mosra deleted the basis-no-bundled-zstd branch September 4, 2023 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants