Conversation
…ast (though, chocolatey looks better ever second...)
…y to force add to Windows Path
Refactor OpenCL installation steps for Windows and remove verification step.
Updated the R-CMD-check workflow to include Intel OpenCL installation steps for Windows and removed macOS and Ubuntu configurations.
Updated OpenCL environment variables and added debugging step.
Update path format for vcpkg installation on Windows.
Added environment variable registrations for CLBlast.
There was a problem hiding this comment.
Pull request overview
This PR adds Windows CI build support to RcppBandicoot by implementing a Windows-specific configuration system that detects and configures OpenCL and CLBlast dependencies via environment variables.
- Introduces
configure.winshell script for Windows build configuration - Adds templated
Makevars.win.inthat gets processed by configure.win - Updates GitHub Actions workflow to install OpenCL SDK and Intel CPU runtime on Windows
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| configure.win | New Windows configuration script that detects OpenCL/CLBlast from environment variables and generates Makevars.win and R/flags.R |
| src/Makevars.win.in | Template file for Windows-specific compiler and linker flags, processed by configure.win |
| src/Makevars.win | Removed static version in favor of generated file from template |
| src/.gitignore | Added Makevars and Makevars.win to ignore list since they are now generated files |
| .github/workflows/R-CMD-check.yaml | Added Windows build steps: vcpkg installation of OpenCL/CLBlast, Intel OpenCL runtime installation, and registry configuration |
| .gitattributes | Ensures shell scripts and configure files use LF line endings for cross-platform compatibility |
| .Rbuildignore | Excludes install.log and .gitattributes from R package builds |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| REG ADD "HKLM\SOFTWARE\Khronos\OpenCL\Vendors" /v $dllPath /t REG_DWORD /d 0 /f | ||
| REG ADD "HKLM\SOFTWARE\Wow6432Node\Khronos\OpenCL\Vendors" /v $dllPath /t REG_DWORD /d 0 /f |
There was a problem hiding this comment.
Potential security risk: Registry modifications using REG ADD require elevated privileges and should verify success. These commands modify HKLM (HKEY_LOCAL_MACHINE) which typically requires administrator rights. If the commands fail silently, OpenCL runtime won't be properly registered. Add error checking after each REG ADD command to ensure they succeeded, or wrap them in a try-catch block with appropriate error handling.
| OPENCL_TARGET_VERSION=300 | ||
|
|
||
| ## Kernel source directory (matches configure.ac logic) | ||
| BANDICOOT_KERNELS_DIR=$("${R_HOME}/bin/Rscript" -e 'cat(paste(head(.libPaths(),1), "RcppBandicoot", "include", "bandicoot_bits", "ks", "", sep="/"))') |
There was a problem hiding this comment.
Multiple issues with BANDICOOT_KERNELS_DIR path handling: (1) The path uses forward slashes from R (sep="/") but is being used on Windows where backslashes are native; (2) No validation that the Rscript command succeeded or returned a valid path; (3) The path may contain spaces or special characters that will break sed substitution on line 109. Consider: validating the path exists, using normalizePath() in R for proper Windows path handling, and escaping special sed characters before substitution.
| BANDICOOT_KERNELS_DIR=$("${R_HOME}/bin/Rscript" -e 'cat(paste(head(.libPaths(),1), "RcppBandicoot", "include", "bandicoot_bits", "ks", "", sep="/"))') | |
| BANDICOOT_KERNELS_DIR=$("${R_HOME}/bin/Rscript" -e 'p <- file.path(head(.libPaths(), 1L), "RcppBandicoot", "include", "bandicoot_bits", "ks"); p <- normalizePath(p, winslash="\\", mustWork=FALSE); if (dir.exists(p)) cat(p)') | |
| if [ $? -ne 0 ] || [ -z "${BANDICOOT_KERNELS_DIR}" ] || [ ! -d "${BANDICOOT_KERNELS_DIR}" ]; then | |
| echo "ERROR: Could not determine BANDICOOT_KERNELS_DIR" | |
| exit 1 | |
| fi |
| #- {os: macos-latest, r: 'release'} | ||
| #- {os: ubuntu-latest, r: 'release'} |
There was a problem hiding this comment.
The macOS and Ubuntu build configurations are commented out, leaving only Windows enabled. While the PR description mentions this is for testing Windows builds, this should not be committed to the main branch as it disables CI testing for other platforms. Either re-enable these platforms once Windows testing is complete, or add a clear TODO comment explaining when they will be re-enabled.
| - name: Install Intel CPU Runtime for OpenCL (Windows) | ||
| if: runner.os == 'Windows' | ||
| shell: pwsh | ||
| run: | | ||
| # Download | ||
| curl -o opencl-installer.exe "${{ env.INTEL_OPENCL_URL }}" | ||
|
|
||
| # Extract MSI from the self-extracting exe | ||
| $proc = Start-Process "./opencl-installer.exe" "-s -x -f extracted" -NoNewWindow -PassThru | ||
| $proc.WaitForExit() | ||
|
|
||
| # Install via msiexec | ||
| $msi = Get-ChildItem ./extracted/*.msi | Select-Object -First 1 | ||
| $proc = Start-Process "msiexec" "/i `"$msi`" /qn /l*! install.log" -NoNewWindow -PassThru | ||
| $proc.WaitForExit() | ||
|
|
||
| if ($proc.ExitCode -ne 0) { | ||
| Get-Content install.log | ||
| exit $proc.ExitCode | ||
| } | ||
|
|
||
| Remove-Item -Recurse -Force extracted, opencl-installer.exe |
There was a problem hiding this comment.
The cache step is configured but never checked for a cache hit. The Intel OpenCL Runtime installation (lines 77-98) runs unconditionally, even if the cache was restored. This wastes build time re-installing when the cache is available. Add a condition like "if: runner.os == 'Windows' && steps.cache-opencl-win.outputs.cache-hit != 'true'" to the installation step.
| if [ "${HAVE_OPENCL}" = "0" ]; then | ||
| echo "" | ||
| echo "WARNING: No GPU backend detected!" | ||
| echo "" | ||
| echo "RcppBandicoot requires OpenCL headers to compile on Windows." | ||
| echo "Set the following environment variables before building:" | ||
| echo "" | ||
| echo " OPENCL_CPPFLAGS - Path to OpenCL headers (e.g., -IC:/OpenCL/include)" | ||
| echo " OPENCL_LIBS - OpenCL library flags (e.g., -LC:/OpenCL/lib -lOpenCL)" | ||
| echo " CLBLAST_CPPFLAGS - Path to CLBlast headers (optional)" | ||
| echo " CLBLAST_LIBS - CLBlast library flags (optional)" | ||
| echo "" | ||
| fi |
There was a problem hiding this comment.
The script continues to generate R/flags.R even when OpenCL is not detected (HAVE_OPENCL=0). While this may be intentional to allow builds without GPU support, the warning message on lines 82-93 suggests OpenCL is required ("RcppBandicoot requires OpenCL headers to compile on Windows"). This is contradictory. Either the script should exit with an error when HAVE_OPENCL=0, or the warning message should be softened to indicate GPU features will be unavailable but the build can continue.
| sed -e "s|@BANDICOOT_CXXFLAGS@|${BANDICOOT_CXXFLAGS}|g" \ | ||
| -e "s|@BANDICOOT_LIBS@|${BANDICOOT_LIBS}|g" \ | ||
| -e "s|@OPENMP_CXXFLAGS@|${OPENMP_CXXFLAGS}|g" \ | ||
| -e "s|@OPENCL_TARGET_VERSION@|${OPENCL_TARGET_VERSION}|g" \ | ||
| -e "s|@BANDICOOT_KERNELS_DIR@|${BANDICOOT_KERNELS_DIR}|g" \ | ||
| src/Makevars.win.in > src/Makevars.win |
There was a problem hiding this comment.
The sed substitution does not escape special sed characters that may appear in the variable values. If BANDICOOT_KERNELS_DIR or other paths contain characters like '&', '', or '/', the sed command will fail or produce incorrect output. The '/' is especially problematic since Windows paths converted from backslashes often use it. Consider escaping these characters before substitution or using a different templating approach.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

In this PR, we add in OpenCL headers for windows as well as the prebuilt CLBlast binaries.
By default, the GitHub action runner for Windows does not have a GPU; however, the CPU instruction set should hopefully take. We'll know soon enough. Worst case scenario this is the basis for a community call for help.
To get this to work, we've added:
configure.winsrc/Makevars.winThen, updated the GitHub actions worker to install OpenCL + CLBlast directly. We may switch this over to chocolate.