Skip to content

Install ZFP filter shared library to lib/plugin instead of plugin#270

Merged
lrknox merged 5 commits into
HDFGroup:masterfrom
lrknox:fix-libh5zzfp-install-dest
Jul 17, 2026
Merged

Install ZFP filter shared library to lib/plugin instead of plugin#270
lrknox merged 5 commits into
HDFGroup:masterfrom
lrknox:fix-libh5zzfp-install-dest

Conversation

@lrknox

@lrknox lrknox commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

H5Z-ZFP/src/CMakeLists.txt (vendored via subtree) hardcodes the shared plugin's install destination to plugin, instead of following this project's lib/plugin convention. Relocate the installed file at install time so libh5zzfp.so lands in /lib/plugin like every other filter, without patching the vendored subtree.

H5Z-ZFP/src/CMakeLists.txt (vendored via subtree) hardcodes the shared
plugin's install destination to plugin, instead of following this
project's lib/plugin convention. Relocate the installed file at
install time so libh5zzfp.so lands in /lib/plugin like
every other filter, without patching the vendored subtree.
@lrknox
lrknox marked this pull request as draft July 11, 2026 14:05
@lrknox lrknox self-assigned this Jul 11, 2026
@lrknox

lrknox commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator Author

Additional fix needed for a packaging test failure on ubuntu.

CPack (e.g. the DEB generator used by the Ubuntu CI job) stages
installs into  while leaving CMAKE_INSTALL_PREFIX at its
literal configured value. install(TARGETS)/install(FILES) handle this
automatically via the DESTDIR-aware file(INSTALL), but the plain
file(MAKE_DIRECTORY)/file(RENAME) calls added to relocate libh5zzfp.so
did not, so packaging tried to create/rename outside the staging tree
and failed with 'No such file or directory'. Prepend {DESTDIR} by
hand to match.
@lrknox

lrknox commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator Author

All tests now pass.

@lrknox
lrknox marked this pull request as ready for review July 11, 2026 14:52
Comment thread ZFP/CMakeLists.txt Outdated
file(RENAME
\"\${_h5zfp_destdir}/plugin/$<TARGET_FILE_NAME:h5z_zfp_shared>\"
\"\${_h5zfp_destdir}/${H5ZFP_INSTALL_LIB_DIR}/$<TARGET_FILE_NAME:h5z_zfp_shared>\")
")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the file renaming logic, this seems quite a bit like hacking around the ZFP code and also doesn't seem particularly maintainable; changing the installation prefix should have been as easy as setting a CMake variable before the add_subdirectory call. If that's not the case, we should be able to make a PR to the ZFP code so that's possible.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

claude says that because "plugin" is a literal string in these lines 62 - 64 of ZFP/H5Z-ZFP/src/CMakeLists.txt:
install(TARGETS h5z_zfp_shared
RUNTIME DESTINATION plugin
ARCHIVE DESTINATION plugin
LIBRARY DESTINATION plugin)
setting a variable beforehand won't have any effect. We'll need a PR to change "plugin" to a variable in those lines in src/CMakeLists.txt in the ZFP code?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think the ZFP code should allow overriding the install destination. If GNUInstallDirs is followed, then it should probably be ${CMAKE_INSTALL_LIBDIR}/plugin by default anyway, unless the project wants to keep $prefix/plugin as the default.

lrknox and others added 3 commits July 14, 2026 15:43
…ting after install

H5Z-ZFP/src/CMakeLists.txt now honors H5Z_ZFP_PLUGIN_INSTALL_DIR (default
"plugin", unchanged for standalone builds) instead of hardcoding the shared
plugin's install destination. The wrapper sets it to H5ZFP_INSTALL_LIB_DIR
before add_subdirectory(), replacing the install(CODE ...) rename/DESTDIR
workaround from the previous commit. This mirrors a change proposed
upstream to LLNL/H5Z-ZFP.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@lrknox
lrknox requested a review from jhendersonHDF July 17, 2026 12:43
@lrknox
lrknox merged commit bf50108 into HDFGroup:master Jul 17, 2026
32 of 35 checks passed
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