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

HDF5 conflicts #129

Closed
KrisThielemans opened this issue Jun 29, 2018 · 8 comments
Closed

HDF5 conflicts #129

KrisThielemans opened this issue Jun 29, 2018 · 8 comments
Assignees
Milestone

Comments

@KrisThielemans
Copy link
Member

we're getting conflicts between HDF5 versions. Easiest route might be to use static linking when building SIRF and ISMRMRD.

@KrisThielemans
Copy link
Member Author

KrisThielemans commented Aug 8, 2018

confirmed with @johannesmayer that HDF5 conflicts easily happen even on the VM. We had the system HDF5, the SuperBuild HDF5 and ITK comes with HDF5 as well. Matlab does as well. All different versions. By the way, the GE converter needs to GE Orchestra version of HDF5...

HDF5 library has a habit of causing segfaults when there's a version mismatch anywhere, so this needs to be avoided.

I lean now towards relying on a system HDF5 by default (i.e. to not make the situation worse), and introduce static linking. The latter should be doable by adding -DHDF5_USE_STATIC_LIBRARIES=ON in the External*cmake. I don't think I'd put it in SIRF's CMakeLists.txt.

Anyone any other opinions? @bathomas, @casperdcl ?

@paskino
Copy link
Contributor

paskino commented Aug 8, 2018

Can't we rely only on the one we build?

@KrisThielemans
Copy link
Member Author

we're trying to, but failing :-; I did notice there were a few HDF5_LIBRARIES missing in the External* files. @johannesmayer, maybe you could generate a PR with those fixes to the SuperBuild. However, even then we had conflicts until we deleted all our own HDF5 files, and just relied on the system one. I now think that relying on the system one is best, unless we cannot (e.g. because it's too old).

@KrisThielemans
Copy link
Member Author

check if ITK can be build with a specific HDF5.

change option USE_SYSTEM_HDF5 depending on find_package

@KrisThielemans
Copy link
Member Author

KrisThielemans commented Aug 10, 2018

check if ITK can be build with a specific HDF5.

yes: -DITK_USE_SYSTEM_HDF5=ON

@KrisThielemans
Copy link
Member Author

The situation on specifying HDF5 location is actually complicated.

At present we set HDF5_ROOT, HDF5_INCLUDE_DIRS and most of the time HDF5_LIBRARIES. However, if USE_SYSTEM_HDF5=OFF, HDF5_LIBRARIES is actually empty.

According to the FindHDF5 doc, only HDF5_ROOT should be used and the others will be ignored. However, with CMake 3.7.2, setting the others actually prepends them to the final version seen after find_package(HDF5), therefore taking precedence (which I guess is good, although unnecessary). I think with more recent CMake, those variables might be ignored, but I haven't checked properly.

Because of all this, I suggest removing the explicit setting of HDF5_INCLUDE_DIRS and HDF5_LIBRARIES.

However, this seems to mean that with current master, we do set HDF5_ROOT everywhere ok, except for ITK. But ITK by default builds its own HDF5, which one would hope doesn't conflict with anything else (libraries are called differently, e.g. libitkhdf5.a, and include files should only be found when using itkhdf5/hdf5.h etc). I haven't been able yet to let anything in SIRF depend on an itkhdf5 library.

All in all, it's a mistery why @johannesmayer had HDF5 conflicts, and that's of course worrying (as it means I don't know how to fix it).

@johannesmayer, do you remember what depended on the itkhdf5 library?

@KrisThielemans
Copy link
Member Author

I've created #143 for this. I'm not 100% sure if this is a good idea though. On the other hand, we might only see trouble once using ITK to read HDF5 files.

@KrisThielemans
Copy link
Member Author

this seems largely obsolete. We still have some HDF5 problems, but there is likely no tmuch here that is relevant.

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

No branches or pull requests

3 participants