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: make zlib a private dependency #1176

Closed
wants to merge 1 commit into from
Closed

Conversation

Tachi107
Copy link
Member

Before this patch, the zlib.h header was included by pistache/http.h, meaning that users had to install the development headers on zlib to build projects which depend on Pistache (in other terms, it was a transitive, public, API dependency). Since zlib.h was included by http.h just to get the value of zlib's default compression level, I simply hardcoded it to 6; it's unlikely to ever change.

@Tachi107 Tachi107 force-pushed the tachi-private-zlib branch 2 times, most recently from 9600ebe to 3c7a812 Compare November 30, 2023 11:19
Before this patch, the zlib.h header was included by pistache/http.h,
meaning that users had to install the development headers on zlib to
build projects which depend on Pistache (in other terms, it was a
transitive, public, API dependency). Since zlib.h was included by http.h
just to get the value of zlib's default compression level, I simply
hardcoded it to 6; it's unlikely to ever change.
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.09%. Comparing base (31bc54d) to head (b381c22).
Report is 217 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1176   +/-   ##
=======================================
  Coverage   78.08%   78.09%           
=======================================
  Files          53       53           
  Lines        7077     7075    -2     
=======================================
- Hits         5526     5525    -1     
+ Misses       1551     1550    -1     

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

@kiplingw
Copy link
Member

But is this a good idea? Is there any particular reason why the include needs to be removed? Was it having a noticeable impact on preprocessor burden at compile time? You are right that the constant is unlikely to change, but it still could. zlib has gone through many changes and likely still will.

@kiplingw
Copy link
Member

Hey @Tachi107, do you want me to close this or do you need more time?

@Tachi107
Copy link
Member Author

Adding a normalized 1..0 value independent of the underlying compression algorithm used seems like a better solution (which still needs to be implemented). Closing.

@Tachi107 Tachi107 closed this Sep 22, 2024
@kiplingw kiplingw deleted the tachi-private-zlib branch September 22, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants