-
Notifications
You must be signed in to change notification settings - Fork 313
cosmos: add experimental c wrapper around cosmos sdk #2906
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds an experimental C wrapper around the Azure Cosmos DB SDK for Rust, introducing a new cosmosclient
package that exports a C-compatible API. The goal is to explore publishing a C library version of the Cosmos DB SDK.
Key changes:
- New
cosmosclient
crate that builds both static and dynamic C libraries - C header generation using cbindgen with version information
- Basic C test infrastructure using CMake for validation
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
sdk/cosmos/cosmosclient/src/lib.rs |
Core library implementation with C-compatible API and version function |
sdk/cosmos/cosmosclient/Cargo.toml |
Package configuration for building C libraries (cdylib/staticlib) |
sdk/cosmos/cosmosclient/build.rs |
Build script for generating C headers and build identifiers |
sdk/cosmos/cosmosclient/CMakeLists.txt |
CMake configuration for C test compilation and linking |
sdk/cosmos/cosmosclient/c_tests/version.c |
C test program to validate version consistency |
sdk/cosmos/azure_data_cosmos/Cargo.toml |
Added "c" feature for exposing additional types to C wrapper |
Cargo.toml |
Added cosmosclient to workspace members |
b636a92
to
a663837
Compare
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.
Just two minor nits.
sdk/cosmos/cosmosclient/Cargo.toml
Outdated
@@ -0,0 +1,22 @@ | |||
[package] | |||
name = "cosmosclient" |
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.
possibly cosmos_c_client
for disambiguation?
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 agree it's a little bit ambiguous.
The problem is that the name
field determines the name of the output library (not just the file name, but also internal metadata in the library files themselves) and there doesn't appear to be a way to change that. Though I may be able to do something by explicitly specifying the [lib]
target 🤔 let me give that a shot.
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.
Ok, that works. package.name
and lib.name
can be different, the former determines the name of the package for things like cargo build --package
and the latter determines the output binary name.
sdk/cosmos/cosmosclient/src/lib.rs
Outdated
|
||
const VERSION: &CStr = c_str!(env!("CARGO_PKG_VERSION")); | ||
|
||
#[no_mangle] |
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.
This is a 100% style nit, but I generally prefer putting attributes after the rustdoc comment - since the attributes bind to the function I like putting them as close to the function declaration as I can.
Putting them before the rustdoc comment leads me to forget they exist :).
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.
That totally makes sense, I agree.
9917e41
to
e8dcbb9
Compare
/azp run rust - pullrequest |
Azure Pipelines successfully started running 1 pipeline(s). |
The vcpkg issue should be fixed now after #3076 but you also have a problem the Analyze step found:
|
/azp run rust - pullrequest |
Azure Pipelines successfully started running 1 pipeline(s). |
e8f157b
to
a281040
Compare
You'll need to rebase again. There was some breaking change in |
This is a bit of an experiment, but I'm game to merge it if @heaths and other Cosmos folks like it.
We want to explore the option of publishing a C library of the Azure Cosmos DB SDK for Rust. This PR adds the basic framework to start that effort:
cosmosclient
that depends onazure_data_cosmos
. This package builds acdylib
andstaticlib
target (libcosmosclient.a
/libcosmosclient.so
/cosmosclient.dll
/etc.) that exports a C-compatible API. For now, that API is extremely simple (cosmosclient_version
).sdk/cosmos/cosmosclient/c_tests
that will be built and linked against the cosmosclient library. The tests are simple pass/fail programs. If the test returns a0
exit code, it passed, otherwise, it failed.build.rs
script incosmosclient
to producesdk/cosmos/cosmosclient/include/cosmosclient.h
, which is a C header file defining the API surface ofcosmosclient
. We should publish this as an artifact at some point, but for now it's there to be used by tests.