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

Fix libzstd-dev on debian build #1261

Conversation

EdgarModesto23
Copy link
Contributor

@EdgarModesto23 EdgarModesto23 commented Nov 12, 2024

From #1247

This patch fixes issues mentioned by @kiplingw on #1247 (comment).

The issue was caused because the patch that adds zstd support uses the ZSTD_defaultCLevel() function. which is only supported by libzstd v1.5.0+. The current configuration uses libzstd v1.4.8

This PR fixes that by specifying that pistache depends on libzstd >= 1.5.4.

@kiplingw
Copy link
Member

Unfortunately that does not fix the problem as I previously alluded to. Version 1.4.8 is what ships with Jammy. Merging will continue to break all builds on Jammy. There are several options:

  • Drop Pistache support entirely for Jammy;
  • We continue supporting Jammy, but just don't build with ZSTD support on it;
  • Check at configure time for the presence of ZSTD_defaultCLevel(), after verifying the library is present, and then define some preprocessor definition similar to how AC_CHECK_FUNC works;
  • Rewrite your code so that, when building with ZSTD support, it does not use any interfaces introduced after 1.4.8.

I think the third option is the best engineering approach.

@kiplingw kiplingw added bug portability dependencies Pull requests that update a dependency file labels Nov 12, 2024
Copy link
Member

@kiplingw kiplingw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comments.

@EdgarModesto23
Copy link
Contributor Author

@kiplingw Got it. I think in this case, option 4 is the easiest solution. Since ZSTD_defaultCLevel() just returns the default for ZSTD_CLEVEL_DEFAULT, which is 3 according to documentation.

@kiplingw
Copy link
Member

In that case then I'll close this PR and you can amend #1247.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug dependencies Pull requests that update a dependency file portability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants