Make non-system include paths accessible at runtime #19179
Conversation
The current implementation faces issues with accessing header files
from external libraries stored in non-system paths when the C++
interpreter tries to resolve them at runtime. This access is necessary
under certain conditions even if a corresponding module file exists.
To address this issue, we need to ensure that non-system path includes,
which are known at build time, remain accessible at runtime. The
proposed solution leverages the existing `ROOT_INCLUDE_PATH` environment
variable.
This commit introduces a new CMake variable `DEFAULT_ROOT_INCLUDE_PATH`
and a helper function `IF_NOT_SYSTEM_PATH_APPEND`. This function allows
us to conditionally append include paths of external libraries to
`DEFAULT_ROOT_INCLUDE_PATH`, for example
`IF_NOT_SYSTEM_PATH_APPEND("${Vc_INCLUDE_DIR}" DEFAULT_ROOT_INCLUDE_PATH)`
The `DEFAULT_ROOT_INCLUDE_PATH` variable is then utilized to populate the
`ROOT_INCLUDE_PATH` environment variable in the configuration files
(`thisroot.{sh,csh,fish,bat}`).
If the library's include path is dynamically modified between build and
run time, the configuration can be updated by patching the
`thisroot.{sh,csh,fish,bat}` files.
LukasBreitwieser
force-pushed
the
default-root-include-path
branch
from
fdc4019 to
4c0e474
Compare
Test Results 20 files 20 suites 3d 9h 25m 7s ⏱️ For more details on these failures, see this check. Results for commit 6feedf8. ♻️ This comment has been updated with latest results. |
cmake/modules/RootMacros.cmake
Outdated
| set(${is_system_path} true PARENT_SCOPE) | ||
| endif() | ||
| endforeach() | ||
| set(${is_system_path} false PARENT_SCOPE) |
There was a problem hiding this comment.
The canonical bools are all caps, and we can break out early:
| set(${is_system_path} true PARENT_SCOPE) | |
| endif() | |
| endforeach() | |
| set(${is_system_path} false PARENT_SCOPE) | |
| set(${is_system_path} TRUE PARENT_SCOPE) | |
| return() | |
| endif() | |
| endforeach() | |
| set(${is_system_path} FALSE PARENT_SCOPE) |
| @@ -89,6 +89,10 @@ clean_environment() | |||
| default_manpath="" | |||
| fi | |||
| fi | |||
| if [ -n "${ROOT_INCLUDE_PATH-}" ]; then | |||
| drop_from_path "$ROOT_INCLUDE_PATH" "@DEFAULT_ROOT_INCLUDE_PATH@" | |||
| ROOT_INCLUDE_PATH=$newpath | |||
There was a problem hiding this comment.
newpath seems to be undefined here, or did I miss something?
There was a problem hiding this comment.
newpath is set by the function drop_from_path.
cmake/modules/RootMacros.cmake
Outdated
| # The 2nd argument is the variable that the path gets appended to if it is | ||
| # not a system path. | ||
| #---------------------------------------------------------------------------- | ||
| function (IF_NOT_SYSTEM_PATH_APPEND path variable) |
There was a problem hiding this comment.
Is this function ever going to be called with another variable than the default root include path? If yes, fair enough, otherwise I would call it something like BUILD_ROOT_INCLUDE_PATH("${Vc_INCLUDE_DIR}"), and remove the second argument, since any negations in names are hard to parse.
| ROOT_INCLUDE_PATH=@DEFAULT_ROOT_INCLUDE_PATH@ | ||
| export ROOT_INCLUDE_PATH | ||
| else | ||
| ROOT_INCLUDE_PATH=@DEFAULT_ROOT_INCLUDE_PATH@:$ROOT_INCLUDE_PATH | ||
| export ROOT_INCLUDE_PATH |
There was a problem hiding this comment.
| ROOT_INCLUDE_PATH=@DEFAULT_ROOT_INCLUDE_PATH@ | |
| export ROOT_INCLUDE_PATH | |
| else | |
| ROOT_INCLUDE_PATH=@DEFAULT_ROOT_INCLUDE_PATH@:$ROOT_INCLUDE_PATH | |
| export ROOT_INCLUDE_PATH | |
| export ROOT_INCLUDE_PATH=@DEFAULT_ROOT_INCLUDE_PATH@ | |
| else | |
| export ROOT_INCLUDE_PATH=@DEFAULT_ROOT_INCLUDE_PATH@:$ROOT_INCLUDE_PATH |
There was a problem hiding this comment.
For the sake of consistency, I followed the style of all other environment variables in thisroot.sh
|
@LukasBreitwieser, what happens when you install (and therefore relocate) ROOT? Given that these paths are system paths, things should continue to work, won't they? |
| ROOT_INCLUDE_PATH=@DEFAULT_ROOT_INCLUDE_PATH@ | ||
| export ROOT_INCLUDE_PATH | ||
| else | ||
| ROOT_INCLUDE_PATH=@DEFAULT_ROOT_INCLUDE_PATH@:$ROOT_INCLUDE_PATH |
There was a problem hiding this comment.
Do you consider this an issue if DEFAULT_ROOT_INCLUDE_PATH is empty? Not just here but in the other shell setups?
There was a problem hiding this comment.
I don't think this should be an issue. This case is also covered by all GHA runners.
|
I am getting an error in the lcg build with this |
|
What do you practically mean by: ? |
|
Thank you all for your comments!
Yes this works. Only third party include paths (i.e., include paths outside the root src/build/install directories) should be added. Perhaps a good idea to add additional checks for these conditions. @pcanal @andresailer |
The current implementation faces issues with accessing header files from external libraries stored in non-system paths when the C++ interpreter tries to resolve them at runtime. This access is necessary under certain conditions even if a corresponding module file exists. Related issues: SPI-2794, #18949
To address this issue, we need to ensure that non-system path includes, which are known at build time, remain accessible at runtime. The proposed solution leverages the existing
ROOT_INCLUDE_PATHenvironment variable.This commit introduces a new CMake variable
DEFAULT_ROOT_INCLUDE_PATHand a helper functionIF_NOT_SYSTEM_PATH_APPEND. This function allows us to conditionally append include paths of external libraries toDEFAULT_ROOT_INCLUDE_PATH, for exampleIF_NOT_SYSTEM_PATH_APPEND("${Vc_INCLUDE_DIR}" DEFAULT_ROOT_INCLUDE_PATH)The
DEFAULT_ROOT_INCLUDE_PATHvariable is then utilized to populate theROOT_INCLUDE_PATHenvironment variable in the configuration files (thisroot.{sh,csh,fish,bat}).If the library's include path is dynamically modified between build and run time, the configuration can be updated by patching the
thisroot.{sh,csh,fish,bat}files.