feat: support to build on windows#383
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 5eac5a4 in 26 seconds. Click for details.
- Reviewed
1657lines of code in26files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_pJhzKOeid9d1REDO
You can customize 
|
Hey @peilinok |
There was a problem hiding this comment.
Pull request overview
Adds Windows (MSVC) build support for runanywhere-commons, including toolchain/CMake updates and Windows-safe replacements for POSIX-only code paths.
Changes:
- Adds Windows/MSVC compatibility for time formatting and weak-symbol result free functions.
- Replaces POSIX directory iteration with
std::filesystemwhere needed and adds Windows ONNX path handling. - Introduces Windows CMake + dependency fetching updates and a new
build-windows.bathelper script.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_json.cpp | Uses gmtime_s on MSVC for UTC time conversion |
| sdk/runanywhere-commons/src/features/vlm/vlm_component.cpp | Switches model file discovery to std::filesystem for cross-platform iteration |
| sdk/runanywhere-commons/src/features/tts/rac_tts_service.cpp | Avoids duplicate result-free symbol on Windows/MSVC |
| sdk/runanywhere-commons/src/features/stt/rac_stt_service.cpp | Avoids duplicate result-free symbol on Windows/MSVC |
| sdk/runanywhere-commons/src/features/result_free.cpp | Adds MSVC-safe (non-weak) definitions on Windows |
| sdk/runanywhere-commons/src/features/llm/rac_llm_service.cpp | Avoids duplicate result-free symbol on Windows |
| sdk/runanywhere-commons/src/features/embeddings/rac_embeddings_service.cpp | Avoids duplicate result-free symbol on Windows |
| sdk/runanywhere-commons/src/backends/onnx/wakeword_onnx.cpp | Adds Windows UTF-8 → wide path handling when constructing Ort::Session |
| sdk/runanywhere-commons/src/backends/onnx/rac_str_trans.h | Adds Windows string conversion utility API |
| sdk/runanywhere-commons/src/backends/onnx/rac_str_trans.cpp | Implements Windows string conversion utilities via Win32 APIs |
| sdk/runanywhere-commons/src/backends/onnx/rac_backend_onnx_register.cpp | Removes C99 designated initializers (MSVC compatibility) |
| sdk/runanywhere-commons/src/backends/onnx/onnx_backend.cpp | Uses std::filesystem on Windows for directory scans |
| sdk/runanywhere-commons/src/backends/onnx/CMakeLists.txt | Adds Windows Sherpa-ONNX import + builds rac_str_trans.cpp on Windows |
| sdk/runanywhere-commons/scripts/load-versions.sh | Prints Windows ONNX/Sherpa versions |
| sdk/runanywhere-commons/scripts/build-windows.bat | New Windows build + packaging script |
| sdk/runanywhere-commons/include/rac/features/vad/rac_vad_types.h | Replaces designated initializers with positional initializers for MSVC |
| sdk/runanywhere-commons/include/rac/features/tts/rac_tts_types.h | Replaces designated initializers with positional initializers for MSVC |
| sdk/runanywhere-commons/include/rac/features/stt/rac_stt_types.h | Replaces designated initializers with positional initializers for MSVC |
| sdk/runanywhere-commons/include/rac/backends/rac_vad_onnx.h | Replaces designated initializers with positional initializers for MSVC |
| sdk/runanywhere-commons/include/rac/backends/rac_tts_onnx.h | Replaces designated initializers with positional initializers for MSVC |
| sdk/runanywhere-commons/include/rac/backends/rac_stt_onnx.h | Replaces designated initializers with positional initializers for MSVC |
| sdk/runanywhere-commons/cmake/LoadVersions.cmake | Logs Windows ONNX/Sherpa versions |
| sdk/runanywhere-commons/cmake/FetchONNXRuntime.cmake | Adds Windows ONNX Runtime download/import logic |
| sdk/runanywhere-commons/VERSIONS | Adds Windows ONNX Runtime + Sherpa-ONNX version pins |
| sdk/runanywhere-commons/CMakeLists.txt | Adds Windows platform handling and MSVC compile/link flag setup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int len = MultiByteToWideChar(CP_ACP, 0, str.c_str(), -1, NULL, 0); | ||
| wchar_t* buffer = new wchar_t[len]; | ||
| MultiByteToWideChar(CP_ACP, 0, str.c_str(), -1, buffer, len); | ||
| std::wstring wstr(buffer); | ||
| delete[] buffer; |
There was a problem hiding this comment.
MultiByteToWideChar/WideCharToMultiByte can return 0 on failure. In that case new wchar_t[len] (len==0) plus constructing std::wstring(buffer) is undefined behavior (reads past a zero-sized buffer). Fix by checking len > 0 (and that the second conversion succeeded) before allocating/constructing; otherwise return an empty string (or propagate an error). Apply the same pattern to all conversions in this file (both MultiByteToWideChar and WideCharToMultiByte calls).
| add_compile_options(/W4 /WX- /wd4996 /wd4576 /FS) # W4 = level 4 warnings, WX- = warnings as errors off, 4996 = deprecated function warnings, 4576 = non-standard cast syntax, FS = force synchronous PDB writes | ||
| # Per-config flags: use generator expressions so multi-config generators (e.g. Visual Studio) get correct flags per config. /O2 and /RTC1 are incompatible. | ||
| add_compile_options("$<$<CONFIG:Release>:/O2 /DNDEBUG /GL /MP>") # O2 = optimize for speed, GL = whole program optimization, MP = multi-processor | ||
| add_compile_options("$<$<CONFIG:RelWithDebInfo>:/O2 /DNDEBUG /GL /MP>") | ||
| add_compile_options("$<$<CONFIG:MinSizeRel>:/O1 /DNDEBUG /GL /MP>") | ||
| add_compile_options("$<$<CONFIG:Debug>:/Od /Zi>") # Od = disable optimizations, Zi = debug information (no /RTC1 here; let CMake default or use /RTC1 only if desired) |
There was a problem hiding this comment.
The generator-expression compile options contain multiple flags inside a single quoted argument (e.g. "/O2 /DNDEBUG /GL /MP"). CMake will treat that as one option (with spaces), which MSVC won’t parse as separate flags. Also, the set(CMAKE_CXX_FLAGS "... \ ...") uses backslashes that will be passed literally into the compiler command line (not as line continuations), producing invalid flags. Split each compiler flag into its own list element (including inside generator expressions), and remove the literal backslashes by using a proper CMake list (or multiple add_compile_options(...) entries without embedded \).
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} \ | ||
| /Zi \ | ||
| /arch:SSE2") |
There was a problem hiding this comment.
The generator-expression compile options contain multiple flags inside a single quoted argument (e.g. "/O2 /DNDEBUG /GL /MP"). CMake will treat that as one option (with spaces), which MSVC won’t parse as separate flags. Also, the set(CMAKE_CXX_FLAGS "... \ ...") uses backslashes that will be passed literally into the compiler command line (not as line continuations), producing invalid flags. Split each compiler flag into its own list element (including inside generator expressions), and remove the literal backslashes by using a proper CMake list (or multiple add_compile_options(...) entries without embedded \).
| set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} /MTd") | ||
| set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} /MT") |
There was a problem hiding this comment.
This mixes conflicting CRT choices: /MT[d] forces static CRT, while MSVC_RUNTIME_LIBRARY "MultiThreadedDLL" selects /MD (dynamic CRT). Additionally, checking CMAKE_BUILD_TYPE is unreliable for multi-config generators (Visual Studio typically leaves it empty), which would incorrectly set the Debug CRT even for Release builds. Pick one mechanism (prefer MSVC_RUNTIME_LIBRARY) and set it per-config using generator expressions (or separate config properties), and remove the manual /MT mutations to CMAKE_CXX_FLAGS_*.
| set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} /MTd") | |
| set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} /MT") | |
| # Configure MSVC runtime library per configuration using CMake's built-in mechanism (static CRT, Debug suffix in Debug builds) | |
| set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>") |
| # Set runtime library (use /MD for Release, /MDd for Debug) | ||
| if(CMAKE_BUILD_TYPE STREQUAL "Release") | ||
| set_property(TARGET rac_commons PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreadedDLL") | ||
| # Enable link-time code generation for Release builds | ||
| set_target_properties(rac_commons PROPERTIES | ||
| INTERPROCEDURAL_OPTIMIZATION ON | ||
| ) | ||
| else() | ||
| set_property(TARGET rac_commons PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreadedDebugDLL") | ||
| endif() |
There was a problem hiding this comment.
This mixes conflicting CRT choices: /MT[d] forces static CRT, while MSVC_RUNTIME_LIBRARY "MultiThreadedDLL" selects /MD (dynamic CRT). Additionally, checking CMAKE_BUILD_TYPE is unreliable for multi-config generators (Visual Studio typically leaves it empty), which would incorrectly set the Debug CRT even for Release builds. Pick one mechanism (prefer MSVC_RUNTIME_LIBRARY) and set it per-config using generator expressions (or separate config properties), and remove the manual /MT mutations to CMAKE_CXX_FLAGS_*.
| # Set runtime library (use /MD for Release, /MDd for Debug) | |
| if(CMAKE_BUILD_TYPE STREQUAL "Release") | |
| set_property(TARGET rac_commons PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreadedDLL") | |
| # Enable link-time code generation for Release builds | |
| set_target_properties(rac_commons PROPERTIES | |
| INTERPROCEDURAL_OPTIMIZATION ON | |
| ) | |
| else() | |
| set_property(TARGET rac_commons PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreadedDebugDLL") | |
| endif() | |
| # Set runtime library: use /MD (MultiThreadedDLL) for non-Debug, /MDd (MultiThreadedDebugDLL) for Debug | |
| set_property(TARGET rac_commons PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>DLL") | |
| # Enable link-time code generation (IPO) for Release builds only | |
| set_target_properties(rac_commons PROPERTIES | |
| INTERPROCEDURAL_OPTIMIZATION "$<$<CONFIG:Release>:ON>" | |
| ) |
|
|
||
| :: Pass -G and -A only when user specified them; quote generator so names with spaces work | ||
| set "GEN_ARCH_ARGS=" | ||
| if not "%CMAKE_GENERATOR%"=="" set "GEN_ARCH_ARGS=-G "%CMAKE_GENERATOR%"" |
There was a problem hiding this comment.
The set line that tries to embed quotes is syntactically broken in cmd.exe (set "GEN_ARCH_ARGS=-G "%CMAKE_GENERATOR%"" will terminate the assignment early). Also, placing %BUILD_EXTRA% after -j risks --verbose being interpreted as the optional job count argument to -j. Build the generator args using safe quoting (e.g., keep the outer set "VAR=..." but escape/structure the inner quotes correctly), and pass verbosity separately from parallelism (e.g., cmake --build ... %BUILD_EXTRA% --parallel).
| if not "%CMAKE_GENERATOR%"=="" set "GEN_ARCH_ARGS=-G "%CMAKE_GENERATOR%"" | |
| if not "%CMAKE_GENERATOR%"=="" set "GEN_ARCH_ARGS=-G ^"%CMAKE_GENERATOR%^"" |
| echo [STEP] Building... | ||
| set "BUILD_EXTRA=" | ||
| if "%VERBOSE%"=="1" set "BUILD_EXTRA=--verbose" | ||
| cmake --build . --config %BUILD_TYPE% -j %BUILD_EXTRA% |
There was a problem hiding this comment.
The set line that tries to embed quotes is syntactically broken in cmd.exe (set "GEN_ARCH_ARGS=-G "%CMAKE_GENERATOR%"" will terminate the assignment early). Also, placing %BUILD_EXTRA% after -j risks --verbose being interpreted as the optional job count argument to -j. Build the generator args using safe quoting (e.g., keep the outer set "VAR=..." but escape/structure the inner quotes correctly), and pass verbosity separately from parallelism (e.g., cmake --build ... %BUILD_EXTRA% --parallel).
| free(service); | ||
| } | ||
|
|
||
| #if !defined(_WIN32) || !defined(_MSC_VER) |
There was a problem hiding this comment.
The Windows/MSVC exclusion guard here differs from the LLM/embeddings versions (which use #if !defined(_WIN32)). As written, MinGW/clang (where _WIN32 is defined but _MSC_VER is not) will compile this definition while result_free.cpp also provides a (weak) definition, increasing the chance of duplicate-symbol/linker behavior differences across toolchains. Consider using a consistent rule across all rac_*_result_free implementations (e.g., exclude on all Windows, or exclude only on MSVC, but do it uniformly in STT/TTS/LLM/embeddings).
| #if !defined(_WIN32) || !defined(_MSC_VER) | |
| #if !defined(_WIN32) |
| # Set C++ standard based on platform | ||
| if(WIN32) | ||
| target_compile_features(rac_commons PUBLIC cxx_std_20) | ||
| else() | ||
| target_compile_features(rac_commons PUBLIC cxx_std_17) | ||
| endif() |
There was a problem hiding this comment.
Requiring C++20 only on Windows is a public-facing build requirement change for consumers linking against rac_commons (it propagates via PUBLIC). Since the Windows changes shown (e.g., std::filesystem) only require C++17, it would be safer to keep a consistent minimum (C++17) across platforms unless there’s a concrete C++20-only dependency. If you do need C++20 on Windows, consider documenting why and avoiding propagating it publicly unless necessary.
| # Set C++ standard based on platform | |
| if(WIN32) | |
| target_compile_features(rac_commons PUBLIC cxx_std_20) | |
| else() | |
| target_compile_features(rac_commons PUBLIC cxx_std_17) | |
| endif() | |
| # Set C++ standard (minimum) across platforms | |
| target_compile_features(rac_commons PUBLIC cxx_std_17) |
| * | ||
| * String transformation utilities for Windows. | ||
| * This class is used to convert strings between ASCII, UTF-8, and Unicode. | ||
| * This class need to move to runanywhere-core in the future as a common utility class. |
There was a problem hiding this comment.
Grammar: change 'need' to 'needs' so the sentence reads correctly.
| * This class need to move to runanywhere-core in the future as a common utility class. | |
| * This class needs to move to runanywhere-core in the future as a common utility class. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} /MTd") | ||
| set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} /MT") |
There was a problem hiding this comment.
Contradictory MSVC runtime library settings
Lines 159-160 set /MT (static CRT) and /MTd (static debug CRT) via CMAKE_CXX_FLAGS, but later at lines 362-370, the code sets MSVC_RUNTIME_LIBRARY to "MultiThreadedDLL" (/MD) and "MultiThreadedDebugDLL" (/MDd). These are contradictory: /MT links the CRT statically, while /MD links it dynamically.
When both flags end up on the same compiler command line, MSVC will emit warning D9025 (overriding /MT with /MD) and depending on the order, the final setting is unpredictable. Worse, if different translation units or linked libraries use different CRT settings, you'll get linker errors (LNK2038: mismatch detected for 'RuntimeLibrary').
Pick one strategy and apply it consistently. If the intent is dynamic CRT (/MD), remove the /MT//MTd flags here. If static CRT is intended, update the MSVC_RUNTIME_LIBRARY property below to match.
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/runanywhere-commons/CMakeLists.txt
Line: 159-160
Comment:
**Contradictory MSVC runtime library settings**
Lines 159-160 set `/MT` (static CRT) and `/MTd` (static debug CRT) via `CMAKE_CXX_FLAGS`, but later at lines 362-370, the code sets `MSVC_RUNTIME_LIBRARY` to `"MultiThreadedDLL"` (`/MD`) and `"MultiThreadedDebugDLL"` (`/MDd`). These are contradictory: `/MT` links the CRT statically, while `/MD` links it dynamically.
When both flags end up on the same compiler command line, MSVC will emit warning D9025 (overriding `/MT` with `/MD`) and depending on the order, the final setting is unpredictable. Worse, if different translation units or linked libraries use different CRT settings, you'll get linker errors (LNK2038: mismatch detected for 'RuntimeLibrary').
Pick one strategy and apply it consistently. If the intent is dynamic CRT (`/MD`), remove the `/MT`/`/MTd` flags here. If static CRT is intended, update the `MSVC_RUNTIME_LIBRARY` property below to match.
How can I resolve this? If you propose a fix, please make it concise.| #if defined(_WIN32) | ||
| try { | ||
| for (const auto& entry : std::filesystem::directory_iterator(model_path)) { | ||
| std::string filename = entry.path().filename().string(); | ||
| std::string full_path = entry.path().string(); | ||
|
|
||
| if (filename.find("encoder") != std::string::npos && filename.size() > 5 && | ||
| filename.substr(filename.size() - 5) == ".onnx") { | ||
| encoder_path = full_path; | ||
| RAC_LOG_DEBUG("ONNX.STT", "Found encoder: %s", encoder_path.c_str()); | ||
| } else if (filename.find("decoder") != std::string::npos && filename.size() > 5 && | ||
| filename.substr(filename.size() - 5) == ".onnx") { | ||
| decoder_path = full_path; | ||
| RAC_LOG_DEBUG("ONNX.STT", "Found decoder: %s", decoder_path.c_str()); | ||
| } else if (filename == "tokens.txt" || (filename.find("tokens") != std::string::npos && | ||
| filename.find(".txt") != std::string::npos)) { | ||
| tokens_path = full_path; | ||
| RAC_LOG_DEBUG("ONNX.STT", "Found tokens: %s", tokens_path.c_str()); | ||
| } | ||
| } | ||
| } catch (const std::filesystem::filesystem_error&) { | ||
| RAC_LOG_ERROR("ONNX.STT", "Cannot open model directory: %s", model_path.c_str()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Windows STT path misses NeMo CTC model detection
The Windows #if defined(_WIN32) directory-scanning code only looks for encoder/decoder/tokens files, but it does NOT detect NeMo CTC single-file models (model.int8.onnx or model.onnx). Compare with the non-Windows path at lines 219-226, which explicitly populates nemo_ctc_model_path.
This means on Windows, if a user provides a directory containing a NeMo CTC model (a single model.onnx or model.int8.onnx without separate encoder/decoder), the nemo_ctc_model_path variable will remain empty. The downstream fallback logic that checks nemo_ctc_model_path as an alternative when encoder_path is empty will not work, potentially causing model loading to fail.
Add the same NeMo CTC single-file model detection logic to the Windows path:
} else if ((filename == "model.int8.onnx" || filename == "model.onnx") &&
encoder_path.empty()) {
if (filename == "model.int8.onnx" || nemo_ctc_model_path.empty()) {
nemo_ctc_model_path = full_path;
RAC_LOG_DEBUG("ONNX.STT", "Found single-file model: %s", nemo_ctc_model_path.c_str());
}
}Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/runanywhere-commons/src/backends/onnx/onnx_backend.cpp
Line: 171-194
Comment:
**Windows STT path misses NeMo CTC model detection**
The Windows `#if defined(_WIN32)` directory-scanning code only looks for encoder/decoder/tokens files, but it does NOT detect NeMo CTC single-file models (`model.int8.onnx` or `model.onnx`). Compare with the non-Windows path at lines 219-226, which explicitly populates `nemo_ctc_model_path`.
This means on Windows, if a user provides a directory containing a NeMo CTC model (a single `model.onnx` or `model.int8.onnx` without separate encoder/decoder), the `nemo_ctc_model_path` variable will remain empty. The downstream fallback logic that checks `nemo_ctc_model_path` as an alternative when `encoder_path` is empty will not work, potentially causing model loading to fail.
Add the same NeMo CTC single-file model detection logic to the Windows path:
```cpp
} else if ((filename == "model.int8.onnx" || filename == "model.onnx") &&
encoder_path.empty()) {
if (filename == "model.int8.onnx" || nemo_ctc_model_path.empty()) {
nemo_ctc_model_path = full_path;
RAC_LOG_DEBUG("ONNX.STT", "Found single-file model: %s", nemo_ctc_model_path.c_str());
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| #if defined(_WIN32) | ||
| try { | ||
| for (const auto& entry : std::filesystem::directory_iterator(model_path)) { | ||
| std::string filename = entry.path().filename().string(); | ||
| if (filename.size() > 5 && filename.substr(filename.size() - 5) == ".onnx") { | ||
| model_onnx_path = entry.path().string(); | ||
| RAC_LOG_DEBUG("ONNX.TTS", "Found model file: %s", model_onnx_path.c_str()); | ||
| break; | ||
| } | ||
| } | ||
| } catch (const std::filesystem::filesystem_error&) { | ||
| /* ignore */ | ||
| } |
There was a problem hiding this comment.
Windows TTS path skips int8 model preference logic
The Windows #if defined(_WIN32) path for TTS model discovery just picks the first .onnx file found in the directory. The non-Windows path (lines 733-752) first explicitly checks for model.int8.onnx (the preferred int8 quantized Kokoro model) before falling back to scanning.
On Windows, if a directory contains both model.onnx and model.int8.onnx, the iteration order from std::filesystem::directory_iterator is unspecified, so the code might load the full-size model instead of the preferred int8 model. Consider mirroring the non-Windows logic: check model.int8.onnx first via stat(), then fall back to directory scanning.
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/runanywhere-commons/src/backends/onnx/onnx_backend.cpp
Line: 719-731
Comment:
**Windows TTS path skips int8 model preference logic**
The Windows `#if defined(_WIN32)` path for TTS model discovery just picks the first `.onnx` file found in the directory. The non-Windows path (lines 733-752) first explicitly checks for `model.int8.onnx` (the preferred int8 quantized Kokoro model) before falling back to scanning.
On Windows, if a directory contains both `model.onnx` and `model.int8.onnx`, the iteration order from `std::filesystem::directory_iterator` is unspecified, so the code might load the full-size model instead of the preferred int8 model. Consider mirroring the non-Windows logic: check `model.int8.onnx` first via `stat()`, then fall back to directory scanning.
How can I resolve this? If you propose a fix, please make it concise.| free(service); | ||
| } | ||
|
|
||
| #if !defined(_WIN32) |
There was a problem hiding this comment.
Inconsistent preprocessor guard vs other files
This file uses #if !defined(_WIN32) to guard the weak symbol definition, while rac_stt_service.cpp and rac_tts_service.cpp use the more precise #if !defined(_WIN32) || !defined(_MSC_VER). The rac_embeddings_service.cpp also uses the simpler #if !defined(_WIN32).
The inconsistency means on Windows with a non-MSVC compiler (e.g., MinGW/Clang), the !defined(_WIN32) guard will exclude the definition even though __attribute__((weak)) is supported by those compilers. Consider aligning all four files to use the same guard: #if !defined(_WIN32) || !defined(_MSC_VER).
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/runanywhere-commons/src/features/llm/rac_llm_service.cpp
Line: 211
Comment:
**Inconsistent preprocessor guard vs other files**
This file uses `#if !defined(_WIN32)` to guard the weak symbol definition, while `rac_stt_service.cpp` and `rac_tts_service.cpp` use the more precise `#if !defined(_WIN32) || !defined(_MSC_VER)`. The `rac_embeddings_service.cpp` also uses the simpler `#if !defined(_WIN32)`.
The inconsistency means on Windows with a non-MSVC compiler (e.g., MinGW/Clang), the `!defined(_WIN32)` guard will exclude the definition even though `__attribute__((weak))` is supported by those compilers. Consider aligning all four files to use the same guard: `#if !defined(_WIN32) || !defined(_MSC_VER)`.
How can I resolve this? If you propose a fix, please make it concise.| return std::string(); | ||
| } | ||
|
|
||
| int len = WideCharToMultiByte(CP_ACP, 0, wstr.c_str(), -1, NULL, 0, NULL, NULL); | ||
| char* buffer = new char[len]; | ||
| WideCharToMultiByte(CP_ACP, 0, wstr.c_str(), -1, buffer, len, NULL, NULL); | ||
| std::string str(buffer); | ||
| delete[] buffer; | ||
| return str; |
There was a problem hiding this comment.
Memory leak if MultiByteToWideChar fails
If the first MultiByteToWideChar call returns 0 (error, e.g. invalid code page sequence), len will be 0, and new wchar_t[0] allocates a zero-sized array. The second MultiByteToWideChar call will then fail with buffer size 0 but the allocation will still proceed. More critically, the same pattern repeats in every function in this file (ascii_to_utf8, utf8_to_ascii, etc.) — none check the return value of the Win32 conversion calls.
Consider checking whether len is 0 (indicating an error) before allocating and proceeding, or use std::vector/std::wstring with proper sizing to avoid manual new/delete and potential leaks on early returns.
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/runanywhere-commons/src/backends/onnx/rac_str_trans.cpp
Line: 33-41
Comment:
**Memory leak if `MultiByteToWideChar` fails**
If the first `MultiByteToWideChar` call returns 0 (error, e.g. invalid code page sequence), `len` will be 0, and `new wchar_t[0]` allocates a zero-sized array. The second `MultiByteToWideChar` call will then fail with buffer size 0 but the allocation will still proceed. More critically, the same pattern repeats in every function in this file (`ascii_to_utf8`, `utf8_to_ascii`, etc.) — none check the return value of the Win32 conversion calls.
Consider checking whether `len` is 0 (indicating an error) before allocating and proceeding, or use `std::vector`/`std::wstring` with proper sizing to avoid manual `new`/`delete` and potential leaks on early returns.
How can I resolve this? If you propose a fix, please make it concise.| #include <cstdlib> | ||
| #include <cstring> | ||
| #include <dirent.h> | ||
| #include <filesystem> |
There was a problem hiding this comment.
Unconditional <filesystem> include breaks older non-Windows toolchains
The #include <dirent.h> was replaced with #include <filesystem> unconditionally (not gated by #if defined(_WIN32)). While the rest of the codebase uses #if defined(_WIN32) / #else guards to choose between <filesystem> and <dirent.h>, this file now always includes <filesystem>. This works because vlm_component.cpp uses std::filesystem for all platforms, but it requires C++17 <filesystem> support on all platforms.
The main CMakeLists.txt only sets cxx_std_20 for Windows and cxx_std_17 for other platforms. Most modern compilers (GCC 8+, Clang 7+) do support <filesystem> with C++17, but some older toolchains or embedded systems used for Android NDK may not have full <filesystem> support. This could cause build failures on Android or older Linux toolchains.
Verify that all target toolchains support <filesystem>, or guard this with platform-specific #ifdef as done in onnx_backend.cpp.
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/runanywhere-commons/src/features/vlm/vlm_component.cpp
Line: 12
Comment:
**Unconditional `<filesystem>` include breaks older non-Windows toolchains**
The `#include <dirent.h>` was replaced with `#include <filesystem>` unconditionally (not gated by `#if defined(_WIN32)`). While the rest of the codebase uses `#if defined(_WIN32)` / `#else` guards to choose between `<filesystem>` and `<dirent.h>`, this file now always includes `<filesystem>`. This works because `vlm_component.cpp` uses `std::filesystem` for all platforms, but it requires C++17 `<filesystem>` support on all platforms.
The main `CMakeLists.txt` only sets `cxx_std_20` for Windows and `cxx_std_17` for other platforms. Most modern compilers (GCC 8+, Clang 7+) do support `<filesystem>` with C++17, but some older toolchains or embedded systems used for Android NDK may not have full `<filesystem>` support. This could cause build failures on Android or older Linux toolchains.
Verify that all target toolchains support `<filesystem>`, or guard this with platform-specific `#ifdef` as done in `onnx_backend.cpp`.
How can I resolve this? If you propose a fix, please make it concise.
|
Hi @Siddhesh2377 , |
|
ok @peilinok so i am keeping this PR open, take your time and then submit the changes 😊 |
|
Hey @peilinok I am Converting this to a Draft Please Fix the Issues soon 😊 |
Hi @Siddhesh2377 , I'll do my best to finalize the Windows build scripts during the Chinese New Year holiday. I'll keep you posted on my progress. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Sure thing 😊👍🏻 |
peilinok
force-pushed
the
sylar/win
branch
2 times, most recently
from
f1327c6 to
8115780
Compare
shubhammalhotra28
force-pushed
the
main
branch
from
3f2c8d2 to
eaf9e08
Compare
|
Hey @peilinok 👋 — We had a security incident where credentials were Your work is completely unaffected — you just need to update your fork git remote add upstream https://github.com/RunanywhereAI/runanywhere-sdks.git Sorry for the inconvenience, and thank you for the Windows build support |
Thanks for the heads-up and the clear steps! I’ll rebase my branch on the updated main and push my commits shortly. |
peilinok
force-pushed
the
sylar/win
branch
2 times, most recently
from
70fd3c3 to
d014847
Compare
|
Hey @shubhammalhotra28 , 
What would you suggest we do to resolve these errors? Should we upgrade to a newer C++ version, or replace all the designated initializers? |
3 participants
Description
Brief description of the changes made.
Type of Change
Testing
Platform-Specific Testing (check all that apply)
Swift SDK / iOS Sample:
Kotlin SDK / Android Sample:
Flutter SDK / Flutter Sample:
React Native SDK / React Native Sample:
Web SDK / Web Sample:
Labels
Please add the appropriate label(s):
SDKs:
Swift SDK- Changes to Swift SDK (sdk/runanywhere-swift)Kotlin SDK- Changes to Kotlin SDK (sdk/runanywhere-kotlin)Flutter SDK- Changes to Flutter SDK (sdk/runanywhere-flutter)React Native SDK- Changes to React Native SDK (sdk/runanywhere-react-native)Web SDK- Changes to Web SDK (sdk/runanywhere-web)Commons- Changes to shared native code (sdk/runanywhere-commons)Sample Apps:
iOS Sample- Changes to iOS example app (examples/ios)Android Sample- Changes to Android example app (examples/android)Flutter Sample- Changes to Flutter example app (examples/flutter)React Native Sample- Changes to React Native example app (examples/react-native)Web Sample- Changes to Web example app (examples/web)Checklist
Screenshots
Attach relevant UI screenshots for changes (if applicable):
Important
Add support for building the project on Windows with Windows-specific configurations and handling across build scripts and source files.
CMakeLists.txtforrunanywhere-commons.build-windows.batscript for building on Windows with options for backends and build types.FetchONNXRuntime.cmaketo handle Windows-specific ONNX Runtime versions.rac_str_trans.cppandrac_str_trans.hfor string transformations on Windows.onnx_backend.cppandwakeword_onnx.cppto usestd::filesystemfor directory operations on Windows.rac_backend_onnx_register.cppandresult_free.cppfor handling weak symbols.VERSIONSfile to include Windows-specific version entries for ONNX and Sherpa-ONNX.load-versions.shto export Windows-specific version variables.telemetry_json.cppfor Windows-specific time handling.vlm_component.cppto usestd::filesystemfor cross-platform compatibility.This description was created by
for 5eac5a4. You can customize this summary. It will automatically update as commits are pushed.
Greptile Summary
This PR adds Windows (MSVC) build support to the
runanywhere-commonsnative C/C++ library across 26 files. The changes include CMake platform detection and MSVC compiler flags, ONNX Runtime and Sherpa-ONNX fetching for Windows, replacing C99 designated initializers with positional initializers for MSVC compatibility, replacing POSIXdirent.hwithstd::filesystemfor directory iteration, handling MSVC's lack of weak symbol support, adding a Windows build script (build-windows.bat), and a new string conversion utility (rac_str_trans) for UTF-8/wide-char interop required by ONNX Runtime's Windows API./MT(static CRT) while the target-level properties setMSVC_RUNTIME_LIBRARYto"MultiThreadedDLL"(/MD). This contradiction will cause linker errors or unpredictable behavior.onnx_backend.cppdoes not detect single-file NeMo CTC models (model.onnx/model.int8.onnx), while the non-Windows path does..onnxfile found rather than preferringmodel.int8.onnxas the non-Windows code does.rac_llm_service.cppandrac_embeddings_service.cppuse#if !defined(_WIN32)whilerac_stt_service.cppandrac_tts_service.cppuse the more correct#if !defined(_WIN32) || !defined(_MSC_VER).<filesystem>in vlm_component.cpp: Replaced<dirent.h>with<filesystem>for all platforms, which may break on older Android NDK toolchains.Confidence Score: 2/5
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[CMakeLists.txt] -->|Platform Detection| B{WIN32?} B -->|Yes| C[Set RAC_PLATFORM_WINDOWS] B -->|No| D[Existing: macOS/Linux/Android/iOS] C --> E[MSVC Compiler Flags] E --> F["/MT static CRT ⚠️ Conflicts with /MD below"] C --> G[Target Properties] G --> H["MSVC_RUNTIME_LIBRARY = MultiThreadedDLL (/MD)"] C --> I[FetchONNXRuntime.cmake] I --> J{Architecture?} J -->|x64| K[Download win-x64 ONNX Runtime] J -->|ARM64| L[Download win-arm64 ONNX Runtime] C --> M[Sherpa-ONNX Windows] M --> N[third_party/sherpa-onnx-windows] C --> O[rac_str_trans.cpp] O --> P[UTF-8 ↔ wchar_t conversion] P --> Q[wakeword_onnx.cpp: create_onnx_session] Q --> R[Ort::Session with wide path] C --> S[Weak Symbol Workaround] S --> T[result_free.cpp: Remove __attribute__ weak] S --> U[Service files: #if guards]Last reviewed commit: 5eac5a4
(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!
Context used:
dashboard- CLAUDE.md (source)