[core] Use shared library dir as anchor in GetIncludeDir()
#20924
+36
−15
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
guitargeek
added
in:Build System
in:Core Libraries
build-python-wheels
guitargeek
force-pushed
the
sharedlib_to_incdir
branch
from
d937f4b to
065f051
Compare
Contributor
Author
Contributor
Author
|
Also, here are my next plans in this
|
Member
However, we should warn if |
guitargeek
force-pushed
the
sharedlib_to_incdir
branch
from
065f051 to
cb2b4a9
Compare
Member
What kind of movement is done? For Wheels is |
Using the shared library directory as the anchor to resolve the resource directories like `GetIncludeDir()` is more direct and robust than using `GetRootSys()`, which loses its meaning in some installations (e.g. what should it be when ROOT is installed by the package manager? Just `/`?). Most importantly, this change lifts some more constraint from how the install tree can be reorganized after the fact. One can for example move the include and library directory around as wanted, as long as their relative paths don't change. This fixes the Python wheels, which were broken by commit a5b1ed9. As of commit a5b1ed9, ROOT became smarter about finding the include path in the install tree, inferring the correct relative path from $ROOTSYS and the CMAKE_INSTALL_INCLUDEDIR variable at build time. Before, that only happened for gnuinstall=ON, but now it always happens. However, for the Python wheel, we are breaking the assumptions that ROOT makes by moving around directories in the install tree. This commit fixes that situation.
guitargeek
force-pushed
the
sharedlib_to_incdir
branch
from
cb2b4a9 to
6b8877e
Compare
Test Results 22 files 22 suites 3d 13h 2m 16s ⏱️ Results for commit 6b8877e. ♻️ This comment has been updated with latest results. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using the shared library directory as the anchor to resolve the resource directories like
GetIncludeDir()is more direct and robust than usingGetRootSys(), which loses its meaning in some installations (e.g. what should it be when ROOT is installed by the package manager? Just/?).Most importantly, this change lifts some more constraint from how the install tree can be reorganized after the fact. One can for example move the include and library directory around as wanted, as long as their relative paths don't change. This fixes the Python wheels, which were broken by commit a5b1ed9.
As of commit a5b1ed9, ROOT became smarter about finding the include path in the install tree, inferring the correct relative path from $ROOTSYS and the CMAKE_INSTALL_INCLUDEDIR variable at build time. Before, that only happened for gnuinstall=ON, but now it always happens.
However, for the Python wheel, we are breaking the assumptions that ROOT makes by moving around directories in the install tree. This commit fixes that situation.
I tested this with three build configurations:
CMAKE_INSTALL_INCLUDEDIRsetCMAKE_INSTALL_INCLUDEDIRrelative pathCMAKE_INSTALL_INCLUDEDIRabsolute pathIn all cases,
gROOT->GetIncludeDir()returns the correct path, both via therootexecutable in the build and install tree.