Static libsz and libaec, switch to HDF5::HDF5 in cmake, rename ENABLE_SPECTRE_DEBUG to SPECTRE_DEBUG in cmake, fix incorrect CCE assert #5972
Conversation
| string(FIND | ||
| "${_HDF5_INTERFACE_LINK_LIBS}" "libsz.so" _LOCATION_OF_LIBSZ) | ||
| if (NOT ${_LOCATION_OF_LIBSZ} EQUAL -1) | ||
| find_library(_libsz NAMES libsz.a) |
There was a problem hiding this comment.
libsz is an optional dependency of hdf5 that you specify when building hdf5. Also when building hdf5 you can specify if you want to build static libs, shared libs, or both. So if a static build of everything is important, can we just make sure to build hdf5 in an all-static way instead of trying to patch transitive dependencies in the spectre build system? For an all-static build we have to control the environment very carefully anyway, probably in a container, so we can make sure to build hdf5 correctly as well.
There was a problem hiding this comment.
I'm not sure what you're looking for in terms of changes or are suggesting. This works in the container for producing a portable statically linked executable. I linked against static HDF5 and then found out that libsz was linked dynamically for that. Are you suggesting we compile HDF5 ourselves statically in the container?
There was a problem hiding this comment.
Yes that's what I thought, in fact I thought we had to build a modified container where everything is statically linked anyway?
There was a problem hiding this comment.
Not that I know of, but I haven't thought about the containers in a while. I don't really have time to reconfigure the way we build everything in the container, so from my side it's either this or I can't distribute CCE for the ETK workshop...
|
That's one mouthful of a PR title |
|
Yea... But I didn't want to "sneak" anything in 📦 |
|
But that's what the description is for! |
|
Oh, I view that as "details" :P And I couldn't think of how to summarize 4 tangential things into one catch short word :P |
This resolves issues with static libs not linking against HDF5 dependencies.
| @@ -39,9 +39,11 @@ std::pair<size_t, size_t> create_span_for_time_value( | |||
| lower_bound < upper_bound, | |||
| "The supplied `lower_bound` is greater than `upper_bound`, which is not " | |||
| "permitted"); | |||
| ASSERT(2 * interpolator_length + pad < upper_bound, | |||
| ASSERT(2 * interpolator_length + pad <= upper_bound, | |||
There was a problem hiding this comment.
I can't figure out what this function does, but this new check is what is required to avoid underflow in the math, so it's probably correct.
There was a problem hiding this comment.
Yea, I traced this back a decent bit through the code, never fully understood it but understood it well enough to expect several other ASSERTs to trigger if it was wrong :)
Proposed changes
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments