Repository navigation
[graf2d] add headers to private file sets - #23958
ferdymercury wants to merge 2 commits into
Conversation
Test Results 21 files 21 suites 3d 9h 45m 12s ⏱️ For more details on these failures, see this check. Results for commit f1d152d. ♻️ This comment has been updated with latest results. |
|
I would be really in favor trying to extend Trying to add extra argument which will create library file set. Proposed solution creates too much similar-looking code in our cmake files |
|
Please see #20515 (comment) on why I strongly advocate not to do this. In any case, this PR solves a very specific issue as NFC, the discussion can be left for later for the more challenging part of Using the file sets. |
|
You propose to go in opposite direction and move out functionality from Because some IDEs can better works with such cmake files? |
My proposal is to leave out that discussion for the future, and in the meanwhile solve a longstanding issue (https://its.cern.ch/jira/browse/ROOT-10915) without changing any functionality at the cost of a minimal repetition.
In my opinion, those are two independent aspects. One can still go for this short-term solution solving an issue in an atomic way, and later on, if you want to stick to the current macro, expand it accordingly to deal with file sets how the ROOT team decides. |
Normal way to resolve this issue would be extend You propose solution which tends to other direction. And if we starts moving in this direction it finally leads us to elimination of For me such direction change is important point for discussion. |
nope, that would not work. See many files (subfolders) in core/, for example that do not follow |
|
To me, #20515 was a useful probe that the FILE_SET mechanism works. But the follow-ups only make sense in light of #23061, which means each one gets reviewed on the strength of a future PR rather than on its own (hence the back-and-forth here). Instead, I’d like to see the end state directly: #23061 grown into a buildable draft that shows what a handful of representative packages (a plain STL-package one, one with src/ private headers, a hand-rolled one like lz4) and the macro would look like at the end of this campaign. No need to polish it, the point is that we can make an informed decision on the direction once, and then roll it out tree-wide mechanically. Continuing package by package doesn’t seem productive at this point, especially since CI is already struggling to keep up. tldr; Could you extend #23061 into a draft that reaches the end result on a few representative packages, so we decide once and skip the overhead of many small PRs, intermediate states and commit churn? |
|
Thanks for the feedback! Btw the PRs are now done, I've split them in 12, since often I've been asked not to mix things from different folders/reviewers to easen review. Sorry for the CI hassle.
This point I don't understand. If that's the case, wouldn't it make sense to close https://its.cern.ch/jira/browse/ROOT-10915 as not-planned? To me, they are two fully different features. One is having all headers as part of the CMake targets. This is useful for clang-tidy, for find-and-search, finding out "oh shit we forgot to add this target here", etc. etc. One can not just re-use ROOT_STL macro to have this because
A fully different feature is what can be all done with these new file-sets. Like avoiding copy-to-build-dir move headers, Globbing actions by generate dict, standard installation paths, etc. But this is a whole huge Pandora box. I really wanted to avoid mixing these two features.
Not that easy but I'll try... |
There is now a working example in #23061. There is a minor issue left with Ninja and modules and the move_headers target, but one can already get the general idea:
So overall, much better readability, and anyone in the world knowing CMake can now understand what is being defined, no need to remember the ROOT argot about LIBRARIES and dependencies, etc. |

see rationale in #20515 (comment)