conflict with exported CMake target for HDF5 with ITK_USE_SYSTEM_HDF5=OFF

[KrisThielemans]

Description

When using ITK_USE_SYSTEM_HDF5=OFF, ITK exports hdf5_cpp-shared and hdf5-shared (or the non-shared) targets but this is incorrect for 2 reasons:

• they point to the ITK HDF5 libraries which are not compatible with not normal HDF5 libraries
• they cause conflicts when using find_package(HDF5) after find_package(ITK)

In my opinion, those targets should be renamed to itkhdf*.

Steps to Reproduce

The following CMakeLists.txt illustrates both problems

make_minimum_required(VERSION 3.9.0)
PROJECT(TEST)
include(CMakePrintHelpers)
find_package(ITK)
cmake_print_properties(
    TARGETS hdf5_cpp-shared hdf5-shared
    PROPERTIES IMPORTED_LOCATION_RELEASE IMPORTED_LOCATION_RELWITHDEBINFO IMPORTED_LOCATION_DEBUG 
          IMPORTED_LOCATION 
    )
find_package(HDF5 COMPONENTS C REQUIRED)

Expected behavior

This should just find an "external" HDF5

Actual behavior

After building/installing ITK master and HDF5 1.13.1, CMake outputs

Properties for TARGET hdf5_cpp-shared:
   hdf5_cpp-shared.IMPORTED_LOCATION_RELEASE = <NOTFOUND>
   hdf5_cpp-shared.IMPORTED_LOCATION_RELWITHDEBINFO = "C:/Users/krisf/INSTALL/bin/itkhdf5_cpp-shared-5.2.dll"
   hdf5_cpp-shared.IMPORTED_LOCATION_DEBUG = <NOTFOUND>
   hdf5_cpp-shared.IMPORTED_LOCATION = <NOTFOUND>

 Properties for TARGET hdf5-shared:
   hdf5-shared.IMPORTED_LOCATION_RELEASE = <NOTFOUND>
   hdf5-shared.IMPORTED_LOCATION_RELWITHDEBINFO = "C:/Users/krisf/Documents/INSTALL/bin/itkhdf5-shared-5.2.dll"
   hdf5-shared.IMPORTED_LOCATION_DEBUG = <NOTFOUND>
   hdf5-shared.IMPORTED_LOCATION = <NOTFOUND>

CMake Error at C:/Users/krisf/Documents/INSTALL/cmake/hdf5-targets.cmake:37 (message):
  Some (but not all) targets in this export set were already defined.

  Targets Defined: hdf5-shared;hdf5_cpp-shared

  Targets not yet defined:
  hdf5-static;mirror_server;mirror_server_stop;hdf5_cpp-static

Reproducibility

all the time.

Versions

Tested with ITK 5.2.1 and current master

Environment

Tested on Windows 10 with VC 2019, CMake 3.22.3, but this will happen everywhere.

[KrisThielemans]

Probably root-cause of microsoft/vcpkg#15502

[dzenanz]

@hjmjohnson @seanm do you agree with "those targets should be renamed to itkhdf*"?

@KrisThielemans PRs are welcome.

[seanm]

I'm not qualified to say, I know very little about cmake.

But I do observe that literally everything else in the lib folder when I install itk is prefixed with libitk, at least with the build options I use.

[dzenanz]

Since HDF5 has a rich CMake support, I think we try to just compile it. I think we do that because we think it can be used as a normal HDF5 build. I guess Kris feels differently?

Maybe find_package needs to be modified? @bradking do you have an opinion?

Or maybe it is just needed to have find_package(HDF5) before find_package(ITK), if the application developer intends to use another HDF5 build?

[bradking]

When ITK is built with bundled third-party libraries, we try to mangle their symbols to avoid conflicts during linking. See Modules/ThirdParty/HDF5/src/itkhdf5/src/itk_hdf5_mangle.h for example. For third-party libs other than HDF5, we also patch the CMakeLists.txt files to add the itk prefix to the target names. That's probably needed for HDF5 too.

[dzenanz]

We already do that for HDF5. I think the problem here are CMake target names.

[bradking]

Yes, that's why I mentioned the CMakeLists.txt files for our imported HDF5 code may need to be updated to add the itk prefix to target names.

[KrisThielemans]

@bradking , I cannot find where you wrote this.

Would this need some sed magic in https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/ThirdParty/HDF5/UpdateFromUpstream.sh?

After changing the tags, presumably we'd also need to change

ITK/Modules/ThirdParty/HDF5/CMakeLists.txt

Lines 80 to 84 in 9de2b57

	if(BUILD_SHARED_LIBS)
	set(ITKHDF5_LIBRARIES hdf5_cpp-shared hdf5-shared)
	else()
	set(ITKHDF5_LIBRARIES hdf5_cpp-static hdf5-static)
	endif()

[bradking]

I cannot find where you wrote this.

In #3315 (comment):

we also patch the CMakeLists.txt files to add the itk prefix to the target names. That's probably needed for HDF5 too.

Anyway, the UpdateFromUpstream.sh script probably won't be able to reliably find the right places to update. It currently prints out a reminder to manually update the mangling. We'd probably have to manually edit the CMakeLists.txt files from HDF5 to add the target name prefix too.

[dzenanz]

manually edit the CMakeLists.txt files from HDF5 to add the target name prefix

We do occasionally manually edit HDF5's source code, 1be1b17 is a recent example. Changing CMake target names should not be hard. That would prevent people from using bundled HDF5 as a normal HDF5 build. But I guess there aren't many who do that, so it wouldn't be a big loss.

Kris, do you mind making a PR?