-
Notifications
You must be signed in to change notification settings - Fork 66
Jan24 patches #575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: jan24_patches
Are you sure you want to change the base?
Jan24 patches #575
Conversation
141551b to
14a53f3
Compare
14a53f3 to
9f1b240
Compare
Also fix an issue with the usage of the pythia6 library. The libarary wasn't found any longer such that it is explicitly added as dependency.
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.
9f1b240 to
a212554
Compare
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.
|
I have added two more commits which now also build pythia8 and boost with the proper settings. I had overlokked both packages before. If your are fine with the PR I would like to squash some of the new commits and merge the PR. |
👍 |
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.
066b1c3 to
24febab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses compilation and build system compatibility issues for CMake 4.0 and Apple Clang 17, with specific focus on macOS SDK handling. The changes enable FairSoft to work with newer toolchain versions while maintaining backward compatibility.
Key changes include:
- Updated CMake version requirements across multiple packages to support CMake 4.0
- Fixed compilation issues with Apple Clang 17 through patches and SDK handling
- Enhanced macOS build system to use proper tools (bash, make, patch) and SDK configuration
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/legacy/fairroot.sh.in | Added C standard specification for FairRoot build |
| legacy/*/fix_cmake.patch | Updated cmake_minimum_required statements to support CMake 4.0.1 |
| legacy/setup-macos.sh | Added bash and make packages to macOS dependencies |
| legacy/root/*.patch | Applied fixes for Apple Clang 17 compatibility and SDK handling |
| legacy/geant4/*.patch | Fixed typos and CMake compatibility issues |
| legacy/advanced.md | Updated documentation about SDK usage with newer compilers |
| legacy/README.md | Added detailed macOS compilation instructions |
| cmake/legacy.cmake | Enhanced macOS tool detection and SDK propagation |
| FairSoftConfig.cmake | Modified to respect externally set CMAKE_OSX_SYSROOT |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| 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. |
Copilot
AI
Oct 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: 'becuase' should be 'because'.
| 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. |
| 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. |
Copilot
AI
Oct 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: 'ot' should be 'to'.
| so one needs ot install a version of the **patch** command with brew. | |
| so one needs to install a version of the **patch** command with brew. |
| 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' |
Copilot
AI
Oct 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String literal 'osx_sdk' should be the variable osx_sdk without quotes to use the actual SDK path value.
| + my_env["SDKROOT"] = 'osx_sdk' | |
| + my_env["SDKROOT"] = osx_sdk |
|
@copilot Summarize all changes by commit and check whether the commit describes the changes adequately. If needed, suggest improvements. |
Commit Summary for PR #575I've analyzed all 21 commits in this PR. Here's a comprehensive summary organized by topic: 1. CMake 4.0 Compatibility Fixes (7 commits)These commits fix build systems to support CMake 4.0.1 by updating
Assessment: ✅ Commit messages are clear and consistent. The pattern is good. 2. Geant Fixes (2 commits)
3. macOS SDK and Clang 17 Support (7 commits)
4. GCC 15 Compatibility (3 commits)
5. Debian 13 Compatibility (1 commit)
Overall AssessmentStrong Points:
Suggestions for Improvement:
Minor Issues Found:
Overall, the commits are well-structured and the PR is ready for merge after addressing the minor suggestions above! 🎉 |
|
@fuhlig1 copilot does not yet respond to questions posted here directly, so I installed github copilot CLI and told it to reply to it, which it did as my user. |
4924b1f to
5a73be2
Compare
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. 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 is found.
Properly pass CMAKE_OSX_SYSROOT to the build and test processes. Don't test building onnxruntime if CMake version >= 4.0.0
5a73be2 to
63cbc99
Compare
|
Hi, before I discovered the patches branches I also fixed the cmake issue for myself. Perhaps the solution proposed by cmake which does not require patching cmake files but passing proper options, would be better to use here? See here: b56c23d |
- Don't overwrite CMAKE_OSX_SYSROOT which is passed as CMake parameter
- Pass CMAKE_OSX_SYSROOT to all package build systems
- Fix ROOT build system such that rootcint takes into account the defined CMAKE_OSX_SYSROOT