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

Apply vcpkg patch #245

Closed
wants to merge 3 commits into from
Closed

Apply vcpkg patch #245

wants to merge 3 commits into from

Conversation

tbeu
Copy link
Owner

@tbeu tbeu commented Mar 29, 2024

@MaartenBent
Copy link
Collaborator

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
Copy link
Owner Author

tbeu commented Apr 1, 2024

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

@tbeu tbeu closed this Apr 1, 2024
@tbeu tbeu removed the request for review from MaartenBent April 1, 2024 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants