-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[cmake] Add core/base headers to target HEADERS file_set #18419
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
Conversation
Test Results 22 files 22 suites 4d 0h 58m 57s ⏱️ Results for commit f85a692. ♻️ This comment has been updated with latest results. |
|
Also potentially related: #17607 |
e9a87ee to
347cde1
Compare
hageboeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ferdymercury, would you agree that the same can be achieved with less code, since CMake already does the expansion to absolute paths for us?
as suggested by hageboeck
This version of CMake was released in March 2022 https://github.com/Kitware/CMake/releases/tag/v3.23.0 This version is required to use FILE_SETs, which could help towards root-project#16327 and root-project#18419.
guitargeek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version of CMake was released in March 2022 https://github.com/Kitware/CMake/releases/tag/v3.23.0 This version is required to use FILE_SETs, which could help towards root-project#16327 and root-project#18419.
To improve IDE experience. See also root-project#18419 and root-project#20515
This version of CMake was released in March 2022 https://github.com/Kitware/CMake/releases/tag/v3.23.0 This version is required to use FILE_SETs, which could help towards root-project#16327 and root-project#18419. Since cmake_minimum_required updates the cmake_policy from 3.20 to 3.23, we need to take care of a change that happened from 3.20 to 3.21 that leads to an error in the CheckCompiler CMake module, so set the proper policy flag to old to avoid that error on some platforms.


This Pull request:
Changes or fixes:
Gives a taste on how it would look like to properly add HEADERS to targets in CMake.
See also https://www.studyplan.dev/cmake/cmake-file-sets
This does not just help with the IDE workflow, it gives an idea on future paths:
One could make them PUBLIC instead of PRIVATE, then you would not need the target_include_directories line, and installing and cpacking would be easier.
Related:
Checklist:
This PR fixes #