Make non-system include paths accessible at runtime by LukasBreitwieser · Pull Request #19179 · root-project/root

LukasBreitwieser · 2025-06-25T14:37:30Z

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_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.

github-actions · 2025-06-25T21:47:33Z

Test Results

22 files 22 suites 3d 19h 21m 59s ⏱️
3 691 tests 3 686 ✅ 0 💤 5 ❌
79 254 runs 79 249 ✅ 0 💤 5 ❌

For more details on these failures, see this check.

Results for commit 8d801f2.

♻️ This comment has been updated with latest results.

hageboeck

Nice! I added a few comments below

cmake/modules/RootMacros.cmake

config/thisroot.sh

cmake/modules/RootMacros.cmake

config/thisroot.sh

hageboeck · 2025-06-26T13:35:21Z

@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?

cmake/modules/RootMacros.cmake

andresailer · 2025-06-26T15:54:49Z

config/thisroot.sh

+      ROOT_INCLUDE_PATH=@DEFAULT_ROOT_INCLUDE_PATH@
+      export ROOT_INCLUDE_PATH
+   else
+      ROOT_INCLUDE_PATH=@DEFAULT_ROOT_INCLUDE_PATH@:$ROOT_INCLUDE_PATH


Do you consider this an issue if DEFAULT_ROOT_INCLUDE_PATH is empty? Not just here but in the other shell setups?

I don't think this should be an issue. This case is also covered by all GHA runners.

config/thisroot.sh

andresailer · 2025-06-27T07:37:34Z

I am getting an error in the lcg build with this

[100%] Generating tutorials/hsimple.root
root.exe: /build/jenkins/workspace/lcg_nightly_pipeline/build/projects/ROOT-default-root-include-path/src/ROOT/default-root-include-path/interpreter/llvm-project/clang/lib/Serialization/ASTReader.cpp:4332: clang::ASTReader::ASTReadResult clang::ASTReader::ReadModuleMapFileBlock(RecordData&, ModuleFile&, const ModuleFile*, unsigned int): Assertion `M && M->Name == F.ModuleName && "found module with different name"' failed.

https://lcgapp-services.cern.ch/cdash/build/98193/files

pcanal · 2025-06-27T17:06:56Z

What do you practically mean by:

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 · 2025-06-30T12:48:04Z

Thank you all for your comments!

@hageboeck

@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?

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
If the include path (to e.g. Vc) at runtime is different to the one at build time, which might happen for spack installations using its binary cache.
In that case the thisroot.* files must be patched.

@andresailer
Thank you for running the lcg build. Will investigate the failure.

cmake/modules/RootMacros.cmake

LukasBreitwieser · 2025-08-12T09:04:16Z

Reported CI test failures also present on yesterdays ROOT Main nightly.

@andresailer I sent a PR for lcgcmake.
I ran the dev3 lcgcmake build on alma9 and root-tests passed. Do you want to check the other stacks too?

andresailer · 2025-08-14T08:52:38Z

I think for me this is fine as it is. Thanks!

andresailer · 2025-09-15T14:52:44Z

What is the status here? Is this going to get merged soon?

andresailer · 2025-09-15T15:57:50Z

The tests added in #19667
are not working in the nightly tests because

root/tutorials/CMakeLists.txt

Line 23 in c8faaaf

ROOT_INCLUDE_PATH=${CMAKE_BINARY_DIR}/tutorials/io/tree

overwrites our existing ROOT_INCLUDE_PATH. So would be nice if this here is merged, or we need a fix in that other line.

cmake/modules/RootMacros.cmake

hageboeck

Please see a possible oversight below, but otherwise, LGTM!

+don't forget to squash, please. 🙂

cmake/modules/RootMacros.cmake

hageboeck · 2025-10-01T12:30:04Z

CMakeLists.txt

+  set(hsimple_cmd COMMAND ROOT_INCLUDE_PATH="${DEFAULT_ROOT_INCLUDE_PATH}" ${ld_library_path}=${CMAKE_LIBRARY_OUTPUT_DIRECTORY}:$ENV{${ld_library_path}}
+                          ROOT_INCLUDE_PATH=${DEFAULT_ROOT_INCLUDE_PATH}


This looks like a small oversight:

Suggested change

set(hsimple_cmd COMMAND ROOT_INCLUDE_PATH="${DEFAULT_ROOT_INCLUDE_PATH}" ${ld_library_path}=${CMAKE_LIBRARY_OUTPUT_DIRECTORY}:$ENV{${ld_library_path}}

ROOT_INCLUDE_PATH=${DEFAULT_ROOT_INCLUDE_PATH}

set(hsimple_cmd COMMAND ROOT_INCLUDE_PATH="${DEFAULT_ROOT_INCLUDE_PATH}" ${ld_library_path}=${CMAKE_LIBRARY_OUTPUT_DIRECTORY}:$ENV{${ld_library_path}}

I think the additional unquoted ROOT_INCLUDE_PATH=${DEFAULT_ROOT_INCLUDE_PATH} actually made things work. Because the quotes get escaped by cmake and then things no longer work.

This might well be the case. @LukasBreitwieser ?

I can tell you things definitely do not work (https://lcgapp-services.cern.ch/cdash/build/119422/files) and if I drop the quotes things are working.

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 `BUILD_ROOT_INCLUDE_PATH`. This function allows us to append include paths of external libraries to `BUILD_ROOT_INCLUDE_PATH`, for example: `DEFAULT_ROOT_INCLUDE_PATH("${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.

…re on stacks as reported by Andre Sailer on root-project#19179

…re on stacks (#20098) as reported by Andre Sailer on #19179 CMake escapes the quotes when the command is executed (at least on Linux) which renders the ROOT_INCLUDE_PATH broken Co-authored-by: Lukas Johannes Breitwieser <Lukas Johannes Breitwieser> Co-authored-by: Andre Sailer <andre.philippe.sailer@cern.ch>

LukasBreitwieser requested review from bellenot and dpiparo as code owners June 25, 2025 14:37

LukasBreitwieser force-pushed the default-root-include-path branch from fdc4019 to 4c0e474 Compare June 25, 2025 14:45

LukasBreitwieser requested a review from guitargeek June 25, 2025 14:52

ferdymercury requested a review from hageboeck June 25, 2025 16:26

dpiparo assigned LukasBreitwieser Jun 25, 2025

hageboeck requested changes Jun 26, 2025

View reviewed changes

cmake/modules/RootMacros.cmake Outdated Show resolved Hide resolved

config/thisroot.sh Show resolved Hide resolved

cmake/modules/RootMacros.cmake Outdated Show resolved Hide resolved

config/thisroot.sh Show resolved Hide resolved

andresailer reviewed Jun 26, 2025

View reviewed changes

cmake/modules/RootMacros.cmake Show resolved Hide resolved

andresailer reviewed Jun 26, 2025

View reviewed changes

hageboeck reviewed Jun 27, 2025

View reviewed changes

config/thisroot.sh Show resolved Hide resolved

andresailer reviewed Jul 1, 2025

View reviewed changes

cmake/modules/RootMacros.cmake Outdated Show resolved Hide resolved

LukasBreitwieser force-pushed the default-root-include-path branch from 6feedf8 to 8d5fa90 Compare July 28, 2025 10:06

LukasBreitwieser force-pushed the default-root-include-path branch from 7c876fb to b418258 Compare August 6, 2025 09:23

LukasBreitwieser force-pushed the default-root-include-path branch 2 times, most recently from ec6ae3c to e18a505 Compare August 22, 2025 17:33

LukasBreitwieser force-pushed the default-root-include-path branch from e18a505 to 848a6f0 Compare September 4, 2025 09:35

LukasBreitwieser force-pushed the default-root-include-path branch from 848a6f0 to 1e15e33 Compare September 17, 2025 12:58

andresailer requested a review from hageboeck September 24, 2025 15:33

hageboeck reviewed Sep 25, 2025

View reviewed changes

cmake/modules/RootMacros.cmake Outdated Show resolved Hide resolved

hageboeck approved these changes Oct 1, 2025

View reviewed changes

hageboeck added the pr:squash on merge label Oct 1, 2025

LukasBreitwieser force-pushed the default-root-include-path branch from c378442 to f23735b Compare October 10, 2025 10:18

LukasBreitwieser force-pushed the default-root-include-path branch from f23735b to 8d801f2 Compare October 10, 2025 12:32

dpiparo merged commit 301e078 into root-project:master Oct 11, 2025
87 of 96 checks passed

LukasBreitwieser pushed a commit to LukasBreitwieser/root that referenced this pull request Oct 13, 2025

Remove quotes around DEFAULT_ROOT_INCLUDE_PATH to address build failu…

9e4b590

…re on stacks as reported by Andre Sailer on root-project#19179

LukasBreitwieser mentioned this pull request Oct 13, 2025

Remove quotes around DEFAULT_ROOT_INCLUDE_PATH to address build failure on stacks #20098

Merged

jmcarcell mentioned this pull request Oct 15, 2025

Vc.pcm not found when running root after building with external Vc with runtime_cxxmodules ON #18949

Closed

1 task

andresailer mentioned this pull request Dec 16, 2025

ROOTTEST_COMPILE_MACRO: fix default_root_include_path #20737

Merged

1 task

		set(hsimple_cmd COMMAND ROOT_INCLUDE_PATH="${DEFAULT_ROOT_INCLUDE_PATH}" ${ld_library_path}=${CMAKE_LIBRARY_OUTPUT_DIRECTORY}:$ENV{${ld_library_path}}
		ROOT_INCLUDE_PATH=${DEFAULT_ROOT_INCLUDE_PATH}

Conversation

LukasBreitwieser commented Jun 25, 2025

Uh oh!

github-actions bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

hageboeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hageboeck commented Jun 26, 2025

Uh oh!

Uh oh!

andresailer Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

LukasBreitwieser Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andresailer commented Jun 27, 2025

Uh oh!

pcanal commented Jun 27, 2025

Uh oh!

LukasBreitwieser commented Jun 30, 2025

Uh oh!

Uh oh!

LukasBreitwieser commented Aug 12, 2025

Uh oh!

andresailer commented Aug 14, 2025

Uh oh!

andresailer commented Sep 15, 2025

Uh oh!

andresailer commented Sep 15, 2025

Uh oh!

Uh oh!

hageboeck left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hageboeck Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

andresailer Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

hageboeck Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

andresailer Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions bot commented Jun 25, 2025 •

edited

Loading

hageboeck left a comment •

edited

Loading