Try and replace the vortex submodule with a staticlib - #91
Conversation
Signed-off-by: Adam Gutglick <adam@spiraldb.com>
There was a problem hiding this comment.
mirror the linked Vortex toolchain file, in case its load bearing
There was a problem hiding this comment.
I guess this should never matter, as we don't rely linking Rust code from diff repos on an ABI level, but only ever the C ABI.
Signed-off-by: Adam Gutglick <adam@spiraldb.com>
#8494) ## Summary Turns out, if you mark a crate as "staticlib", it will always build the static library, even if you try and pull it as a rust dependency (Like we do for `vortex-duckdb` in `vortex-sqllogictest` and benchmarks). This PR splits the `vortex-duckdb` crate into two: 1. The core stays in `vortex-duckdb`, which is now a plain old "rlib". 2. A new `vortex-extension` crate that is a "staticlib", which is in the `duckdb-vortex` repo (vortex-data/duckdb-vortex#91). 3. This saves about **1GB* per build of `vortex-duckdb`. --------- Signed-off-by: Adam Gutglick <adam@spiraldb.com>
myrrc
left a comment
myrrc
left a comment
There was a problem hiding this comment.
Can you please change the typing here? Also, please update the README with description how we depend on Vortex, and what to change if you want a local checkout (link to Cargo.toml)?
|
|
||
| extern "C" { | ||
| DUCKDB_EXTENSION_API void vortex_init(duckdb::DatabaseInstance &db) { | ||
| vortex_init_rust(reinterpret_cast<duckdb_database>(&db)); |
There was a problem hiding this comment.
We have access to duckdb C API here, let's use duckdb_database.
| crate-type = ["staticlib", "cdylib"] | ||
|
|
||
| [dependencies] | ||
| vortex-duckdb = { git = "https://github.com/vortex-data/vortex.git", rev = "b68abbb68427ed545dbd18ee3b383f78d9c34d43" } |
There was a problem hiding this comment.
Can you add a commented vortex-duckdb entry pointing to local directory with comment "This is how you can use local vortex checkout"?
There was a problem hiding this comment.
This is more of a cargo thing but you can point to a local path with path = ...
| CRATES vortex-duckdb | ||
| MANIFEST_PATH vortex-extension/Cargo.toml | ||
| CRATES vortex-extension | ||
| CRATE_TYPES ${VORTEX_LIB_TYPE} |
There was a problem hiding this comment.
Will this still work with shared vortex build?
| crate-type = ["staticlib", "cdylib"] | ||
|
|
||
| [dependencies] | ||
| vortex-duckdb = { git = "https://github.com/vortex-data/vortex.git", rev = "b68abbb68427ed545dbd18ee3b383f78d9c34d43" } |
There was a problem hiding this comment.
Ideally revs are only ever tagged commits. tag = "v0.47.0
There was a problem hiding this comment.
of course, just need to wait for a release.
| [lib] | ||
| name = "vortex_duckdb" | ||
| path = "src/lib.rs" | ||
| crate-type = ["staticlib", "cdylib"] |
There was a problem hiding this comment.
I guess the commit title kinda suggests we only build a staticlib? Might fine tune the PR/commit title here.
myrrc
left a comment
myrrc
left a comment
There was a problem hiding this comment.
If you verified it works with USE_SHARED_VORTEX=1, LGTM
|
One thing left here, we will merge this tomorrow. |
* Try and replace the vortex submodule with a staticlib Signed-off-by: Adam Gutglick <adam@spiraldb.com> * toolchain Signed-off-by: Adam Gutglick <adam@spiraldb.com> * fastlanes debug symbols Signed-off-by: Adam Gutglick <adam@spiraldb.com> * cargo fix Signed-off-by: Adam Gutglick <adam@spiraldb.com> * CR Signed-off-by: Adam Gutglick <adam@spiraldb.com> --------- Signed-off-by: Adam Gutglick <adam@spiraldb.com> (cherry picked from commit f22fc51)
3 participants
This is the other size of vortex-data/vortex#8494. Bumps the build to the last Vortex commit as of this PR.