diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index 34cbbbfab..37ee06451 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -26,7 +26,8 @@ jobs: # jobs (mainly test-coverage) to run on every commit in PRs so as to not slow down dev. # GHA does run these jobs concurrently but even so reducing the load seems like a good idea. - {os: windows-latest, r: 'devel'} - # - {os: macOS-latest, r: 'release'} # test-coverage.yaml uses macOS + - {os: macos-15-intel, r: 'release'} + - {os: macos-15, r: 'release'} # TODO(remotes>2.5.0): Use 24.04[noble?] - {os: ubuntu-22.04, r: 'release', rspm: "https://packagemanager.rstudio.com/cran/__linux__/jammy/latest"} # - {os: ubuntu-22.04, r: 'devel', rspm: "https://packagemanager.rstudio.com/cran/__linux__/jammy/latest", http-user-agent: "R/4.1.0 (ubuntu-22.04) R (4.1.0 x86_64-pc-linux-gnu x86_64 linux-gnu) on GitHub Actions" } @@ -57,8 +58,8 @@ jobs: uses: actions/cache@v4 with: path: ${{ env.R_LIBS_USER }} - key: ${{ runner.os }}-${{ hashFiles('.github/R-version') }}-1-${{ hashFiles('.github/depends.Rds') }} - restore-keys: ${{ runner.os }}-${{ hashFiles('.github/R-version') }}-1- + key: ${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('.github/R-version') }}-1-${{ hashFiles('.github/depends.Rds') }} + restore-keys: ${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('.github/R-version') }}-1- - name: Install system dependencies if: runner.os == 'Linux' @@ -68,6 +69,21 @@ jobs: eval sudo $cmd done < <(Rscript -e 'writeLines(remotes::system_requirements("ubuntu", "22.04"))') + - name: Install R Package Build Dependencies on MacOS, from https://github.com/stan-dev/cmdstanr/pull/1072/files + if: runner.os == 'macOS' + uses: r-hub/actions/setup-r-sysreqs@v1 + with: + type: 'minimal' + + - name: Install and configure OpenMP runtime + if: runner.os == 'macOS' + run: | + if clang --version | grep 'clang version 17'; then + curl -O https://mac.r-project.org/openmp/openmp-19.1.0-darwin20-Release.tar.gz + sudo tar fvvxz openmp-19.1.0-darwin20-Release.tar.gz -C / + rm -f openmp-19.1.0-darwin20-Release.tar.gz + fi # otherwise R-bundled runtime is fine + - name: Install dependencies run: | remotes::install_deps(dependencies = TRUE) diff --git a/NEWS.md b/NEWS.md index 3110f0031..b87b1e7d6 100644 --- a/NEWS.md +++ b/NEWS.md @@ -311,6 +311,8 @@ 6. Using a double vector in `set()`'s `i=` and/or `j=` no longer throws a warning about preferring integer, [#6594](https://github.com/Rdatatable/data.table/issues/6594). While it may improve efficiency to use integer, there's no guarantee it's an improvement and the difference is likely to be minimal. The coercion will still be reported under `datatable.verbose=TRUE`. For package/production use cases, static analyzers such as `lintr::implicit_integer_linter()` can also report when numeric literals should be rewritten as integer literals. +7. Enhanced tests for OpenMP support, detecting incompatibilities such as R-bundled runtime _vs._ newer Xcode and testing for a manually installed runtime from , [#6622](https://github.com/Rdatatable/data.table/issues/6622). Thanks to @dvg-p4 for initial report and testing, @twitched for the pointers, @tdhock and @aitap for the fix. + ## data.table [v1.17.8](https://github.com/Rdatatable/data.table/milestone/41) (6 July 2025) 1. Internal functions used to signal errors are now marked as non-returning, silencing a compiler warning about potentially unchecked allocation failure. Thanks to Prof. Brian D. Ripley for the report and @aitap for the fix, [#7070](https://github.com/Rdatatable/data.table/pull/7070). diff --git a/configure b/configure index c6492f54f..3892dc544 100755 --- a/configure +++ b/configure @@ -68,89 +68,79 @@ fi # Aside: ${SHLIB_OPENMP_CFLAGS} does not appear to be defined at this point according to Matt's testing on # Linux, and R CMD config SHLIB_OPENMP_CFLAGS also returns 'no information for variable'. That's not # inconsistent with R-exts$1.2.1.1, though, which states it's 'available for use in Makevars' (so not -# necessarily here in configure). Hence use -fopenmp directly for this detection step. +# necessarily here in configure). # printf not echo to pass checkbashisms w.r.t. to the \n +# Specifically use schedule(dynamic) to distinguish incompatibilities in OpenMP runtime on macOS, #7318 cat < test-omp.c -#include -int main() { - return omp_get_num_threads(); +void test_openmp(int * result) { + int sum = 0; +#ifdef _OPENMP + #pragma omp parallel for reduction(+:sum) num_threads(2) schedule(dynamic) + for (int i = 1; i <= 2; ++i) sum += i; +#endif + *result = sum; } EOF -detect_openmp () { - - if [ "$(uname)" = "Linux" ]; then - - printf "%s" "* checking if R installation supports OpenMP without any extra hints... " - if "${R_HOME}/bin/R" CMD SHLIB test-omp.c >> config.log 2>&1; then - echo "yes" - export R_OPENMP_ENABLED=1 - return - else - echo "no" - fi - +test_openmp_variant () { + cflags="${PKG_CFLAGS:+${PKG_CFLAGS} }$1" + libs="${PKG_LIBS:+${PKG_LIBS} }$2" + printf "%s" "* checking if OpenMP works with CFLAGS='$cflags' LIBS='$libs'... " + if ! PKG_CFLAGS="$cflags" PKG_LIBS="$libs" "${R_HOME}"/bin/R CMD SHLIB --preclean --clean test-omp.c >>config.log 2>&1; then + echo "no" + return 1 + fi - printf "%s" "* checking if R installation supports openmp with \"-fopenmp\" flag... " - if ${CC} ${CFLAGS} -fopenmp test-omp.c >> config.log 2>&1; then - echo "yes" - export PKG_CFLAGS="${PKG_CFLAGS} -fopenmp" - export R_OPENMP_ENABLED=1 - return - else - echo "no" - fi - fi # uname=Linux + if ! "${R_HOME}"/bin/Rscript -e ' +dll <- paste0("test-omp", .Platform$dynlib.ext) +dyn.load(dll) +ans <- .C("test_openmp", ans = integer(1))$ans +stopifnot(identical(ans, 3L)) +' >> config.log 2>&1; then + echo "no" + return 1 + fi - if [ "$(uname)" = "Darwin" ]; then + export PKG_CFLAGS="${PKG_CFLAGS} ${1}" + export PKG_LIBS="${PKG_LIBS} ${2}" + export R_OPENMP_ENABLED=1 + echo "yes" + return 0 +} +detect_openmp () { + # OpenMP flags provided in environment variables or specified when configuring R? Does -fopenmp work? + test_openmp_variant "" "" \ + || test_openmp_variant '$(SHLIB_OPENMP_CFLAGS)' '$(SHLIB_OPENMP_CFLAGS)' \ + || test_openmp_variant -fopenmp -fopenmp \ + && return + + case "$(uname)" in + Darwin) # https://mac.r-project.org/openmp - printf "%s" "* checking if R installation supports OpenMP with \"-Xclang -fopenmp\" ... " - if CPPFLAGS="${CPPFLAGS} -Xclang -fopenmp" PKG_LIBS="-lomp" "${R_HOME}/bin/R" CMD SHLIB test-omp.c >> config.log 2>&1; then - echo "yes" - export PKG_CFLAGS="${PKG_CFLAGS} -Xclang -fopenmp" - export PKG_LIBS="${PKG_LIBS} -lomp" - export R_OPENMP_ENABLED=1 - return - else - echo "no" - fi - - # https://github.com/Rdatatable/data.table/issues/6409 - printf "%s" "* checking if R installation supports OpenMP with \"-fopenmp\" ... " - if CPPFLAGS="${CPPFLAGS} -fopenmp" "${R_HOME}/bin/R" CMD SHLIB test-omp.c >> config.log 2>&1; then - echo "yes" - export PKG_CFLAGS="${PKG_CFLAGS} -fopenmp" - export PKG_LIBS="${PKG_LIBS} -fopenmp" - export R_OPENMP_ENABLED=1 - return - else - echo "no" - fi + test_openmp_variant "-Xclang -fopenmp" "-lomp" && return if [ "$(uname -m)" = "arm64" ]; then HOMEBREW_PREFIX=/opt/homebrew else HOMEBREW_PREFIX=/usr/local fi - - if [ -e "${HOMEBREW_PREFIX}/opt/libomp" ]; then - printf "%s" "* checking if libomp installation at ${HOMEBREW_PREFIX}/opt/libomp can be used... " - LIBOMP_INCLUDE="-I${HOMEBREW_PREFIX}/opt/libomp/include -Xclang -fopenmp" - LIBOMP_LINK="-L${HOMEBREW_PREFIX}/opt/libomp/lib -lomp" - if ${CC} ${CFLAGS} ${LIBOMP_INCLUDE} ${LIBOMP_LINK} test-omp.c >> config.log 2>&1; then - echo "yes" - export PKG_CFLAGS="${PKG_CFLAGS} ${LIBOMP_INCLUDE}" - export PKG_LIBS="${PKG_LIBS} ${LIBOMP_LINK}" - export R_OPENMP_ENABLED=1 - return - else - echo "no" - fi + test -e "${HOMEBREW_PREFIX}/opt/libomp" ] \ + && test_openmp_variant \ + "-I${HOMEBREW_PREFIX}/opt/libomp/include -Xclang -fopenmp" \ + "-L${HOMEBREW_PREFIX}/opt/libomp/lib -lomp" \ + && return + + if "${CC}" --version | grep -q 'Apple clang 17'; then + test -e /usr/local/lib/libomp.dylib \ + && test_openmp_variant "-Xclang -fopenmp" "/usr/local/lib/libomp.dylib" \ + && return + echo "*** All OpenMP runtime tests failed and you're running Apple clang 17." + echo "*** Do you need a new OpenMP runtime from ?" fi - - fi # uname=Darwin + ;; + esac # No support for OpenMP available export R_OPENMP_ENABLED=0 @@ -168,14 +158,10 @@ if [ "${R_OPENMP_ENABLED}" = "0" ]; then echo "*** https://github.com/Rdatatable/data.table/wiki/Installation" echo "*** Continuing installation without OpenMP support..." echo "***" - sed -e "s|@openmp_cflags@||" src/Makevars.in > src/Makevars -else - sed -e "s|@openmp_cflags@|\$(SHLIB_OPENMP_CFLAGS)|" src/Makevars.in > src/Makevars fi # retain user supplied PKG_ env variables, #4664. See comments in Makevars.in too. -sed -e "s|@PKG_CFLAGS@|$PKG_CFLAGS|" src/Makevars > src/Makevars.tmp && mv src/Makevars.tmp src/Makevars -sed -e "s|@PKG_LIBS@|$PKG_LIBS|" src/Makevars > src/Makevars.tmp && mv src/Makevars.tmp src/Makevars +sed -e "s|@PKG_CFLAGS@|$PKG_CFLAGS|" -e "s|@PKG_LIBS@|$PKG_LIBS|" src/Makevars.in > src/Makevars # optional dependency on zlib if [ "$NOZLIB" = "1" ]; then diff --git a/src/Makevars.in b/src/Makevars.in index fcfaceba9..0e90bfd6d 100644 --- a/src/Makevars.in +++ b/src/Makevars.in @@ -1,5 +1,5 @@ -PKG_CFLAGS = @PKG_CFLAGS@ @openmp_cflags@ @zlib_cflags@ -PKG_LIBS = @PKG_LIBS@ @openmp_cflags@ @zlib_libs@ +PKG_CFLAGS = @PKG_CFLAGS@ @zlib_cflags@ +PKG_LIBS = @PKG_LIBS@ @zlib_libs@ # See WRE $1.2.1.1. But retain user supplied PKG_* too, #4664. # WRE states ($1.6) that += isn't portable and that we aren't allowed to use it. # Otherwise we could use the much simpler PKG_LIBS += @openmp_cflags@ -lz.