-
Notifications
You must be signed in to change notification settings - Fork 55
Revisit OpenMP detection #499
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
Conversation
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.
Pull Request Overview
This PR implements a comprehensive OpenMP detection mechanism inspired by the data.table package's configure script. It replaces the previous simpler approach with a decision-tree based detection that handles both Linux and macOS platforms with multiple fallback strategies.
Key changes:
- Platform-specific OpenMP detection with separate logic for Linux and macOS
- Multiple detection strategies including homebrew support on macOS (Intel and ARM64)
- Extended Makevars template to accept PKG_CXXFLAGS and PKG_LIBS from configure
- CI matrix expanded to test both with and without OpenMP installer on macOS
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| configure.ac | Implements new decision-tree based OpenMP detection with platform-specific logic for Linux and macOS, including homebrew support |
| configure | Generated script from configure.ac with the new OpenMP detection logic |
| src/Makevars.in | Extended to accept PKG_CXXFLAGS and PKG_LIBS substitutions for custom compiler/linker flags |
| DESCRIPTION | Version bump to 15.2.2-0.1 |
| .github/workflows/ci.yaml | Added matrix configuration to test macOS with and without OpenMP installer script |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Best value add I had seen from Copilot. Caught a few not-that-easy-to-spot gotchas I had left. |
|
@eddelbuettel the issue here is this will tick So, this approach while more rigorous, will fail on an invocation of |
|
There is still the override we added in a previous PR: RcppArmadillo/inst/include/RcppArmadillo/config/RcppArmadilloConfigGenerated.h.in Lines 25 to 38 in e49b8ce
I think that is the best we can do. And users who know what they are doing can set local #define statements too to override. I think net-net we are still improving as we will be able to get more OpenMP to macOS users of Armadillo. |
| ## Additional Apple check | ||
| AC_MSG_CHECKING([for macOS]) | ||
| RSysinfoName=$("${R_HOME}/bin/Rscript" --vanilla -e 'cat(Sys.info()[["sysname"]])') | ||
| if test x"${RSysinfoName}" = x"Darwin"; then |
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.
Can we add this check back in to disable the #define ARMA_DONT_USE_OPENMP?
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.
Hm, I actually went on purpose to the 'try three different ways in sequence' model from data.table to then accept the result. And not to ignore it just because we're on macOS.
Wouldn't one possible fix be to say "oh so you want to use sourceCpp() and compile locally so could we ask you to reinstall RcppArmadillo from source" ?
|
Going to move forward, merge this and release. We can always refine in a subsequent PR. |
This PR implements the
data.table"decision tree" in theirconfigureshell script viaconfigure.ac. There are two broad cases: Linux and macOS, and within each different strategies are tested. It accommodates macOS three different ways, including homebrew.@coatless When you have a moment, please eyeball. It still contains a CI matrix with/without calling your installer script so we see these two aspects. I do not think I can automate homebrew testing but I trust the
data.tablechoices.I may comment-out some of the bits in the CI file before merging but for now it is helpful to have the additional output.