Skip to content
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

cmake exported target uses undefined hdf5::hdf5 imported target #1444

Closed
jschueller opened this issue Jul 22, 2019 · 17 comments · Fixed by #2890
Closed

cmake exported target uses undefined hdf5::hdf5 imported target #1444

jschueller opened this issue Jul 22, 2019 · 17 comments · Fixed by #2890
Assignees
Milestone

Comments

@jschueller
Copy link
Contributor

jschueller commented Jul 22, 2019

on mingw (win32), compiling netcdf with cmake, with a cmake-compiled hdf5 using its exported target (module mode, not config mode), results in an installed netcdf-targets.cmake that contains the hdf5::hdf5 target (and hdf5::hdf5_hl) in the INTERFACE_LINK_LIBRARIES properties.

But then, when using netcdf in module mode (with find_package(netCDF)) these hdf5 targets are undefined (the hdf5 cmake config is not included), and cmake is not happy:

-- Configuring done
CMake Error at CMake/vtkModule.cmake:2968 (add_library):
  Target "IONetCDF" links to target "hdf5::hdf5_hl-shared" but the target was not found

We first encountered this bug in vtk: https://gitlab.kitware.com/vtk/vtk/issues/17637

A workaround is to provide manually hdf5 paths when configuring netcdf, doing so results in actual library paths being written to the exported targets file instead of hdf5 targets.

@mathstuf
Copy link
Contributor

Well, netCDFConfig.cmake should do a find_package(HDF5) and may provide hints to where it found it at build-time via the HDF5_DIR and/or HDF5_ROOT variables.

@WardF
Copy link
Member

WardF commented Jul 23, 2019

The netCDFConfig.cmake file is generated automatically; let me take a look to see if I can find out how it is properly modified/updated.

@WardF WardF self-assigned this Jul 23, 2019
@WardF WardF added this to the 4.7.1 milestone Jul 23, 2019
@jschueller
Copy link
Contributor Author

jschueller commented Jul 24, 2019

@WardF
Copy link
Member

WardF commented Aug 15, 2019

Coming back to revisit 4.7.1 issues in the lead-up to the release. @jschueller thanks for the pointer.

@WardF WardF pinned this issue Aug 15, 2019
@WardF WardF modified the milestones: 4.7.1, 4.7.2 Oct 15, 2019
@WardF WardF modified the milestones: 4.7.2, 4.7.3 Oct 28, 2019
@jsharpe
Copy link
Contributor

jsharpe commented Dec 18, 2019

Note that a similar issue is with missing a dependency on MPI as well.
This commit:
bc2a3ba introduces a call to find_package(MPI) on MSVC but surely this should be done for all platforms that use mpi as ncdispatch.h includes mpi.h if HDF5_PARALLEL or USE_PNETCDF is defined.

@jsharpe
Copy link
Contributor

jsharpe commented Dec 19, 2019

I think the issue here is that cmake's current FIndHDF5.cmake file doesn't define any imported targets (the hdf5::hdf5 target) but hdf5 build with cmake defines a cmake config file that does provide this target. See https://gitlab.kitware.com/cmake/cmake/issues/18560 for the upstream discussions regarding this.

So I suspect the best route to fixing this is to ignore the hdf5 provided config files by passing NO_MODULE to the find_package(HDF5) calls?

@mathstuf
Copy link
Contributor

VTK has a patched FindHDF5 that contains imported targets. It's been working pretty well there, but every time I breathe on it, new bugs crop up :) . I'll resurrect my MR to put it in over the holidays.

@jsharpe
Copy link
Contributor

jsharpe commented Dec 19, 2019

@mathstuf Its actually in the context of VTK and the superbuild that I was seeing issues with this.
I've had to define -DHDF5_NO_FIND_PACKAGE_CONFIG_FILE:BOOL=ON on dependencies of hdf5 to get it to work correctly for the netcdf build but then I have to manually define HDF5_HL_LIBRARIES and HDF5_C_LIBRARY_hdf5_hl to get it to pass the configuration step in paraview / VTK.

@mathstuf
Copy link
Contributor

That's probably with VTK 8.2 / ParaView 5.6 and older (where the build system choked on imported targets). Now with VTK 8.90 / ParaView 5.7 and newer, imported targets are heavily preferred (non-targets are usually warned about if it comes to the attention of the new build system infrastructure).

@jschueller
Copy link
Contributor Author

hdf5 logic is complicated #877

@WardF
Copy link
Member

WardF commented Apr 28, 2020

@jschueller Indeed.

@WardF
Copy link
Member

WardF commented Apr 28, 2020

Addressing this now before it slips under the radar again.

@jschueller
Copy link
Contributor Author

hi @WardF with your patch now I get just "hdf5_hl-shared" instead of the previous "hdf5::hdf5_hl-shared", but it's not right yet ;)

@WardF
Copy link
Member

WardF commented Apr 29, 2020

Yes, the logic for HDF5 is tricky. Thanks for checking in, I was going to ask, once I had something that looked like a fix. I imagine I'll need to pattern something off of the hdf5 logic found in the netcdf-c top-level CMakeLists.txt file. It's good to have something to do :).

@WardF WardF modified the milestones: 4.8.0, 4.8.2 Aug 30, 2021
@WardF WardF removed this from the 4.8.2 milestone Jun 15, 2022
@WardF WardF added this to the 4.9.1 milestone Jun 15, 2022
@WardF WardF modified the milestones: 4.9.1, 4.9.2 Feb 13, 2023
@WardF WardF modified the milestones: 4.9.2, 4.9.3 May 16, 2023
@Chrismarsh
Copy link

Is there progress on this? I see that a netcdf-c cmake built library has a pkgconfig file that references libraries that don't exist (Libs.private: -lhdf5_hl-shared -lhdf5-shared)

Name: netCDF
Description: NetCDF Client Library for C
URL: https://www.unidata.ucar.edu/netcdf
Version: 4.9.2
Libs: -L${libdir} -lnetcdf
Libs.private: -lhdf5_hl-shared -lhdf5-shared -lm -lz -lzstd -lbz2 -lcurl -lpnetcdf -lxml2
Cflags: -I${includedir}

@WardF
Copy link
Member

WardF commented Feb 26, 2024

Wow this has really lingered. My sincere, embarrassed apologies. @K20shores this feels like something that might fit in with the work we're doing over at #2847?

@K20shores
Copy link
Contributor

@WardF, yes, I think the updates we've added may fix this. I'll have to check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants