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: MPI but no HDF5 #108

Merged
merged 1 commit into from
Mar 26, 2018
Merged

Fix: MPI but no HDF5 #108

merged 1 commit into from
Mar 26, 2018

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Mar 26, 2018

Fixes the build if MPI is found but HDF5 is not.

There are in principle several ways to react on that in the code, either by calling the alternative constructor, returning a dummy and writing an error, throwing or by re-designing the #if in the HDF5 parallel backend.

@C0nsultant feel free to revise the strategy as it suits you :-)

Fixes the build if MPI is found but HDF5 is not.

There are in principle several ways to react on that in the code,
either by calling the alternative constructor, returning a dummy
and writing an error, throwing or by re-designing the `#if` in
the HDF5 parallel backend.
@ax3l ax3l requested a review from C0nsultant March 26, 2018 20:12
@ax3l ax3l added this to To do in First Stable Release via automation Mar 26, 2018
@ax3l ax3l merged commit f906871 into openPMD:dev Mar 26, 2018
First Stable Release automation moved this from To do to Done Mar 26, 2018
@ax3l ax3l deleted the fix-MPIbutNoHDF5 branch March 26, 2018 22:17
@C0nsultant
Copy link
Member

There was a fail-safe for HDF5+noMPI and noHDF5+MPI already: The construction of the handler would throw an error.
... Yes, I used throw inside a constructor. And yes, I know that is horrible.

@ax3l
Copy link
Member Author

ax3l commented Apr 5, 2018

Yes, I saw that :D

But on top of that was a compile error when MPI was found but no HDF5 because the interfaces of the fallback itself differed :)

@C0nsultant
Copy link
Member

C0nsultant commented Apr 5, 2018

So the fallback does not actually work for both combinations. Thanks for pointing this one out.

This PR is a fix for the moment, but I will think about a solution that handles this more generally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants