-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[core] Generalize TROOT::GetIncludeDir() using build tree marker #20828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
FoundationUtils::GetIncludeDir() from RootSysTROOT::GetIncludeDir()
00c457f to
c0e43a8
Compare
TROOT::GetIncludeDir()|
Hi @ellert, I can't think of a better solution. What do you think? |
3801c9a to
a8725ae
Compare
a8725ae to
909168a
Compare
| 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(); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
909168a to
5babc8c
Compare
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.
5babc8c to
78364a5
Compare
Test Results 22 files 22 suites 3d 20h 9m 22s ⏱️ Results for commit 78364a5. |
Introducing a new marker file generalizes
TROOT::GetIncludeDir()towork in all cases for both relocated install and build trees, even if
the
CMAKE_INSTALL_INCLUDEDIRis different frominclude.This follows up on guitargeek@43ec084, which was incomplete and left ROOT
configuration in a broken state, because
ROOTINCDIRwas not definedanymore but still used.
The
RConfigure.hheader still defines a few other internal macros withhardcoded 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=OFFand absolute paths forgnuinstall=ON, which isnowhere documented and breaks relocatable builds. Also, they use
ROOTSYSin the case ofgnuinstall=OFF, which we want to migrate awayfrom.
The
ROOT::FoundationUtils::GetIncludeDir()function was also removedand completely absorbed into
TROOT::GetIncludeDir(), as it was notused in any other place.
Also, allow absolute
CMAKE_INSTALL_INCLUDEDIRpaths.Follows up on:
ROOTSYSin TSystem include path #20823 (comment)