Do not include ROOT's CMake "use" file to avoid potential nlohmann_json conflicts #556
Conversation
ROOT's "use" file executes CMake {include,link}_directories as well
as appending to CMAKE_CXX_FLAGS and executing add_definitions. The
primary issue here is the use of include_directories, which
- adds "-I" rather than "-isystem" paths to compile commands
- mixes these with project-local setttings
- flags appear in all subsequent compilations at the same directory
scope any further subdirectories
This was observed to cause an issue at link time for Celeritas programs
using nlohmann_json, where the found ROOT had been installed with a
builtin nlohmann_json of different version to that found by CMake. The
above scoped injection of the ROOT include path meant that different
nlohmann_json headers were used at library, application compile time
resulting in undefined symbol errors.
Remove inclusion of ROOT's use file from Celeritas build scripts.
ROOTConfig.cmake, and hence find_package(ROOT) now set up the needed
CMAKE_MODULE_PATH and other ROOT-related variables with relevant compile
options propagated through target usage requirements.
|
Weird, thanks @drbenmorgan . I thought including this file was necessary to get the ROOT macros to load (since it seems like the path to that file might be an implementation detail?). If this works, then great! and we could even move it outside the function, which I created mainly to avoid propagating the other variables/includes/etc. |
|
Hi @sethrj, indeed I think it was necessary in older ROOT versions, but current versions include the |
src/celeritas/CMakeLists.txt
Outdated
| @@ -140,7 +140,6 @@ if(CELERITAS_USE_ROOT) | |||
| # Call the ROOT library generation inside a function to prevent ROOT-defined | |||
| # directory properties from escaping | |||
There was a problem hiding this comment.
Since this no longer applies, and the function is only called once (just for this purpose), can you delete the function and just inline the content?
There was a problem hiding this comment.
Done in latest commit - the use of include_directories is safe in the sense it's only adding project local paths, as is the setting of CMAKE_LIBRARY_OUTPUT_DIRECTORY.
Longer term, I think use of include_directories can be removed completely if this code block is moved after the declaration of the celeritas target. ROOT will then pick up usage requirements from the target. However, that will require a celeritas_target_sources command analogous to target_sources to add the object lib to celeritas afterwards. Best done as/when that functionality becomes needed more widely.
|
If the CI works, then it's an "old enough" change for me. We're going to have to build with relatively recent versions of vecgeom etc., so we shouldn't worry too much about compatibility with older software. |
Use of a function to hide ROOT setting directory properties is now redundant as ROOT "use" file no longer included. Inline function code in script, removing/updating function variables as needed.
|
Just for reference after a bit more digging, this occurred when building Celeritas on macOS against a Homebrew stack with ROOT 6.26/06. That still uses a builtin nlohmann_json as Homebrew haven't yet patched for root-project/root#11130 and root-project/root#11312, whilst Spack have. Once there's a version bump in Homebrew to ROOT 6.26/08, that should be fixable. Edit: coming soon, including removal of builtin nlohmann_json: Homebrew/homebrew-core#114543 |
sethrj
added
core
sethrj
added
external
ROOT's "use" file for CMake executes CMake
include_directories,link_directoriesandadd_definitionsas well as appending toCMAKE_CXX_FLAGSamong others. The primary issue here is the use ofinclude_directories, which-Irather than-isystempaths to compile commandsThis was observed to cause an issue at link time for Celeritas programs using
nlohmann_json, where the found ROOT install was using a builtinnlohmann_jsonof different version to that found by CMake. The above scoped injection of the ROOT include path meant that differentnlohmann_jsonheaders were used at library and application compile time resulting in undefined symbol errors when linking.Whilst that's fundamentally an issue with the install of ROOT I was building against (to be fixed!), Celeritas does not appear to need the use file. ROOT's CMake config file should include
ROOTMacros.cmakefor the dictionary generation, and usage requirements should be propagated through the imported targets. I've removed inclusion of ROOT's use file from Celeritas build scripts in this PR, and builds appear o.k. both on macOS and CentOS with different ROOT installs, however, let's see if CI throws up any issues.