Conversation
5fd9924 to
d6909d5
Compare
There was a problem hiding this comment.
Pull request overview
This PR strengthens the Nim FFI coverage by adding an “all endpoints” test that forces the linker to pull in every exported symbol, and fixes an ABI mismatch for installation_name by changing it to return repr_c::String by value.
Changes:
- Add
test_all_endpoints.nimto call every Nimimportcproc at least once (linker/symbol coverage + basic runtime sanity). - Fix
installation_nameABI by switching from an explicit out-parameter to a return value on the Rust side. - Update Nimble/CI to run the new comprehensive test via
nimble test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
nim-bindings/tests/test_all_endpoints.nim |
New comprehensive endpoint/linker coverage test for Nim bindings. |
nim-bindings/src/bindings.nim |
Updates doc comment around installation_name (ownership/freeing note removed). |
nim-bindings/conversations_example.nimble |
Adds task test to compile/run the new all-endpoints test. |
conversations/src/api.rs |
Changes installation_name to return repr_c::String directly (ABI fix). |
.github/workflows/ci.yml |
Runs nimble test instead of nimble pingpong. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d6909d5 to
7988091
Compare
7988091 to
681c17c
Compare
kaichaosun
reviewed
Feb 27, 2026
a058472 to
73a598e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add nim-bindings/tests/test_all_endpoints.nim which imports bindings directly and calls every FFI proc, forcing the linker to include all symbols. This catches link-time and runtime issues that the pingpong example missed because unused symbols were optimised out. Running the new test revealed an ABI mismatch in installation_name: the Rust function used an explicit out-parameter but ReprCString has only flat fields, so Nim emits it as a C return value. CI now runs nimble test next to nimble pingpong.
73a598e to
974d06d
Compare
igor-sirotin
approved these changes
Feb 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add nim-bindings/tests/test_all_endpoints.nim which imports bindings directly and calls every FFI proc, forcing the linker to include all symbols. This catches link-time and runtime issues that the pingpong example missed because unused symbols were optimised out.
Running the new test revealed an ABI mismatch in installation_name: the Rust function used an explicit out-parameter but ReprCString has only flat fields, so Nim emits it as a C return value.
CI now runs nimble test next to nimble pingpong.