Moved C Bridge from dotnet-sdk to core-sdk repo #951
Conversation
maciejdudko
force-pushed
the
core-c-bridge
branch
from
2006620 to
c0eb690
Compare
There was a problem hiding this comment.
Looks great, thanks for all the work here! Only a minor CI suggestion and code-gen/validation suggestion (that can be deferred).
Also, looking forward to seeing maybe a .NET draft PR that changes submodule to this branch and just to confirm we don't need any more change on this side to be usable.
| }; | ||
| } | ||
|
|
||
| async fn call_workflow_service( |
There was a problem hiding this comment.
I think we should either change this to be code generated (w/ CI check confirming code gen is accurate), or something else to confirm that every string is represented here. In .NET we do have a test IIRC to confirm every gRPC call is available. In Ruby we ended up writing a code gen to generate these big match statements (though if the match statement gets too big it can have stack problems in debug build).
If we'd like though, we can defer this to another PR. But we do need something at some point to confirm this doesn't get out of sync with the proto RPC names.
There was a problem hiding this comment.
I made a test that makes sure we have all RPC calls:
sdk-core/core-c-bridge/src/tests/mod.rs
Line 126 in c0eb690
It gets run by CI: https://github.com/temporalio/sdk-core/actions/runs/16004477686/job/45147643644?pr=951#step:7:579
There was a problem hiding this comment.
Thanks! This is good enough for me. I do think we may want code gen one day, but so long as the test is there to make sure we have to add them as they come up, sounds good.
There was a problem hiding this comment.
I think we should have a CI step that confirms that there is no git diff after build (thereby confirming that the checked in header is the latest)
There was a problem hiding this comment.
Added. Here's an example failure: https://github.com/temporalio/sdk-core/actions/runs/16008506207/job/45160619527?pr=951#step:12:1
…tion tests on Linux/ARM. Breaking changes: - Renamed crate to `temporal_sdk_core_c_bridge` - Renamed header file to `temporal-sdk-core-c-bridge.h` - Added `temporal_core_` prefix to exported C functions and `TemporalCore` prefix to exported types - Changed `TestServerOptions::download_ttl_ms` to `download_ttl_seconds` Other: - Upgraded to edition 2024, cleaned up dependencies - Added missing RPC calls - Added some smoke tests for the bridge - Moved LTO profile to workspace root - Various lints and formatting
…ated header to test CI diff check (it should fail).
maciejdudko
force-pushed
the
core-c-bridge
branch
from
2d6ced1 to
35f8f82
Compare
There was a problem hiding this comment.
Looking at temporalio/sdk-dotnet#494, I think we're good here
| #![allow(dead_code)] | ||
| #![allow(unused_imports)] |
There was a problem hiding this comment.
Would be good to make these specific to the area they're needed in
There was a problem hiding this comment.
Oops. Fixed in new PR #954
What was changed
Moved C Bridge from dotnet-sdk to core-sdk repo.
The PR has been squashed into 2 commits to make it easier to review:
💥 There are breaking changes to the C bridge:
temporal_sdk_core_c_bridgetemporal-sdk-core-c-bridge.htemporal_core_prefix to exported C functions andTemporalCoreprefix to exported typesTestServerOptions::download_ttl_mstodownload_ttl_secondsOther changes:
There's also one unrelated change to integration tests that fixes unused import warning on Linux/ARM.
Why?
It makes the C bridge available for other languages that prefer to use C interface to interact with Rust. See temporalio/sdk-dotnet#423
Checklist
README was updated to mention the new crate.