Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Cpp] Use target_include_dirs for cmake targets #4661

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

njnobles
Copy link

Use target_include_dirs to attach include directories to targets. This allows setting include directories for the INSTALL_INTERFACE which will get exported into antlr4-targets.cmake.

antlr4-targets.cmake will now have an INTERFACE_INCLUDE_DIRECTORIES property set like so:

set_target_properties(antlr4_shared PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "ANTLR4CPP_EXPORTS"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include/antlr4-runtime"
)

When downstream users pull in antlr4-runtime using find_package(antlr4-runtime), the imported targets antlr4_static and antlr4_shared will automatically pick up the appropriate include directories instead of needing to manually use ANTLR4_INCLUDE_DIR.

Signed-off-by: Nick Nobles <nicholas_nobles@intuit.com>
@njnobles
Copy link
Author

Hi @mike-lischke,
Would you be able to review this when you have some time?
Thanks!

@mike-lischke
Copy link
Member

Sorry, I don't know enough about cmake to validate the changes.

@njnobles
Copy link
Author

Thanks for responding @mike-lischke
I put together this small test repo that should help validate the changes: https://github.com/njnobles/antlr-cmake-example
It sets up 2 simple projects, one to build the runtime and one to consume the runtime. It has options to build without my fix applied which should demonstrate the issue I'm trying to fix and another option to build with my fix.
Should be able to build on any platform.

Would this be enough to help you validate? Or is there someone else that I could request a review from that might have more cmake knowledge?
Thank you!

@mike-lischke
Copy link
Member

Thanks for providing that test repo, it helps to understand.

@parrt I think this PR can be merged.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants