-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add zstd support #2088
base: master
Are you sure you want to change the base?
Add zstd support #2088
Conversation
@davidalo could you rebase your code based on the latest master branch, and fix the format errors reported by 'test/style-check'? Thanks! |
test/test.cc
Outdated
#else | ||
EXPECT_TRUE(ret == detail::EncodingType::None); | ||
#endif | ||
} | ||
|
||
TEST(ParseAcceptEncoding3, AcceptEncoding) { | ||
Request req; | ||
req.set_header("Accept-Encoding", "br;q=1.0, gzip;q=0.8, *;q=0.1"); | ||
req.set_header("Accept-Encoding", "br;q=1.0, gzip;q=0.8, gzip;q=0.8, *;q=0.1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a mistake? (gzip;q=0.8
appears twice now.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good catch. Changed, please check now.
eb12664
to
c269370
Compare
c269370
to
290eb0a
Compare
Done! |
All tests are passing on my side. |
Well, is CI running any of the tests? (Ignoring the internal server error on GH's end.) On Ubuntu and macOS, tests are run through Let me know if you need assistance with any of that. |
@davidalo as @falbrechtskirchinger mentioned, the unit tests should run on the GitHub Actions CI. The following lines are for the Brotri support. You can do the same or similar for the zstd support. Lines 18 to 21 in 85b5cdd
cpp-httplib/.github/workflows/test.yaml Line 54 in 85b5cdd
cpp-httplib/.github/workflows/test.yaml Line 106 in 85b5cdd
cpp-httplib/.github/workflows/test.yaml Line 117 in 85b5cdd
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to add to httplibConfig.cmake.in
find_package(ZSTD REQUIRED) | ||
set(HTTPLIB_IS_USING_ZSTD TRUE) | ||
elseif(HTTPLIB_USE_ZSTD_IF_AVAILABLE) | ||
find_package(ZSTD QUIET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find_package is case sensitive, so use zstd
(lower-case) as that's how it's installed with its zstdConfig.cmake
file.
elseif(HTTPLIB_USE_ZSTD_IF_AVAILABLE) | ||
find_package(ZSTD QUIET) | ||
# FindZLIB doesn't have a ZLIB_FOUND variable, so check the target. | ||
if(TARGET ZSTD::ZSTD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The target is zstd::libzstd
@@ -227,6 +242,7 @@ target_link_libraries(${PROJECT_NAME} ${_INTERFACE_OR_PUBLIC} | |||
$<$<BOOL:${HTTPLIB_IS_USING_BROTLI}>:Brotli::encoder> | |||
$<$<BOOL:${HTTPLIB_IS_USING_BROTLI}>:Brotli::decoder> | |||
$<$<BOOL:${HTTPLIB_IS_USING_ZLIB}>:ZLIB::ZLIB> | |||
$<$<BOOL:${HTTPLIB_IS_USING_ZSTD}>:ZSTD::ZSTD> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The target is zstd::libzstd
@@ -4,6 +4,7 @@ | |||
* HTTPLIB_USE_OPENSSL_IF_AVAILABLE (default on) | |||
* HTTPLIB_USE_ZLIB_IF_AVAILABLE (default on) | |||
* HTTPLIB_USE_BROTLI_IF_AVAILABLE (default on) | |||
* HTTPLIB_USE_ZSTD_IF_AVAILABLE (default on) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comment for HTTPLIB_REQUIRE_ZSTD
No description provided.