-
-
Notifications
You must be signed in to change notification settings - Fork 308
Rename the install config variables to indicate library state #5716
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
Conversation
Assuming that all variables ending with "_ENABLED" are library state indicators and not build options, this should be clearly stated in the release note and prominently where appropriate in INSTALL _CMake and the USING*CMake.txt files in release_docs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comment for HDF5Examples/C/H5FLT/CMakeLists.txt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR systematically renames HDF5 configuration variables across the CMake build system to better reflect library state rather than build options. The change addresses issue #5713 by transforming variable names from patterns like HDF5_BUILD_*
and HDF5_ENABLE_*
to HDF5_SUPPORTS_*
to clarify that these variables indicate the capabilities of an installed HDF5 library, not configuration options for building it.
The core template file config/install/hdf5-config.cmake.in
is updated to define variables with the new naming convention (e.g., HDF5_SUPPORTS_FORTRAN
, HDF5_SUPPORTS_PARALLEL
, HDF5_SUPPORTS_ZLIB_SUPPORT
). This template is consumed by external projects using CMake's find_package(HDF5)
mechanism to determine what features are available in the installed library.
The change propagates throughout the HDF5Examples directory, updating conditional compilation logic in CMakeLists.txt files and source file configurations. A backward compatibility macro EXTERNAL_HDF5_STATUS
is introduced in config/examples/HDF5AsSubdirMacros.cmake
to provide conversion between old and new variable names, helping smooth the transition for existing projects.
The refactoring improves semantic clarity by distinguishing between build-time configuration options (what users can set) and runtime library capabilities (what features are available in an installed library). This is particularly important for the CMake configuration system where downstream projects need to understand what capabilities exist in the HDF5 installation they're linking against.
PR Description Notes:
- There's a discrepancy between the PR description (which mentions
_ENABLED
suffixes) and the actual implementation (which usesSUPPORTS_
prefixes) - A syntax error exists on line 211 of
HDF5Examples/CMakeLists.txt
with an extra closing brace
Confidence score: 1/5
- This PR contains critical naming inconsistencies that will likely cause build failures across the HDF5Examples project
- Score reflects significant discrepancy between PR description (
_ENABLED
suffixes) and actual implementation (SUPPORTS_
prefixes), plus a syntax error - Multiple files need immediate attention due to inconsistent variable naming patterns that could break CMake configuration
24 files reviewed, 26 comments
) | ||
|
||
if (HDF5_ENABLE_ZLIB_SUPPORT) | ||
if (HDF5_SUPPORTS_ZLIB_SUPPORT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Variable name inconsistent with PR convention. Should be HDF5_ZLIB_SUPPORT_ENABLED
to match the _ENABLED
suffix pattern used in other files.
if (HDF5_SUPPORTS_ZLIB_SUPPORT) | |
if (HDF5_ZLIB_SUPPORT_ENABLED) |
endif () | ||
|
||
if (HDF5_ENABLE_SZIP_SUPPORT) | ||
if (HDF5_SUPPORTS_SZIP_SUPPORT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Variable name inconsistent with PR convention. Should be HDF5_SZIP_SUPPORT_ENABLED
to match the _ENABLED
suffix pattern used in other files.
if (HDF5_SUPPORTS_SZIP_SUPPORT) | |
if (HDF5_SZIP_SUPPORT_ENABLED) |
|
||
# detect whether the encoder is present. | ||
if (${HDF5_ENABLE_ZLIB_SUPPORT}) | ||
if (${HDF5_SUPPORTS_ZLIB_SUPPORT}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Variable name inconsistency: should be HDF5_ZLIB_SUPPORT_ENABLED
to match the naming pattern used in C_sourcefiles.cmake and the broader refactoring effort
if (${HDF5_SUPPORTS_ZLIB_SUPPORT}) | |
if (${HDF5_ZLIB_SUPPORT_ENABLED}) |
endif () | ||
|
||
if (${HDF5_ENABLE_SZIP_SUPPORT}) | ||
if (${HDF5_SUPPORTS_SZIP_SUPPORT}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Variable name inconsistency: should be HDF5_SZIP_SUPPORT_ENABLED
to match the naming pattern used in C_sourcefiles.cmake and the broader refactoring effort
if (${HDF5_SUPPORTS_SZIP_SUPPORT}) | |
if (${HDF5_SZIP_SUPPORT_ENABLED}) |
) | ||
|
||
if (HDF5_ENABLE_ZLIB_SUPPORT) | ||
if (HDF5_SUPPORTS_ZLIB_SUPPORT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Variable name inconsistency: should be HDF5_ZLIB_SUPPORT_ENABLED
to match the pattern used in other files in this PR
if (HDF5_SUPPORTS_ZLIB_SUPPORT) | |
if (HDF5_ZLIB_SUPPORT_ENABLED) |
HDF5Examples/C/H5G/CMakeLists.txt
Outdated
#endif () | ||
|
||
if (HDF5_BUILD_TOOLS) | ||
if (HDF5_SUPPORTS_TOOLS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Variable name inconsistency: should be HDF5_TOOLS_ENABLED
not HDF5_SUPPORTS_TOOLS
to match the PR pattern
if (HDF5_SUPPORTS_TOOLS) | |
if (HDF5_TOOLS_ENABLED) |
HDF5Examples/C/H5G/CMakeLists.txt
Outdated
) | ||
set_tests_properties (${EXAMPLE_VARNAME}_${testname} PROPERTIES DEPENDS ${EXAMPLE_VARNAME}_${testname}-clearall) | ||
if (HDF5_BUILD_TOOLS) | ||
if (HDF5_SUPPORTS_TOOLS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Variable name inconsistency: should be HDF5_TOOLS_ENABLED
not HDF5_SUPPORTS_TOOLS
if (HDF5_SUPPORTS_TOOLS) | |
if (HDF5_TOOLS_ENABLED) |
HDF5Examples/C/H5D/CMakeLists.txt
Outdated
#endif () | ||
|
||
if (HDF5_BUILD_TOOLS) | ||
if (HDF5_SUPPORTS_TOOLS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Inconsistent naming pattern: other variables in this PR use HDF5_*_ENABLED
suffix, but this uses HDF5_SUPPORTS_TOOLS
. Verify this naming convention is intentional across the codebase.
HDF5Examples/C/H5G/CMakeLists.txt
Outdated
-P "${H5EX_RESOURCES_DIR}/runTest.cmake" | ||
) | ||
if (HDF5_BUILD_TOOLS) | ||
if (HDF5_SUPPORTS_TOOLS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Variable name inconsistency: should be HDF5_TOOLS_ENABLED
not HDF5_SUPPORTS_TOOLS
if (HDF5_SUPPORTS_TOOLS) | |
if (HDF5_TOOLS_ENABLED) |
HDF5Examples/C/H5G/CMakeLists.txt
Outdated
) | ||
set_tests_properties (${EXAMPLE_VARNAME}_${testname} PROPERTIES DEPENDS ${EXAMPLE_VARNAME}_${testname}-clearall) | ||
if (HDF5_BUILD_TOOLS) | ||
if (HDF5_SUPPORTS_TOOLS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Variable name inconsistency: should be HDF5_TOOLS_ENABLED
not HDF5_SUPPORTS_TOOLS
if (HDF5_SUPPORTS_TOOLS) | |
if (HDF5_TOOLS_ENABLED) |
ENABLED was the old change and SUPPORTS is the change agreed by discussion |
config/install/hdf5-config.cmake.in
Outdated
set (${HDF5_PACKAGE_NAME}_BUILD_CPP_LIB @HDF5_BUILD_CPP_LIB@) | ||
set (${HDF5_PACKAGE_NAME}_BUILD_JAVA @HDF5_BUILD_JAVA@) | ||
set (${HDF5_PACKAGE_NAME}_FORTRAN_ENABLED @HDF5_BUILD_FORTRAN@) | ||
set (${HDF5_PACKAGE_NAME}_CPP_LIB_ENABLED @HDF5_BUILD_CPP_LIB@) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about removing _LIB_
for CPP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ENABLED is the old form
config/install/hdf5-config.cmake.in
Outdated
set (${HDF5_PACKAGE_NAME}_BUILD_HL_LIB @HDF5_BUILD_HL_LIB@) | ||
set (${HDF5_PACKAGE_NAME}_BUILD_SHARED_LIBS @H5_ENABLE_SHARED_LIB@) | ||
set (${HDF5_PACKAGE_NAME}_BUILD_STATIC_LIBS @H5_ENABLE_STATIC_LIB@) | ||
set (${HDF5_PACKAGE_NAME}_HL_LIB_ENABLED @HDF5_BUILD_HL_LIB@) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about renaming _LIB_
to _LIBS_
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ENABLED is the old form
config/install/hdf5-config.cmake.in
Outdated
set (${HDF5_PACKAGE_NAME}_ENABLE_SZIP_SUPPORT @HDF5_ENABLE_SZIP_SUPPORT@) | ||
set (${HDF5_PACKAGE_NAME}_ENABLE_SZIP_ENCODING @HDF5_ENABLE_SZIP_ENCODING@) | ||
set (${HDF5_PACKAGE_NAME}_ZLIB_SUPPORT_ENABLED @HDF5_ENABLE_ZLIB_SUPPORT@) | ||
set (${HDF5_PACKAGE_NAME}_SZIP_SUPPORT_ENABLED @HDF5_ENABLE_SZIP_SUPPORT@) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about renaming SZIP_SUPPORT_
to SZIP_DECODING_
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ENABLED is the old form
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@byrnHDF , please see my 3 comments.
The intention was not to change the name more then necessary - easy substitution of ENABLE/BUILD to SUPPORTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This review covers only the changes made since the last review (commit 4879893), not the entire PR.
This incremental update continues the systematic renaming of HDF5 configuration variables, but unfortunately introduces critical inconsistencies in the naming convention that could break builds and cause configuration mismatches across the codebase.
Major Issues Identified:
The most significant problem is the complete inconsistency in variable naming patterns across files:
- PR Description Claims: Variables should use
HDF5_*_ENABLED
suffix pattern (e.g.,HDF5_TOOLS_ENABLED
) - Template File Uses:
HDF5_PROVIDES_*
pattern (e.g.,HDF5_PROVIDES_TOOLS
) inconfig/install/hdf5-config.cmake.in
- Many Files Use:
HDF5_PROVIDES_*
pattern instead of the claimed_ENABLED
suffix - Critical Bug: Line 97 in
hdf5-config.cmake.in
still references old variable nameHDF5_THREADS_ENABLED
instead of newHDF5_PROVIDES_THREADS
This creates a three-way mismatch between documentation, implementation, and actual usage that will cause runtime configuration errors.
The Changes:
This update systematically renames variables across 20+ CMake files in the HDF5Examples directory and configuration templates. The goal is to change variables from sounding like build "options" (e.g., HDF5_BUILD_TOOLS
, HDF5_ENABLE_PARALLEL
) to reflecting library "state" (e.g., what features the installed library provides).
How It Fits the Codebase:
These variables are used by downstream applications via CMake's find_package(HDF5)
mechanism to determine what capabilities are available in the installed HDF5 library. The renaming makes the API more semantically correct by clearly indicating these represent library capabilities rather than configurable options.
Important Files Changed
Click to expand table
Filename | Score | Overview |
---|---|---|
config/install/hdf5-config.cmake.in |
1/5 | CRITICAL BUG: Line 97 references undefined variable HDF5_THREADS_ENABLED instead of HDF5_PROVIDES_THREADS , will cause runtime error |
HDF5Examples/FORTRAN/H5D/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_TOOLS instead of expected HDF5_TOOLS_ENABLED , inconsistent with PR documentation |
HDF5Examples/FORTRAN/H5D/Fortran_sourcefiles.cmake |
2/5 | Uses HDF5_PROVIDES_*_SUPPORT pattern instead of documented HDF5_*_SUPPORT_ENABLED pattern |
HDF5Examples/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_* pattern instead of documented _ENABLED suffix, plus syntax error fix on line 211 |
HDF5Examples/config/cmake/HDFExampleMacros.cmake |
2/5 | Inconsistent with PR description - uses _PROVIDES suffix instead of _ENABLED suffix |
HDF5Examples/FORTRAN/H5G/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_TOOLS instead of expected HDF5_TOOLS_ENABLED pattern |
HDF5Examples/C/Perf/CMakeLists.txt |
2/5 | NAMING INCONSISTENCY: Uses HDF5_PROVIDES_TOOLS instead of HDF5_ENABLED_TOOLS pattern |
HDF5Examples/C/H5FLT/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_TOOLS instead of expected HDF5_TOOLS_ENABLED , inconsistent with PR pattern |
HDF5Examples/FORTRAN/HL/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_TOOLS instead of expected HDF5_TOOLS_ENABLED , creates naming inconsistency |
HDF5Examples/C/H5G/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_TOOLS instead of expected HDF5_TOOLS_ENABLED , deviates from PR naming pattern |
config/examples/HDF5AsSubdirMacros.cmake |
2/5 | Good backward compatibility macro but contains variable name inconsistencies and potential typos |
HDF5Examples/CXX/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_HL_LIB instead of expected HDF5_ENABLED_HL_LIB , inconsistent with PR description |
HDF5Examples/C/H5D/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_TOOLS instead of expected HDF5_TOOLS_ENABLED , creates naming inconsistency |
HDF5Examples/C/H5VDS/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_TOOLS instead of expected HDF5_TOOLS_ENABLED , inconsistent with PR pattern |
HDF5Examples/C/H5D/C_sourcefiles.cmake |
2/5 | Uses HDF5_PROVIDES_*_SUPPORT instead of expected HDF5_*_SUPPORT_ENABLED pattern |
HDF5Examples/C/HL/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_TOOLS instead of expected HDF5_TOOLS_ENABLED , inconsistent with PR pattern |
HDF5Examples/FORTRAN/H5T/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_TOOLS instead of expected HDF5_TOOLS_ENABLED , inconsistent with PR naming |
HDF5Examples/C/H5PAR/C_sourcefiles.cmake |
2/5 | Uses HDF5_PROVIDES_SUBFILING_VFD instead of expected HDF5_SUBFILING_VFD_ENABLED , naming inconsistency |
HDF5Examples/C/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_* pattern which may be correct per template, but inconsistent with PR description |
HDF5Examples/FORTRAN/H5PAR/Fortran_sourcefiles.cmake |
2/5 | Uses HDF5_PROVIDES_SUBFILING_VFD instead of expected HDF5_SUBFILING_VFD_ENABLED pattern |
HDF5Examples/C/H5T/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_TOOLS instead of expected HDF5_TOOLS_ENABLED , creates naming inconsistency |
HDF5Examples/FORTRAN/CMakeLists.txt |
2/5 | Uses HDF5_PROVIDES_* pattern instead of expected _ENABLED suffix, inconsistent with PR description |
release_docs/RELEASE.txt |
3/5 | Documentation describes HDF5_PROVIDES_* pattern but PR claims HDF5_*_ENABLED , plus macro name discrepancy |
HDF5Examples/JAVA/H5D/Java_sourcefiles.cmake |
4/5 | Correctly follows the pattern established in hdf5-config.cmake.in template using HDF5_PROVIDES_* |
24 files reviewed, 7 comments
#endif () | ||
|
||
if (HDF5_BUILD_TOOLS) | ||
if (HDF5_PROVIDES_TOOLS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Inconsistent naming pattern: this file uses HDF5_PROVIDES_TOOLS
but the PR summary indicates other files use HDF5_TOOLS_ENABLED
. This could cause configuration mismatches.
if (EXISTS "${H5EXAMPLES_SOURCE_DIR}/FORTRAN" AND IS_DIRECTORY "${H5EXAMPLES_SOURCE_DIR}/FORTRAN") | ||
option (H5EX_BUILD_FORTRAN "Build examples FORTRAN support" OFF) | ||
if (H5EX_BUILD_FORTRAN AND HDF5_BUILD_FORTRAN) | ||
if (H5EX_BUILD_FORTRAN AND HDF5_PROVIDES_FORTRAN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Inconsistent naming pattern: this file uses HDF5_PROVIDES_FORTRAN
but the PR description indicates the pattern should be HDF5_*_ENABLED
(e.g., HDF5_FORTRAN_ENABLED
)
if (EXISTS "${H5EXAMPLES_SOURCE_DIR}/C/H5FLT" AND IS_DIRECTORY "${H5EXAMPLES_SOURCE_DIR}/C/H5FLT") | ||
option (H5EX_BUILD_FILTERS "Build examples PLUGIN filter support" OFF) | ||
if (H5EX_BUILD_FILTERS AND HDF5_ENABLE_PLUGIN_SUPPORT) | ||
if (H5EX_BUILD_FILTERS AND HDF5_PROVIDES_PLUGIN_SUPPORT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Inconsistent naming pattern: this file uses HDF5_PROVIDES_PLUGIN_SUPPORT
but the PR description indicates it should be HDF5_PLUGIN_SUPPORT_ENABLED
if (H5EX_BUILD_FORTRAN AND HDF5_PROVIDES_FORTRAN) | ||
add_subdirectory (FORTRAN) | ||
endif () | ||
if (H5EX_BUILD_JAVA AND HDF5_BUILD_JAVA) | ||
if (H5EX_BUILD_JAVA AND HDF5_PROVIDES_JAVA) | ||
add_subdirectory (JAVA) | ||
endif () | ||
if (H5EX_BUILD_CXX AND HDF5_BUILD_CPP_LIB) | ||
if (H5EX_BUILD_CXX AND HDF5_PROVIDES_CPP_LIB) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: All variable names in this section use HDF5_PROVIDES_*
pattern which is inconsistent with the HDF5_*_ENABLED
pattern described in the PR and used in other files
|
||
#-- Add High Level Examples | ||
if (H5EX_BUILD_HL AND HDF5_BUILD_HL_LIB) | ||
if (H5EX_BUILD_HL AND HDF5_PROVIDES_HL_LIB) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: Variable name inconsistent with PR naming convention. PR description indicates using _ENABLED
suffix but this uses _PROVIDES
. Should be HDF5_ENABLED_HL_LIB
to match the established pattern.
if (H5EX_BUILD_HL AND HDF5_PROVIDES_HL_LIB) | |
if (H5EX_BUILD_HL AND HDF5_ENABLED_HL_LIB) |
|
||
The variables used in hdf5-config.cmake to indicate what options were used to | ||
build the installed library have been renamed. All HDF5_BUILD/ENABLE_{feature} | ||
variables are now HDF5_PROVIDES_{feature}. This more clearly indicates that these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Critical naming inconsistency: Documentation claims variables are renamed to HDF5_PROVIDES_{feature}
but PR description and other files use HDF5_*_ENABLED
pattern. This will confuse users about the actual variable names.
) | ||
|
||
if (HDF5_ENABLE_SUBFILING_VFD) | ||
if (HDF5_PROVIDES_SUBFILING_VFD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Variable name inconsistency across the PR: while the PR description claims the pattern should be HDF5_*_ENABLED
suffix, this implementation uses HDF5_PROVIDES_SUBFILING_VFD
. This conflicts with the stated naming convention and could cause integration issues.
Fixes #5713 so that the variable names sound more like a "state" instead of like an "option". This hdf5-config file reflects the status of the options used to build the library for use by applications.
After discussion decided that the pattern should be HDF5_SUPPORTS_{feature} then suggested that can be confusing with ones that end in SUPPORT - we will go with HDF5_PROVIDES_feature