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

Make KS_ZLIB flag public #75

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ithron
Copy link

@ithron ithron commented Jan 24, 2025

When using the runtime with zlib support with cmake, the required KS_ZLIB flags is not propagated to the linking target.

When using the runtime with zlib support with cmake, the required KS_ZLIB flags is not propagated to the linking target.
@generalmimon generalmimon changed the title Make KS_ZLIP flag public Make KS_ZLIB flag public Jan 25, 2025
@generalmimon
Copy link
Member

@ithron Makes sense, thanks for the catch. Could you please fix the typo in the commit message? (It should be KS_ZLIB, not KS_ZLIP.)

I'm thinking how we can test this in CI - this is a very easy mistake to make, so we should have some sort of safety net to ensure it doesn't happen again.

If I understand correctly, the problem with -DKS_ZLIB having PRIVATE scope is that if you include kaitai/kaitaistream.h from an external project, then KS_ZLIB will not be defined and thus the process_zlib method will not exist?

#ifdef KS_ZLIB
/**
* Performs an unpacking ("inflation") of zlib-compressed data with usual zlib headers.
* @param data data to unpack
* @return unpacked data
* @throws IOException
*/
static std::string process_zlib(std::string data);
#endif

ithron and others added 2 commits January 25, 2025 18:18
When using the runtime with zlib support with cmake, the required KS_ZLIB flags is not propagated to the linking target.
@ithron
Copy link
Author

ithron commented Jan 25, 2025

Fixed the typo.

Yes, that's exactly the issue.
I think the issue went unnoticed because the unit tests don't use the zlib function. Since the unit tests are linked against the runtime using cmake, they would not compile if one of the tests tried to access process_zlib().

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

Successfully merging this pull request may close these issues.

2 participants