-
Notifications
You must be signed in to change notification settings - Fork 13.7k
cmake : move OpenSSL linking to vendor/cpp-httplib #17177
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
cmake : move OpenSSL linking to vendor/cpp-httplib #17177
Conversation
Signed-off-by: Adrien Gallouët <angt@huggingface.co>
If you spot a problem, you should raise an issue to the original repo: https://github.com/yhirose/cpp-httplib
|
|
ok sorry I totally missed that the version was hard-coded in if someone upgrade vendor version, please also change the version in the file. otherwise the old version will be pulled each time a contributor run the script |
|
@angt can you check if SSL is working now? httplib is bump to 0.27.0 |
|
It works now :) |
|
hmm, seems like the newer httplib version breaks the Vision OS build (again): https://github.com/ggml-org/llama.cpp/actions/runs/19278477351/job/55123989425?pr=17177 |
|
|
||
| # 3rd party libs | ||
| option(LLAMA_CURL "llama: use libcurl to download model from an URL" ON) | ||
| option(LLAMA_HTTPLIB "llama: if libcurl is disabled, use httplib to download model from an URL" 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.
@ggerganov tagging you for visibility, I had to add this to be able to exclude httplib from visionOS build, as it's not compatible
|
Why was visionOS not an issue before? |
|
That's because we only compile part of the code with header-only. When the lib is moved to a dedicated targer, it must compile all functions. Unused functions are discarded at linking |
|
Instead of splitting maybe we can create a generic |
no because server.cpp compilation time will still be long. |
|
Hmm you sure? Currently it’s very slow because every time we include |
compiling server.cpp is slow because it include httplib.h directly with implementations inside, not because it's included via common.h and no, include common.h does not also include httplib.h (if yes, why?). if you look into common.h, there is no other include that links to either json.hpp or httplib.h. only either including download.h or http.h does that. and lastly, splitting cpp/h is actually an option provided by httplib. I don't see why we can't use it. Indeed, we should, as we don't want to re-compile the same code in multiple places. a good example is |
|
Re. your point about only one single compilation unit. Currently without splitting cpp/h, the lib will be compiled in 2 places, both libcommon (only the httplib::Client is used) and llama-server (httplib::Server), so it's being compiled twice now. Ofc we can also just move the server part to http.h (I remember we discussed about this back then). But we are still unsure how the implementation should look like. I think we can consider moving back to header-only once we figure this out. |
ggerganov
left a comment
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.
That's because we only compile part of the code with header-only. When the lib is moved to a dedicated targer, it must compile all functions. Unused functions are discarded at linking
https://github.com/ggml-org/llama.cpp/actions/runs/19278477351/job/55123989425#step:5:217
This is a compile error, not a linker error. In both cases the entire code of httplib is compiled.
Most likely httplib does not include certain headers that are needed for VisionOS. Before we likely included those header indirectly, before including the httplib. Ideally httplib should start including these headers and it would not be necessary to turn it off on our end.
|
No, I meant that it was included in |
|
@angt the total compilation time stays unchanged. But each time I modify the code in server.cpp then recompile it, without having httplib in its own unit, it takes 24 seconds to finish. After the split it only takes 16s. And that's what matters for someone who spend quite a lot of time working on server.cpp Without splitting, what's your solution then? |
I have no issue with the splitting solution :) just pointing out another way without touching the header-only provided by My point was that having an Another benefit is that with our own interface, |
This commit fixes the compilation with OpenSSL, but it doesn’t fix SSL....
The use of version 0.20.1 of cpp-httplib (instead of 0.27.0) might be the only culprit, no idea how to update the code tho
cc @ngxson 👀