Skip to content

Conversation

@guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Jan 9, 2026

Introducing a new marker file generalizes TROOT::GetIncludeDir() to
work in all cases for both relocated install and build trees, even if
the CMAKE_INSTALL_INCLUDEDIR is different from include.

This follows up on guitargeek@43ec084, which was incomplete and left ROOT
configuration in a broken state, because ROOTINCDIR was not defined
anymore but still used.

The RConfigure.h header still defines a few other internal macros with
hardcoded paths, and if the suggested mechanism proves to be stable for
the include directory, we can also migrate the other macros. They are
fragile anyway. For example, they contain relative paths for
gnuinstall=OFF and absolute paths for gnuinstall=ON, which is
nowhere documented and breaks relocatable builds. Also, they use
ROOTSYS in the case of gnuinstall=OFF, which we want to migrate away
from.

The ROOT::FoundationUtils::GetIncludeDir() function was also removed
and completely absorbed into TROOT::GetIncludeDir(), as it was not
used in any other place.

Also, allow absolute CMAKE_INSTALL_INCLUDEDIR paths.

Follows up on:

@guitargeek guitargeek self-assigned this Jan 9, 2026
@guitargeek guitargeek added in:Build System in:Core Libraries clean build Ask CI to do non-incremental build on PR labels Jan 9, 2026
@guitargeek guitargeek changed the title [core] Always infer FoundationUtils::GetIncludeDir() from RootSys [core] Improve TROOT::GetIncludeDir() Jan 9, 2026
@guitargeek guitargeek changed the title [core] Improve TROOT::GetIncludeDir() [core] Generalize TROOT::GetIncludeDir() using build tree marker Jan 9, 2026
@guitargeek
Copy link
Contributor Author

Hi @ellert, I can't think of a better solution. What do you think?

@guitargeek guitargeek force-pushed the incpath_followup branch 2 times, most recently from 3801c9a to a8725ae Compare January 9, 2026 16:37
const TString& TROOT::GetIncludeDir() {
// Avoid returning a reference to a temporary because of the conversion
// between std::string and TString.
const static TString includedir = ROOT::FoundationUtils::GetIncludeDir();
Copy link
Member

Choose a reason for hiding this comment

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

Could you extend the commit log to explain why the structure is changed to remove ROOT::FoundationUtils::GetIncludeDir (instead of putting the new implementation there).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! The explanation is simple: there was not other use of ROOT::FoundationUtils::GetIncludeDir, so the indirection was not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

there was not other use of ROOT::FoundationUtils::GetIncludeDir

There used to be ... See the completely related commit 99b31ee.
(ie. Does gDriverConfig->fTROOT__GetIncludeDir() relate to the current work?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For rootcling, the fTROOT_GetIncludeDir() already points to TROOT::GetIncludeDir():
https://github.com/root-project/root/blob/master/core/metacling/src/rootclingTCling.cxx#L40

For rootcling_stage1, there is a separate implementation, which is AFAIK correct and has to only work for the build tree:
https://github.com/root-project/root/blob/master/core/rootcling_stage1/src/rootcling_stage1.cxx#L26

Thanks for pointing out 99b31ee. This commit explains why FoundationUtils::GetIncludeDir() formerly existed but is not relevant anymore.

Introducing a new marker file generalizes `TROOT::GetIncludeDir()` to
work in all cases for both relocated install and build trees, even if
the `CMAKE_INSTALL_INCLUDEDIR` is different from `include`.

This follows up on 43ec084, which was incomplete and left ROOT
configuration in a broken state, because `ROOTINCDIR` was not defined
anymore but still used.

The `RConfigure.h` header still defines a few other internal macros with
hardcoded paths, and if the suggested mechanism proves to be stable for
the include directory, we can also migrate the other macros. They are
fragile anyway. For example, they contain relative paths for
`gnuinstall=OFF` and absolute paths for `gnuinstall=ON`, which is
nowhere documented and breaks relocatable builds. Also, they use
`ROOTSYS` in the case of `gnuinstall=OFF`, which we want to migrate away
from.

The `ROOT::FoundationUtils::GetIncludeDir()` function was also removed
and completely absorbed into `TROOT::GetIncludeDir()`, as it was not
used in any other place.

Also, allow absolute `CMAKE_INSTALL_INCLUDEDIR` paths.
@github-actions
Copy link

Test Results

    22 files      22 suites   3d 20h 9m 22s ⏱️
 3 791 tests  3 791 ✅ 0 💤 0 ❌
80 314 runs  80 314 ✅ 0 💤 0 ❌

Results for commit 78364a5.

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

Labels

clean build Ask CI to do non-incremental build on PR in:Build System in:Core Libraries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants