Skip to content
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

[Request] Test for compiler compatibility with -fopenmp before using SHLIB_OPENMP_CFLAGS #2161

Closed
jimhester opened this issue May 12, 2017 · 3 comments · Fixed by #3984
Closed
Labels
Milestone

Comments

@jimhester
Copy link
Contributor

@jimhester jimhester commented May 12, 2017

This will allow installation of data.table on systems / compilers that do not support openmp, even if the site Makeconf file defines SHLIB_OPENMP_CFLAGS. This is the case on macOS as of R 3.4.0 with the default clang compiler.

Something like the following should be sufficient, note that ifeq is a GNU make extension, so it will need to be noted in your DESCRIPTION

CXX11=$(shell "${R_HOME}"/bin/R CMD config CXX11)
ifeq ($(shell $(CXX11) -fopenmp -E -xc++ - 2>&1 >/dev/null && echo 'true'), true)
  PKG_CFLAGS=$(SHLIB_OPENMP_CFLAGS)
  PKG_LIBS=$(SHLIB_OPENMP_CFLAGS)
endif
@bryanhanson
Copy link

@bryanhanson bryanhanson commented May 28, 2017

This issue is currently blocking use of data.table on Mac with the devel version of R, which in turn blocks a number of other packages, and in turn, development and updating of packages:

> sessionInfo()
R Under development (unstable) (2017-05-26 r72742)
Platform: x86_64-apple-darwin15.6.0 (64-bit)
Running under: macOS Sierra 10.12.5

Matrix products: default
BLAS: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRblas.0.dylib
LAPACK: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets 
[6] methods   base     

loaded via a namespace (and not attached):
[1] compiler_3.5.0 tools_3.5.0   

@jimhester
Copy link
Contributor Author

@jimhester jimhester commented Oct 9, 2019

Just a note that this still causes issues (e.g. this new one on R community), particularly after a new release when CRAN has not yet built a binary. Now that you are using a configure script (#3951) it might be worth adding this test to the configure script so compilation will work with the native macOS toolchain (albeit without parallelization).

@mattdowle
Copy link
Member

@mattdowle mattdowle commented Oct 14, 2019

@jimhester Thanks for the note. I thought it already worked with compilers without openmp, and that's checked in CRAN_Release.cmd here:

# Check compiles ok when OpenMP is disabled
. But clearly I am wrong and that isn't sufficient. I'll try and get my head around what you're saying and suggesting.

I get a bit frustrated with claims from users that data.table is blocking anything. It's MacOS that's the problem ... why doesn't it include openmp by default? It all works smoothly on Linux and Windows.

jimhester added a commit to jimhester/data.table that referenced this issue Oct 21, 2019
This detects support for OpenMP and only includes the compiler flags for
it if OpenMP is supported.

This solves the common case on macOS, because R on macOS ships with
`-fopenmp` in SHLIB_OPENMP_CFLAGS, as the CRAN compiler toolchain _does_
support OpenMP, however the XCode toolchain distributed by Apple _does
 not_ support OpenMP, which causes data.table to fail to compile from
 source.

Fixes Rdatatable#2161
jimhester added a commit to jimhester/data.table that referenced this issue Oct 21, 2019
This detects support for OpenMP and only includes the compiler flags for
it if OpenMP is supported.

This solves the common case on macOS, because R on macOS ships with
`-fopenmp` in SHLIB_OPENMP_CFLAGS, as the CRAN compiler toolchain _does_
support OpenMP, however the XCode toolchain distributed by Apple _does
 not_ support OpenMP, which causes data.table to fail to compile from
 source.

Fixes Rdatatable#2161
@jangorecki jangorecki added this to the 1.12.7 milestone Dec 6, 2019
@mattdowle mattdowle removed this from the 1.12.7 milestone Dec 8, 2019
@mattdowle mattdowle added this to the 1.12.9 milestone Dec 8, 2019
@jangorecki jangorecki removed this from the 1.12.11 milestone May 26, 2020
@jangorecki jangorecki added this to the 1.12.9 milestone May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants