-
Notifications
You must be signed in to change notification settings - Fork 13.6k
vendor: split httplib to cpp/h files #17150
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
Conversation
common/CMakeLists.txt
Outdated
| set(CMAKE_REQUIRED_INCLUDES ${SAVED_CMAKE_REQUIRED_INCLUDES}) | ||
| if (OPENSSL_VERSION_SUPPORTED) | ||
| message(STATUS "OpenSSL found: ${OPENSSL_VERSION}") | ||
| set(LLAMA_COMMON_EXTRA_LIBS ${LLAMA_COMMON_EXTRA_LIBS} cpp-httplib) # requires httplib for SSL support |
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.
Not sure why this is needed - seems incorrect.
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 line next to this suggests that it's used by httplib, CPPHTTPLIB_OPENSSL_SUPPORT
So I assume that it need to be present here. Seems to be used by the download functionality (alternative to curl), right?
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.
Otherwise, I think it should be in the if NOT LLAMA_CURL condition, meaning if we don't use curl, then we will use httplib. Though I'm not sure if this is correct. CC @angt
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.
Ah nevermind, it's correct that libcommon needs cpp-httplib if CURL is not used. Last commit should fix the CI error: https://github.com/ggml-org/llama.cpp/actions/runs/19241641967/job/55005558638?pr=17150
| set(LLAMA_COMMON_EXTRA_LIBS ${LLAMA_COMMON_EXTRA_LIBS} ${CURL_LIBRARIES}) | ||
| else() | ||
| # otherwise, use cpp-httplib | ||
| set(LLAMA_COMMON_EXTRA_LIBS ${LLAMA_COMMON_EXTRA_LIBS} cpp-httplib) |
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.
nit: add a TODO to make LLAMA_COMMON_EXTRA_LIBS a list and use:
list(APPEND LLAMA_COMMON_EXTRA_LIBS ...)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.
added in 3397349
Split
httplib.hinto cpp and header files, allowing it to be compiled as separated unitRef script: https://github.com/yhirose/cpp-httplib/blob/master/httplib.h