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

Fix Kealib interface target #45

Merged
merged 4 commits into from
Dec 5, 2023
Merged

Fix Kealib interface target #45

merged 4 commits into from
Dec 5, 2023

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Dec 2, 2023

This PR fixes using libkea::Kealib with static library linkage. This change comes from trying to add kealib support to the vcpkg port of GDAL: microsoft/vcpkg#35437

If I understand the intention correctly, Kealib is just a constant interface name for the variable actual library target name. However, ATM it is hard-coded to use the shared link library which isn't created in static builds, and there is a TODO comment attached to this CMake code. This PR suggests to simply let CMake take care of forwarding transitive usage requirements from Kealib's interface link libraries by refering to the actual library as a cmake target.

It wouldn't be necessary to add any interface include directories for pure CMake target usage. But I left the installed interface include directory in case some consumer wants to read the property.

It is just a stable interface name for the variable actual library target name. CMake takes care of forwarding transitive usage requirements from Kealib's interface link libraries.
@gillins
Copy link
Member

gillins commented Dec 4, 2023

Thanks @dg0yt - cmake is a mystery to me...
Any idea why the Python bindings now fail to link on Windows? Is there a problem with that CMakeLists.txt?

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 4, 2023

Thanks @dg0yt - cmake is a mystery to me... Any idea why the Python bindings now fail to link on Windows? Is there a problem with that CMakeLists.txt?

I have no exact idea about the python issue now. But the vcpkg reviewers just notified me about hdf5 targets appearing in the exported link libraries at least for static linkage. I know how to fix that, but ATM I'm surprised that it pops up at all, with module-mode find_package(HDF5) and using HDF5_LIBRARIES. This might be related, but I don't want to explain details before verifying.

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 4, 2023

Hm, sometimes vcpkg doesn't seem to have a good effect on the CMake ecosystem:
https://gitlab.kitware.com/cmake/cmake/-/merge_requests/7416

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 5, 2023

This passed GHA in my fork.

The problem was caused by CMake's HDF5 find module behaving unusual (returning imported targets in HDF5_LIBRARIES). This is resolved by find_dependency(HDF5) in Kealib CMake config. It doesn't add any further obligations for downstream users of Kealib.

Given that the installed Kealib headers don't expose HDF5 headers, I also marked the link relationship as PRIVATE. If downstream users want to use HDF5, they have to do it explicitly. This also makes it possible to skip find_dependency(HDF5) for shared linkage of Kealib.

Last not least I dropped the explicit include directory property on Kealib. Transitive dependencies and nothing else.

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 5, 2023

CC @gillins. Does this PR accomplishes what you wanted to originally achieve with #31, or does it conflict?

@gillins gillins merged commit c0beb79 into ubarsc:master Dec 5, 2023
2 checks passed
@gillins
Copy link
Member

gillins commented Dec 5, 2023

Thanks for your help @dg0yt I really struggle with this stuff.

@dg0yt dg0yt deleted the patch-1 branch December 6, 2023 01:50
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