Update master branch to feb26 release#585
Conversation
It's only relevant to our legacy system, so let's move it there.
When brew installs software to a directory other than /usr/local boost can't find the packages since they are not in the default search paths. This is the case on Apple silicon. Let CMake find the packages and pass the proper information to the boost build process. Add CMake module to find the ZSTD installation.
This feature is frequently broken on various platforms.
Add dependency for the protobuf development package where missing.
When rerunning the build process several times CMake tried to apply the patches several times. Now the patch stage is only done once which fixes the issue.
DDS is removed from the all traget such that it isn't build any longer with the other packages. If the installation of DDS is needed one needs to install the package explicitly.
Switch off the root builtin version if a system installation exist. On macosx there is a compilation error otherwise.
With new versions of the OS and/or the command line tools a OpenGL version installed with brew was used which somehow doesn't work. With the change the system version from the OpenGL framework is used.
The default setting for the pythia8 interface was switched from ON to OFF such that we have to switch it on explicitely. The change in ROOT was done with commit [ebe6694](root-project/root@ebe6694)
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.
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 #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 #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.
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.
FairSoft release feb26
📝 WalkthroughWalkthroughThis PR modernizes FairSoft with conditional macOS SDK detection, updates ~15 dependency versions and adds/improves build patches, relocates the CMake bootstrap script to ChangesFairSoft feb26 Release Update
Sequence Diagram(s)Not applicable—this PR consists primarily of configuration updates, dependency version bumps, and setup/documentation changes without introducing new multi-component control flows or external interactions that would benefit from visualization. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmake/legacy.cmake (1)
538-559:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate
onnxruntimeon CMake 3.28 or later to align with test configuration.
FairSoft_test.cmakeconditionally buildsonnxruntimeonly whenCMAKE_VERSION >= 3.28.0, reflecting the minimum requirement of ONNX Runtime v1.24.1. The unconditionalExternalProject_Add(onnxruntime ...)incmake/legacy.cmakecontradicts this, exposing users on CMake 3.24–3.27 to a broken optional target.Wrap the onnxruntime definition in:
if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.28.0) list(APPEND packages onnxruntime) set(onnxruntime_version "1.24.1") ExternalProject_Add(onnxruntime ...) else() message(STATUS "Skipping optional onnxruntime target: requires CMake >= 3.28.0") endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/legacy.cmake` around lines 538 - 559, The onnxruntime ExternalProject_Add block (including list(APPEND packages onnxruntime) and set(onnxruntime_version "1.24.1")) must be gated by a CMake version check to avoid creating the optional onnxruntime target on older CMake; wrap the existing onnxruntime definition (the ExternalProject_Add(onnxruntime ...) block and the preceding list()/set() lines) in if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.28.0) ... else() and in the else branch emit a status message like "Skipping optional onnxruntime target: requires CMake >= 3.28.0" so behavior matches FairSoft_test.cmake.
🧹 Nitpick comments (3)
legacy/setup-opensuse.sh (1)
1-5: ⚡ Quick winEnable shell strict mode to prevent partial system mutations on failure.
Given package management and filesystem mutations, fail-fast behavior is important here.
Suggested hardening
#! /bin/bash +set -euo pipefail zypper refresh && zypper update -y🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@legacy/setup-opensuse.sh` around lines 1 - 5, This script lacks shell strict mode and can leave partial system state on failure; modify legacy/setup-opensuse.sh (right after the #! /bin/bash shebang) to enable fail-fast and safer variable/pipe handling by adding strict-mode settings (e.g., set -euo pipefail and a safe IFS) and optionally a simple trap to report errors; ensure these settings apply before running zypper refresh/update/install so that functions/commands in the script (the zypper sequence) abort on error and do not continue partially.legacy/setup-fedora.sh (1)
14-14: ⚡ Quick winUse
python3-develinstead ofpython-develfor Fedora 39+.Line 14 uses
python-devel, which is not the canonical package for modern Fedora. Fedora 39+ provides this functionality throughpython3-devel; whilepython-develmay resolve via RPM "Provides", the recommended package name ispython3-devel.Proposed change
- python-devel python3-numpy python3-pygments python3-pyyaml readline-devel \ + python3-devel python3-numpy python3-pygments python3-pyyaml readline-devel \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@legacy/setup-fedora.sh` at line 14, Replace the obsolete package token `python-devel` with `python3-devel` in the package installation list so Fedora 39+ installs the canonical Python development package; locate the occurrence of `python-devel` in the setup script's package list and change it to `python3-devel`, keeping the surrounding line continuation/backslash and spacing intact.legacy/advanced.md (1)
62-72: 💤 Low valueConsider minor grammar and clarity improvements.
The new paragraph effectively explains the SDK versioning issue and solution. A few small refinements would improve readability:
- Line 62: "The downside with" → "The downside of"
- Line 64: "the connected SDK" → "the associated SDK"
- Line 67: "we add the possibility to define" → "it is now possible to define" (more direct)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@legacy/advanced.md` around lines 62 - 72, The paragraph contains minor grammar and clarity issues: replace "The downside with" with "The downside of", change "the connected SDK" to "the associated SDK", and rephrase "we add the possibility to define an older SDK when running CMake" to "it is now possible to define an older SDK when running CMake"; keep the rest of the paragraph intact (references: the phrase "The downside with", "the connected SDK", and "we add the possibility to define an older SDK when running CMake").
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmake/legacy.cmake`:
- Around line 370-374: The conditional checking CMAKE_SYSTEM_PROCESSOR only
matches "arm" and misses "aarch64", causing incorrect -DVECGEOM_VECTOR
selection; update the MATCHES pattern (or add an additional check) to include
aarch64 (e.g. match "arm" or "aarch64") so that the set(vector
"-DVECGEOM_VECTOR=empty") branch is chosen on ARM64 systems; ensure you
reference the CMAKE_SYSTEM_PROCESSOR check and the set(vector
"-DVECGEOM_VECTOR=...") assignments when making the change.
In `@legacy/bootstrap-cmake.sh`:
- Around line 45-47: The checksum verification currently always calls sha256sum
-c checksum.txt which fails on macOS; update the verification to detect which
tool is available and fall back to macOS-compatible shasum: after creating
checksum.txt, check if command -v sha256sum >/dev/null && run sha256sum -c
checksum.txt, else run shasum -a 256 -c checksum.txt (or exit nonzero if neither
exists). Reference the variables/file names cmakechecksums, checksum.txt,
grepstring and the sha256sum call when adding this conditional fallback.
- Around line 7-18: The script currently calls realpath on $1 (installdir)
unconditionally which fails for non-existent paths on Linux; change the logic to
only require an existing path on macOS (Darwin) and defer canonicalization on
Linux: detect platform with uname, and for Darwin keep the realpath + existence
check using installdir=$(realpath "$1") and the subsequent error branch, but for
other platforms set installdir="$1" (or use realpath -m if available) and do not
error; then after the mkdir -p later in the script canonicalize/install absolute
path (e.g., installdir=$(realpath "$installdir")) so creation succeeds on Linux
while preserving macOS behavior.
In `@legacy/README.md`:
- Around line 77-79: Add the missing fenced-code block language identifiers by
updating the code fences that contain the lines starting with
"-DCMAKE_OSX_SYSROOT=<full path to SDK directory>" and
"-DCMAKE_OSX_SYSROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk" to
use "bash" (i.e., change the opening triple-backticks to ```bash) so both code
blocks comply with markdownlint MD040; locate these blocks in legacy/README.md
and update both occurrences accordingly.
- Line 89: Remove the redundant notice line that reads "**macOS users**: Notice
[macOS SDK](advanced.md#macos-sdk)!" from README.md (the duplicate that appears
after the "3.1 CMake configure step for macOS users" section) so the earlier
detailed section remains the single reference to advanced.md#macos-sdk; simply
delete that duplicate line.
In `@legacy/setup-debian.sh`:
- Around line 8-16: The version-guard for CMake should use the actual minimum
required by the project: change the min_cmake value used in the install/guard
logic from "3.2" to "3.5" (matching the cmake_minimum_required(VERSION
3.5...3.28 FATAL_ERROR) in CMakeLists.txt) so the check in the setup script (the
min_cmake variable and its comparison logic around cmake installation) will
correctly prevent installing a too-old CMake.
In `@legacy/setup-opensuse.sh`:
- Around line 18-19: Replace branching on the unstable VERSION value with the
stable VERSION_ID from /etc/os-release: change the os_version assignment
(variable os_version) to read VERSION_ID (e.g. os_version=$(grep '^VERSION_ID='
/etc/os-release | cut -d\" -f2) or source /etc/os-release and use $VERSION_ID)
and update any other checks that currently grep for 'VERSION=' to use
'VERSION_ID' instead (including the other occurrence that references
os_version).
- Around line 28-30: The script currently removes distro Python and rewrites
/usr/bin/python3 via the commands "zypper remove -y python3 python3-devel", "rm
/usr/bin/python3", and "ln -s /usr/bin/python3.10 /usr/bin/python3"; instead,
revert those three operations: do not uninstall system python or delete
/usr/bin/python3, and remove the symlink step; change the build to call the
explicit interpreter "python3.10" (or adjust PATH to put a local shim in
/usr/local/bin) or use a virtualenv/venv created with python3.10 so system
package management remains intact and tools depending on /usr/bin/python3 are
not broken.
---
Outside diff comments:
In `@cmake/legacy.cmake`:
- Around line 538-559: The onnxruntime ExternalProject_Add block (including
list(APPEND packages onnxruntime) and set(onnxruntime_version "1.24.1")) must be
gated by a CMake version check to avoid creating the optional onnxruntime target
on older CMake; wrap the existing onnxruntime definition (the
ExternalProject_Add(onnxruntime ...) block and the preceding list()/set() lines)
in if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.28.0) ... else() and in the else
branch emit a status message like "Skipping optional onnxruntime target:
requires CMake >= 3.28.0" so behavior matches FairSoft_test.cmake.
---
Nitpick comments:
In `@legacy/advanced.md`:
- Around line 62-72: The paragraph contains minor grammar and clarity issues:
replace "The downside with" with "The downside of", change "the connected SDK"
to "the associated SDK", and rephrase "we add the possibility to define an older
SDK when running CMake" to "it is now possible to define an older SDK when
running CMake"; keep the rest of the paragraph intact (references: the phrase
"The downside with", "the connected SDK", and "we add the possibility to define
an older SDK when running CMake").
In `@legacy/setup-fedora.sh`:
- Line 14: Replace the obsolete package token `python-devel` with
`python3-devel` in the package installation list so Fedora 39+ installs the
canonical Python development package; locate the occurrence of `python-devel` in
the setup script's package list and change it to `python3-devel`, keeping the
surrounding line continuation/backslash and spacing intact.
In `@legacy/setup-opensuse.sh`:
- Around line 1-5: This script lacks shell strict mode and can leave partial
system state on failure; modify legacy/setup-opensuse.sh (right after the #!
/bin/bash shebang) to enable fail-fast and safer variable/pipe handling by
adding strict-mode settings (e.g., set -euo pipefail and a safe IFS) and
optionally a simple trap to report errors; ensure these settings apply before
running zypper refresh/update/install so that functions/commands in the script
(the zypper sequence) abort on error and do not continue partially.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d18d4ecb-8201-4039-815f-049bb7d4bc3d
📒 Files selected for processing (25)
FairSoftConfig.cmakeFairSoft_test.cmakebootstrap-cmake.shcmake/legacy.cmakelegacy/README.mdlegacy/advanced.mdlegacy/boost/support-numpy-2.patchlegacy/bootstrap-cmake.shlegacy/dds/relax_protobuf_requirement.patchlegacy/dependencies.mdlegacy/onnxruntime/fix_gcc13_compilation.patchlegacy/onnxruntime/fix_python_detection.patchlegacy/onnxruntime/install_config_files.patchlegacy/pythia6/add_missing_extern_keyword.patchlegacy/root/fix_macos_sdk_for_rootcling.patchlegacy/root/fix_macosx_findOpenGL.patchlegacy/root/fix_root_install_external_tars_with_cmake_3_24.patchlegacy/root/fix_rpath_info.patchlegacy/setup-debian.shlegacy/setup-fedora.shlegacy/setup-macos.shlegacy/setup-opensuse.shtest/container/legacy/CMakeLists.txttest/container/legacy/linux-cmake.def.intest/legacy/fairroot.sh.in
💤 Files with no reviewable changes (8)
- legacy/dds/relax_protobuf_requirement.patch
- legacy/onnxruntime/install_config_files.patch
- bootstrap-cmake.sh
- legacy/onnxruntime/fix_python_detection.patch
- legacy/pythia6/add_missing_extern_keyword.patch
- legacy/boost/support-numpy-2.patch
- legacy/root/fix_root_install_external_tars_with_cmake_3_24.patch
- legacy/root/fix_macosx_findOpenGL.patch
| if(CMAKE_SYSTEM_PROCESSOR MATCHES "arm") | ||
| set(vector "-DVECGEOM_VECTOR=empty") | ||
| else() | ||
| set(vector "-DVECGEOM_VECTOR=native") | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What values does CMake's CMAKE_SYSTEM_PROCESSOR commonly report on Linux ARM64 and macOS arm64 hosts?
💡 Result:
On Linux ARM64 hosts, CMake’s CMAKE_SYSTEM_PROCESSOR commonly reports: aarch64. CMake documents that when not cross-compiling, CMAKE_SYSTEM_PROCESSOR has the same value as CMAKE_HOST_SYSTEM_PROCESSOR [1]. For Unix-like systems, CMAKE_HOST_SYSTEM_PROCESSOR is set to the output of uname -m on GNU/Linux (among others) [2]. On ARM64 Linux, uname -m commonly yields aarch64 (Linux toolchain naming convention; see examples and discussion such as Ubuntu/Raspberry Pi where uname reports aarch64 for ARM64 systems) [3]. Therefore, on Linux ARM64 hosts, CMAKE_SYSTEM_PROCESSOR is commonly aarch64. On macOS arm64 (Apple Silicon) hosts, CMake’s CMAKE_SYSTEM_PROCESSOR commonly reports: arm64 (or can be controlled as arm64 explicitly). CMake documents that on macOS the value of uname -m is used by default for CMAKE_HOST_SYSTEM_PROCESSOR [2], but that on Apple Silicon hosts the value printed by uname -m may vary based on CMake’s architecture and the invoking process tree; CMake 3.19.2+ provides CMAKE_APPLE_SILICON_PROCESSOR to specify the host architecture explicitly, and that variable must be either arm64 or x86_64 [2][4]. It also documents that CMAKE_APPLE_SILICON_PROCESSOR exists to set CMAKE_HOST_SYSTEM_PROCESSOR (and therefore, when not cross-compiling, to affect CMAKE_SYSTEM_PROCESSOR) to arm64/x86_64 explicitly [2][4]. Consequently, on macOS arm64 hosts using native arm64 CMake/invocation, CMAKE_SYSTEM_PROCESSOR commonly ends up as arm64; if uname -m varies due to process-tree differences, setting CMAKE_APPLE_SILICON_PROCESSOR=arm64 forces arm64 [2][4].
Citations:
- 1: https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_PROCESSOR.html
- 2: https://cmake.org/cmake/help/latest/variable/CMAKE_HOST_SYSTEM_PROCESSOR.html
- 3: https://unix.stackexchange.com/questions/683360/why-does-hostnamectl-return-arm64-but-uname-returns-aarch64-ubuntu-20-04-2-on
- 4: https://cmake.org/cmake/help/latest/variable/CMAKE_APPLE_SILICON_PROCESSOR.html
🏁 Script executed:
# Check if the file exists and read the relevant section
if [ -f "cmake/legacy.cmake" ]; then
echo "=== Lines 360-380 of cmake/legacy.cmake ==="
sed -n '360,380p' cmake/legacy.cmake
else
echo "File not found at cmake/legacy.cmake"
find . -name "legacy.cmake" -type f 2>/dev/null
fiRepository: FairRootGroup/FairSoft
Length of output: 833
🏁 Script executed:
# Search for other CMAKE_SYSTEM_PROCESSOR references in cmake files
echo "=== All CMAKE_SYSTEM_PROCESSOR references ==="
rg "CMAKE_SYSTEM_PROCESSOR" cmake/ --context 3Repository: FairRootGroup/FairSoft
Length of output: 392
aarch64 on Linux ARM64 is not matched by the current pattern.
CMAKE_SYSTEM_PROCESSOR reports aarch64 on Linux ARM64 systems, but the condition MATCHES "arm" uses regex matching that does not match aarch64. This causes ARM64 builds to incorrectly select -DVECGEOM_VECTOR=native instead of empty.
Suggested fix
-if(CMAKE_SYSTEM_PROCESSOR MATCHES "arm")
+if(CMAKE_SYSTEM_PROCESSOR MATCHES "^(arm|arm64|aarch64)")
set(vector "-DVECGEOM_VECTOR=empty")
else()
set(vector "-DVECGEOM_VECTOR=native")
endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/legacy.cmake` around lines 370 - 374, The conditional checking
CMAKE_SYSTEM_PROCESSOR only matches "arm" and misses "aarch64", causing
incorrect -DVECGEOM_VECTOR selection; update the MATCHES pattern (or add an
additional check) to include aarch64 (e.g. match "arm" or "aarch64") so that the
set(vector "-DVECGEOM_VECTOR=empty") branch is chosen on ARM64 systems; ensure
you reference the CMAKE_SYSTEM_PROCESSOR check and the set(vector
"-DVECGEOM_VECTOR=...") assignments when making the change.
| if [ -z "$1" ] | ||
| then | ||
| echo "Usage: bootstrap-cmake.sh INSTALLDIR" | ||
| exit 1 | ||
| else | ||
| installdir=$(realpath "$1") | ||
| if [[ $? -ne 0 ]]; then | ||
| echo "" | ||
| echo "Install directory $installdir must exist on macOS." | ||
| echo "Please create the directory." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
target="$tmp/nonexistent-install-dir"
if realpath "$target" >/dev/null 2>&1; then
echo "realpath accepts non-existing paths on this host"
else
echo "realpath requires path to exist on this host"
fi
rm -rf "$tmp"Repository: FairRootGroup/FairSoft
Length of output: 111
🏁 Script executed:
cat legacy/bootstrap-cmake.shRepository: FairRootGroup/FairSoft
Length of output: 1706
Fix install-dir resolution to allow non-existent directories on Linux.
Line 12 calls realpath before directory creation, which fails on non-existent paths. This prevents Linux users from specifying a new install directory, even though line 56 later attempts to create it with mkdir -p. On macOS (Darwin), the directory must exist, so the behavior should be platform-specific.
Proposed fix
-if [ -z "$1" ]
+if [ "$#" -ne 1 ]
then
echo "Usage: bootstrap-cmake.sh INSTALLDIR"
exit 1
else
- installdir=$(realpath "$1")
- if [[ $? -ne 0 ]]; then
- echo ""
- echo "Install directory $installdir must exist on macOS."
- echo "Please create the directory."
- exit 1
- fi
+ if [[ "$(uname -s | tr '[:upper:]' '[:lower:]')" == "darwin" ]]; then
+ installdir=$(realpath "$1") || {
+ echo ""
+ echo "Install directory $1 must exist on macOS."
+ echo "Please create the directory."
+ exit 1
+ }
+ else
+ mkdir -p "$1"
+ installdir=$(realpath "$1")
+ fi
fi🧰 Tools
🪛 Shellcheck (0.11.0)
[style] 13-13: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?.
(SC2181)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@legacy/bootstrap-cmake.sh` around lines 7 - 18, The script currently calls
realpath on $1 (installdir) unconditionally which fails for non-existent paths
on Linux; change the logic to only require an existing path on macOS (Darwin)
and defer canonicalization on Linux: detect platform with uname, and for Darwin
keep the realpath + existence check using installdir=$(realpath "$1") and the
subsequent error branch, but for other platforms set installdir="$1" (or use
realpath -m if available) and do not error; then after the mkdir -p later in the
script canonicalize/install absolute path (e.g., installdir=$(realpath
"$installdir")) so creation succeeds on Linux while preserving macOS behavior.
| curl -L -O -s "${cmakebaseurl}${cmakeversion}/$cmakechecksums" | ||
| grep "$grepstring" $cmakechecksums > checksum.txt | ||
| sha256sum -c checksum.txt |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
uname -s
command -v sha256sum || true
command -v shasum || trueRepository: FairRootGroup/FairSoft
Length of output: 72
🏁 Script executed:
cat -n legacy/bootstrap-cmake.sh | sed -n '30,55p'Repository: FairRootGroup/FairSoft
Length of output: 1168
🏁 Script executed:
grep -n "sha256sum\|shasum" legacy/bootstrap-cmake.shRepository: FairRootGroup/FairSoft
Length of output: 95
🏁 Script executed:
head -20 legacy/bootstrap-cmake.shRepository: FairRootGroup/FairSoft
Length of output: 378
Add macOS-compatible checksum verifier fallback.
Line 47 unconditionally uses sha256sum, which is not available on standard macOS installations. The Darwin branch (lines 31–37) only handles download variables; the checksum verification runs on all platforms and fails on macOS without Homebrew.
Proposed fix
curl -L -O -s "${cmakebaseurl}${cmakeversion}/$cmakechecksums"
grep "$grepstring" $cmakechecksums > checksum.txt
-sha256sum -c checksum.txt
+if command -v sha256sum >/dev/null 2>&1; then
+ sha256sum -c checksum.txt
+elif command -v shasum >/dev/null 2>&1; then
+ shasum -a 256 -c checksum.txt
+else
+ echo "No SHA-256 checker found (need sha256sum or shasum)."
+ exit 1
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| curl -L -O -s "${cmakebaseurl}${cmakeversion}/$cmakechecksums" | |
| grep "$grepstring" $cmakechecksums > checksum.txt | |
| sha256sum -c checksum.txt | |
| curl -L -O -s "${cmakebaseurl}${cmakeversion}/$cmakechecksums" | |
| grep "$grepstring" $cmakechecksums > checksum.txt | |
| if command -v sha256sum >/dev/null 2>&1; then | |
| sha256sum -c checksum.txt | |
| elif command -v shasum >/dev/null 2>&1; then | |
| shasum -a 256 -c checksum.txt | |
| else | |
| echo "No SHA-256 checker found (need sha256sum or shasum)." | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@legacy/bootstrap-cmake.sh` around lines 45 - 47, The checksum verification
currently always calls sha256sum -c checksum.txt which fails on macOS; update
the verification to detect which tool is available and fall back to
macOS-compatible shasum: after creating checksum.txt, check if command -v
sha256sum >/dev/null && run sha256sum -c checksum.txt, else run shasum -a 256 -c
checksum.txt (or exit nonzero if neither exists). Reference the variables/file
names cmakechecksums, checksum.txt, grepstring and the sha256sum call when
adding this conditional fallback.
| ``` | ||
| -DCMAKE_OSX_SYSROOT=<full path to SDK directory> | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add language identifiers to code blocks.
The code blocks should specify bash as the language for proper syntax highlighting and to comply with markdown best practices.
📝 Proposed fix
Line 77:
-```
+```bash
-DCMAKE_OSX_SYSROOT=<full path to SDK directory>
Line 83:
```diff
-```
+```bash
-DCMAKE_OSX_SYSROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk
</details>
As per coding guidelines, markdownlint flagged these as violations of MD040 (fenced-code-language).
Also applies to: 83-85
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @legacy/README.md around lines 77 - 79, Add the missing fenced-code block
language identifiers by updating the code fences that contain the lines starting
with "-DCMAKE_OSX_SYSROOT=" and
"-DCMAKE_OSX_SYSROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk" to
use "bash" (i.e., change the opening triple-backticks to ```bash) so both code
blocks comply with markdownlint MD040; locate these blocks in legacy/README.md
and update both occurrences accordingly.
</details>
<!-- fingerprinting:phantom:triton:puma -->
<!-- d98c2f50 -->
<!-- This is an auto-generated comment by CodeRabbit -->
|
|
||
| More information can be found at | ||
|
|
||
| **macOS users**: Notice [macOS SDK](advanced.md#macos-sdk)! |
There was a problem hiding this comment.
Remove redundant macOS notice.
This line now appears immediately after the detailed "3.1 CMake configure step for macOS users" section (lines 54-88), which already references the advanced.md macOS SDK section. The duplicate notice is redundant and may confuse readers about where to find the information.
✂️ Proposed fix
-**macOS users**: Notice [macOS SDK](advanced.md#macos-sdk)!
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **macOS users**: Notice [macOS SDK](advanced.md#macos-sdk)! |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@legacy/README.md` at line 89, Remove the redundant notice line that reads
"**macOS users**: Notice [macOS SDK](advanced.md#macos-sdk)!" from README.md
(the duplicate that appears after the "3.1 CMake configure step for macOS users"
section) so the earlier detailed section remains the single reference to
advanced.md#macos-sdk; simply delete that duplicate line.
| bison build-essential bzip2 ca-certificates cmake coreutils \ | ||
| curl debianutils file findutils flex g++ gcc gfortran git gzip \ | ||
| hostname libbz2-dev libcurl4-openssl-dev libgsl-dev libicu-dev \ | ||
| libfftw3-dev libprotobuf-dev \ | ||
| libgl1-mesa-dev libglu1-mesa-dev libgrpc++-dev \ | ||
| liblzma-dev libncurses-dev libreadline-dev libsqlite3-dev libssl-dev libtool \ | ||
| libx11-dev libxerces-c-dev libxext-dev libxft-dev \ | ||
| libxml2-dev libxmu-dev libxpm-dev libyaml-cpp-dev libzstd-dev lsb-release make patch \ | ||
| python3-dev protobuf-compiler-grpc rsync sed subversion tar unzip wget xutils-dev xz-utils |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find declared CMake minimums in repository:"
rg -nP 'cmake_minimum_required\s*\(\s*VERSION\s+[0-9]+(\.[0-9]+){0,2}' --iglob 'CMakeLists.txt' -C1
echo
echo "Inspect Debian setup script CMake install path:"
rg -nP 'apt-get\s+-y\s+install|cmake' legacy/setup-debian.sh -C2
echo
echo "In the target Debian image/environment, verify package candidate and installed version:"
echo " apt-cache policy cmake"
echo " cmake --version"Repository: FairRootGroup/FairSoft
Length of output: 713
🏁 Script executed:
#!/bin/bash
echo "=== Full legacy/setup-debian.sh file ==="
cat -n legacy/setup-debian.sh
echo
echo "=== Check for any existing version guards ==="
rg -n 'compare-versions|--version|cmake.*version' legacy/setup-debian.sh
echo
echo "=== Verify CMakeLists.txt minimum version ==="
rg -n 'cmake_minimum_required' CMakeLists.txt -A1 -B1Repository: FairRootGroup/FairSoft
Length of output: 1192
Correct the minimum CMake version in the proposed guard to match the actual requirement.
The suggestion to add a version guard is sound—the script currently installs cmake without verification. However, the patch must set min_cmake="3.5" to match the actual minimum declared in CMakeLists.txt (line 12: cmake_minimum_required(VERSION 3.5...3.28 FATAL_ERROR)), not 3.2. With the current patch, cmake 3.2–3.4 would pass the check and then fail later at build time, providing no benefit.
Corrected patch
apt-get -y install autoconf automake binutils \
bison build-essential bzip2 ca-certificates cmake coreutils \
curl debianutils file findutils flex g++ gcc gfortran git gzip \
hostname libbz2-dev libcurl4-openssl-dev libgsl-dev libicu-dev \
libfftw3-dev libprotobuf-dev \
libgl1-mesa-dev libglu1-mesa-dev libgrpc++-dev \
liblzma-dev libncurses-dev libreadline-dev libsqlite3-dev libssl-dev libtool \
libx11-dev libxerces-c-dev libxext-dev libxft-dev \
libxml2-dev libxmu-dev libxpm-dev libyaml-cpp-dev libzstd-dev lsb-release make patch \
python3-dev protobuf-compiler-grpc rsync sed subversion tar unzip wget xutils-dev xz-utils
+min_cmake="3.5"
+installed_cmake="$(cmake --version | awk 'NR==1{print $3}')"
+dpkg --compare-versions "$installed_cmake" ge "$min_cmake" || {
+ echo "ERROR: cmake >= $min_cmake required, found $installed_cmake" >&2
+ exit 1
+}
apt-get -y clean🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@legacy/setup-debian.sh` around lines 8 - 16, The version-guard for CMake
should use the actual minimum required by the project: change the min_cmake
value used in the install/guard logic from "3.2" to "3.5" (matching the
cmake_minimum_required(VERSION 3.5...3.28 FATAL_ERROR) in CMakeLists.txt) so the
check in the setup script (the min_cmake variable and its comparison logic
around cmake installation) will correctly prevent installing a too-old CMake.
| os_version=$(cat /etc/os-release | grep 'VERSION=' | cut -d\" -f2) | ||
| if [[ "${os_version}" == "15.6" ]]; then |
There was a problem hiding this comment.
Use VERSION_ID for OS branching, not VERSION.
Line 18 parses VERSION, which may contain non-stable text formatting; branching on it can silently miss the intended path. Prefer VERSION_ID from /etc/os-release.
Suggested fix
-os_version=$(cat /etc/os-release | grep 'VERSION=' | cut -d\" -f2)
-if [[ "${os_version}" == "15.6" ]]; then
+. /etc/os-release
+os_version="${VERSION_ID}"
+if [[ "${os_version}" == "15.6" ]]; thenAlso applies to: 31-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@legacy/setup-opensuse.sh` around lines 18 - 19, Replace branching on the
unstable VERSION value with the stable VERSION_ID from /etc/os-release: change
the os_version assignment (variable os_version) to read VERSION_ID (e.g.
os_version=$(grep '^VERSION_ID=' /etc/os-release | cut -d\" -f2) or source
/etc/os-release and use $VERSION_ID) and update any other checks that currently
grep for 'VERSION=' to use 'VERSION_ID' instead (including the other occurrence
that references os_version).
| zypper remove -y python3 python3-devel | ||
| rm /usr/bin/python3 | ||
| ln -s /usr/bin/python3.10 /usr/bin/python3 |
There was a problem hiding this comment.
Avoid removing system Python and manually rewriting /usr/bin/python3.
Lines 28–30 can break distro-managed tools and leave the system in an inconsistent package state. For build tooling, use an explicit interpreter path (python3.10) or local PATH override instead of uninstalling system Python.
Safer direction
- zypper remove -y python3 python3-devel
- rm /usr/bin/python3
- ln -s /usr/bin/python3.10 /usr/bin/python3
+ # Keep system python intact; use explicit interpreter for build steps
+ export FAIRSOFT_PYTHON=/usr/bin/python3.10
+ # or: export PATH="/usr/bin:${PATH}" and call python3.10 explicitly where needed📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| zypper remove -y python3 python3-devel | |
| rm /usr/bin/python3 | |
| ln -s /usr/bin/python3.10 /usr/bin/python3 | |
| # Keep system python intact; use explicit interpreter for build steps | |
| export FAIRSOFT_PYTHON=/usr/bin/python3.10 | |
| # or: export PATH="/usr/bin:${PATH}" and call python3.10 explicitly where needed |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@legacy/setup-opensuse.sh` around lines 28 - 30, The script currently removes
distro Python and rewrites /usr/bin/python3 via the commands "zypper remove -y
python3 python3-devel", "rm /usr/bin/python3", and "ln -s /usr/bin/python3.10
/usr/bin/python3"; instead, revert those three operations: do not uninstall
system python or delete /usr/bin/python3, and remove the symlink step; change
the build to call the explicit interpreter "python3.10" (or adjust PATH to put a
local shim in /usr/local/bin) or use a virtualenv/venv created with python3.10
so system package management remains intact and tools depending on
/usr/bin/python3 are not broken.
6 participants
No description provided.