Refactor CI for Windows x64 - #3119
Conversation
Summary of ChangesHello @csukuangfj, 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 refactors the continuous integration (CI) setup for Windows x64 by streamlining how ONNX Runtime static libraries are configured and downloaded. The changes aim to improve the maintainability and flexibility of the build system by centralizing configuration logic and ensuring consistent support across various build types, thereby reducing redundancy and potential for errors. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis pull request consolidates Windows x64 build infrastructure by removing a debug-specific workflow file, expanding the main Windows workflow to support multiple build types (Release, Debug, RelWithDebInfo, MinSizeRel), and refactoring CMake logic to handle per-build-type ONNX Runtime hash mappings and artifact management. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request refactors the CMake scripts for building on Windows x64, consolidating the logic for different build types into a single file and removing a redundant one. This simplifies the overall build process, reduces code duplication, and improves maintainability. The changes also enhance readability and make the scripts more robust on Windows by using appropriate environment variables. Overall, this is an excellent refactoring. I have one minor suggestion to further improve Windows compatibility.
| ${CMAKE_SOURCE_DIR}/onnxruntime-win-x64-static_lib-${onnxruntime_crt}-1.23.2.tar.bz2 | ||
| ${CMAKE_BINARY_DIR}/onnxruntime-win-x64-static_lib-${onnxruntime_crt}-1.23.2.tar.bz2 | ||
| /tmp/onnxruntime-win-x64-static_lib-${onnxruntime_crt}-1.23.2.tar.bz2 | ||
| $ENV{HOME}/Downloads/${onnxruntime_filename} |
There was a problem hiding this comment.
For better compatibility on Windows, it's recommended to use $ENV{USERPROFILE} instead of $ENV{HOME} to refer to the user's home directory. $ENV{HOME} is not a standard environment variable on Windows, whereas $ENV{USERPROFILE} is. Using the standard variable will make the script more robust across different Windows environments.
$ENV{USERPROFILE}/Downloads/${onnxruntime_filename}
Summary by CodeRabbit
New Features
Chores