fix(onnxruntime): Fix Windows MSVC compilation errors#2841
Conversation
- Fix CMake syntax warning in src/CMakeLists.txt (message function quoting) - Replace Python-style 'and' with C++ '&&' in ct-transformer-online.cpp - Fix ssize_t redefinition conflict between kaldi and openfst on Windows - Update yaml-cpp CMake policies from OLD to NEW for modern CMake compatibility These changes enable successful compilation with MSVC on Windows (VS 2022) while maintaining compatibility with other platforms. Tested with: Visual Studio 2022, Windows 11, CMake 3.x
![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 includes several build and compatibility improvements, such as fixing CMake message syntax, standardizing logical operators in C++, and resolving ssize_t definition conflicts for MSVC. Feedback was provided to remove redundant commented-out code in kaldi-types.h and to update a stale comment in yaml-cpp/CMakeLists.txt regarding the minimum CMake version.
| // Don't define ssize_t as macro - let fst/types.h handle it | ||
| // #define ssize_t SSIZE_T // This causes conflicts with fst/types.h |
There was a problem hiding this comment.
Avoid leaving commented-out code in the codebase. Since the ssize_t macro is being removed to avoid conflicts and is now handled by fst/types.h, the dead code should be deleted entirely to maintain cleanliness. The explanatory comment is sufficient on its own.
| // Don't define ssize_t as macro - let fst/types.h handle it | |
| // #define ssize_t SSIZE_T // This causes conflicts with fst/types.h | |
| // Don't define ssize_t as macro - let fst/types.h handle it |
| ## Due to Mac OSX we need to keep compatibility with CMake 2.6 | ||
| # see http://www.cmake.org/Wiki/CMake_Policies | ||
| cmake_minimum_required(VERSION 2.6) | ||
| cmake_minimum_required(VERSION 2.8.12) |
There was a problem hiding this comment.
The minimum CMake version has been increased to 2.8.12, but the comment on line 4 still refers to compatibility with version 2.6. Please update the comment to ensure it remains consistent with the actual code requirements.
LauraGPT
left a comment
LauraGPT
left a comment
There was a problem hiding this comment.
The changes look reasonable for Windows MSVC compatibility (adding platform-specific includes, fixing type definitions, CMake adjustments). However, I cannot verify the build on a Windows MSVC environment at this time. Could you confirm that the full runtime builds and passes basic inference tests on Windows with these changes?
2 participants
Fix CMake syntax warning in src/CMakeLists.txt (message function quoting)
Replace Python-style 'and' with C++ '&&' in ct-transformer-online.cpp
Fix ssize_t redefinition conflict between kaldi and openfst on Windows
Update yaml-cpp CMake policies from OLD to NEW for modern CMake compatibility
These changes enable successful compilation with MSVC on Windows (VS 2022)
while maintaining compatibility with other platforms.
Tested with: Visual Studio 2022, Windows 11, CMake 3.x