Skip to content

Conversation

@ferdymercury
Copy link
Collaborator

@ferdymercury ferdymercury commented Apr 15, 2025

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:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #

@github-actions
Copy link

github-actions bot commented Apr 15, 2025

Test Results

    22 files      22 suites   4d 0h 58m 57s ⏱️
 3 781 tests  3 781 ✅ 0 💤 0 ❌
81 190 runs  81 190 ✅ 0 💤 0 ❌

Results for commit f85a692.

♻️ This comment has been updated with latest results.

@pcanal
Copy link
Member

pcanal commented Apr 16, 2025

Also potentially related: #17607

@ferdymercury ferdymercury added this to the 6.38.00 milestone Apr 22, 2025
@dpiparo dpiparo closed this Jun 29, 2025
@dpiparo dpiparo reopened this Jun 29, 2025
@ferdymercury ferdymercury marked this pull request as ready for review July 16, 2025 09:04
@ferdymercury ferdymercury requested a review from bellenot as a code owner July 16, 2025 09:04
@ferdymercury ferdymercury requested a review from hageboeck July 16, 2025 09:04
Copy link
Member

@hageboeck hageboeck left a 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
@ferdymercury
Copy link
Collaborator Author

Simplification done!

Btw this is the result in the GUI / IDE after this change:

image

Unfolded:

image

@ferdymercury ferdymercury changed the title [RFC] [cmake] Add core/base headers to target HEADERS file_set [cmake] Add core/base headers to target HEADERS file_set Jul 18, 2025
ferdymercury added a commit to ferdymercury/root that referenced this pull request Sep 22, 2025
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 guitargeek closed this Nov 24, 2025
@guitargeek guitargeek reopened this Nov 24, 2025
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'm looking forward how this gets extended to more ROOT packages.

See also:

@guitargeek guitargeek merged commit 6d3e9ed into root-project:master Nov 25, 2025
50 of 54 checks passed
ferdymercury added a commit to ferdymercury/root that referenced this pull request Nov 25, 2025
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.
@ferdymercury ferdymercury deleted the cmakefileset branch November 26, 2025 08:23
ferdymercury added a commit to ferdymercury/root that referenced this pull request Nov 26, 2025
ferdymercury added a commit to ferdymercury/root that referenced this pull request Nov 26, 2025
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.
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.

6 participants