Conversation
One of the installed cmake scripts of vecgeom has a runtime dependency of CMake 3.24. To fix the issues that this breaks when using CMake < 3.24 during configuration of ROOT we don't allow CMake < 3.24 from the beginning. Fix license header.
📝 WalkthroughWalkthroughUpdates build and test orchestration, macOS SDK/sysroot handling, dependency versions and patches, macOS-specific build fixes (ROOT, VecGeom, pythia8), test submission flow, and multiple distro bootstrap/setup scripts and docs. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
FairSoft_test.cmake (1)
183-203:⚠️ Potential issue | 🟠 MajorA
ddsbuild failure preventsonnxruntimefrom being tested.Line 195 checks
NOT _ctest_build_errors, but_ctest_build_errorsis updated by theddsbuild on line 187. Since these are independent optional packages, addsfailure shouldn't blockonnxruntimetesting. Consider tracking their error counts separately or resetting before the onnxruntime build.🔧 Proposed fix — use separate error tracking
# Test the optional packages dds and onnxruntime # onnxruntime needs at least CMAKE 3.28 if(NOT _ctest_build_errors) ctest_build(RETURN_VALUE _ctest_build_retval NUMBER_ERRORS _ctest_build_errors TARGET "dds" FLAGS "-j${NCPUS}") if (_ctest_build_errors) set(_ctest_build_retval 255) endif() endif() fairsoft_ctest_submit() - if(NOT _ctest_build_errors AND CMAKE_VERSION VERSION_GREATER_EQUAL 3.28.0) + if(NOT _ctest_build_retval_main AND CMAKE_VERSION VERSION_GREATER_EQUAL 3.28.0) ctest_build(RETURN_VALUE _ctest_build_retval NUMBER_ERRORS _ctest_build_errors TARGET "onnxruntime" FLAGS "-j${NCPUS}") if (_ctest_build_errors) set(_ctest_build_retval 255) endif() endif()This requires saving the main build's error state before the optional package builds. Alternatively, if the intent is indeed to skip onnxruntime when dds fails, a comment clarifying this dependency would help.
🤖 Fix all issues with AI agents
In `@cmake/legacy.cmake`:
- Around line 378-388: The vecgeom ExternalProject_Add invocation is missing the
source cache dependency so its downloads can race with source extraction; update
the DEPENDS clause of ExternalProject_Add(vecgeom) to include the source-cache
dependency used elsewhere (either ${DEPENDS_ON_SOURCE_CACHE} or
${extract_source_cache_target}) alongside vc so that vecgeom waits for the
source cache extraction before starting.
- Around line 372-376: Update the processor check so ARM and AArch64 both select
the scalar VecGeom backend: change the if condition that currently uses
if(${CMAKE_SYSTEM_PROCESSOR} MATCHES "arm") to use CMAKE_SYSTEM_PROCESSOR
MATCHES "arm|aarch64" (i.e. include "aarch64" in the regex) and remove the
unnecessary ${} expansion; keep the existing set(vector
"-DVECGEOM_VECTOR=empty") for the ARM/AArch64 branch and set(vector
"-DVECGEOM_VECTOR=native") in the else branch.
In `@legacy/advanced.md`:
- Around line 62-71: Fix three minor grammar issues in the paragraph: change
"rootcling take the setting" to "rootcling takes the setting", change "With this
changes" to "With these changes", and change "macosx 15" to "macOS 15" so the
sentences read correctly and use proper casing.
In `@legacy/bootstrap-cmake.sh`:
- Line 22: The cmakeversion variable (cmakeversion="4.2.3") combined with the
existing filename construction breaks on macOS because CMake ships macOS
tarballs as "macos-universal" rather than "darwin-{arch}"; update the download
filename logic in legacy/bootstrap-cmake.sh (where filenames are built from
cmakeversion and platform/arch) to special-case darwin for v4.2.3 (and any
future versions that use macos-universal): when uname reports Darwin, map the
platform to "macos-universal" and use that exact token (instead of
darwin-{arm64,x86_64}) when composing the tarball name and checksum lookup so
the script requests cmake-4.2.3-macos-universal.tar.gz and the corresponding SHA
file.
In `@legacy/onnxruntime/fix_gcc13_compilation.patch`:
- Around line 20-22: The string(FIND) call uses an unquoted variable which can
be split if /etc/fedora-release contains list separators; change file(READ
"/etc/fedora-release" fedora_info) and then call string(FIND "${fedora_info}"
"38" fedora38) (quote the variable and the needle) and adjust the following
if(${fedora38} GREATER -1) block to match parent indentation; ensure quotes
around literals/variables in string(FIND) and keep consistent indentation for
the if/endif scope.
In `@legacy/README.md`:
- Around line 54-89: In the "3.1 CMake configure step for macOS users" section
of README.md fix the typos and redundant notice: change the phrase "on needs" to
"one needs", correct "macOs" to "macOS" wherever it appears (e.g., the example
comment and mentions), and remove or rephrase the final redundant notice line
"macOS users: Notice [macOS SDK](advanced.md#macos-sdk)!" so it doesn't repeat
the earlier guidance; update the paragraph text accordingly to preserve meaning
and capitalization consistency.
In `@legacy/root/fix_macos_sdk_for_rootcling.patch`:
- Around line 64-67: The comment for the --macosx-sdk case is incorrect; update
the inline comment in the case block (the line above the assignment to out in
the --macosx-sdk handler) so it describes that this option outputs the macOS SDK
path (e.g., "Output the macOS SDK path") rather than "Output the prefix"; locate
the --macosx-sdk case where out is set to "$out `@macosxsdk`@" and replace the
comment accordingly.
- Around line 87-88: The new elseif branch that sets rootcling_env (elseif(APPLE
AND CMAKE_OSX_SYSROOT) ... set(rootcling_env ...)) incorrectly hardcodes
LD_LIBRARY_PATH; change it to use the existing ${ld_library_path} variable so it
resolves to DYLD_LIBRARY_PATH on macOS (and LD_LIBRARY_PATH on Linux) and
include SDKROOT=${CMAKE_OSX_SYSROOT} as before; update the set(rootcling_env
...) call in that branch to mirror the else branch pattern using
${ld_library_path} and SDKROOT for consistency.
In `@legacy/setup-fedora.sh`:
- Line 12: The package list line containing "libunistring-devel libuuid-devel
libxml2-devel libzstd-devel lz4-devel m4 make make mesa-libGL-devel" has a
duplicated "make"; remove the second "make" from that package list in
legacy/setup-fedora.sh so the list contains only one "make" token.
In `@legacy/setup-opensuse.sh`:
- Line 19: The assignment to os_version uses `${...}` (parameter expansion)
instead of command substitution so the pipeline is never executed; replace the
expression with a proper command substitution using $(...) or, better, source
/etc/os-release and read VERSION or VERSION_ID directly. Update the line that
sets os_version (the os_version=... expression) to either use os_version=$(grep
'VERSION=' /etc/os-release | cut -d\" -f2) or source /etc/os-release and set
os_version="$VERSION" (or "$VERSION_ID") to reliably capture the OS version.
- Around line 12-13: Remove the duplicated package list entry "libXmu-devel
libXpm-devel libXrender-devel liblz4-devel libzstd-devel lsb-release Mesa-devel"
in legacy/setup-opensuse.sh: keep a single occurrence of that package string in
the zypper install arguments (remove the copy-paste duplicate), and scan the
surrounding install command to ensure there are no other repeated package
tokens.
- Around line 20-35: The conditional checks use the literal Jinja/Ansible string
"{{ os_version }}" so both branches never run; change the conditional
expressions in the [[ ... ]] tests to use the shell variable (e.g. "$os_version"
or "${os_version}") instead of the literal, updating both occurrences of [[ "{{
os_version }}" == "15.6" ]] and [[ "{{ os_version }}" == "16.0" ]] so the
intended package-install branches (gcc14/python310/protobuf-c-devel) execute at
runtime.
- Around line 29-31: Don't remove the system python package; replace the three
commands (zypper remove -y python3 python3-devel, rm /usr/bin/python3, ln -s
/usr/bin/python3.10 /usr/bin/python3) with a safer approach: leave the system
package installed and either register python3.10 with update-alternatives (so
/usr/bin/python3 is managed) or, if you must create a symlink, only create it
without removing the package and guard the removal with a safe check (use a
force/remove-if-present behavior) and verify /usr/bin/python3.10 exists before
linking; update the script around the zypper/remove and ln -s operations to
implement one of these safer flows.
🧹 Nitpick comments (9)
legacy/onnxruntime/fix_gcc13_compilation.patch (2)
29-36: Inconsistent indentation onendif()closings.Lines 35 and 36 use different indentation levels (5 and 3 spaces respectively) compared to the rest of the block which uses multiples of 2. This is likely a copy-paste artifact.
Proposed fix
if(NOT(Fedora38_FOUND)) if(CMAKE_CXX_COMPILER_VERSION GREATER_EQUAL 13.1 AND NOT(APPLE)) set(mlas_platform_srcs_avx2 ${mlas_platform_srcs_avx2} ${MLAS_SRC_DIR}/x86_64/cvtfp16Avx.S ) - endif() - endif() + endif() + endif()
49-50:__GNUC_RH_RELEASE__used withoutdefined()guard — may trigger-Wundefwarnings.On non-Red Hat toolchains,
__GNUC_RH_RELEASE__is undefined. While undefined macros evaluate to0in#ifexpressions (so the logic is correct), compilers with-Wundefenabled will emit a warning. Adefined()check makes the intent explicit.Proposed fix
-#if (defined(_MSC_VER) && (_MSC_VER >= 1933)) || (defined(__GNUC__) && (__GNUC__ >= 13) && (__GNUC_RH_RELEASE__ != 7)) +#if (defined(_MSC_VER) && (_MSC_VER >= 1933)) || (defined(__GNUC__) && (__GNUC__ >= 13) && (!defined(__GNUC_RH_RELEASE__) || (__GNUC_RH_RELEASE__ != 7)))legacy/root/fix_rpath_info.patch (1)
31-42: Inconsistent MSVC guard forROOT_APPEND_LIBDIR_TO_INSTALL_RPATHin pythonizations.In the CPyCppyy and cppyy-backend patches, the
ROOT_APPEND_LIBDIR_TO_INSTALL_RPATHcall is guarded byif(NOT MSVC), but inpythonizations/CMakeLists.txt(line 39) it's called unconditionally. If this is intentional (e.g., MSVC doesn't reach this code path), a brief comment would clarify; otherwise, wrap it in the same guard for consistency.legacy/README.md (2)
77-84: Add language specifiers to fenced code blocks.Static analysis (markdownlint MD040) flags lines 77 and 83 for missing language specifiers on fenced code blocks. Adding
bashortextimproves rendering and accessibility.
163-168: Trailing whitespace on line 164.Line 164 contains only whitespace. Consider removing the trailing spaces.
cmake/legacy.cmake (4)
62-70: Prefer variable name over expansion inif()condition.On Line 62,
${CMAKE_MAKE_PROGRAM}is eagerly expanded beforeif()evaluates theMATCHESpredicate. This works in common cases, but paths with spaces or semicolons would break the condition. The idiomatic CMake way is to pass the variable name directly:-if(APPLE AND ${CMAKE_MAKE_PROGRAM} MATCHES ".*/make$") +if(APPLE AND CMAKE_MAKE_PROGRAM MATCHES ".*/make$")CMake's
if()auto-dereferences variable names inMATCHESexpressions.
110-110: Remove leftover debug artifact.This commented-out
FATAL_ERRORmessage is a debug leftover and should be removed before merging.-#message(FATAL_ERROR "CMAKE_DEFAULT_ARGS: ${CMAKE_DEFAULT_ARGS}")
429-431: Dead condition:CMAKE_VERSION VERSION_GREATER 3.15is always true.Since
cmake_minimum_requiredis now 3.24 (Line 8), theCMAKE_VERSION VERSION_GREATER 3.15check on Line 429 is always satisfied. This can be simplified:-if(APPLE AND CMAKE_VERSION VERSION_GREATER 3.15) - set(root_builtin_glew "-Dbuiltin_glew=ON") -endif() +if(APPLE) + set(root_builtin_glew "-Dbuiltin_glew=ON") +endif()Or merge it into the existing
if(APPLE)block that follows on Line 432.
112-117: Ensure consistent RPATH configuration across all CMake-based packages.
FAIRSOFT_RPATH_ARGSis applied to only 7 of 21 packages: zeromq, clhep, geant4, vmc, geant3, vgm, and geant4_vmc. The remaining 14 packages (fmt, fairlogger, fairmq, hepmc, vc, vecgeom, flatbuffers, faircmakemodules, boost, dds, pythia8, root, onnxruntime, fairsoft-config) do not receive these RPATH arguments. ROOT handles RPATH explicitly with-Drpath=ON, but packages like fmt, fairlogger, fairmq, hepmc, vc, vecgeom, and flatbuffers have no visible RPATH configuration. This inconsistency could result in some libraries not being built with proper RPATH support, potentially leaving gaps in eliminating(DY)LD_LIBRARY_PATHreliance.
Add additional packages.
The default bash version on macosx is to old to support all features needed in the test suite of FairRoot. The system version of make doesn't properly support the jobserver such that the build process ins't properly parallelized.
With this commit the correct RPATH information is added to the shared libraries and binaries of all installed packages. For packages not adding the info already properly we add some CMake variables in our meta build system to pass the needed information to the CMake based build system of the individual packages. With this change the FairSoft installtion is definitely not relocatable any longer, but it is not clear if it was before. With the changes the environment varaible (DY)LD_LIBRARY_PATH shouldn't be needed any longer. But this fact still has to be tested. This PR fixes FairRootGroup#535
On some systems the default compiler is to old to compile FairSoft. Allow using a newer non system compiler by using CMAKE variables which are properly passed to the package build systems.
Before a SDK defined via the CMake parameter CMAKE_OSX_SYSROOT was overwritten with the value extracted from brew.
Pythia6 isn't supported by ROOT any longer.
Take code form Cern GitLab repo. There is no package any longer.
In contrast to the default packages onnxruntime needs newer versions of CMake (cmake >=3.28) and the compiler (gcc >= 11.1). For the optional package this is tolerated. The error messages are meaningful and point to the problem of insufficient versions. Do not search for any packages in the system for packages added using FetchContent. The packages included with onnxruntime are verry specific and may interfere with system installed packages. Add patch needed for Fedora 38. Fixes FairRootGroup#510
Add a patch such that rootcling also uses the specified SDK if defined with -DCMAKE_OSX_SYSROOT. This allows in some cases to compile older ROOT versions with newer compiler versions. For example it becomes possible to compile ROOT 6.30 with Apple Clang 17 on macosx 15.6. All projects depending on ROOT need also to set CMAKE_OSX_SYSROOT to the same value used when building ROOT. Otherwise the dictionary generation may fail. Currently there is no way to automatically extract the information from the ROOT CMake configuration.
Describe some known problems on macosx and the corresponding solutions.
Pass the needed information to the boost build system if a non standard SDK is used.
Pass the needed information to the pythia8 build system if a non standard SDK is used.
Properly pass CMAKE_OSX_SYSROOT to the build and test processes.
Some legacy C code can't be compiled with C standard version 23 which is the new default for gcc15 so use C standard version 11 for the compilation.
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
FairSoft_test.cmake (1)
183-204:⚠️ Potential issue | 🟠 MajorOptional
dds/onnxruntimebuild failures cascade and block the main test suite.The
_ctest_build_errors/_ctest_build_retvalvariables are shared between the main build and the optional package builds. Ifddsfails on line 190,_ctest_build_retvalis set to 255 (line 191), and then line 232 skips all FairRoot tests with "Skipping Tests, Build Failed." This means an optional package failure prevents the main test suite from running.Consider isolating the optional builds' error state, or resetting
_ctest_build_retvalafter optional builds so the main test suite still runs:Proposed fix
# Test the optional packages dds and onnxruntime # onnxruntime needs at least CMAKE 3.28 + set(_optional_build_failed FALSE) if(NOT _ctest_build_errors) ctest_build(RETURN_VALUE _ctest_build_retval NUMBER_ERRORS _ctest_build_errors TARGET "dds" FLAGS "-j${NCPUS}") if (_ctest_build_errors) - set(_ctest_build_retval 255) + set(_optional_build_failed TRUE) + set(_ctest_build_errors 0) endif() endif() fairsoft_ctest_submit() - if(NOT _ctest_build_errors AND CMAKE_VERSION VERSION_GREATER_EQUAL 3.28.0) + if(NOT _optional_build_failed AND CMAKE_VERSION VERSION_GREATER_EQUAL 3.28.0) ctest_build(RETURN_VALUE _ctest_build_retval NUMBER_ERRORS _ctest_build_errors TARGET "onnxruntime" FLAGS "-j${NCPUS}") if (_ctest_build_errors) - set(_ctest_build_retval 255) + set(_optional_build_failed TRUE) endif() endif() fairsoft_ctest_submit() + # Reset so optional failures don't block the main test suite + set(_ctest_build_retval 0)
🤖 Fix all issues with AI agents
In `@legacy/bootstrap-cmake.sh`:
- Around line 34-37: The grep pattern is wrapped in single quotes so ${unames}
and ${unamem} are not expanded; update the assignment of grepstring (the
variable named grepstring near where cmaketargz is set) to use double quotes so
the shell expands ${unames} and ${unamem} (e.g.,
grepstring="${unames}.*${unamem}.*\.tar\.gz"), ensuring the regex and escaped
dot remain intact for correct checksum matching.
- Around line 13-18: The script's install-directory check uses a misspelled
command `eche` which will cause "command not found" before the exit; replace the
`eche "Please create the directory."` invocation with `echo "Please create the
directory."` so the explanatory message is printed correctly in the
install-directory check block (the conditional that tests `$? -ne 0` and
currently prints "Install directory $installdir must exist on macOS.").
In `@legacy/README.md`:
- Around line 87-89: Combine the incomplete sentence "More information can be
found at" and the separate "**macOS users**: Notice [macOS
SDK](advanced.md#macos-sdk)!" into a single concise sentence that links to the
advanced section (e.g., "For macOS users, see the macOS SDK section
(advanced.md#macos-sdk) for more information.") and remove the redundant
standalone notice line so the README reads as one complete, non-repetitive
sentence.
In `@legacy/setup-opensuse.sh`:
- Line 18: The os_version assignment uses an unanchored grep which matches both
VERSION= and VERSION_ID= causing os_version to contain multiple lines and
breaking subsequent comparisons; fix by sourcing /etc/os-release (e.g., .
/etc/os-release) and then use the VERSION variable directly, or if you prefer to
keep parsing, change the grep to anchor '^VERSION=' so os_version only captures
the intended VERSION value used in the comparisons that follow.
🧹 Nitpick comments (7)
legacy/root/fix_rpath_info.patch (2)
22-27: Nit: extra blank line afterendif().There's a double blank line separating the new block from
set_property. One blank line would be consistent with the surrounding style.
39-40: Inconsistent MSVC guard compared to the other two patch hunks.In CPyCppyy and cppyy-backend,
ROOT_APPEND_LIBDIR_TO_INSTALL_RPATHis guarded byif(NOT MSVC), but here in pythonizations it's called unconditionally. If the function is safe to call on MSVC (likely a no-op), this works but is inconsistent. Consider wrapping for consistency:Proposed diff
-ROOT_APPEND_LIBDIR_TO_INSTALL_RPATH(${libname} ${CMAKE_INSTALL_LIBDIR}) +if(NOT MSVC) + ROOT_APPEND_LIBDIR_TO_INSTALL_RPATH(${libname} ${CMAKE_INSTALL_LIBDIR}) +endif()legacy/onnxruntime/fix_gcc13_compilation.patch (2)
35-36: Inconsistent indentation onendif()closings.Line 35 uses 9 spaces and line 36 uses 7 spaces. These should be aligned with their corresponding
if()blocks (8 spaces each, matching the surrounding scope).♻️ Proposed fix
- endif() - endif() + endif() + endif()
49-50: The__GNUC_RH_RELEASE__guard is functionally correct but brittle.On non–Red Hat compilers,
__GNUC_RH_RELEASE__is undefined and evaluates to0in the preprocessor, so0 != 7istrue— preserving the original behavior. On Fedora 38's GCC 13 (where__GNUC_RH_RELEASE__is7), it correctly disables the AVX NE CONVERT path.Consider wrapping the check with
defined()for clarity and to avoid relying on the implicit-zero behavior of undefined macros:♻️ Proposed alternative
-#if (defined(_MSC_VER) && (_MSC_VER >= 1933)) || (defined(__GNUC__) && (__GNUC__ >= 13) && (__GNUC_RH_RELEASE__ != 7)) +#if (defined(_MSC_VER) && (_MSC_VER >= 1933)) || (defined(__GNUC__) && (__GNUC__ >= 13) && (!defined(__GNUC_RH_RELEASE__) || (__GNUC_RH_RELEASE__ != 7)))legacy/README.md (2)
77-77: Fenced code blocks are missing language identifiers.Static analysis (markdownlint MD040) flags the code blocks on lines 77 and 83. Adding a language hint (e.g.
```bash) improves rendering and accessibility.📝 Proposed fix
-``` +```bash -DCMAKE_OSX_SYSROOT=<full path to SDK directory>-``` +```bash -DCMAKE_OSX_SYSROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdkAlso applies to: 83-83
163-164: Trailing whitespace in the table area.Line 164 appears to contain only whitespace. This is a minor formatting artifact that could be cleaned up.
cmake/legacy.cmake (1)
280-285:CMAKE_POLICY_VERSION_MINIMUMis a CMake 4.x workaround — document intent.Lines 285, 319, and 330 all pass
-DCMAKE_POLICY_VERSION_MINIMUM=3.5to older upstream projects. This is the standard approach to suppress CMake 4.x policy errors in projects that haven't updated theircmake_minimum_required. Consider adding a brief comment near the first usage (or nearFAIRSOFT_RPATH_ARGS) explaining this pattern so future maintainers don't accidentally remove it.
The name of the tar file as well as the content has changed on macOS. Adapt the installation to those changes.
Only build onnxruntime optionaly if cmake >= 3.28.0.
3 participants
This PR updates nearly all packages to their latest available versions. The main changes are