Test Rust API on Windows - #3385
Conversation
dosubot
Bot
added
the
size:L
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis pull request extends Windows CI/CD support by adding MSVC tooling setup and DLL handling to the Rust test workflow, refines CMake platform conditionals from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to improve the cross-platform compatibility and user experience for the Rust API, particularly focusing on Windows. It refines the build system's platform-specific logic and provides extensive documentation to guide users through setting up the Rust development environment on various operating systems, thereby streamlining the process of integrating and utilizing the Rust API. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the Rust API support on Windows by adjusting CMake configurations to correctly handle Windows-specific linking requirements. It also updates the setup documentation in rust-api-examples/README.md and sherpa-onnx/rust/sherpa-onnx/src/lib.rs to provide clear guidance for users on Linux, macOS, and Windows. A minor update to the release script ensures version consistency across files.
| //! Example download URLs for `v1.12.31`: | ||
| //! | ||
| //! - Linux x86_64: | ||
| //! [sherpa-onnx-v1.12.31-linux-x64-shared.tar.bz2](https://github.com/k2-fsa/sherpa-onnx/releases/download/v1.12.31/sherpa-onnx-v1.12.31-linux-x64-shared.tar.bz2) | ||
| //! - Linux aarch64: | ||
| //! [sherpa-onnx-v1.12.31-linux-aarch64-shared-cpu.tar.bz2](https://github.com/k2-fsa/sherpa-onnx/releases/download/v1.12.31/sherpa-onnx-v1.12.31-linux-aarch64-shared-cpu.tar.bz2) | ||
| //! - macOS: | ||
| //! [sherpa-onnx-v1.12.31-osx-universal2-shared.tar.bz2](https://github.com/k2-fsa/sherpa-onnx/releases/download/v1.12.31/sherpa-onnx-v1.12.31-osx-universal2-shared.tar.bz2) | ||
| //! - Windows x64: | ||
| //! [sherpa-onnx-v1.12.31-win-x64-shared-MT-Release.tar.bz2](https://github.com/k2-fsa/sherpa-onnx/releases/download/v1.12.31/sherpa-onnx-v1.12.31-win-x64-shared-MT-Release.tar.bz2) |
There was a problem hiding this comment.
The example download URLs hardcode v1.12.31. While rust-api-examples/README.md mentions using the latest release, this lib.rs documentation does not. This could lead to outdated information and user confusion if not updated with each release. Consider adding a note here similar to the README.md or using a placeholder for the version.
There was a problem hiding this comment.
Pull request overview
Improve Windows support for the Rust API by updating docs, build/install scripts, and CI to ensure required shared/import libraries are discoverable and tests run on Windows.
Changes:
- Add Windows-focused setup guidance for the Rust crate and examples (library discovery at build/run time).
- Update CMake + packaging to include/import
onnxruntime.libon Windows and tighten rpath logic to UNIX platforms. - Extend Rust package CI to run on
windows-latestwith MSVC toolchain setup.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sherpa-onnx/rust/sherpa-onnx/src/lib.rs | Add crate-level setup documentation incl. Windows runtime DLL discovery guidance |
| sherpa-onnx/rust/sherpa-onnx-sys/build.rs | Clarify linking intent for sherpa-onnx C API |
| sherpa-onnx/rust/.gitignore | Ignore Cargo.lock under sherpa-onnx/rust/ |
| sherpa-onnx/python/csrc/CMakeLists.txt | Apply rpath only on UNIX platforms |
| sherpa-onnx/csrc/CMakeLists.txt | Apply rpath/flags only on UNIX platforms; adjust Python rpath condition |
| rust-api-examples/run-zipvoice-tts.sh | Use quieter tar xf extraction |
| rust-api-examples/README.md | Expand setup instructions; add Windows-specific guidance for examples |
| new-release.sh | Ensure version replacement also updates Rust crate docs |
| cmake/onnxruntime-win-x86.cmake | Install onnxruntime.lib into lib/ for Windows x86 |
| cmake/onnxruntime-win-x64.cmake | Install onnxruntime.lib into lib/ for Windows x64 |
| cmake/onnxruntime-win-arm64.cmake | Install onnxruntime.lib into lib/ for Windows arm64 |
| CMakeLists.txt | Apply RPATH-related configuration only on UNIX platforms |
| .github/workflows/windows-x86.yaml | Copy onnxruntime.lib into packaged artifacts when present |
| .github/workflows/windows-x64.yaml | Copy onnxruntime.lib; update release tag value |
| .github/workflows/windows-arm64.yaml | Copy onnxruntime.lib into packaged artifacts when present |
| .github/workflows/test-rust-package.yaml | Add Windows to matrix; configure MSVC; adjust PATH/RUSTFLAGS and run-step DLL handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [[ "${{ matrix.os }}" == "windows-latest" ]]; then | ||
| export PATH="$SHERPA_ONNX_LIB_DIR:$PATH" | ||
| mkdir -p rust-api-examples/target/debug/examples | ||
| cp -v "$SHERPA_ONNX_LIB_DIR"/*.dll rust-api-examples/target/debug/examples/ |
There was a problem hiding this comment.
cp -v "$SHERPA_ONNX_LIB_DIR"/*.dll ... will fail the step when no DLLs exist in $SHERPA_ONNX_LIB_DIR (very common if DLLs live under bin/ in the archive). Guard the copy (e.g., check for matches / enable nullglob) and/or copy from the directory that actually contains the DLLs (often $PWD/$d/bin) to avoid spurious CI failures.
| cp -v "$SHERPA_ONNX_LIB_DIR"/*.dll rust-api-examples/target/debug/examples/ | |
| if compgen -G "$SHERPA_ONNX_LIB_DIR"/*.dll > /dev/null; then | |
| cp -v "$SHERPA_ONNX_LIB_DIR"/*.dll rust-api-examples/target/debug/examples/ | |
| else | |
| echo "No DLLs found in $SHERPA_ONNX_LIB_DIR, skipping copy." | |
| fi |
| ``` | ||
|
|
||
| If Windows still reports that it cannot find a DLL, or if it loads a wrong | ||
| `onnxruntime.dll`, copy the DLL files from `%SHERPA_ONNX_LIB_DIR%` to the same |
There was a problem hiding this comment.
This section mixes Windows cmd.exe syntax (%SHERPA_ONNX_LIB_DIR%) with surrounding Git-Bash/MSYS-style export ... commands. To prevent users from copying the wrong syntax, either (a) change %SHERPA_ONNX_LIB_DIR% to $SHERPA_ONNX_LIB_DIR to match the shown shell, or (b) explicitly provide distinct snippets for PowerShell / cmd.exe vs Git Bash.
| `onnxruntime.dll`, copy the DLL files from `%SHERPA_ONNX_LIB_DIR%` to the same | |
| `onnxruntime.dll`, copy the DLL files from `$SHERPA_ONNX_LIB_DIR` to the same |
| //! | ||
| //! ```bash | ||
| //! export SHERPA_ONNX_LIB_DIR=/path/to/sherpa-onnx/lib | ||
| //! export PATH="$SHERPA_ONNX_LIB_DIR:$PATH" | ||
| //! ``` | ||
| //! |
There was a problem hiding this comment.
The Windows setup snippet uses export, which only applies to bash-like shells (Git Bash/MSYS/WSL), not PowerShell or cmd.exe. Consider clarifying the intended shell (e.g., “Git Bash”) and/or adding PowerShell equivalents (e.g., $env:SHERPA_ONNX_LIB_DIR=..., $env:Path="...;$env:Path"). This avoids confusion for Windows users following crate-level docs.
| //! | |
| //! ```bash | |
| //! export SHERPA_ONNX_LIB_DIR=/path/to/sherpa-onnx/lib | |
| //! export PATH="$SHERPA_ONNX_LIB_DIR:$PATH" | |
| //! ``` | |
| //! | |
| //! | |
| //! ### Git Bash / MSYS2 / WSL (bash) | |
| //! | |
| //! ```bash | |
| //! export SHERPA_ONNX_LIB_DIR=/path/to/sherpa-onnx/lib | |
| //! export PATH="$SHERPA_ONNX_LIB_DIR:$PATH" | |
| //! ``` | |
| //! | |
| //! ### PowerShell | |
| //! | |
| //! ```powershell | |
| //! $env:SHERPA_ONNX_LIB_DIR = "C:\path\to\sherpa-onnx\lib" | |
| //! $env:Path = "$env:SHERPA_ONNX_LIB_DIR;$env:Path" | |
| //! ``` | |
| //! |
| } | ||
|
|
||
| // Link the dynamic libraries regardless (cargo will fail later if not found) | ||
| // Always link against the public sherpa-onnx C API import library. |
There was a problem hiding this comment.
“import library” is Windows-specific terminology; on Linux/macOS this is a shared library (.so/.dylib). Since this build script runs cross-platform, consider rewording to “shared library” (or “link library”) to stay accurate on all targets.
| // Always link against the public sherpa-onnx C API import library. | |
| // Always link against the public sherpa-onnx C API shared library. |
|
|
||
| install(FILES ${onnxruntime_lib_files} DESTINATION lib) | ||
| install(FILES ${onnxruntime_lib_files} DESTINATION bin) | ||
| install(FILES "${onnxruntime_SOURCE_DIR}/lib/onnxruntime.lib" DESTINATION lib) |
There was a problem hiding this comment.
This install(FILES ...) will hard-fail at install/package time if ${onnxruntime_SOURCE_DIR}/lib/onnxruntime.lib doesn’t exist for a given onnxruntime layout/version. To make packaging resilient, guard it with an if(EXISTS ...) (and optionally emit a clear message(FATAL_ERROR ...) only when the build truly requires it). The same issue applies to the analogous x86/arm64 CMake files.
| install(FILES "${onnxruntime_SOURCE_DIR}/lib/onnxruntime.lib" DESTINATION lib) | |
| if(EXISTS "${onnxruntime_SOURCE_DIR}/lib/onnxruntime.lib") | |
| install(FILES "${onnxruntime_SOURCE_DIR}/lib/onnxruntime.lib" DESTINATION lib) | |
| else() | |
| message(WARNING "onnxruntime import library not found at '${onnxruntime_SOURCE_DIR}/lib/onnxruntime.lib'; it will not be installed.") | |
| endif() |
| repo_name: k2-fsa/sherpa-onnx | ||
| repo_token: ${{ secrets.UPLOAD_GH_SHERPA_ONNX_TOKEN }} | ||
| tag: v1.12.28 | ||
| tag: v1.12.31 |
There was a problem hiding this comment.
Hardcoding the release tag increases the risk of uploading artifacts to the wrong GitHub Release when versions change. Prefer using the tag from the workflow context (e.g., ${{ github.ref_name }}) so the workflow automatically targets the pushed tag.
| tag: v1.12.31 | |
| tag: ${{ github.ref_name }} |
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores