Conversation
R/Attributes.R
Outdated
| # get the C++ compiler from R | ||
| r <- file.path(R.home("bin"), "R") | ||
| cxx <- tryCatch( | ||
| system2(r, c("CMD", "config", "CXX"), stdout = TRUE), |
There was a problem hiding this comment.
Strictly speaking, this might not be correct if an alternate plugin causes a different variant of the C++ compiler to be used (e.g. cpp11 will use whatever compiler is registered for CXX11, which in theory might not match CXX). Is this worth worrying about?
There was a problem hiding this comment.
Oh dear. The baker's dozen of CXX11 to CXX26. Good question. On the other hand if we cannot trust R to tell it about its CXX, who can we trust?
FWIW for me R CMD config CXX and R CMD config CXX17 return the same. [ Checks. ] But that is due to my personal .R/Makevars.
There was a problem hiding this comment.
Yeah, it'd be odd to have one compiler for CXX11, and another for CXX14 -- doubly so on macOS. But lots of people will configure CXX without thinking to configure all of the other versioned CXX variants...
But I now remembered we can get closer using R CMD SHLIB, which lets R choose the appropriate C++ compiler based on whatever environment variables / user Makevars configuration is set, so maybe that's more appropriate. I just made a change to that effect.
Oof. This PR was supposed to be simple!
There was a problem hiding this comment.
Nothing in life ever is provided you look closely enough....
| prefix <- if (machine == "arm64") "/opt/homebrew" else "/usr/local" | ||
|
|
||
| # build flags | ||
| cxxflags <- sprintf("-I%s/opt/libomp/include -fopenmp", prefix) |
There was a problem hiding this comment.
I confirmed that the version of LLVM clang provided in Homebrew accepts -fopenmp as-is, but I'm adding the library paths for libomp just in case.
|
@coatless could you chime in during the next day or two? |
coatless
left a comment
There was a problem hiding this comment.
This hits all the main points. The only note I have is with the R CMD SHLIB seeming to be more complex and slightly more brittle; but, I'm sure @kevinushey has a good reason for it.
| .plugins[["openmp"]] <- function() { | ||
| list(env = list(PKG_CXXFLAGS="-fopenmp", | ||
| PKG_LIBS="-fopenmp")) | ||
| .plugins[["openmp"]] <- if (Sys.info()[["sysname"]] == "Darwin") { |
There was a problem hiding this comment.
I really like how the detection simplifies the function definition on load.
| # extract the compiler invocation from the shlib output | ||
| # use some heuristics here... | ||
| index <- grep("make would use", output) | ||
| compile <- output[[index + 1L]] | ||
|
|
||
| # use everything up to the first include flag, which is normally | ||
| # the R headers from CPPFLAGS | ||
| idx <- regexpr(" -I", compile, fixed = TRUE) | ||
| cxx <- substring(compile, 1L, idx - 1L) |
There was a problem hiding this comment.
So, this would trigger:
R CMD SHLIB --dry-run $TMPDIR/openmp-detect-.cppGiving:
make cmd is
make -f '/Library/Frameworks/R.framework/Resources/etc/Makeconf' -f '/Library/Frameworks/R.framework/Resources/share/make/shlib.mk' -f '/Users/ronin/.R/Makevars' SHLIB_LDFLAGS='$(SHLIB_CXXLDFLAGS)' SHLIB_LD='$(SHLIB_CXXLD)' SHLIB='/var/folders/b0/vt_1hj2d6yd8myx9lwh81pww0000gn/T//Rtmpcxzs34/openmp-detect-137df4ba71a0a.so' OBJECTS='/var/folders/b0/vt_1hj2d6yd8myx9lwh81pww0000gn/T//Rtmpcxzs34/openmp-detect-137df4ba71a0a.o'
make would use
clang++ -arch arm64 -std=gnu++17 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I/opt/R/arm64/include -I/opt/R/arm64/include -fPIC -falign-functions=64 -Wall -g -O2 -c /var/folders/b0/vt_1hj2d6yd8myx9lwh81pww0000gn/T//Rtmpcxzs34/openmp-detect-137df4ba71a0a.cpp -o /var/folders/b0/vt_1hj2d6yd8myx9lwh81pww0000gn/T//Rtmpcxzs34/openmp-detect-137df4ba71a0a.o if test "z/var/folders/b0/vt_1hj2d6yd8myx9lwh81pww0000gn/T//Rtmpcxzs34/openmp-detect-137df4ba71a0a.o" != "z"; then \ echo clang++ -arch arm64 -std=gnu++17 -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -L"/Library/Frameworks/R.framework/Resources/lib" -L/opt/R/arm64/lib -L/opt/R/arm64/lib -o /var/folders/b0/vt_1hj2d6yd8myx9lwh81pww0000gn/T//Rtmpcxzs34/openmp-detect-137df4ba71a0a.so /var/folders/b0/vt_1hj2d6yd8myx9lwh81pww0000gn/T//Rtmpcxzs34/openmp-detect-137df4ba71a0a.o -F/Library/Frameworks/R.framework/.. -framework R ; \ clang++ -arch arm64 -std=gnu++17 -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -L"/Library/Frameworks/R.framework/Resources/lib" -L/opt/R/arm64/lib -L/opt/R/arm64/lib -o /var/folders/b0/vt_1hj2d6yd8myx9lwh81pww0000gn/T//Rtmpcxzs34/openmp-detect-137df4ba71a0a.so /var/folders/b0/vt_1hj2d6yd8myx9lwh81pww0000gn/T//Rtmpcxzs34/openmp-detect-137df4ba71a0a.o -F/Library/Frameworks/R.framework/.. -framework R ; \ fiAfter the regex, we're left with:
clang++ -arch arm64 -std=gnu++17This is the same as:
R CMD config CXXclang++ -arch arm64 -std=gnu++17
I'm not immediately seeing a huge benefit for a disk write and a regex to end up back at CXX config. Is there something regarding a custom toolchain that this would protect against?
There was a problem hiding this comment.
See earlier discussion between @kevinushey and myself. It is possible that, say, CXX != CXX17 so that asking for the former does not give us the answer.
I agree with the sentiment of this being more brittle but on balance that may be what we have to go with.
There was a problem hiding this comment.
Yeah, that's exactly it. For example, if the USE_CXX17 environment variable is set, then CXX17 will be used during compilation. Some users might be doing things like this in their .Renviron to enforce the use of their configured CXX17 compiler for different packages.
(And, as far as I can see, there isn't a way to ask R "what compiler do you want to use right now?" beyond something like this.)
|
So I think I am going to merge this one now, with a big thank you to both of you. Working on macOS remains both very rewarding (say those who use it 😉 ) and challenging (say those supporting it) but this ought to be a net step ahead. |
Pull Request Template for Rcpp
A somewhat tidier (IMHO) alternative to the approach in #1409. It also avoids changing the definition of the plugin on non-macOS systems.
Checklist
R CMD checkstill passes all tests