Make non-system include paths accessible at runtime #19179

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.

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.

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

Test Results

20 files 20 suites 3d 9h 11m 55s ⏱️
3 066 tests 3 066 ✅ 0 💤 0 ❌
59 718 runs 59 718 ✅ 0 💤 0 ❌

Results for commit 107b911.

♻️ This comment has been updated with latest results.

hageboeck

Nice! I added a few comments below

hageboeck · 2025-06-26T13:16:31Z

cmake/modules/RootMacros.cmake

+      set(${is_system_path} true PARENT_SCOPE)
+    endif()
+  endforeach()
+  set(${is_system_path} false PARENT_SCOPE)


The canonical bools are all caps, and we can break out early:

Suggested change

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)

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

config/thisroot.sh

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


newpath seems to be undefined here, or did I miss something?

hageboeck · 2025-06-26T13:24:41Z

cmake/modules/RootMacros.cmake

+# 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)


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.

hageboeck · 2025-06-26T13:30:41Z

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
+      export ROOT_INCLUDE_PATH


Suggested change

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

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?

andresailer · 2025-06-26T15:48:38Z

cmake/modules/RootMacros.cmake

+  foreach (dir ${CMAKE_SYSTEM_PATH})
+    if ("${path}" STREQUALS "$dir")
+      set(${is_system_path} true PARENT_SCOPE)
+    endif()


Suggested change

endif()

return()

endif()

Maybe my cmake is too rusty, but shouldn't we not always set is_system_path to false at the end of the function?

Indeed, I think the return is needed

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?

hageboeck · 2025-06-27T06:58:08Z

config/thisroot.sh

@@ -162,6 +166,14 @@ set_environment()
   else
      JUPYTER_CONFIG_DIR=$ROOTSYS/etc/notebook:$JUPYTER_CONFIG_DIR; export JUPYTER_CONFIG_DIR
   fi
+
+   if [ -z "${ROOT_INCLUDE_PATH-}" ]; then


The variable name contains an extra - if I'm not mistaken, so this would always be zero.

This is bash syntax for using a default value (of nothing) when the variable is not set.

OK, I had to read up on the subtle difference between ${VAR:-} vs ${VAR-} ...

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

andresailer reviewed Jun 26, 2025

View reviewed changes

hageboeck reviewed Jun 27, 2025

View reviewed changes

Fix platform specific path separation

107b911

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make non-system include paths accessible at runtime #19179

Make non-system include paths accessible at runtime #19179

Uh oh!

LukasBreitwieser commented Jun 25, 2025

Uh oh!

github-actions bot commented Jun 25, 2025 •

edited

Loading

Uh oh!

hageboeck left a comment

Uh oh!

hageboeck Jun 26, 2025 •

edited

Loading

Uh oh!

hageboeck Jun 26, 2025

Uh oh!

hageboeck Jun 26, 2025

Uh oh!

hageboeck Jun 26, 2025

Uh oh!

hageboeck commented Jun 26, 2025

Uh oh!

andresailer Jun 26, 2025

Uh oh!

hageboeck Jun 27, 2025

Uh oh!

andresailer Jun 26, 2025

Uh oh!

hageboeck Jun 27, 2025

Uh oh!

andresailer Jun 27, 2025

Uh oh!

hageboeck Jun 27, 2025 •

edited

Loading

Uh oh!

andresailer commented Jun 27, 2025

Uh oh!

pcanal commented Jun 27, 2025

Uh oh!

Uh oh!

Make non-system include paths accessible at runtime #19179

Are you sure you want to change the base?

Make non-system include paths accessible at runtime #19179

Uh oh!

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!

hageboeck Jun 26, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hageboeck commented Jun 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hageboeck Jun 27, 2025 • 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!

andresailer commented Jun 27, 2025

Uh oh!

pcanal commented Jun 27, 2025

Uh oh!

Uh oh!

github-actions bot commented Jun 25, 2025 •

edited

Loading

hageboeck Jun 26, 2025 •

edited

Loading

hageboeck Jun 27, 2025 •

edited

Loading