Skip to content

[graf2d] add headers to private file sets - #23958

Open
ferdymercury wants to merge 2 commits into
root-project:masterfrom
ferdymercury:hg2d
Open

ferdymercury wants to merge 2 commits into
root-project:masterfrom
ferdymercury:hg2d

Conversation

@ferdymercury

@ferdymercury ferdymercury commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

see rationale in #20515 (comment)

@ferdymercury
ferdymercury marked this pull request as ready for review November 2, 2026 19:02
@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Test Results

    21 files      21 suites   3d 9h 45m 12s ⏱️
 3 884 tests  3 883 ✅ 0 💤 1 ❌
72 201 runs  72 200 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit f1d152d.

♻️ This comment has been updated with latest results.

@linev

linev commented Nov 4, 2026

Copy link
Copy Markdown
Member

I would be really in favor trying to extend ROOT_STANDARD_LIBRARY_PACKAGE macro.

Trying to add extra argument which will create library file set.
And then try it with several "normal' libraries to see how it works.
If there are complications or special cases - try to cover them with extra parameters or extra args.

Proposed solution creates too much similar-looking code in our cmake files

@ferdymercury

ferdymercury commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator Author

Please see #20515 (comment) on why I strongly advocate not to do this.
Creating custom macros totally destroys IDE experience. You can't control click etc
Doing things standard lets you control click on files and removes ambiguity. People constantly confused about what is dependencies and what is libraries.
I agree about the repetition, but this is soon gone, see #23061 where there will be no repetition. Macro is just reserved for custom things that CMake can't describe: Dictionary generation
For the rest it can describe natively and the rest is just adding a layer of smoke.

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.

@linev

linev commented Nov 4, 2026

Copy link
Copy Markdown
Member

You propose to go in opposite direction and move out functionality from ROOT_STANDARD_LIBRARY_PACKAGE

Because some IDEs can better works with such cmake files?
This should be agreed first inside ROOT team

@ferdymercury

Copy link
Copy Markdown
Collaborator Author

You propose to go in opposite direction and move out functionality from ROOT_STANDARD_LIBRARY_PACKAGE

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.

Because some IDEs can better works with such cmake files? This should be agreed first inside ROOT team

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.

@ferdymercury

ferdymercury commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator Author

I agree about the repetition, but this is soon gone,

Just to justify that the repetition would be gone in the future if you one would go towards standard CMake code. This pseudocode shows how it would look like:

image

The new code has some more extra lines because I invented private headers that don't exist, just to showcase the idea. But there would be no repetition and one can compact it even more if needed.

Creating custom arguments in ROOT-STL CMake macro to handle with all this complexity of arguments would be for me as odd as creating in year 2026 TString when there is std::string
Also, you do not need to be a ROOT expert to understand the picture on the right. You need to dive through hunderes of lines of code to understand the left part.

Of course, one can think of hybrid approaches, such as defining target_sources with standard CMake, and then have ROOT_INSTALL_STANDARD_LIBRARY_PACKAGE(Graf) dealing with more repetitive parts.

@linev

linev commented Nov 4, 2026

Copy link
Copy Markdown
Member

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.

Normal way to resolve this issue would be extend ROOT_STANDARD_LIBRARY_PACKAGE functionality.
Because it should be done for every library generated in the ROOT.
And I think it should be possible to implement.

You propose solution which tends to other direction. And if we starts moving in this direction it finally leads us to elimination of ROOT_STANDARD_LIBRARY_PACKAGE macro and writing custom cmake rules for each ROOT library.

For me such direction change is important point for discussion.

@ferdymercury

Copy link
Copy Markdown
Collaborator Author

would be extend ROOT_STANDARD_LIBRARY_PACKAGE functionality.

nope, that would not work. See many files (subfolders) in core/, for example

https://github.com/ferdymercury/root/blame/754aee0631665b7dc7c3ede3dcc735c3dcb7e696/core/lz4/CMakeLists.txt

that do not follow ROOT_STANDARD_LIBRARY_PACKAGE functionality.

@guitargeek

Copy link
Copy Markdown
Contributor

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?

@ferdymercury

Copy link
Copy Markdown
Collaborator Author

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.

But the follow-ups only make sense in light of #23061

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) many headers are left out intentionally from the dictionary generation, but are still valid headers that should be part of the the CMake target,
  • b) several targets have the rolled-out variant where you do not call ROOT_STL macro but rather target_sources

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.

Could you extend #23061 into a draft that reaches the end result on a few representative packages

Not that easy but I'll try...

@ferdymercury

Copy link
Copy Markdown
Collaborator Author

Could you extend #23061 into a draft that reaches the end result on a few representative packages

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:

  • sources are defined with standard CMake commands that already describe a rich variety of options.
  • dictionary generation, for which there is obviously no CMake standard command, still goes over ROOT macro.
  • installation macro has changed from ROOT_INSTALL_HEADERS to ROOT_INSTALL_TARGET.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants