CMake attempts to set properties on hdf5-static instead of hdf5-shared when BUILD_STATIC_LIBS is set to OFF

[rdebroiz]

Description

CMake attempts to properties on hdf5-static instead of hdf5-shared when BUILD_STATIC_LIBS is set to OFF

ITK/Modules/ThirdParty/HDF5/CMakeLists.txt

Line 84 in 15af3ae

if(BUILD_SHARED_LIBS)

The test to decide if it is shared or static is performed on BUILD_SHARED_LIBS, perhaps it should be done on BUILD_STATIC_LIBS ?

If one manually set BUILD_SHARED_LIBS to ON in the CMake cache, it fixes the issue.

Steps to Reproduce

1. Configure CMake with BUILD_STATIC_LIBS set to OFF
2. Get:

set_property could not find TARGET hdf5-static.  
Perhaps it has not yet been created.
Call Stack (most recent call first):
  CMake/ITKModuleMacros.cmake:464 (itk_module_target_name)
  Modules/ThirdParty/HDF5/src/CMakeLists.txt:106 (itk_module_target)

Versions

v5.4.0

Environment

CMake 3.29.6

[github-actions]

Thank you for contributing an issue! 🙏

Welcome to the ITK community! 🤗👋☀️

We are glad you are here and appreciate your contribution. Please keep in mind our community participation guidelines. 📜
Also, please check existing open issues and consider discussion on the ITK Discourse. 📖

This is an automatic message. Allow for time for the ITK community to be able to read the issue and comment on it.

[N-Dekker]

• For the record, the interesting if(BUILD_SHARED_LIBS) set(ITKHDF5_LIBRARIES hdf5_cpp-shared...) appears introduced with pull request COMP: Fix linking problems with external HDF5 #2247 commit df5f961, by @KrisThielemans, Jan 10, 2021.

[KrisThielemans]

I cannot see anything wrong with my PR 😄 . I have no idea what the interaction is between BUILD_SHARED_LIBS and BUILD_STATIC_LIBS. It seems a bit surprising that both exist (and are not the opposite of eachother).

[N-Dekker]

Thanks for double-checking @KrisThielemans 👍 I have to admit I don't understand the problem well enough 🤷

[rdebroiz]

Hello, To give more context:

I just wanted to recompile a fresh ITK build after I stopped using it for a few years, when I pressed configure for the first time, I got this:

Because I wanted a dynamic build and BUILD_STATIC_LIBS was just in front of me, I set it to OFF without concern for BUILD_SHARED_LIBS which is at this step actually already in the cache, but as an advanced variable.

The helper message was just "Build Static Libraries" I thought setting it to OFF would be enough for a dynamic build.

The idea of BUILD_STATIC_LIBS is to allow both dynamic and static build for hdf5 and openj2
It is declared here as an option in the hdf5 module:

ITK/Modules/ThirdParty/HDF5/src/itkhdf5/CMakeLists.txt

Lines 509 to 531 in 15af3ae

	#-----------------------------------------------------------------------------
	# Option to Build Shared and Static libs, default is both
	#-----------------------------------------------------------------------------
	option (BUILD_STATIC_LIBS "Build Static Libraries" ON)
	set (H5_ENABLE_STATIC_LIB NO)
	option (BUILD_SHARED_LIBS "Build Shared Libraries" ON)
	set (H5_ENABLE_SHARED_LIB NO)
	# ITK --start
	option (ONLY_SHARED_LIBS "Only Build Shared Libraries" ${BUILD_SHARED_LIBS})
	# ITK --stop
	mark_as_advanced (ONLY_SHARED_LIBS)
	if (BUILD_STATIC_LIBS)
	set (H5_ENABLE_STATIC_LIB YES)
	endif ()
	if (BUILD_SHARED_LIBS)
	set (H5_ENABLE_SHARED_LIB YES)
	endif ()
	# Force only shared libraries if all OFF
	if (NOT BUILD_STATIC_LIBS AND NOT BUILD_SHARED_LIBS)
	set (ONLY_SHARED_LIBS ON CACHE BOOL "Only Build Shared Libraries" FORCE)
	endif ()

There is a test to take care of both variable being set to OFF (my case), it will set a new variable ONLY_SHARED_LIBS to ON in order to force a dynamic build.
The problem is that this variable is not tested when hdf5 targets rare created.

The most conservative way to fix the problem is to add a test on ONLY_SHARED_LIBS

if (BUILD_SHARED_LIBS OR ONLY_SHARED_LIBS)

here:

ITK/Modules/ThirdParty/HDF5/src/itkhdf5/src/CMakeLists.txt

Line 1251 in 15af3ae

if (BUILD_SHARED_LIBS)

here:

ITK/Modules/ThirdParty/HDF5/src/itkhdf5/hl/src/CMakeLists.txt

Line 60 in 15af3ae

if (BUILD_SHARED_LIBS)

and here:

ITK/Modules/ThirdParty/HDF5/src/itkhdf5/c++/src/CMakeLists.txt

Line 107 in 15af3ae

if (BUILD_SHARED_LIBS)

IMHO, it would be more user-friendly to keep things specific to one module using variable specific for the module -- If it was me, I would remove the BUILD_STATIC_LIBS option and use H5_ENABLE_STATIC_LIB and H5_ENABLE_SHARED_LIB declared later on as option. Same thing for opnjp2 if it is needed.

[rdebroiz]

PS: I can do a PR with the conservative fix if you consider it relevant.

[dzenanz]

A PR with the conservative fix sounds good!

[dzenanz]

Ahh, but that would be a difference relative to upstream.

[dzenanz]

You would need to mark deviations with

 # ITK --start 
 ...
 # ITK --stop 

Think whether the additional maintenance is justified by convenience that change would bring, and how often that convenience would be exercised.

[dzenanz]

Maybe better would be a PR against upstream HDF5' relevant branches, and than updating ITK's bundled version of HDF5? But that is a lot more work.

[rdebroiz]

Maybe better would be a PR against upstream HDF5' relevant branches, and than updating ITK's bundled version of HDF5? But that is a lot more work.

Sounds like a lot of work indeed.
In addition, the bug only rises later during ITK's configuration (even though it seems to be an inconsistency in HDF5), when it tries to add properties on non-existing HDF5 targets. HDF5 maintainers might not accept the changes for a 1.12.4 version (the bundled version is 1.12.1 which I could not find in the HDF5 repo). The current version of HDF5 is 1.16.0, and it seems that changes occurred regarding thees things, it might be fixed already.

Think whether the additional maintenance is justified by convenience that change would bring, and how often that convenience would be exercised.

TBH I don't feel qualified to decide about that. If I'm the only one stepping on this, it might be easier to let things as they are.

[rdebroiz]

I'd had to set BUILD_SHARED_LIBS to ON at some point anyway, perhaps the right thing to do is to set it visible in the cache (not mark as advanced) for convenience ? Perhaps also on the contrary mark as advance BUILD_STATIC_LIBS and set it to a sensible value in Modules/ThirdParty/HDF5/CMakeLists.txt ?