search

tbeu / matio

MATLAB MAT File I/O Library
https://matio.sourceforge.io
BSD 2-Clause "Simplified" License
330 stars 97 forks source link

Apply vcpkg patch #245

Closed tbeu closed 3 months ago

tbeu commented 3 months ago

This applies the vcpkg patch of https://github.com/microsoft/vcpkg/pull/18254/files#diff-178cf9aa459272fa0e399f50edc8b80e65bdd1dc3066dfa0ea01d323e9cbe9dc for hdf5 config. @MaartenBent What do you think? Thanks.

MaartenBent commented 3 months ago

It makes HDF5 required when MATIO_WITH_HDF5 option is true, which I suppose is fine.

I think finding in CONFIG mode is also fine. But personally I would check both modes. First find_package(HDF5 CONFIG), and if it didn't find any, also try find_package(HDF5). But you can't use REQUIRED in this case because it will abort after the first one.

There are already checks for added targets, just above the modified lines:

    elseif(HDF5_USE_STATIC_LIBRARIES AND TARGET hdf5::hdf5-static)
        # static target from hdf5 1.10 or 1.12 config
        target_link_libraries(MATIO::HDF5 INTERFACE hdf5::hdf5-static)
    elseif(NOT HDF5_USE_STATIC_LIBRARIES AND TARGET hdf5::hdf5-shared)
        # shared target from hdf5 1.10 or 1.12 config
        target_link_libraries(MATIO::HDF5 INTERFACE hdf5::hdf5-shared)

so I don't know why they are added again. This should not be needed.

I would also keep the check for target HDF5::HDF5. Maybe this target is only created when finding in not-CONFIG mode, but there is no reason to remove it.

tbeu commented 3 months ago

Thanks for the feedback. I needed to revert to get it building on OpenBSD and Solaris. Thus, it cannot be merged.