Repository navigation
Fix various issues in package configuration file - #6617
jhendersonHDF merged 2 commits into
Conversation
Fix find_dependency() calls so that PRIVATE-linked libraries are only propagated as transitive link requirements for static library targets Add missing find_dependency() calls for some PRIVATE-linked libraries Fix issue where parallel-enabled HDF5 can fail to locate MPI Fortran support, even when HDF5 Fortran support isn't requested Set HDF5_LIB_TYPE to only shared or static, depending on requested library type, rather than a list that could include both shared and static Fix HDF5_LIB_TYPE being undefined when components are specified, but shared/static isn't Reduce scope of modifications to CMAKE_MODULE_PATH so changes aren't propagated to consuming projects Add check for both shared and static libraries being requested and fail if so Remove enable_language() call in favor of checking enabled languages and failing if required language isn't enabled Add missing CMake variable for digitally signed plugins feature Fix CMake variable for HDF5_DIMENSION_SCALES_NEW_REF option
jhendersonHDF
added
Component - Documentation
Review ChecklistThis PR touches the following areas. Each needs a sign-off
|
|
I manually tested the issues fixed by these changes, but plan to follow this up with a separate PR that adds testing of the installed .cmake files. |
|
This pull request has had no activity for 30 days and has been marked stale. Push a commit or comment to keep it open, or it will be flagged for maintainer review. |
| #----------------------------------------------------------------------------- | ||
| set (${HDF5_PACKAGE_NAME}_INCLUDE_DIR "@PACKAGE_INCLUDE_INSTALL_DIR@" "${${HDF5_PACKAGE_NAME}_MPI_C_INCLUDE_DIRS}") | ||
| if (${HDF5_PACKAGE_NAME}_PROVIDES_PARALLEL) | ||
| unset (_hdf5_mpi_components) |
There was a problem hiding this comment.
Against a parallel HDF5 install, consumers that don't include C in the components list won't get the MPI::MPI_C target. The MPI target is always required when HDF5 is built with parallel support, so if e.g. a Fortran project only requests the Fortran component, the MPI target won't be created and the build will fail.
There was a problem hiding this comment.
Similarly, dropping enable-language here means that any consumers of parallel HDF5 now have to enable C directly. This isn't technically a requirements change, but builds that previously configured successfully will now fail until they modify their build code. Should we list this as a breaking change?
There was a problem hiding this comment.
Remember that this is from the consumer side, so MPI::MPI_C isn't necessarily needed here. For the cases where this matters, we also inadvertently have a dependency on C being enabled already due to the find_dependency(Threads) call, but that could be made more explicit. The problem is whether or not the dependency on C exists depends on the particular components being requested.
I'm fine with listing the enable_language() change as a breaking change, but I typically don't do so in cases like this where:
- The behavior was incorrect to begin with
- The chances of someone having logic that runs into this case is very low
- Consumers relying on this logic would have been incorrect and inviting bad behavior
The enable_language() call has to be removed anyway due to https://gitlab.kitware.com/cmake/cmake/-/work_items/26751.
| # Handle all other requested components | ||
| #----------------------------------------------------------------------------- | ||
| set (libtype ${${HDF5_PACKAGE_NAME}_LIB_TYPE}) | ||
| foreach (comp IN LISTS ${HDF5_PACKAGE_NAME}_FIND_COMPONENTS) |
There was a problem hiding this comment.
The new loop doesn't remove shared/static from HDF5_FIND_COMPONENTS before the loop, and the loop only resets hdf5_comp2. That means that with the default components C HL static, the iteration where comp = static won't match any branch, and will then reuse hdf5_comp from a previous iteration (hdf5_comp = hdf5_hl in this case). Variables for a non-existent component will then be populated.
There was a problem hiding this comment.
Yes, removing shared / static in the previous version of this logic was a mistake and the components logic likely needs updating to deal with the change. Since we aren't currently dealing with the components correctly anyway, this is a non-issue for the time being, but should be dealt with when the component checking is fixed.
| check_required_components(${HDF5_PACKAGE_NAME}_${libtype}) | ||
| endforeach () | ||
| # Should be last | ||
| check_required_components (${HDF5_PACKAGE_NAME}_${${HDF5_PACKAGE_NAME}_LIB_TYPE}) |
There was a problem hiding this comment.
This required components check iterates over HDF5_[static/shared]_FIND_COMPONENTS, neither of which is ever set, so this check doesn't do anything. Pre-existing, but now would be a good time to fix this.
There was a problem hiding this comment.
Yes, due to mistakes made (and not checked) during or around the initial version of this file, the check_required_components() call has likely never worked. But I chose not to fix this here because I suspect it will have unexpected issues when we actually start checking that components are enabled when we weren't previously and that's something I didn't want to do in a minor release.
|
Merging this as I'd really like these changes to go in sooner rather than later and they should also help the architecture in #6600. Specifically, the I'll follow up with a PR to include the |
Keep only option() and mark_as_advanced() for HDF5_DIMENSION_SCALES_NEW_REF in CMakeBuildOptions.cmake and move the H5_DIMENSION_SCALES_WITH_NEW_REF define back to hl/, set in the parent scope so it reaches H5pubconf.h. Drop the DIMENSION_SCALES_WITH_NEW_REF variable and have H5build_settings.cmake.c.in use the option value directly, as the other configuration files already do since HDFGroup#6617. Say "old-style reference path" in the CHANGELOG entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Fix leaked dataset ID in H5DSattach_scale() The loop that recreates the references already stored in the scale's REFERENCE_LIST attribute reopens each one with H5Ropen_object(), but only closed the resulting identifier when H5Rcreate_object() failed. On the success path it was dropped, leaking one dataset ID per reference already attached to the scale -- and each leaked ID kept its file open, so a later H5Fcreate() with H5F_ACC_TRUNC on the same file failed with "unable to truncate a file which is already open". The matching loop in H5DSdetach_scale() has always closed it there; this brings attach into line. Only reachable through a pass-through VOL connector: H5DSwith_new_ref() sets is_new_ref = (config_flag || !native), so a native object without H5_DIMENSION_SCALES_WITH_NEW_REF takes the old-style reference branch, which opens nothing. Fixes #6639 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Reclaim the references H5DSattach_scale() reads, and test the leak Closing the reopened dataset ID was not enough to make the file closeable. Rewriting REFERENCE_LIST also acquired two things it never released: - the references read from the old attribute into ndsbuf were freed without being destroyed, and - ndsbuf_w was reclaimed with SID, the old dataspace, which is one element shorter than the buffer -- the reference just appended at index nelmts - 1 was left behind. Either one keeps the file open, which is the symptom reported in #6639: a later H5Fcreate(H5F_ACC_TRUNC) on the same file fails with "unable to truncate a file which is already open". H5DSdetach_scale() reclaims both; attach now does too. test_attach_scale_id_leak() in hl/test/test_ds.c attaches one scale to four datasets and checks that only the scale is still open afterwards and that the file can then be truncated. Reverting either fix fails it: without the H5Dclose, 7 dataset IDs are open instead of 1; without the reclaims, the truncate fails. The test needs the new-reference path, which is reachable only with H5_DIMENSION_SCALES_WITH_NEW_REF or behind a connector whose terminal connector is not native -- H5VLobject_is_native() compares the terminal class, so stacking the pass-through connector over the native one does not reach it. The test skips itself on the old-reference path. Comments and documentation that described the path as "non-native VOL connector" or "only a pass-through VOL connector" are corrected to say that. -DHDF5_DIMENSION_SCALES_NEW_REF=ON was itself a no-op: the option was declared in hl/CMakeLists.txt, a child scope the top-level H5pubconf.h configure_file() never saw, so H5_DIMENSION_SCALES_WITH_NEW_REF was never defined and libhdf5.settings reported nothing. Moved to CMakeBuildOptions.cmake, which is included before src/ and hl/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Address review comments on dimension scale option and CHANGELOG Keep only option() and mark_as_advanced() for HDF5_DIMENSION_SCALES_NEW_REF in CMakeBuildOptions.cmake and move the H5_DIMENSION_SCALES_WITH_NEW_REF define back to hl/, set in the parent scope so it reaches H5pubconf.h. Drop the DIMENSION_SCALES_WITH_NEW_REF variable and have H5build_settings.cmake.c.in use the option value directly, as the other configuration files already do since #6617. Say "old-style reference path" in the CHANGELOG entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 participants
Fix find_dependency() calls so that PRIVATE-linked libraries are only propagated as transitive link requirements for static library targets
Add missing find_dependency() calls for some PRIVATE-linked libraries
Fix issue where parallel-enabled HDF5 can fail to locate MPI Fortran support, even when HDF5 Fortran support isn't requested
Set HDF5_LIB_TYPE to only shared or static, depending on requested library type, rather than a list that could include both shared and static
Fix HDF5_LIB_TYPE being undefined when components are specified, but shared/static isn't
Reduce scope of modifications to CMAKE_MODULE_PATH so changes aren't propagated to consuming projects
Add check for both shared and static libraries being requested and fail if so
Remove enable_language() call in favor of checking enabled languages and failing if required language isn't enabled
Add missing CMake variable for digitally signed plugins feature
Fix CMake variable for HDF5_DIMENSION_SCALES_NEW_REF option