Fix Rust and C++ bridge release verification - #4117
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe Rust workflow now uses the ABI smoke harness from ChangesSDK smoke-test integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Setup as C++ setup
participant BridgeRun as bridge_cpp run.sh
participant BridgeCffi as bridge_cffi
participant Smoke as runtime_smoke
Setup->>BridgeCffi: Build dev-profile runtime
Setup->>BridgeRun: Set BAML_RUNTIME_PATH
BridgeRun->>Smoke: Pass resolved runtime path
Smoke->>BridgeCffi: Load runtime and execute smoke test
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/sdks/cpp/bridge_cpp/tests/run.sh`:
- Around line 18-20: Update the library-existence check in run.sh to resolve the
platform-specific runtime_lib name first, then guard the cargo build with [[ -f
"$libdir/$runtime_lib" ]]. Remove the combined ls glob check so an unmatched
alternate library name cannot trigger an unnecessary rebuild.
In `@baml_language/sdks/cpp/bridge_cpp/tests/runtime_smoke.cc`:
- Around line 42-45: Replace the assert(ready) check in the runtime smoke test
with an explicit timeout failure path that remains active under NDEBUG,
returning a nonzero exit code or aborting before calling started.state->wait()
when ready is false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f8ec8f74-625b-44ee-8aa8-1ea9147ec357
📒 Files selected for processing (4)
.github/workflows/verify-rust-sdk.reusable.yamlbaml_language/sdk_tests/crates/cpp/setup.shbaml_language/sdks/cpp/bridge_cpp/tests/run.shbaml_language/sdks/cpp/bridge_cpp/tests/runtime_smoke.cc
Binary size checks passed✅ 7 passed
Generated by |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/sdks/cpp/bridge_cpp/tests/run.sh (1)
51-51: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the
BAML_LIBRARY_PATHoverride.If callers set only
BAML_LIBRARY_PATH, this assignment adds the script’s defaultBAML_RUNTIME_PATH; the loader then raisesBAML_RUNTIME_CONFIG_CONFLICTbecause the values differ. IncludeBAML_LIBRARY_PATHin runtime-path selection and avoid injecting a conflicting value here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/cpp/bridge_cpp/tests/run.sh` at line 51, Update the runtime invocation in run.sh to honor an existing BAML_LIBRARY_PATH override when selecting the runtime path, rather than always injecting the script’s default BAML_RUNTIME_PATH. Ensure callers setting only BAML_LIBRARY_PATH do not receive a conflicting BAML_RUNTIME_PATH value, while retaining the default runtime path when no override is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@baml_language/sdks/cpp/bridge_cpp/tests/run.sh`:
- Line 51: Update the runtime invocation in run.sh to honor an existing
BAML_LIBRARY_PATH override when selecting the runtime path, rather than always
injecting the script’s default BAML_RUNTIME_PATH. Ensure callers setting only
BAML_LIBRARY_PATH do not receive a conflicting BAML_RUNTIME_PATH value, while
retaining the default runtime path when no override is provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2649fcf5-c80e-4e3a-814e-776fdf67b9d7
📒 Files selected for processing (2)
baml_language/sdks/cpp/bridge_cpp/tests/run.shbaml_language/sdks/cpp/bridge_cpp/tests/runtime_smoke.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- baml_language/sdks/cpp/bridge_cpp/tests/runtime_smoke.cc
This branch was previously deployed
1 participant
Summary
Production bridge code is unchanged, so this has no runtime performance impact.
Testing
Fixes the bridge failures in https://github.com/BoundaryML/baml/actions/runs/29874397753
Summary by CodeRabbit