-
Notifications
You must be signed in to change notification settings - Fork 39
Wasm HTTPUtil on main #76
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
af36634 to
a2cc411
Compare
|
This is ready to be merged on my side |
| SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR} | ||
| INCLUDE_DIR ${CMAKE_CURRENT_LIST_DIR}/extension/httpfs/include | ||
| ${LOAD_HTTPFS_TESTS} | ||
| LINKED_LIBS "../../third_party/mbedtls/libduckdb_mbedtls.a" |
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 this necessary here?
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.
Unfortunately, yes, I have not found a clean way to clean this up.
It should picked up from CMake, but that requires some changes in duckdb/duckdb that are not there (and unsure if they can land today).
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.
I can fix this up, question is: it's a blocker?
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.
I don't know what problems it causes but it seems hacky - maybe it's fine. I'll leave it up to you
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.
It does smell badly, you are right, now triggered to find the actual fix.
This is not super pretty, since I have not found a more proper way to feature detect HTTPFSClient than
#ifdef's onEMSCRIPTENbeing defined.Weakest part of this PR are the changes in extension/httpfs/httpfs_client.cpp, open to suggestions.
EDIT: I did changed approach, and skip the file at the CMake level
After this PR
httpfsis re-enabled for all platforms.