Allow build and test on macosx with non default SDK #1616
Conversation
📝 WalkthroughWalkthroughAdds macOS SDKROOT handling to ROOT dictionary generation (CMake module and shell script) by deriving SDKROOT from CMAKE_OSX_SYSROOT and passing it to rootcling. Refactors test project scripts to build CMake parameters dynamically, optionally handle double-configure, and conditionally include CMAKE_OSX_SYSROOT. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant SH as generate_dictionary_root.sh
participant CMake as CMake custom command
participant Env as Environment
participant RC as rootcling
Dev->>CMake: Configure build (Apple or non-Apple)
CMake->>SH: Invoke generate_dictionary_root.sh
alt Apple with CMAKE_OSX_SYSROOT set
SH->>Env: Export LD_LIBRARY_PATH, DYLD_LIBRARY_PATH, SDKROOT
Note right of SH: SDKROOT derived from CMAKE_OSX_SYSROOT
else Other platforms / no sysroot
SH->>Env: Export LD_LIBRARY_PATH, DYLD_LIBRARY_PATH
end
SH->>RC: Execute rootcling with env
RC-->>SH: Generate dictionary
sequenceDiagram
autonumber
actor Dev as Developer
participant TP as test_project_*_containers.sh
participant CMake as CMake
participant Build as make
Dev->>TP: Run test script [args]
TP->>TP: Initialize parameters (e.g., -DCMAKE_CXX_STANDARD)
alt Double-configure flag
TP->>TP: Set parameters and/or run second configure (root vs stl variants)
else Single configure
TP->>TP: Keep parameters as-is
end
opt macOS sysroot available
TP->>TP: Append -DCMAKE_OSX_SYSROOT
end
TP->>CMake: cmake $parameters ..
CMake-->>TP: Configure results
TP->>Build: make -j
Build-->>TP: Build output
TP-->>Dev: Report success/failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmake/modules/FairRootTargetRootDictionary.cmake(1 hunks)cmake/scripts/generate_dictionary_root.sh.in(1 hunks)templates/test_project_root_containers.sh.in(1 hunks)templates/test_project_stl_containers.sh.in(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-06-14T10:16:00.050Z
Learnt from: dennisklein
PR: FairRootGroup/FairRoot#1560
File: cmake/private/FairRootConfig.cmake.in:13-13
Timestamp: 2024-06-14T10:16:00.050Z
Learning: `PACKAGE_PROJECT_CMAKEMOD_DIR` is defined by the call to `configure_package_config_file()` in the CMake configuration process for FairRoot.
Applied to files:
templates/test_project_stl_containers.sh.in
fuhlig1
force-pushed
the
vdev_patches
branch
from
b5bb9ad to
0320867
Compare
This change allows to build dictionaries with a non default macosx software development package (SDK). This is needed to be able to compile older ROOT versions on new compiler versions.
Pass the needed information about CMAKE_OSX_SYSROOT to rootcling such that the template tests also work on macosx with a non standard SDK.
|
could you please have a look if the commits are okay from your side. I would like to merge them such that I can port them to the patches branches. My tests run nearly on all systems. The only problem still is the on ubuntu 24.04 where doing a 'cp -a' of the templates directory results in a crash of the tests but this problem is unrelated to the actual test performed. |
|
the same problem with So the problem is probably due to the handling of the /tmp directory from the container. A possible workaround would be to use |
Replaces test_project_root_containers.sh.in and test_project_stl_containers.sh.in with a single parameterized test_project.sh.in script to eliminate code duplication.
- Add proper argument parsing with case statement - Use modern cmake -S/-B and --build syntax - Replace backticks with $() syntax - Add trap for cleanup on exit - Use bash arrays for cmake parameters - Add -u and -o pipefail to set flags - Make -x conditional on DEBUG environment variable - Add template directory validation - Use readonly for immutable variables - Redirect errors to stderr - Add --help flag
A simple |
There was a problem hiding this comment.
looks fine to me, I also did some deduplication/refactorings on the scripts, see fuhlig1/FairRoot@vdev_patches...dennisklein:FairRoot:vdev_patches
If you like them, feel free to fetch them :)
There was a problem hiding this comment.
Pull Request Overview
This PR enables building and testing FairRoot on macOS with non-default SDKs by adding proper SDK path handling. The changes ensure that the CMAKE_OSX_SYSROOT variable is propagated to various build scripts and dictionary generation processes.
- Added conditional CMAKE_OSX_SYSROOT parameter passing to test project build scripts
- Updated dictionary generation to export SDKROOT environment variable when using non-default macOS SDKs
- Modified CMake custom commands to include SDKROOT environment variable for rootcling execution
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| templates/test_project_stl_containers.sh.in | Added CMAKE_OSX_SYSROOT parameter handling and improved script structure |
| templates/test_project_root_containers.sh.in | Added CMAKE_OSX_SYSROOT parameter handling and streamlined reconfigure logic |
| cmake/scripts/generate_dictionary_root.sh.in | Added SDKROOT environment variable export for macOS SDK support |
| cmake/modules/FairRootTargetRootDictionary.cmake | Added SDKROOT environment variable to custom command for rootcling execution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Using `cp -a` mode to the tmp directory from a container fails for ubuntu 24.04 and opensuse 16.0 due failure of preserving permissions. Using `cp -RP` will copy the directory recursivly not following any symbolic links.
Have you tested the changes? I had first a problem that the shell script for the template test wasn't executable which was easy to fix but now I run into problems that headers are not found during dictionary creation. |
|
Nope, had no fairsoft installation lying around. |
|
Compiling FairSoft now and going to test the script changes |
|
I found the first issue but at least for v18.6_patches I wonder how the compilation could have worked before. |
|
Tested FairRoot Also ran the full test suite Let me know, if I can help with any different environment. |
Shell script must be executable.
|
Ok. I don't understand why it worked in your case but at least in the debian13 container the execution of the generated script test_project.sh haven't worked. It simply had no execution rights. After I changed that the tests for this branch, the v19.0_patches and the v18.8_patches branches worked like expected. |
Yes, this was missing, as I acknowledged in my post ;) |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
2 participants
The two commits allow to build and test FairRoot on macosx with a non default SDK. To use this feature a upcoming version of FairSoft is needed. Currently the head of jan24_patches which will soon get the tag jan24p6 or the release candidate branch for the upcoming new release sep25 will be okay.
For Linux systems or macosx systems using the default SDK the commits are not needed.
The two commits must be ported to the following branches:
Checklist: