-
-
Notifications
You must be signed in to change notification settings - Fork 0
Minimal Windows CI Build requirements #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
feb3d90
6cb3060
1786428
3f136a4
e843c34
36d0799
98c6b1a
de1c81f
8967f88
89fae3a
fca36f2
5ea7992
3226f6b
2b6db80
b934a04
bba0ece
cfc9252
823a2c1
a5231d5
567be6d
f6af274
e001f02
6bc4b96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,3 +6,4 @@ | |
| ^autom4te\.cache$ | ||
| ^config\.log$ | ||
| ^config\.status$ | ||
| ^\.gitattributes$ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| # Ensure shell scripts and configure files always use LF line endings | ||
| configure text eol=lf | ||
| configure.ac text eol=lf | ||
| configure.win text eol=lf | ||
| cleanup text eol=lf | ||
| *.sh text eol=lf |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,12 +18,14 @@ jobs: | |||||
| fail-fast: false | ||||||
| matrix: | ||||||
| config: | ||||||
| - {os: macos-latest, r: 'release'} | ||||||
| - {os: ubuntu-latest, r: 'release'} | ||||||
| #- {os: macos-latest, r: 'release'} | ||||||
| #- {os: ubuntu-latest, r: 'release'} | ||||||
| - {os: windows-latest, r: 'release'} | ||||||
|
|
||||||
| env: | ||||||
| GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }} | ||||||
| R_KEEP_PKG_SOURCE: yes | ||||||
| INTEL_OPENCL_URL: "https://registrationcenter-download.intel.com/akdlm/IRC_NAS/b6dccdb7-b503-41ea-bd4b-a78e9c2d8dd6/w_opencl_runtime_p_2025.1.0.972.exe" | ||||||
|
|
||||||
| steps: | ||||||
| - uses: actions/checkout@v4 | ||||||
|
|
@@ -38,12 +40,80 @@ jobs: | |||||
|
|
||||||
| - name: Install clblast via Homebrew (macOS) | ||||||
| if: runner.os == 'macOS' | ||||||
| run: brew install clblast clblas | ||||||
| run: brew install clblast clblas clinfo | ||||||
|
|
||||||
| - name: Install clblast via apt (Ubuntu) | ||||||
| if: runner.os == 'Linux' | ||||||
| run: sudo apt-get update && sudo apt-get install -y opencl-headers ocl-icd-opencl-dev libclblast-dev | ||||||
|
|
||||||
| - name: Cache Intel OpenCL Runtime | ||||||
| if: runner.os == 'Windows' | ||||||
| id: cache-opencl-win | ||||||
| uses: actions/cache@v5 | ||||||
| with: | ||||||
| # Default install location is x86 ... | ||||||
| path: C:\Program Files (x86)\Common Files\Intel\Shared Libraries | ||||||
| key: intel-opencl-runtime-2025.3.1 | ||||||
|
||||||
| key: intel-opencl-runtime-2025.3.1 | |
| key: intel-opencl-runtime-2025.1.0.972 |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,184 @@ | ||||||||||||||
| #!/bin/sh | ||||||||||||||
| ## | ||||||||||||||
| ## RcppBandicoot configure.win | ||||||||||||||
| ## | ||||||||||||||
| ## Windows-specific configuration script | ||||||||||||||
| ## Detects OpenCL and CLBlast from environment variables | ||||||||||||||
| ## | ||||||||||||||
| ## Copyright (C) 2023-2025 James Balamuta | ||||||||||||||
| ## Licensed under GPL-2 or later | ||||||||||||||
| ## | ||||||||||||||
|
|
||||||||||||||
| echo "Configuring RcppBandicoot for Windows..." | ||||||||||||||
|
|
||||||||||||||
| ## Get R_HOME | ||||||||||||||
| : ${R_HOME=$(R RHOME)} | ||||||||||||||
| if test -z "${R_HOME}"; then | ||||||||||||||
| echo "ERROR: Could not determine R_HOME" | ||||||||||||||
| exit 1 | ||||||||||||||
| fi | ||||||||||||||
|
|
||||||||||||||
| ## Default values | ||||||||||||||
| BANDICOOT_CXXFLAGS="" | ||||||||||||||
| BANDICOOT_LIBS="" | ||||||||||||||
| OPENMP_CXXFLAGS='$(SHLIB_OPENMP_CXXFLAGS)' | ||||||||||||||
| 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="/"))') | ||||||||||||||
|
||||||||||||||
| 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 |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| *.o | ||
| *.so | ||
| *.dll | ||
| Makevars | ||
| Makevars.win |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| ## RcppBandicoot Makevars.win.in | ||
| ## | ||
| ## This file is processed by configure.win to generate Makevars.win | ||
| ## It includes GPU backend configuration (OpenCL, CLBlast) for Windows | ||
|
|
||
| PKG_CPPFLAGS = -I../inst/include -DCOOT_TARGET_OPENCL_VERSION=@OPENCL_TARGET_VERSION@ -DCOOT_KERNEL_SOURCE_DIR=\"@BANDICOOT_KERNELS_DIR@\" | ||
|
|
||
| ## Compiler flags from configure.win | ||
| PKG_CXXFLAGS = @BANDICOOT_CXXFLAGS@ | ||
|
|
||
| ## Linker flags from configure.win | ||
| PKG_LIBS = @OPENMP_CXXFLAGS@ @BANDICOOT_LIBS@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.