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

Fix build of parallel #242

Merged
merged 7 commits into from
Nov 1, 2019
Merged

Fix build of parallel #242

merged 7 commits into from
Nov 1, 2019

Conversation

alkino
Copy link
Member

@alkino alkino commented Oct 31, 2019

Fix #241

@alkino
Copy link
Member Author

alkino commented Oct 31, 2019

Should not HIGHFIVE_PARALLEL_HDF5 be ON by default?

@alkino alkino force-pushed the fix_parallel branch 2 times, most recently from 0ddcb7f to 52fab73 Compare October 31, 2019 08:35
@ferdonline
Copy link
Contributor

Error: invalid option: --with-mpi
AFAIK they brew dont have versions compiled with mpi

@alkino
Copy link
Member Author

alkino commented Oct 31, 2019

We merge like that?

.travis.yml Outdated Show resolved Hide resolved
@@ -10,12 +10,12 @@ target_compile_definitions(HighFive INTERFACE ${HDF5_DEFINITIONS})

# MPI
if(HIGHFIVE_PARALLEL_HDF5)
target_include_directories(HighFive SYSTEM INTERFACE ${MPI_C_INCLUDE_PATH})
target_link_libraries(HighFive INTERFACE ${MPI_C_LIBRARIES})
target_include_directories(HighFive SYSTEM INTERFACE ${MPI_CXX_INCLUDE_PATH})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were these changes required to it to succeed? Or is it just an improvement?

@ferdonline
Copy link
Contributor

Should not HIGHFIVE_PARALLEL_HDF5 be ON by default?

I believe not, it would be one more dependency. However, in our internal stack with Spack, yes, we should build with mpi by default.

We now cover the scenarios of installing hdf5 / build HighFive:
 - serial / serial
 - parallel / parallel
 - parallel / serial
@ferdonline
Copy link
Contributor

We should support the scenario added (using a parallel hdf5 but compile serial apps)

@alkino
Copy link
Member Author

alkino commented Nov 1, 2019

Really good catch, I fix it but it's only that findHDF5 don't do its job well

Copy link
Contributor

@ferdonline ferdonline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alkino alkino merged commit 93d877b into master Nov 1, 2019
@alkino alkino deleted the fix_parallel branch November 1, 2019 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build Fails with HIGHFIVE_PARALLEL_HDF5
2 participants