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

Made it easier to use the library with external zstdlib implementations. #228

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ondys
Copy link
Contributor

@ondys ondys commented Apr 24, 2021

Mostly for non CMake builds.

In our internal repository, we have our own version of zstdlib and introducing extra copy is both undesirable and potentially dangerous (due to ODR violations). This allows us to ignore the added zstd/ folder and use our own version of the library.

…ns (mostly in non CMake builds).

In our internal repository, we have our own version of zstdlib and introducing extra copy is both undesirable and potentially dangerous (due to ODR violations).
@richgel999
Copy link
Contributor

richgel999 commented May 3, 2021

Yes I understand (and I figured this would be an issue with some users). After I fix a couple bugs I'll test and merge this in.

@richgel999
Copy link
Contributor

Hmm - I need to think this change through a bit more and test it. However, I feel your pain here and will figure something out.

@thesamesam
Copy link

thesamesam commented Jan 28, 2023

This would ideally allow using a system copy as well for distributions, using e.g. find_package and pkg_find_module.

@akien-mga
Copy link
Contributor

akien-mga commented Feb 17, 2023

I had to do a similar change in Godot, where we also use our own copy of zstd, and it can also be unbundled to link against system libraries on Linux: godotengine/godot#73441

The same change is needed in the transcoder, could you add it to this PR @ondys?

https://github.com/godotengine/godot/pull/73441/files#diff-c96a895f79f493b2670e1b74fe6058808d4dbe9f03f6e712075943c4d0df4801

Edit: I made #344 with this extra change so there's a merge-ready option. I'm happy to close it if this PR is updated with the transcoder fix.

@mosra
Copy link
Contributor

mosra commented Sep 4, 2023

Just in case someone would find this useful, in Magnum I ended up solving this problem by creating the following filesystem hierarchy:

put-this-into-the-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/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 <> instead of "" to make the compiler pick a header from the include path and not itself -- not even GCC/Clang-specific #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 in the include path in which case 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants