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

C Extension API #381

Merged
merged 42 commits into from
Oct 8, 2024
Merged

Conversation

samansmink
Copy link
Collaborator

@samansmink samansmink commented Sep 16, 2024

This PR makes the first step towards proper support for #370.
In the process it also bumps duckdb-rs to duckdb v1.1.1

With this PR I we introduce the experimental duckdb_entrypoint_c_api proc macro that will basically handle everything for you as can be seen in this snippet from crates/duckdb/examples/hello-ext-capi:

#[duckdb_entrypoint_c_api(ext_name = "rusty_quack", min_duckdb_version = "v0.0.1")]
pub unsafe fn ExtensionEntrypoint(con: Connection) -> Result<(), Box<dyn Error>> {
    con.register_table_function::<HelloVTab>("hello").expect("Failed to register hello table function");
    Ok(())
}

Note that this relies on the new extension C api added in duckdb/duckdb#12682.

The main idea is that these binaries are (going to be) forwards compatible with new duckdb releases. This compatibility can be configured using the min_duckdb_version parament of the proc macro. For now, this version is set to a separate C extension API version (which is v0.0.1) in current duckdb stable. However in the future, this version will likely be simply the duckdb version. This means that loadable extensions currently produced with the duckdb_entrypoint_c_api proc are not stable right now. This stability is expected to be possible in DuckDB v1.2.0 in a few months.

Finally, the extension binaries in this PR are not ready to be loaded by DuckDB directly, they need to have their extension metadata appended. I will add this script to the https://github.com/duckdb/extension-ci-tools repo and create a rust extension template that uses the script to perform the final transform step for rust shared libary to loadable duckdb extension

follow ups

  • testing: the upgrade script running the bindgen performs no tests for the pre-generated bindings currently
  • create the demo extension template

samansmink and others added 20 commits August 13, 2024 16:18
some paths were incorrect
optional dependency too
seems like all the scripts assume the submodule exists but its was being gitignored incorrectly
shell early exit and some conditional compilation issues
bash -e is not valid. https://explainshell.com/explain/1/bash i'm guessing this is supposed to actually be set -e or something

removing it everywhere
if there is a need for these to be duplicated, we should share them instead

i think all this stuff should go into a makefile or something which composes a bit better but this works for now
i want to comment out the test so i can use this even if the tests are failing
this is breaking everything
@0xcaff
Copy link
Contributor

0xcaff commented Sep 16, 2024

nice nice! I'll try to get this reviewed today evening or early tomorrow


# Download and extract amalgamation
DUCKDB_VERSION=v1.0.0
# todo: fix this version
DUCKDB_VERSION=v1.1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can remove the todo now

find "$SCRIPT_DIR/../../target" -type f -name bindgen.rs -exec cp {} "$SCRIPT_DIR/src/bindgen_bundled_version_loadable.rs" \;

# Sanity checks
# FIXME: how to test this here?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc some of the tests are bindgen struct layout validations, not sure if these were failing or not

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I will take a look, I think the problem here was that when building with the loadable_extension variant of the bindings, the tests have no way to create a duckdb instance making it basically impossible to run most of the tests

@samansmink
Copy link
Collaborator Author

Have a sample extension template here https://github.com/samansmink/extension-template-rs.

I think once we have this PR merged, I can:

  • create duckdb/extension-template-rs
  • create a new reusable workflow similar to https://github.com/duckdb/extension-ci-tools/blob/main/.github/workflows/_extension_distribution.yml but with just latest OS images everywhere since toolchain conpatibility no longer matters.
  • Move makefile complexity out of the extension-template-rs into extension-ci-tools similar to how the c++ extension template does things.
  • Add things like a rename script, extension uploading script, etc.

Then later I need to think about:

  • How to setup a testing framework for extension-template-rs?
  • Adding this to the community extensions so that people can distribute their Rust-only based extensions.

Copy link
Contributor

@0xcaff 0xcaff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one blocking correctness issue (the pointer in duckdb_entrypoint_c_api's generated output is missing)

Makefile Outdated Show resolved Hide resolved
crates/duckdb-loadable-macros/src/lib.rs Outdated Show resolved Hide resolved
crates/duckdb/examples/hello-ext-capi/main.rs Show resolved Hide resolved
crates/libduckdb-sys/build.rs Show resolved Hide resolved
crates/libduckdb-sys/build.rs Show resolved Hide resolved
crates/libduckdb-sys/build.rs Outdated Show resolved Hide resolved
crates/duckdb-loadable-macros/src/lib.rs Outdated Show resolved Hide resolved
@0xcaff
Copy link
Contributor

0xcaff commented Sep 16, 2024

regarding tests for extensions, a contributor contributed tests to my project https://github.com/0xcaff/duckdb_protobuf/blob/master/packages/duckdb_protobuf/tests/it/main.rs might be useful as a scaffold though there are some rough edges to work out still

@samansmink samansmink merged commit 71b01f7 into duckdb:main Oct 8, 2024
4 checks passed
@samansmink samansmink deleted the poc-rust-c-extension-api branch October 8, 2024 11:09
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.

2 participants