[ferdymercury]

This is a non-functional change, it only improves the IDE experience.

In the future, we could avoid copying headers to build-directory and verbose target_include_directory statements by just using the file_sets. Same with INSTALL, would go through headers.
This could ultimately go into the ROOT custom macros, but for the moment let's only do the minimal changes for the IDE and let the rest for later.
They are all added as private now, in the future we can distinguish private from public file-sets.
As well as DICT_HEADERS for dictionary generation. This might enhance dependency issues and build speed.

Follow-up of #18419

Work towards https://its.cern.ch/jira/browse/ROOT-10915

[guitargeek]

Hi! Can this not be part of the ROOT_STANDARD_LIBRARY_PACKAGE, so we don't duplicate a lot of CMake configuration code?

[ferdymercury]

Hi! Can this not be part of the ROOT_STANDARD_LIBRARY_PACKAGE, so we don't duplicate a lot of CMake configuration code?

Not without a major rework of that macro as well as other macro such as ROOT_OBJECT_LIBRARY, since we have to specify separately DICT_HEADERS vs public headers not in dict vs private headers. Then we would also need to specify the base directory of each group of headers (inc, res)...
As said in the first message, I agree that this is the way to go, but I don't think I can achieve that myself since I am not expert with it. Also, if we touch the macro, then it's difficult to add this modularly.

In my opinion, it would make sense to first add all HEADERS verbosely, which is easy and modular, and one does not risk to break everything, to improve IDE experience in a short time frame. To me, it only makes sense to touch the macros at the same time than the copy-header-to-build-dir is removed by using file-sets instead. But that's a major cleanup of the CMake build system.

Also, there are several places were headers are not explicitly stated but globbed/searched for in inc-dir, so this fixes that too.

[guitargeek]

Hi @ferdymercury, sorry for not reading your initial post carefully enough!

I think even if we can't integrate the it in the existing macros like ROOT_STANDARD_LIBRARY_PACKAGE, we should at least introduce a new macro like ROOT_SET_TARGET_SOURCES to avoid the redundant CMAKE_VERSION check and to make sure that we can find all packages that already set the target sources with git grep. That would be important if we want to merge this later at some point with ROOT_STANDARD_LIBRARY_PACKAGE/ROOT_OBJECT_LIBRARY.

What do you think?

[ferdymercury]

Thanks @guitargeek

What do you think?

I guess the answer depends on whether #19941 is going to get merged for 6.40 or for later. If we want to stay with CMake 3.22 for ROOT 6.40, then yes, it makes total sense to have ROOT_SET_TARGET_SOURCES
If not, I do not see much advantage in an extra wrapper.

Side note: one could even think the other way round for the future.

CMake target_sources defines four types of FILE_SETS

• private headers
• dictionary headers
• Linkdefs
• public headers

Then, ROOT_STL_PACKAGE would look for Target.LinkDefFileSet Target.DictHeaders

rather than passing all the headers again to ROOT_STL_PACKAGE as argument.

[github-actions]

Test Results

    21 files      21 suites   3d 8h 6m 53s ⏱️
 3 883 tests  2 964 ✅   0 💤 919 ❌
72 188 runs  70 811 ✅ 458 💤 919 ❌

For more details on these failures, see this check.

Results for commit e30b896.

♻️ This comment has been updated with latest results.

[ferdymercury]

@guitargeek thanks for merging the CMake version upgrade!

This greatly simplifies this PR. Code repetition is minimized.

Recap:

• This PR has a large diff, but is a no-brainer or an [nfc], it does not change anything, it only improves the IDE experience
• It starts to solve https://its.cern.ch/jira/browse/ROOT-10915
• These changes are semantically partially wrong since headers are added to a "private" file set. However I am strongly in favor of going this way: it makes atomic steps towards solving 10915, rather than trying to fix all in one go. Consider it a temporary solution that will be soon fixed.
• A subsequent PR can deal with using effectively this information from file-sets, but that's rather more involved and prone to issues with install locations, etc. Thus, it's easier to have the very-large-diff here with the no-brainer PR, and the smaller-but-more-conflicting diff of using that info later on.
• I advocate against additional ROOT_SOURCES macros, it makes IDE experience a lot worse, you cannot ctrl+click on the files for example. target_sources is already super-powerful.
• Next steps would be to define everything within target_sources: LINKDEFs, DICT_HEADERS, HEADERS, see [RFC,cmake] A layer of smoke: avoid by using file sets for library generation #23061. Rather than new macros, we do it the other way round, we use the standard CMake target_sources, and later do custom command with that rich CMake target, we read the proper file-sets and act on them.
• A last step would then be to control what is installed from those file-sets, and to remove the annoying copy-to-build-dir custom commands.

If this PR is merged, only Roofit and core will have this. Then I will go folder by folder adding headers to private file-sets. Once all folders have this, 10915 can be closed, and CMake build system optimization can then be started.

Tagging also @hageboeck