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

[build] Fix interface include directories for imported targets #342

Merged
merged 2 commits into from
Jun 7, 2021

Conversation

jacobkahn
Copy link
Contributor

Some versions of CMake (can't figure out exactly which, but some definitely do) don't implicitly add an INTERFACE_INCLUDE_DIRECTORIES property to IMPORTED targets present in generated *Targets.cmake CMake configs.

Specifying INCLUDES DESTINATION include in all install calls for exported targets ensures this property is always present, and fixes cases where linking to imported targets doesn't add the correct include directories.

This PR also fixes an annoying issue where downstream projects would fail to include KenLM headers that transitively included other Kenlm headers (i.e. lm/model.hh) — this was because the installed header dir would be include, but headers would be relative to include/kenlm but placed inside include. Adding include/kenlm to the INSTALL_INTERFACE for all exported targets ensures downstream projects can use their IMPORTED Kenlm targets alone without needing to add hacks to fix include directories.

  • That said, this structure isn't great and should probably be fixed one day, cc @kpu.

cc @padentomasello, @andresy, @vineelpratap, @syhw, @tlikhomanenko

jacobkahn added a commit to jacobkahn/flashlight that referenced this pull request Jun 5, 2021
Awaiting kpu/kenlm#342, which fixes include directories
and makes using imported targets for KenLM in flashlight possible.
@kpu
Copy link
Owner

kpu commented Jun 7, 2021

I'm thinking of changing all the internal header includes to relative paths. Would that help?

@jacobkahn
Copy link
Contributor Author

@kpu yep — that would help a lot!

@kpu kpu merged commit 6438318 into kpu:master Jun 7, 2021
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.

None yet

2 participants