[RF] Add RooFit headers to private CMake filesets

[ferdymercury]

To improve IDE experience.
See also #18419 and #20515

Non-functional change.

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

[github-actions]

Test Results

    23 files      23 suites   3d 17h 2m 44s ⏱️
 3 856 tests  3 856 ✅ 0 💤 0 ❌
79 447 runs  79 447 ✅ 0 💤 0 ❌

Results for commit b3730d2.

♻️ This comment has been updated with latest results.

[guitargeek]

Thank you very much! A few comments inline.

Also, to make this work, you have to drop src/RooXYChi2Var.h from roofit/roofitcore/CMakeLists.txt, because that class doesn't exist anymore.

[guitargeek]

LGTM, thanks for this!

In general, I still don't think we should do this for ROOT at a larger scale until the minimum CMake version is raised or we figure out how to do this the right way with CMake macros or functions to we don't duplicate code all over the place.

But for RooFit in particular, I really like this change, because it also helps to harmonize the code with the CMake code in the standalone RooFit repository!

So for now, I would leave it at the existing precedents and come back to this later.

[ferdymercury]

Thanks!

we figure out how to do this the right way with CMake macros or functions

In my opinion, I think ROOT should think about this from another perspective. Rather than creating custom macros that outside users / developers find hard to understand, and inside users get often confused (LIBRARIES vs DEPENDENCIES, who chose those two names xD), we should make use of standard CMake nomenclature (PUBLIC vs PRIVATE), no one would get confused here.

Here is my proposal for the future, with standard CMake > 3.23 nomenclature:

add_library(RooFitCore)
target_sources(
      RooFitCore
      PRIVATE
          src/BatchModeDataHelpers.cxx
          src/ConstraintHelpers.cxx
          ...
          ...
          src/RooImproperIntegrator1D.h # could also go into a PRIVATE FILE_SET if preferred
          res/RooFitImplHelpers.h
          res/RooUnitTest.h
      PUBLIC
        FILE_SET HEADERS
          TYPE HEADERS
          BASE_DIRS inc/ res/
          FILES
            inc/Roo1DTable.h
            inc/RooAICRegistry.h
            ...
      PUBLIC
        FILE_SET dictionaryHeaders
          TYPE HEADERS
          BASE_DIRS inc/ res/
          FILES
            inc/LinkDef.h
            ...
)
target_link_libraries(RooFitCore
  PUBLIC ${EXTRA_LIBRARIES}
  PRIVATE
    Core
    Hist
    Graf
    Matrix
    Tree
    RIO
    MathCore
    Foam
    RooBatchCompute
    ${EXTRA_DEPENDENCIES}
)
NEW_ROOT_STL_PACKAGE(RooFitCore   DICTIONARY_OPTIONS "-writeEmptyRootPCM")

This way, all the information is 'stored' in a standard CMake target. We do not need the annoying "Transform" or "Preprend" macro.

Instead, you would need to create a NEW_ROOT_STL_PACKAGE macro that traverses the FILE_SET and generates the dictionaries and so on. No need to pass HEADERS and SOURCES or LINKDEF to that macro. Just the name of the CMake target and the dictionary options. Everything is parsed internally, targets must just correctly define Public or Private file_sets and a special Linkdef header fileset.

The old ROOT_STL_PACKAGE can stay of course there for backward compatibility.
But this approach would be much cleaner and help towards many things in installation etc and Debian porting etc.

[guitargeek]

I like that direction, that's exactly where I would like to go!

So then the next step would be to prototype this new NEW_ROOT_STL_PACKAGE macro I guess, before branching out to all of ROOT? RooFit can continue to serve as an area to try things out here.

[ferdymercury]

to prototype this new NEW_ROOT_STL_PACKAGE macro I guess

Yes, indeed.

Retrieving from the defined CMake targets the different file sets is straightforward:

  get_target_property(type ${tgt} TYPE)
  if(NOT ${type} STREQUAL "INTERFACE_LIBRARY")
    get_target_property(SrcList ${tgt} SOURCES)
    get_target_property(SrcDir ${tgt} SOURCE_DIR)
    list(TRANSFORM SrcList PREPEND ${SrcDir}/)
   # do what ever one wants with the list of sources
  endif()
  get_target_property(HeaderList ${tgt} HEADER_SET)
  if(NOT "HeaderList-NOTFOUND" IN_LIST HeaderList)
    # do whatever one wants with the list of headers
  endif()

In the new ROOT_STL macro, one would no longer need the calls to
ROOT_OBJECT_LIBRARY or ROOT_LINKER_LIBRARY since that would be already taken care by standard CMake one layer above via add_library SHARED or OBJECT.

Main step would then be:

ROOT_GENERATE_DICTIONARY(G__${libname} HeaderListFromFileSet
                          ${NO_CXXMODULE_FLAG}
                          ${STAGE1_FLAG}
                          MODULE ${tgt}
                          LINKDEF LinkdefListFromFileSet
                          NODEPHEADERS ${ARG_NODEPHEADERS}
                          OPTIONS ${ARG_DICTIONARY_OPTIONS}
                          DEPENDENCIES PublicDepsFromTgt or maybe not needed and just link to target?
                          BUILTINS Probably no longer needed
                          )

and then instead of calling
ROOT_INSTALL_HEADERS(${ARG_INSTALL_OPTIONS})

one would rely instead on the standard CMake install step specific for file_sets