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

[hdf5] Link zlib using the ZLIB::ZLIB target #18905

Closed
wants to merge 2 commits into from

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Jul 10, 2021

  • What does your PR fix?

    This PR fixes linking of the debug version of hdf5. Before the changes, it was linking to the release version of libz, and this was passed on via the exported cmake config files. This PR resolves the issue by always using the imported target ZLIB::ZLIB so that config resolution can happen as needed.
    This was found when validating the extended gdal cmake wrapper/the linker flags used for a consuming executable. Before the change, it contained both debug and release version of libz:

    ...
    vcpkg_installed/x64-linux/debug/lib/libhdf5_hl_debug.a
    vcpkg_installed/x64-linux/debug/lib/libhdf5_debug.a
    vcpkg_installed/x64-linux/lib/libz.a
    ...
    vcpkg_installed/x64-linux/debug/lib/libjpeg.a
    vcpkg_installed/x64-linux/debug/lib/libz.a
    ...
    
  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all, no.

  • Does your PR follow the maintainer guide?

    yes.

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    yes.

@Neumann-A
Copy link
Contributor

it contained both debug and release version of libz:

Has nothing to to with hdf5. But has everything to do with FindZLIB which only returns a single zlib library in ZLIB_LIBRARIES instead of debug/optimized keywords on linux/osx.
So please add a wrapper to zlib instead which fixes that issue.

I am also pretty sure that the ZLIB target ist not correctly setup on linux (e.g. no _DEBUG and _RELEASE IMPORTED_LOCATION)

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 10, 2021

I am also pretty sure that the ZLIB target ist not correctly setup on linux (e.g. no _DEBUG and _RELEASE IMPORTED_LOCATION)

Well, it seems to be okay since cmake 3.4:
Kitware/CMake@11097f5

I'm testing on Ubuntu 18.04 (CMake 3.10).

@Neumann-A
Copy link
Contributor

Neumann-A commented Jul 10, 2021

@dg0yt You don't need to test it. Just look at the module: It will have only _RELEASE set due to it being unable to differentiate between RELEASE and DEBUG and debug only searching for suffixed names. As such a wrapper needs to set the ZLIB_LIBRARY_(RELEASE|DEBUG) cache vars before the find_package call. This will automatically fix ZLIB_LIBRARIES and the linkage in the hdf5 targets.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 11, 2021

You don't need to test it.

If nobody tests, I don't need to wonder why so many stuff is broken. vcpkg CI just scratches the surface.

@dg0yt dg0yt mentioned this pull request Jul 11, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 11, 2021

This PR should become obsolete with #18914.

@dg0yt dg0yt marked this pull request as draft July 11, 2021 20:54
@PhoebeHui PhoebeHui added the category:port-bug The issue is with a library, which is something the port should already support label Jul 12, 2021
@PhoebeHui
Copy link
Contributor

@dg0yt, thanks for your contribution! ping me if this PR ready for review.

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 6, 2021

With #18914 merged, this PR is obsolete now. hdf5-targets.cmake shows proper zlib dependencies (x64-mingw-static):

set_target_properties(hdf5::hdf5-static PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "_GNU_SOURCE;_FILE_OFFSET_BITS=64;_LARGEFILE64_SOURCE;_LARGEFILE_SOURCE"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "\$<LINK_ONLY:m>;\$<LINK_ONLY:ws2_32>;\$<LINK_ONLY:wsock32>;\$<\$<NOT:\$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/libzlib.a>;\$<\$<CONFIG:DEBUG>:${_IMPORT_PREFIX}/debug/lib/libzlibd.a>;\$<LINK_ONLY:szip-static>;\$<LINK_ONLY:szip-static>;\$<LINK_ONLY:\$<\$<BOOL:OFF>:>>;\$<\$<NOT:\$<PLATFORM_ID:Windows>>:>"
)

@dg0yt dg0yt closed this Aug 6, 2021
@dg0yt dg0yt deleted the hdf5-zlib branch December 12, 2021 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants