-
Notifications
You must be signed in to change notification settings - Fork 116
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
Check if including mpi headers are required #1772
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1772 +/- ##
==========================================
- Coverage 45.49% 45.45% -0.04%
==========================================
Files 551 551
Lines 113149 113149
==========================================
- Hits 51476 51431 -45
- Misses 61673 61718 +45
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
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.
LGTM, apart from one nitpick that this:
Line 831 in 415a69c
message(STATUS " INC | ${MPI_INCLUDE_PATH}") |
should be conditional on
NRN_INCLUDE_MPI_HEADERS
I checked in a macOS wheel from the CI that NRN_INCLUDE_MPI_HEADERS
expands to OFF
, but didn't test the other way around.
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.
LGTM 🚀
Fixes #1771