Skip to content
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 a feature to link custom duckdb extension #1397

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

ocadaruma
Copy link

@ocadaruma ocadaruma commented Sep 3, 2023

First of all, thanks for the great project!

This PR tries to add a feature to link custom duckdb extension to duckdb-wasm, which isn't possible without modifying duckdb-wasm codebase for now.

Since I'm quite new to duckdb, duckdb-wasm development (and even CMake/Typescript projects!), I'm still not sure what's the best approach to implement this feature.

Please feel free to give me your opinions about overall strategy.

Summary of the changes

  • Change lib/CMakeLists.txt to include sub CMake projects which is configured by CUSTOM_EXTENSION_DIRS Makefile variable.
    • CMake projects in these directories are supposed to call register_custom_extension macro to tell duckdb-wasm CMake-build to add ${ext_name}_init function call and link the library
  • Add a capability to skip EH/COI builds
    • My initial motivation was to build custom extension in Rust and run it on duckdb-wasm.
      • We have to use wasm32-unknown-emscripten target in Rust build so that ABI matches to duckdb-wasm's build (it's built by emscripten)
    • However, as of now, wasm-eh is not supported yet
      • Also coi isn't supported
    • Hence we need to have a feature to turn off wasm-eh build.
    • This is done by two Makefile variables DUCKDB_SKIP_BUILD_EH and DUCKDB_SKIP_BUILD_COI.
    • However, due to this "turn-off eh/coi"-mechanism, ifdef-like switches are introduced all around the codes.
      • Please tell me if you have ideas of better strategy

Example

TODOs

  • Currently, tests don't pass if eh/coi are disabled. I should do something also for that.

@carlopi
Copy link
Collaborator

carlopi commented Sep 4, 2023

Thanks a lot for your work! I will later give some comments, but looks strong.

@carlopi
Copy link
Collaborator

carlopi commented Sep 5, 2023

Thanks again for raising this.

Thinking more about this, this is closely linked with some features that I had in the back on my mind for a bit:

  • have a way to build only a subset of the targets (would also allow to iterate during development faster)
  • connected to previous point, but to be reached in a different way, have a way to test on a specific subset of targets
  • have a way to link custom extensions

I am currently reworking some infrastructure to allow duckdb-wasm to build loadable extensions.
Once that lands I will see how this PR can fit, and it would also be cool to investigate whether loadable Rust extensions are viable following a similar path (= forcing mvp target).

@ocadaruma
Copy link
Author

I am currently reworking some infrastructure to allow duckdb-wasm to build loadable extensions.
Once that lands I will see how this PR can fit

That sounds great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants