Further testing windows#367
Conversation
Remove gate that disables Windows NVBench build job in pr.yaml
Attempt to fix "LINK : fatal error LNK1561: entry point must be defined" when building benchmarks which need main function provided by static library libnvbench_main after NVIDIA#350
Act on review feedback regarding corner cases when testing may dependent on the directory it is performed from
4xx responses now fail immediately, and the installer is bounded to 15 minutes before being killed and reported as a timeout.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (16)
💤 Files with no reviewable changes (1)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughPR enables Windows CI for nvbench by adding validated CUDA/std/arch workflow inputs, introducing a CUDA profiler API installer script, handling Windows CUPTI runtime DLL resolution, adjusting MSVC compiler/linker settings, and wiring test runtime environments across multiple test suites. ChangesWindows Build, CUDA Profiler, and Test Runtime
Possibly related PRs
Suggested labels
Suggested reviewers
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/build-windows.yml (1)
122-122: ⚡ Quick winsuggestion: add a fail-fast validation step for
inputs.cuda,inputs.std, andinputs.archbefore pulling/running the container so malformed values fail immediately instead of after expensive setup.As per coding guidelines, for GitHub workflows you should focus on clear failures and avoiding unnecessary CI fanout/expense.
Also applies to: 135-136
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 54c447f2-d3e8-46bf-ae59-c3f64d071670
📒 Files selected for processing (11)
.github/workflows/build-windows.yml.github/workflows/pr.ymlCMakeLists.txtci/windows/install_cuda_profiler_api.ps1cmake/NVBenchCUPTI.cmakecmake/NVBenchConfigTarget.cmakenvbench/CMakeLists.txtnvbench/config.cuh.intesting/axes_metadata.cutesting/cmake/CMakeLists.txttesting/cmake/test_export/CMakeLists.txt
💤 Files with no reviewable changes (1)
- .github/workflows/pr.yml
BuildingUsing Visual Studio Community 2026 and CUDA Toolkit 13.2 on Windows 11, I used the following commands to build and test Setting of Running testsExecuting Tests pass, except the expected |
…v for tests to find CUPTI library
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5ef9adff-1e21-4204-a389-ea494b62f9a7
📒 Files selected for processing (17)
.github/workflows/build-windows.yml.github/workflows/pr.ymlCMakeLists.txtci/windows/build_common.psm1ci/windows/build_nvbench.ps1ci/windows/install_cuda_profiler_api.ps1cmake/NVBenchCUPTI.cmakecmake/NVBenchConfigTarget.cmakeexamples/CMakeLists.txtexec/CMakeLists.txtnvbench/CMakeLists.txtnvbench/config.cuh.intesting/CMakeLists.txttesting/axes_metadata.cutesting/cmake/CMakeLists.txttesting/cmake/test_export/CMakeLists.txttesting/device/CMakeLists.txt
💤 Files with no reviewable changes (1)
- .github/workflows/pr.yml
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ci/windows/build_nvbench.ps1 (1)
33-37: ⚡ Quick winsuggestion: Line 36 forces
-DNVBench_ENABLE_DEVICE_TESTING=ONfor every invocation. Make this configurable (for example, a script parameter with default OFF) so CPU-oriented CI jobs only enable device-test targets when explicitly requested. As per coding guidelines, forci/**/*: “focus on ... targeted build/test behavior, ... GPU availability assumptions, ... and avoiding unnecessary expensive jobs.”nvbench/CMakeLists.txt (1)
143-146: ⚡ Quick winsuggestion: use
LINKER:/INCLUDE:maininstead of raw/INCLUDE:mainfor proper CUDA link driver routingWhen nvcc drives the link step for CUDA-linked consumers, the raw option may not be correctly passed to the host linker, potentially causing the flag to be ignored or build failures. CMake's LINKER: prefix provides proper abstraction-layer handling across toolchains, ensuring this flag reaches the linker correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2ba6dc46-23fc-4217-8363-8149a60b269c
📒 Files selected for processing (17)
.github/workflows/build-windows.yml.github/workflows/pr.ymlCMakeLists.txtci/windows/build_common.psm1ci/windows/build_nvbench.ps1ci/windows/install_cuda_profiler_api.ps1cmake/NVBenchCUPTI.cmakecmake/NVBenchConfigTarget.cmakeexamples/CMakeLists.txtexec/CMakeLists.txtnvbench/CMakeLists.txtnvbench/config.cuh.intesting/CMakeLists.txttesting/axes_metadata.cutesting/cmake/CMakeLists.txttesting/cmake/test_export/CMakeLists.txttesting/device/CMakeLists.txt
💤 Files with no reviewable changes (1)
- .github/workflows/pr.yml
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/build-windows.yml (1)
152-153: ⚡ Quick winsuggestion: Deduplicate
WINDOWS_CI_IMAGEconstruction. The same expression is defined twice (Line 152 and Line 161), which can drift and makedocker pullanddocker runtarget different tags later. Compute it once (job env or a setup step output) and reuse it in both steps.As per coding guidelines, for
.github/**/*: “focus on ... status/check behavior ... and avoiding unnecessary CI fanout.”Also applies to: 161-162
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7533a7c2-7b0b-4e42-aeac-cfdeb8e003e6
📒 Files selected for processing (17)
.github/workflows/build-windows.yml.github/workflows/pr.ymlCMakeLists.txtci/windows/build_common.psm1ci/windows/build_nvbench.ps1ci/windows/install_cuda_profiler_api.ps1cmake/NVBenchCUPTI.cmakecmake/NVBenchConfigTarget.cmakeexamples/CMakeLists.txtexec/CMakeLists.txtnvbench/CMakeLists.txtnvbench/config.cuh.intesting/CMakeLists.txttesting/axes_metadata.cutesting/cmake/CMakeLists.txttesting/cmake/test_export/CMakeLists.txttesting/device/CMakeLists.txt
💤 Files with no reviewable changes (1)
- .github/workflows/pr.yml
ENVIRONMENT_MODIFICATION feature was added in 3.22.0 https://cmake.org/cmake/help/latest/prop_test/ENVIRONMENT_MODIFICATION.html
Check before executing cmake_path() in testing/cmake/CMakeLists.txt Also, use nvbench_get_imported_location to extract imported location
Remove the configure-time imported-location helper entirely.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/ok to test 9486121 |
|
The Code Rabbit is happy, local builds succeeded and tests passed on both Windows and Linux. I am squashing all these commits and pushing them to #354 for final review. Closing this PR now. |
This is a temporary draft PR to work out kinks in Windows building.