Migrate to using Diplomat for FFI#75
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Migrates the project’s non-Rust language bindings (C/C++/Python) from the bespoke C ABI + PyO3/maturin approach to a Diplomat-based FFI pipeline, with Bazel as the primary build/packaging path and updated CI to build/test wheels.
Changes:
- Introduces a Rust Diplomat bridge (
core/ffi/diplomat) and a Bazel-builtdiplomat_codegentool to generate C/C++/nanobind bindings. - Reworks Python packaging to nanobind + Bazel
py_wheel(addspackage_metadata.bzl, wheel rules, updates CI). - Updates examples/docs/build files to use generated Diplomat bindings and new Bazel targets; removes the old C ABI shim and PyO3 crate.
Reviewed changes
Copilot reviewed 42 out of 44 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/update_versions.py | Updates Python version syncing to use package_metadata.bzl instead of removed Python Cargo.toml. |
| tools/requirements.txt | Drops maturin; adds nanobind to the pinned tool requirements. |
| tools/requirements.in | Drops maturin; adds nanobind input requirement. |
| tools/diplomat_codegen/main.rs | Adds a small Rust CLI wrapper around diplomat-tool generation. |
| tools/diplomat_codegen/BUILD.bazel | Bazel rule for building the diplomat_codegen Rust binary. |
| tools/BUILD.bazel | Adjusts update_versions tool deps/data for new Python metadata + rules_python pip repos. |
| tests/BUILD.bazel | Updates pytest dependency label to rules_python pip canonical repo label. |
| python/wheels.bzl | Adds Bazel macros for building nanobind extensions and packaging wheels. |
| python/src/lib.rs | Removed: legacy PyO3 binding implementation. |
| python/pyproject.toml | Removes maturin build-system config; updates Python version requirement/classifiers. |
| python/package_metadata.bzl | Adds Bazel-sourced Python package metadata (name/version/classifiers/urls/etc.). |
| python/dv_py/init.py | Replaces direct PyO3 module exports with a nanobind + compatibility wrapper API. |
| python/Cargo.toml | Removed: legacy Rust cdylib crate used for PyO3/maturin builds. |
| python/BUILD.bazel | Adds codegen + nanobind extension builds + wheel targets; updates test deps. |
| examples/cpp/main.cpp | Migrates example usage to generated Diplomat C++ API patterns. |
| examples/cpp/CMakeLists.txt | Simplifies CMake example wiring (but examples marked temporarily unavailable in docs). |
| examples/cpp/BUILD.bazel | Switches example dependency to generated C++ target. |
| examples/c/main.c | Migrates example usage to generated Diplomat C API (result structs, string views). |
| examples/c/CMakeLists.txt | Updates C example linking (now expects dv_c). |
| examples/c/BUILD.bazel | Switches example dependency to the new //c:dv_c target. |
| examples/README.md | Updates example guidance for Diplomat migration; de-emphasizes CMake. |
| docs/docs/python.md | Updates Python docs to Diplomat+nanoBind + Bazel wheel build workflow. |
| docs/docs/intro.md | Updates language support table and adds repository layout section. |
| docs/docs/dev/code.md | Updates dev build commands for Python wheels/extensions under Bazel. |
| docs/docs/c_cpp.md | Updates C/C++ docs to Diplomat-generated bindings and new Bazel targets. |
| cpp/include/dv_c.h | Removed: legacy C ABI header. |
| cpp/include/dv.hpp | Refactors C++ compatibility wrapper to wrap the generated Diplomat C++ API. |
| cpp/capi/src/lib.rs | Removed: legacy Rust C ABI shim implementation. |
| cpp/capi/Cargo.toml | Removed: legacy Rust crate manifest for the C ABI shim. |
| cpp/README.md | Updates overview to reference Diplomat bridge + new targets (contains a path issue noted in comments). |
| cpp/BUILD.bazel | Replaces Rust C ABI shim build with Diplomat codegen + generated header/library targets. |
| core/ffi/diplomat/src/lib.rs | Adds the Diplomat bridge API over dv_rs::DimensionalVariable. |
| core/ffi/diplomat/Cargo.toml | Adds Cargo manifest for the Diplomat bridge crate. |
| core/ffi/diplomat/BUILD.bazel | Adds Bazel targets for the Diplomat bridge (rust_library + staticlib). |
| c/README.md | Adds readme for the new C bindings directory. |
| c/BUILD.bazel | Adds Diplomat C header generation and cc_library exposing generated headers + static bridge. |
| README.md | Adds top-level repository structure summary; updates copyright year. |
| MODULE.bazel | Removes rules_rust_pyo3; adds diplomat crates; expands Python toolchains/pip hubs. |
| CMakeLists.txt | Reworks CMake integration to run Diplomat codegen via Bazel + build bridge via Cargo. |
| AGENTS.md | Updates contributor guidance to reflect Diplomat + language directory layout. |
| .gitignore | Adjusts ignored CMake build directory patterns. |
| .github/workflows/_python.yaml | Replaces maturin action with Bazel wheel builds + wheel smoke tests across Python versions. |
| .bazelrc | Removes PyO3 stubgen workaround flag (PyO3 no longer used). |
| inline std::string last_error() { | ||
| const char* msg = dv_last_error_message(); | ||
| return msg ? std::string(msg) : std::string(); | ||
| return "dv operation failed"; | ||
| } |
There was a problem hiding this comment.
last_error() now always returns a fixed string, so exceptions thrown by the C++ compatibility wrapper lose the underlying reason (invalid unit, incompatible units, etc.). If the Diplomat bridge can return an error message (instead of std::monostate/()), thread that message through unwrap_* so callers get actionable diagnostics.
| def __lt__(self, other): | ||
| try: | ||
| return self._inner.less_than(other._inner) | ||
| except Exception as exc: | ||
| raise _map_exc(exc) from None |
There was a problem hiding this comment.
Rich comparisons (__lt__, __le__, etc.) assume other has an _inner attribute; comparing against a non-DimensionalVariable will raise and get mapped to DVError, which is surprising for Python comparisons. Add an isinstance(other, DimensionalVariable) guard and return NotImplemented (or raise TypeError) for unsupported types so Python can fall back to reflected comparisons correctly.
| BUILD_BYPRODUCTS | ||
| ${DV_BRIDGE_DIR}/target/release/libdv_diplomat_bridge.a | ||
| ) | ||
|
|
||
| # Library artifact path | ||
| if(APPLE) | ||
| set(DV_CAPI_SHARED ${DV_RUST_DIR}/target/release/libdv_capi.dylib) | ||
| elseif(UNIX) | ||
| set(DV_CAPI_SHARED ${DV_RUST_DIR}/target/release/libdv_capi.so) | ||
| elseif(WIN32) | ||
| set(DV_CAPI_SHARED ${DV_RUST_DIR}/target/release/dv_capi.dll) | ||
| endif() | ||
| set(DV_CAPI_STATIC ${DV_RUST_DIR}/target/release/libdv_capi.a) | ||
| set(DV_DIPLOMAT_STATIC ${DV_BRIDGE_DIR}/target/release/libdv_diplomat_bridge.a) | ||
|
|
||
| add_library(dv_c STATIC IMPORTED GLOBAL) | ||
| set_target_properties(dv_c PROPERTIES | ||
| IMPORTED_LOCATION ${DV_CAPI_STATIC} | ||
| INTERFACE_INCLUDE_DIRECTORIES ${CMAKE_CURRENT_LIST_DIR}/cpp/include | ||
| IMPORTED_LOCATION ${DV_DIPLOMAT_STATIC} | ||
| INTERFACE_INCLUDE_DIRECTORIES ${DV_CODEGEN_C_DIR} |
There was a problem hiding this comment.
The imported static library path is hard-coded to libdv_diplomat_bridge.a. On Windows, Cargo typically produces a .lib (and different naming conventions), so this CMake build will fail there. Add platform-specific handling for the produced static artifact (and consider wiring CARGO_TARGET_DIR if you want predictable output locations).
| - `/Users/alex/code/alextac98/dv/c` | ||
| - `/Users/alex/code/alextac98/dv/cpp` |
There was a problem hiding this comment.
This README currently points to absolute paths under /Users/alex/..., which are machine-specific and will be incorrect for all other contributors. Replace these with repository-relative paths (e.g., c/ and cpp/) and/or link to the relevant Bazel targets/docs pages.
| - `/Users/alex/code/alextac98/dv/c` | |
| - `/Users/alex/code/alextac98/dv/cpp` | |
| - `c/` | |
| - `cpp/` |
| def dv_runtime_package(name, native_target): | ||
| native.genrule( | ||
| name = name, | ||
| srcs = [native_target], | ||
| outs = [ | ||
| "dv_py/dv_ext.so", | ||
| "dv_py/dv_ext.pyd", | ||
| ], | ||
| cmd_bash = "cp $< $(RULEDIR)/dv_py/dv_ext.so && cp $< $(RULEDIR)/dv_py/dv_ext.pyd", | ||
| cmd_bat = "copy /Y \"$(location {})\" \"$(RULEDIR)\\dv_py\\dv_ext.so\" >NUL && copy /Y \"$(location {})\" \"$(RULEDIR)\\dv_py\\dv_ext.pyd\" >NUL".format(native_target, native_target), | ||
| ) |
There was a problem hiding this comment.
dv_runtime_package always produces and copies the extension into both dv_ext.so and dv_ext.pyd. This makes every runtime package (and potentially every wheel) ship two extension files, one of which is irrelevant on the current platform and may confuse downstream packaging/validation. Consider emitting only the platform-appropriate extension filename (e.g., via per-platform genrules + select()-driven filegroup inputs to the wheel) so each artifact contains exactly one extension module.
| native.genrule( | ||
| name = pkg_target, | ||
| srcs = [ | ||
| native_target, | ||
| "dv_py/__init__.py", | ||
| ], | ||
| outs = [ | ||
| "{}/dv_py/__init__.py".format(wheel_dir), | ||
| "{}/dv_py/dv_ext.so".format(wheel_dir), | ||
| "{}/dv_py/dv_ext.pyd".format(wheel_dir), | ||
| ], | ||
| cmd_bash = "cp $(location dv_py/__init__.py) $(RULEDIR)/{0}/dv_py/__init__.py && cp $(location {1}) $(RULEDIR)/{0}/dv_py/dv_ext.so && cp $(location {1}) $(RULEDIR)/{0}/dv_py/dv_ext.pyd".format(wheel_dir, native_target), | ||
| cmd_bat = "copy /Y \"$(location dv_py/__init__.py)\" \"$(RULEDIR)\\{0}\\dv_py\\__init__.py\" >NUL && copy /Y \"$(location {1})\" \"$(RULEDIR)\\{0}\\dv_py\\dv_ext.so\" >NUL && copy /Y \"$(location {1})\" \"$(RULEDIR)\\{0}\\dv_py\\dv_ext.pyd\" >NUL".format(wheel_dir, native_target), | ||
| ) |
There was a problem hiding this comment.
dv_wheel genrule similarly copies the same native_target output to both dv_ext.so and dv_ext.pyd inside the wheel contents. This means published wheels will include an extra, non-importable extension file on each platform (and a potentially misleading filename on Windows/macOS/Linux). Adjust the wheel contents to include only the correct extension suffix per platform, and ensure the __init__.py import continues to resolve the right module.
| #[diplomat::opaque] | ||
| pub struct DimensionalVariable(CoreDV); | ||
|
|
||
| impl DimensionalVariable { | ||
| pub fn new(value: f64, unit: &str) -> Result<Box<DimensionalVariable>, ()> { | ||
| match CoreDV::new(value, unit) { | ||
| Ok(dv) => Ok(Box::new(DimensionalVariable(dv))), | ||
| Err(_) => Err(()), | ||
| } | ||
| } | ||
|
|
||
| pub fn asin_scalar(x: f64) -> Result<Box<DimensionalVariable>, ()> { | ||
| dv_rs::asin(x).map(into_boxed_dv).map_err(|_| ()) | ||
| } | ||
|
|
||
| pub fn acos_scalar(x: f64) -> Result<Box<DimensionalVariable>, ()> { | ||
| dv_rs::acos(x).map(into_boxed_dv).map_err(|_| ()) | ||
| } | ||
|
|
||
| pub fn atan_scalar(x: f64) -> Result<Box<DimensionalVariable>, ()> { | ||
| dv_rs::atan(x).map(into_boxed_dv).map_err(|_| ()) |
There was a problem hiding this comment.
The Diplomat bridge converts all core errors (which are String in dv_rs) into Err(()), which discards the actual error message. This regresses debuggability and breaks the prior C/Python behavior where callers could surface detailed parsing/compatibility errors. Consider returning a string error type through Diplomat (e.g., a small error struct/enum carrying a message, or a DiplomatWrite-produced string) so bindings can propagate meaningful messages.
| #[diplomat::opaque] | |
| pub struct DimensionalVariable(CoreDV); | |
| impl DimensionalVariable { | |
| pub fn new(value: f64, unit: &str) -> Result<Box<DimensionalVariable>, ()> { | |
| match CoreDV::new(value, unit) { | |
| Ok(dv) => Ok(Box::new(DimensionalVariable(dv))), | |
| Err(_) => Err(()), | |
| } | |
| } | |
| pub fn asin_scalar(x: f64) -> Result<Box<DimensionalVariable>, ()> { | |
| dv_rs::asin(x).map(into_boxed_dv).map_err(|_| ()) | |
| } | |
| pub fn acos_scalar(x: f64) -> Result<Box<DimensionalVariable>, ()> { | |
| dv_rs::acos(x).map(into_boxed_dv).map_err(|_| ()) | |
| } | |
| pub fn atan_scalar(x: f64) -> Result<Box<DimensionalVariable>, ()> { | |
| dv_rs::atan(x).map(into_boxed_dv).map_err(|_| ()) | |
| pub struct DVError { | |
| pub message: String, | |
| } | |
| #[diplomat::opaque] | |
| pub struct DimensionalVariable(CoreDV); | |
| impl DimensionalVariable { | |
| pub fn new(value: f64, unit: &str) -> Result<Box<DimensionalVariable>, DVError> { | |
| match CoreDV::new(value, unit) { | |
| Ok(dv) => Ok(Box::new(DimensionalVariable(dv))), | |
| Err(e) => Err(DVError { message: e }), | |
| } | |
| } | |
| pub fn asin_scalar(x: f64) -> Result<Box<DimensionalVariable>, DVError> { | |
| dv_rs::asin(x) | |
| .map(into_boxed_dv) | |
| .map_err(|e| DVError { message: e }) | |
| } | |
| pub fn acos_scalar(x: f64) -> Result<Box<DimensionalVariable>, DVError> { | |
| dv_rs::acos(x) | |
| .map(into_boxed_dv) | |
| .map_err(|e| DVError { message: e }) | |
| } | |
| pub fn atan_scalar(x: f64) -> Result<Box<DimensionalVariable>, DVError> { | |
| dv_rs::atan(x) | |
| .map(into_boxed_dv) | |
| .map_err(|e| DVError { message: e }) |
Migrates the project from using manual bindings to Diplomat for bindings to C/C++ and nanobind for Python. As part of this, the project is reorganized to make it easier to support multiple different languages and types of bindings