Latest updates for the Jan24 patches #581
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The fix prevents a crash about wrong CMake versions when compiling geant4_vmc.
Before a SDK defined via the CMake parameter CMAKE_OSX_SYSROOT was overwritten with the value extracted from brew.
Clang 17 remove the header fp.h which is used in ROOT's copy of libAfterimage. Add a patch which uses math.h instead. The included ROOT version can't be compiled any longer with the SDK 15. Specifing the older SDK 14 when running CMake (with -DCMAKE_OSX_SYSROOT) only works partly since rootcling still uses the latest SDK version. Add a patch such that rootcling also uses the specified SDK. 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. Fix patch for ROOT using CMAKE_OSX_SYSROOT minimize the patch
Fix a typo in the code. This code could actually never be compiled and it is unclear why this compilation error only popped up now with Clang 17.
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.
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.
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. Add some missing include statements for <stdint>. Probably due to header cleanup in the include files of gcc 15.
The 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. For unknown reasons the pythia library isn't found any longer which is fixed by adding possible library names.
Use C11 as standard C version when compiling FairRoot. The C code of the MbsApi can't be compiled with C23 which is the new default version with gcc15.
The version of ftgl contained in the ROOT source code results in a compilation error with the gcc14 compiler suite used in Debian 13 and the gcc15 compiler suite in OpenSuse 16.0 or Ubuntu 26.04. For all other tested system the code is okay. The problem was also found by the Debian team https://www.mail-archive.com/debian-qa-packages@lists.debian.org/msg81745.html and a fix is available from upstream frankheckenbach/ftgl#20 Add a cmake check for OS and apply the fix only in case a Debian 13 or OpenSuse 16.0 or Ubuntu 26.04 is found.
Properly pass CMAKE_OSX_SYSROOT to the build and test processes. Don't test building onnxruntime if CMake version >= 4.0.0
📝 WalkthroughWalkthroughThis pull request updates the FairSoft build system to improve macOS compatibility and extend CMake version support. Changes introduce macOS-specific SDK root path handling, platform-specific patch/make program detection, OS detection logic for conditional patch application, and multiple patches addressing Geant3, Geant4, and ROOT compatibility issues. Documentation is extended with macOS configuration guidance. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
🤖 Fix all issues with AI agents
In `@cmake/legacy.cmake`:
- Around line 426-456: The OS detection uses grep ID= which also matches ID_LIKE
and can produce multiple lines so the MATCHES check may fail; update the
extraction commands for OS_NAME and OS_VERSION to target the exact keys (e.g.,
grep -E '^ID=' and grep -E '^VERSION=' or use awk -F= '/^ID=/{print $2}' and
similar for VERSION) so only the intended values are captured, strip quotes as
currently done, and ensure the if uses a quoted concatenation
("${OS_NAME}${OS_VERSION}") in the MATCHES comparisons; keep the same
set(debian13_patch) assignment logic and refer to OS_NAME, OS_VERSION, and
debian13_patch when making these changes.
In `@FairSoft_test.cmake`:
- Around line 157-169: The else branch in FairSoft_test.cmake only appends
-DCMAKE_OSX_SYSROOT when CMAKE_OSX_SYSROOT is set, skipping the Python and ICU
detection done earlier; update this block so Python and ICU are detected and
appended to the options list regardless of CMAKE_OSX_SYSROOT (i.e., always run
the execute_process calls that compute python_prefix -> -DPython_EXECUTABLE and
icu_prefix -> -DICU_ROOT, and still append the sysroot via -DCMAKE_OSX_SYSROOT
when provided), ensuring the same behavior as FairSoftConfig.cmake for macOS;
reference CMAKE_OSX_SYSROOT, the options list, and the -DPython_EXECUTABLE and
-DICU_ROOT option names when making the change.
In `@legacy/advanced.md`:
- Around line 62-71: Fix two spelling mistakes in the documentation: replace
"becuase" with "because" and "possibilty" with "possibility" in the paragraph
that discusses SDK/ROOT compilation and rootcling (the lines describing
"problems with compilation errors due to **wrong** SDKs are because of ROOT" and
the sentence starting "To overcome the problem and allow a more flexible
compilation..."). Ensure only the two words are corrected and surrounding
text/formatting remains unchanged.
In `@legacy/README.md`:
- Around line 56-85: Fix typos and macOS capitalization in the README section
describing macOS build issues: correct "macosx" to "macOS", "ot" to "to",
"explicitely" to "explicitly", and "on needs" to "one needs"; capitalize
Homebrew where referenced and use consistent formatting for command names
**patch** and **make**; ensure the CMake flag is shown exactly as
-DCMAKE_OSX_SYSROOT=<full path to SDK directory> and the example SDK paths keep
"MacOSX14.sdk" with correct macOS capitalization and spacing in the surrounding
sentences (references: the README text block mentioning patch, make, Homebrew,
-DCMAKE_OSX_SYSROOT, and example SDK paths).
In `@legacy/root/fix_macos_sdk_for_rootcling.patch`:
- Around line 45-50: The code captures the wrong env key and sets SDKROOT to a
literal string: change the source lookup to my_env.get("SDKROOT") (not
"CMAKE_OSX_SYSROOT") and assign the variable value (my_env["SDKROOT"] = osx_sdk)
instead of the literal 'osx_sdk'; keep the existing truthy check (if osx_sdk:)
before setting so SDKROOT is only propagated when present.
🧹 Nitpick comments (1)
test/legacy/fairroot.sh.in (1)
13-19: Prefer a bash array for CMake args to avoid word-splitting.This avoids subtle failures if paths ever contain spaces.
♻️ Safer argument handling
-_arguments="-DCMAKE_INSTALL_PREFIX=$FAIRROOTPATH -DCMAKE_C_STANDARD=11" +_arguments=( + "-DCMAKE_INSTALL_PREFIX=$FAIRROOTPATH" + "-DCMAKE_C_STANDARD=11" +) @@ - _arguments="${_arguments} -DCMAKE_OSX_SYSROOT=@CMAKE_OSX_SYSROOT@" + _arguments+=("-DCMAKE_OSX_SYSROOT=@CMAKE_OSX_SYSROOT@") @@ -cmake -S. -Bbuild ${_arguments} +cmake -S. -Bbuild "${_arguments[@]}"
Comment on lines
+426
to
+456
| # Extract the OS and VERSION on Linux | ||
| if(CMAKE_SYSTEM_NAME STREQUAL "Linux") | ||
| execute_process(COMMAND cat /etc/os-release | ||
| COMMAND grep ID= | ||
| COMMAND grep -v VERSION | ||
| COMMAND cut -d= -f2 | ||
| OUTPUT_VARIABLE OS_NAME | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE | ||
| ) | ||
| string(REPLACE "\"" "" OS_NAME ${OS_NAME}) | ||
|
|
||
| execute_process(COMMAND cat /etc/os-release | ||
| COMMAND grep ID= | ||
| COMMAND grep VERSION | ||
| COMMAND cut -d= -f2 | ||
| COMMAND cut -d\" -f2 | ||
| OUTPUT_VARIABLE OS_VERSION | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE | ||
| ) | ||
|
|
||
| if(${OS_NAME}${OS_VERSION} MATCHES "debian13" OR | ||
| ${OS_NAME}${OS_VERSION} MATCHES "opensuse-leap16.0" OR | ||
| ${OS_NAME}${OS_VERSION} MATCHES "ubuntu26.04" | ||
| ) | ||
| set(debian13_patch COMMAND ${patch} -p1 -i "${CMAKE_SOURCE_DIR}/legacy/root/fix_ftgl.patch") | ||
| else() | ||
| set(debian13_patch) | ||
| endif() | ||
| else() | ||
| set(debian13_patch) | ||
| endif() |
There was a problem hiding this comment.
Linux OS detection can mis-parse ID_LIKE; patch may be skipped.
grep ID= captures both ID and ID_LIKE, which can turn OS_NAME into a list and break the MATCHES check. Use anchored keys and quote the concatenation.
🐛 Robust /etc/os-release parsing
- execute_process(COMMAND cat /etc/os-release
- COMMAND grep ID=
- COMMAND grep -v VERSION
- COMMAND cut -d= -f2
+ execute_process(COMMAND grep "^ID=" /etc/os-release
+ COMMAND cut -d= -f2
OUTPUT_VARIABLE OS_NAME
OUTPUT_STRIP_TRAILING_WHITESPACE
)
- string(REPLACE "\"" "" OS_NAME ${OS_NAME})
+ string(REPLACE "\"" "" OS_NAME "${OS_NAME}")
@@
- execute_process(COMMAND cat /etc/os-release
- COMMAND grep ID=
- COMMAND grep VERSION
- COMMAND cut -d= -f2
- COMMAND cut -d\" -f2
+ execute_process(COMMAND grep "^VERSION_ID=" /etc/os-release
+ COMMAND cut -d= -f2
OUTPUT_VARIABLE OS_VERSION
OUTPUT_STRIP_TRAILING_WHITESPACE
)
+ string(REPLACE "\"" "" OS_VERSION "${OS_VERSION}")
@@
- if(${OS_NAME}${OS_VERSION} MATCHES "debian13" OR
+ if("${OS_NAME}${OS_VERSION}" MATCHES "debian13" OR
${OS_NAME}${OS_VERSION} MATCHES "opensuse-leap16.0" OR
${OS_NAME}${OS_VERSION} MATCHES "ubuntu26.04"
)🤖 Prompt for AI Agents
In `@cmake/legacy.cmake` around lines 426 - 456, The OS detection uses grep ID=
which also matches ID_LIKE and can produce multiple lines so the MATCHES check
may fail; update the extraction commands for OS_NAME and OS_VERSION to target
the exact keys (e.g., grep -E '^ID=' and grep -E '^VERSION=' or use awk -F=
'/^ID=/{print $2}' and similar for VERSION) so only the intended values are
captured, strip quotes as currently done, and ensure the if uses a quoted
concatenation ("${OS_NAME}${OS_VERSION}") in the MATCHES comparisons; keep the
same set(debian13_patch) assignment logic and refer to OS_NAME, OS_VERSION, and
debian13_patch when making these changes.
Comment on lines
+157
to
+169
| if(NOT CMAKE_OSX_SYSROOT) | ||
| execute_process(COMMAND brew --prefix python OUTPUT_VARIABLE python_prefix) | ||
| string(STRIP "${python_prefix}" python_prefix) | ||
| list(APPEND options "-DPython_EXECUTABLE=${python_prefix}/bin/python3") | ||
| execute_process(COMMAND brew --prefix icu4c OUTPUT_VARIABLE icu_prefix) | ||
| string(STRIP "${icu_prefix}" icu_prefix) | ||
| list(APPEND options "-DICU_ROOT=${icu_prefix}") | ||
| execute_process(COMMAND brew ruby -e "puts MacOS.sdk_path" OUTPUT_VARIABLE macos_sdk_path) | ||
| string(STRIP "${macos_sdk_path}" macos_sdk_path) | ||
| list(APPEND options "-DCMAKE_OSX_SYSROOT=${macos_sdk_path}") | ||
| else() | ||
| list(APPEND options "-DCMAKE_OSX_SYSROOT=${CMAKE_OSX_SYSROOT}") | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how FairSoftConfig.cmake handles Python and ICU on Apple
# to verify if behavior is intentionally different in test config
echo "=== FairSoftConfig.cmake Python/ICU handling ==="
rg -n -A5 'if\(APPLE\)' FairSoftConfig.cmake | head -50
echo ""
echo "=== Check if Python/ICU are conditional on CMAKE_OSX_SYSROOT in FairSoftConfig ==="
rg -n 'CMAKE_OSX_SYSROOT' FairSoftConfig.cmake
echo ""
echo "=== Check broader Python/ICU configuration in FairSoftConfig ==="
rg -n -B2 -A2 'Python_EXECUTABLE|ICU_ROOT' FairSoftConfig.cmake | head -80Repository: FairRootGroup/FairSoft
Length of output: 1695
Python and ICU detection skipped when CMAKE_OSX_SYSROOT is pre-set
When CMAKE_OSX_SYSROOT is already defined, the else branch (lines 167-168) only passes the sysroot but omits Python and ICU detection. In contrast, FairSoftConfig.cmake unconditionally detects Python and ICU on Apple regardless of CMAKE_OSX_SYSROOT state. This inconsistency means users who pre-define a custom sysroot may encounter test failures if Python and ICU are not already configured elsewhere. Either unconditionally detect Python and ICU on Apple in this block, or document that users setting CMAKE_OSX_SYSROOT must separately handle Python and ICU configuration.
🤖 Prompt for AI Agents
In `@FairSoft_test.cmake` around lines 157 - 169, The else branch in
FairSoft_test.cmake only appends -DCMAKE_OSX_SYSROOT when CMAKE_OSX_SYSROOT is
set, skipping the Python and ICU detection done earlier; update this block so
Python and ICU are detected and appended to the options list regardless of
CMAKE_OSX_SYSROOT (i.e., always run the execute_process calls that compute
python_prefix -> -DPython_EXECUTABLE and icu_prefix -> -DICU_ROOT, and still
append the sysroot via -DCMAKE_OSX_SYSROOT when provided), ensuring the same
behavior as FairSoftConfig.cmake for macOS; reference CMAKE_OSX_SYSROOT, the
options list, and the -DPython_EXECUTABLE and -DICU_ROOT option names when
making the change.
Comment on lines
+62
to
+71
| The downside with picking the latest SDK version is that some older ROOT | ||
| versions can't be compiled any longer after updating the compiler and the | ||
| connected SDK. As described above the problems with compilation errors due | ||
| to **wrong** SDKs are because of ROOT and there mainly becuase of rootcling. | ||
| To overcome the problem and allow a more flexible compilation also with | ||
| non default SDKs we add the possibilty to define an older SDK when running | ||
| CMake. There are some additional changes in the build system of ROOT such | ||
| that rootcling take the setting properly into account. With this changes it | ||
| becomes possible to install FairSoft jan24 (ROOT 6.30.08) with Apple Clang | ||
| 17 on macosx 15. |
There was a problem hiding this comment.
Fix spelling errors in documentation.
The static analysis correctly identified two typos:
- Line 65: "becuase" → "because"
- Line 67: "possibilty" → "possibility"
📝 Proposed fix
The downside with picking the latest SDK version is that some older ROOT
versions can't be compiled any longer after updating the compiler and the
connected SDK. As described above the problems with compilation errors due
-to **wrong** SDKs are because of ROOT and there mainly becuase of rootcling.
+to **wrong** SDKs are because of ROOT and there mainly because of rootcling.
To overcome the problem and allow a more flexible compilation also with
-non default SDKs we add the possibilty to define an older SDK when running
+non default SDKs we add the possibility to define an older SDK when running
CMake. There are some additional changes in the build system of ROOT such
that rootcling take the setting properly into account. With this changes it
becomes possible to install FairSoft jan24 (ROOT 6.30.08) with Apple Clang
17 on macosx 15.📝 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.
Suggested change
| The downside with picking the latest SDK version is that some older ROOT | |
| versions can't be compiled any longer after updating the compiler and the | |
| connected SDK. As described above the problems with compilation errors due | |
| to **wrong** SDKs are because of ROOT and there mainly becuase of rootcling. | |
| To overcome the problem and allow a more flexible compilation also with | |
| non default SDKs we add the possibilty to define an older SDK when running | |
| CMake. There are some additional changes in the build system of ROOT such | |
| that rootcling take the setting properly into account. With this changes it | |
| becomes possible to install FairSoft jan24 (ROOT 6.30.08) with Apple Clang | |
| 17 on macosx 15. | |
| The downside with picking the latest SDK version is that some older ROOT | |
| versions can't be compiled any longer after updating the compiler and the | |
| connected SDK. As described above the problems with compilation errors due | |
| to **wrong** SDKs are because of ROOT and there mainly because of rootcling. | |
| To overcome the problem and allow a more flexible compilation also with | |
| non default SDKs we add the possibility to define an older SDK when running | |
| CMake. There are some additional changes in the build system of ROOT such | |
| that rootcling take the setting properly into account. With this changes it | |
| becomes possible to install FairSoft jan24 (ROOT 6.30.08) with Apple Clang | |
| 17 on macosx 15. |
🧰 Tools
🪛 LanguageTool
[grammar] ~65-~65: Ensure spelling is correct
Context: ...Ks are because of ROOT and there mainly becuase of rootcling. To overcome the problem a...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~67-~67: Ensure spelling is correct
Context: ...n also with non default SDKs we add the possibilty to define an older SDK when running CMa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In `@legacy/advanced.md` around lines 62 - 71, Fix two spelling mistakes in the
documentation: replace "becuase" with "because" and "possibilty" with
"possibility" in the paragraph that discusses SDK/ROOT compilation and rootcling
(the lines describing "problems with compilation errors due to **wrong** SDKs
are because of ROOT" and the sentence starting "To overcome the problem and
allow a more flexible compilation..."). Ensure only the two words are corrected
and surrounding text/formatting remains unchanged.
Comment on lines
+56
to
+85
| There are some known problems about the compilation of FairSoft on macosx. | ||
|
|
||
| The first two problems are related to the version of the **patch** and **make** | ||
| commands on macosx. | ||
|
|
||
| The **patch** command does not support the needed parameters, | ||
| so one needs ot install a version of the **patch** command with brew. | ||
| The **make** command doesn't properly support the jobsserver which allows | ||
| parallel builds of all the packages contained in FairSoft which slows down | ||
| the installation enormously. The version provided by brew fixes the problem. | ||
| Both packages are already added in the updated setup script for macosx. | ||
| If found the packages from the homebrew installation directory will be used. | ||
|
|
||
| The last problem is related to the macosx, compiler and SDK versions, such | ||
| that it depends on the personal setup. As described in more detail at | ||
| [macOS SDK](advanced.md#macos-sdk)! ROOT is very picky about the compiler | ||
| and the connected SDK. Compiling older ROOT versions with newer compilers | ||
| may need using an older SDK version. If not specified explicitely the | ||
| latest SDK version is used. To use an older SDK version on needs to add the | ||
| following parameter when running CMake | ||
|
|
||
| ``` | ||
| -DCMAKE_OSX_SYSROOT=<full path to SDK directory> | ||
| ``` | ||
|
|
||
| e.g. for Apple Clang 17 on macosx 15 or Apple Clang 16 on macosx 14 | ||
|
|
||
| ``` | ||
| -DCMAKE_OSX_SYSROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk | ||
| ``` |
There was a problem hiding this comment.
Fix typos and macOS capitalization in the new section.
A few small wording issues make the section harder to read.
✍️ Suggested edits
-There are some known problems about the compilation of FairSoft on macosx.
+There are some known problems about the compilation of FairSoft on macOS.
@@
-The **patch** command does not support the needed parameters,
-so one needs ot install a version of the **patch** command with brew.
+The **patch** command does not support the needed parameters,
+so one needs to install a version of the **patch** command with brew.
@@
-The last problem is related to the macosx, compiler and SDK versions, such
+The last problem is related to macOS, compiler and SDK versions, such
@@
-may need using an older SDK version. If not specified explicitely the
-latest SDK version is used. To use an older SDK version on needs to add the
+may need using an older SDK version. If not specified explicitly the
+latest SDK version is used. To use an older SDK version one needs to add the
@@
-e.g. for Apple Clang 17 on macosx 15 or Apple Clang 16 on macosx 14
+e.g. for Apple Clang 17 on macOS 15 or Apple Clang 16 on macOS 14📝 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.
Suggested change
| There are some known problems about the compilation of FairSoft on macosx. | |
| The first two problems are related to the version of the **patch** and **make** | |
| commands on macosx. | |
| The **patch** command does not support the needed parameters, | |
| so one needs ot install a version of the **patch** command with brew. | |
| The **make** command doesn't properly support the jobsserver which allows | |
| parallel builds of all the packages contained in FairSoft which slows down | |
| the installation enormously. The version provided by brew fixes the problem. | |
| Both packages are already added in the updated setup script for macosx. | |
| If found the packages from the homebrew installation directory will be used. | |
| The last problem is related to the macosx, compiler and SDK versions, such | |
| that it depends on the personal setup. As described in more detail at | |
| [macOS SDK](advanced.md#macos-sdk)! ROOT is very picky about the compiler | |
| and the connected SDK. Compiling older ROOT versions with newer compilers | |
| may need using an older SDK version. If not specified explicitely the | |
| latest SDK version is used. To use an older SDK version on needs to add the | |
| following parameter when running CMake | |
| ``` | |
| -DCMAKE_OSX_SYSROOT=<full path to SDK directory> | |
| ``` | |
| e.g. for Apple Clang 17 on macosx 15 or Apple Clang 16 on macosx 14 | |
| ``` | |
| -DCMAKE_OSX_SYSROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk | |
| ``` | |
| There are some known problems about the compilation of FairSoft on macOS. | |
| The first two problems are related to the version of the **patch** and **make** | |
| commands on macOS. | |
| The **patch** command does not support the needed parameters, | |
| so one needs to install a version of the **patch** command with brew. | |
| The **make** command doesn't properly support the jobsserver which allows | |
| parallel builds of all the packages contained in FairSoft which slows down | |
| the installation enormously. The version provided by brew fixes the problem. | |
| Both packages are already added in the updated setup script for macOS. | |
| If found the packages from the homebrew installation directory will be used. | |
| The last problem is related to macOS, compiler and SDK versions, such | |
| that it depends on the personal setup. As described in more detail at | |
| [macOS SDK](advanced.md#macos-sdk)! ROOT is very picky about the compiler | |
| and the connected SDK. Compiling older ROOT versions with newer compilers | |
| may need using an older SDK version. If not specified explicitly the | |
| latest SDK version is used. To use an older SDK version one needs to add the | |
| following parameter when running CMake | |
🧰 Tools
🪛 LanguageTool
[grammar] ~59-~59: Ensure spelling is correct
Context: ... the patch and make commands on macosx. The patch command does not suppor...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~65-~65: Consider using a different verb for a more formal wording.
Context: ...normously. The version provided by brew fixes the problem. Both packages are already ...
(FIX_RESOLVE)
🤖 Prompt for AI Agents
In `@legacy/README.md` around lines 56 - 85, Fix typos and macOS capitalization in
the README section describing macOS build issues: correct "macosx" to "macOS",
"ot" to "to", "explicitely" to "explicitly", and "on needs" to "one needs";
capitalize Homebrew where referenced and use consistent formatting for command
names **patch** and **make**; ensure the CMake flag is shown exactly as
-DCMAKE_OSX_SYSROOT=<full path to SDK directory> and the example SDK paths keep
"MacOSX14.sdk" with correct macOS capitalization and spacing in the surrounding
sentences (references: the README text block mentioning patch, make, Homebrew,
-DCMAKE_OSX_SYSROOT, and example SDK paths).
Comment on lines
+45
to
+50
| + osx_sdk = my_env.get("CMAKE_OSX_SYSROOT") | ||
| + | ||
| if not existing_ldlib: existing_ldlib = "" | ||
| my_env["LD_LIBRARY_PATH"] = os.path.join(rootdir, "lib") + ":" + existing_ldlib | ||
| + if osx_sdk: | ||
| + my_env["SDKROOT"] = 'osx_sdk' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "fix_macos_sdk_for_rootcling.patch" -type fRepository: FairRootGroup/FairSoft
Length of output: 113
🏁 Script executed:
cat -n ./legacy/root/fix_macos_sdk_for_rootcling.patch | head -60 | tail -30Repository: FairRootGroup/FairSoft
Length of output: 1686
Fix SDKROOT propagation bug: literal string instead of variable, and incorrect environment variable source.
Line 50 uses a literal string 'osx_sdk' instead of the variable osx_sdk, preventing SDK propagation. Additionally, line 45 reads from CMAKE_OSX_SYSROOT, but CMake passes this value via the SDKROOT environment variable to the subprocess, so the code should read from SDKROOT instead.
Fix
- osx_sdk = my_env.get("CMAKE_OSX_SYSROOT")
+ osx_sdk = my_env.get("SDKROOT") or my_env.get("CMAKE_OSX_SYSROOT")
@@
- if osx_sdk:
- my_env["SDKROOT"] = 'osx_sdk'
+ if osx_sdk:
+ my_env["SDKROOT"] = osx_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.
Suggested change
| + osx_sdk = my_env.get("CMAKE_OSX_SYSROOT") | |
| + | |
| if not existing_ldlib: existing_ldlib = "" | |
| my_env["LD_LIBRARY_PATH"] = os.path.join(rootdir, "lib") + ":" + existing_ldlib | |
| + if osx_sdk: | |
| + my_env["SDKROOT"] = 'osx_sdk' | |
| osx_sdk = my_env.get("SDKROOT") or my_env.get("CMAKE_OSX_SYSROOT") | |
| if not existing_ldlib: existing_ldlib = "" | |
| my_env["LD_LIBRARY_PATH"] = os.path.join(rootdir, "lib") + ":" + existing_ldlib | |
| if osx_sdk: | |
| my_env["SDKROOT"] = osx_sdk |
🤖 Prompt for AI Agents
In `@legacy/root/fix_macos_sdk_for_rootcling.patch` around lines 45 - 50, The code
captures the wrong env key and sets SDKROOT to a literal string: change the
source lookup to my_env.get("SDKROOT") (not "CMAKE_OSX_SYSROOT") and assign the
variable value (my_env["SDKROOT"] = osx_sdk) instead of the literal 'osx_sdk';
keep the existing truthy check (if osx_sdk:) before setting so SDKROOT is only
propagated when present.
Member
|
On macOS 26.2 (m1) ROOT compilation fails with: ...and more. Full ROOT build log: CLT is also 26.2 |
Member
Author
Did you pass the needed SDK when running CMake? |
Member
I did not :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Main changes: