-
Notifications
You must be signed in to change notification settings - Fork 32
Use curl as optional client v1.4 #96
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
Use curl as optional client v1.4 #96
Conversation
…t overwritten on the stack
Yes this will fix my build errors at duckdb/duckdb-httpfs#96. This CI link has a passing build status https://github.com/duckdb/duckdb-httpfs/actions/runs/16991311944/job/48171130825 ~~[DO NOT MERGE]: I am testing to see if this is the correct fix with [this PR](duckdb/duckdb-httpfs#96) first. I am just updating the duckdb submodule pointer for the httpfs fork to the branch here. If those tests pass then I know what the correct fix is. (don't know how to trigger it otherwise yet)~~ This is prompted by this PR duckdb/duckdb-httpfs#96. Related [CI failure](https://github.com/duckdb/duckdb-httpfs/actions/runs/16932639981/job/47981684022?pr=96#step:26:597) Seems like the httplib has conflicts with the max() function. I've searched for other instances of `::max()` and `::min()` in httplib.hpp and didn't find any. It seems like the proper fix is to use `(std::numeric_limits<size_t>::max)()` as seen on line [96 of httplib.hpp](https://github.com/duckdb/duckdb/blob/1f0de28806a8915c8203dd060dad549f28f5539b/third_party/httplib/httplib.hpp#L96) and that did not fail the windows build
I remember there was a problem when running: SET httpfs_client_implementation='curl';
INSTALL non_existent_extension; throwing an uncaught error, that likely signal a problem elsewhere. Could you check whether that's still the case? Apart from that, also considering this is not the default AND it's on 1.4- branch, I would think this is good to be merged. |
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.
Thanks, looks great to have this!
Other things to consider, as follow ups:
|
Should we merge or is there any blocker? I think merge + bumping duckdb-httpfs hash in duckdb/duckdb would make this more properly available, other steps such as docs can be done then before 1.4.0 |
After duckdb/duckdb#18107 landed in duckdb/duckdb, and moving the duckdb submodule to a recent commit on v1.4-andium, this PR allows to switch at runtime based on the newly added httpfs config option httpfs_client_implementation: