Skip to content

Conversation

@QuLogic
Copy link
Member

@QuLogic QuLogic commented Oct 1, 2015

There are already two HDF5 tests here, but neither of them really work properly, so they've been deleted.

There are already two HDF5 tests here, but neither of them really work,
so they've been deleted.
@baagaard
Copy link

baagaard commented Oct 6, 2015

Before I merge in the new macros, I would like more information. "Neither of them really work properly" is vague. Please provide specifics on what doesn't work. Additionally, there are multiple codes using this repository, so deleting macros can break configure in unanticipated ways. As a result, I would prefer not to remove them unless they are unusable or we verify no code uses them.

@QuLogic
Copy link
Member Author

QuLogic commented Oct 7, 2015

The macros in cit_hdf.m4 (Note, nothing actually uses it.) and cit_hdf5.m4 do not work automatically on Fedora. They require the user to specify any extra libraries if their HDF5 installation is static. Any other special flags also require user intervention. (Yes, many of these can be worked around somewhat with PHDF5_HOME, CPPFLAGS and LDFLAGS, but it's annoying. That's what configure is supposed to figure out, after all!)

You're correct that other code does use these macros, but it should not be a problem since the submodule reference is fixed. I've already opened a PR for citcoms. You've probably seen the PR for pylith already. That leaves pylith_installer, but I can't really get that to actually run even with the old macro (will be fixed by geodynamics/pylith_installer#1). There's only cigma left, but it's already broken.

@QuLogic
Copy link
Member Author

QuLogic commented Oct 8, 2015

Also, using CPPFLAGS and LDFLAGS is a bit heavy-handed. If additional libraries are needed for HDF5, then they get applied everywhere, even on targets that don't use HDF5.

@baagaard-usgs
Copy link
Contributor

Updates to cit_hdf5.m4 have resolved this issue.

Configure should provide HDF5_INCLUDES and HDF5_LDFLAGS.

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.

3 participants