feat: Allow compiling examples without ROOT #4090
Conversation
WalkthroughHmm, changes made they have been. A new CMake option for ROOT I/O is introduced, it is. Conditional logic in CMake files now controls the inclusion of ROOT dependencies and example subdirectories. In Python bindings, adjustments are done—legacy ROOT readers and writers removed, new functions and interface for track parameter lookup added. Various source files in the Examples/Io and Examples/Python directories updated have been. Configurability increased, it is. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as CMake
participant P as Python Bindings
participant R as ROOT Packages
U->>C: Set ACTS_BUILD_EXAMPLES_ROOT (ON/OFF)
alt ROOT Enabled
C->>R: Find and verify ROOT packages
C->>C: Include Root subdirectory and apply compile definitions
C->>P: Link ActsExamplesIoRoot and select ROOT input/output sources
P->>P: Register addRootInput and addRootOutput functions with reader/writer bindings
else ROOT Disabled
C->>P: Select stub implementations for ROOT support
end
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Examples
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
Examples/Python/src/Input.cpp (1)
70-73: A new class binding, introduced you have.
Implementation detail for theITrackParamsLookupReaderis minimal here, hmmm. Large expansions possible in the future, yes. Document or extend it, you could.Examples/Python/src/RootOutput.cpp (2)
152-210: Wise it is to separate measurement logic, root or otherwise.
Two distinct classes for measurement and material writing, you have. Straightforward they seem, but thorough testing with real geometry data, ensure.
212-254: Root-based writer macros, many they be.
Comprehensive coverage of use cases, indeed. Maintain consistency in file naming and paths, you should.Examples/Python/src/RootInput.cpp (1)
28-60: Function addRootInput, to read data in.
A clear approach, using macros for each specialized reader. Verify naming for clarity, you should, especially with many overlapping parameters.Examples/Io/Root/CMakeLists.txt (1)
50-50: Document the purpose of this compile definition, you should!While correct the definition is, its purpose and usage in the codebase, unclear they remain.
Add a comment explaining the purpose of ACTS_EXAMPLES_WITH_ROOT:
+# Define ACTS_EXAMPLES_WITH_ROOT to enable conditional compilation of ROOT-dependent code target_compile_definitions(ActsExamplesIoRoot PUBLIC ACTS_EXAMPLES_WITH_ROOT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CMakeLists.txt(2 hunks)CMakePresets.json(1 hunks)Examples/Io/CMakeLists.txt(1 hunks)Examples/Io/Root/CMakeLists.txt(1 hunks)Examples/Python/CMakeLists.txt(1 hunks)Examples/Python/src/Input.cpp(2 hunks)Examples/Python/src/Material.cpp(3 hunks)Examples/Python/src/ModuleEntry.cpp(2 hunks)Examples/Python/src/Output.cpp(1 hunks)Examples/Python/src/RootInput.cpp(1 hunks)Examples/Python/src/RootOutput.cpp(1 hunks)Examples/Python/src/RootStub.cpp(1 hunks)docs/getting_started.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- Examples/Python/src/Material.cpp
- Examples/Python/src/RootStub.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (14)
Examples/Python/src/Input.cpp (1)
20-20: A new include, necessary it is.
Include line matches the new interface usage, correct it appears.Examples/Python/src/RootOutput.cpp (3)
9-30: Galore of includes, you have.
All relevant headers, correct they seem. Ensure compilation flags remain consistent, you must.
53-74: Function addRootOutput, introduced it is.
Central binding method it serves, grouping many writers in one place, hmm. Good design pattern for modularizing I/O, it appears.
76-150: Writers, declared in plenty they are.
Macros yield clarity, yes, though verifying correctness with actual data flow, necessary it is.Examples/Python/src/RootInput.cpp (2)
9-16: ROOT-based headers, included.
Needed for bridging the I/O functionalities, hmmm. All correct they look.
61-65: Conditional compilation, you have.
WITH_GEOMODEL_PLUGINusage is wise, but test coverage ensure for both on and off states, hmmm.Examples/Python/src/ModuleEntry.cpp (2)
59-60: Clean and orderly, these additions are!Following the established pattern of module initialization functions, these declarations do. Approve them, I do.
150-151: Wise placement of initialization calls, this is!At the end of plugin initializations, these new calls reside. A natural position for ROOT-related functionality, it is.
Examples/Python/src/Output.cpp (1)
211-216: Well-crafted bindings, these are!Using shared_ptr wisely, you do. Following pybind11 best practices, this implementation does.
Examples/Io/CMakeLists.txt (1)
6-6: Consistent with the Force, this CMake change is!Like its siblings EDM4hep and HepMC3, conditional inclusion for Root now exists. A harmonious balance in the build system, this brings.
Examples/Python/CMakeLists.txt (1)
86-94: Well structured, this conditional compilation is!Follow the established patterns it does, hmm. When ROOT not present, stub implementation it provides. Clean and consistent with other similar conditionals in the file, this approach is.
CMakeLists.txt (2)
106-106: Wisely placed, this new option is.Among other example-related options it sits, following the established pattern it does.
511-521: Proper ROOT dependency handling, this is.When ACTS_BUILD_EXAMPLES_ROOT enabled is, required ROOT components it finds. Compatibility it checks, hmm. Good practices these are.
docs/getting_started.md (1)
303-303: Well documented, this new option is.In the build options table, properly placed it is. Format it follows, description clear it provides.
| @@ -17,6 +17,7 @@ | |||
| "ACTS_BUILD_PLUGIN_LEGACY": "ON", | |||
| "ACTS_BUILD_EXAMPLES_DD4HEP": "ON", | |||
| "ACTS_BUILD_EXAMPLES_PYTHON_BINDINGS": "ON", | |||
| "ACTS_BUILD_EXAMPLES_ROOT": "ON", | |||
There was a problem hiding this comment.
Inconsistent default value with CMakeLists.txt, this is.
In CMakeLists.txt, default value OFF it is, but here ON we see. Confusion this may cause, when different build methods users try. Align these defaults we should, hmm.
There was a problem hiding this comment.
The convention is, that in the CmakeLists.txt, all options are off. Therefore, this aligns with our design.
There was a problem hiding this comment.
@benjaminhuth Understand the wisdom of your ways, I do. Clear it is now - CMakeLists.txt defaults to OFF by design, while presets may override, hmm yes.
(_/)
(^.^) ponders deeply about CMake wisdom
✏️ Learnings added
Learnt from: benjaminhuth
PR: acts-project/acts#4090
File: CMakePresets.json:20-20
Timestamp: 2025-02-14T22:15:51.190Z
Learning: In the ACTS project, CMake options in CMakeLists.txt are intentionally set to OFF by default, while CMakePresets.json can override these defaults with ON values as per the project's design convention.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| #include "ActsExamples/MaterialMapping/CoreMaterialMapping.hpp" | ||
| #include "ActsExamples/MaterialMapping/MappingMaterialDecorator.hpp" | ||
| #include "ActsExamples/MaterialMapping/MaterialMapping.hpp" | ||
| #include "ActsExamples/MaterialMapping/MaterialValidation.hpp" | ||
|
|
||
| #ifdef ACTS_EXAMPLES_WITH_ROOT |
There was a problem hiding this comment.
I think we should try to find a way to do this without a preprocessor define. Can we handle this in CMake somehow?
This would probably need to move to the ROOT IO folder.
There was a problem hiding this comment.
Yeah probably we should put this to RootInput.cpp and avoid the preprocessor ifdef.
First step towards
pip install acts?--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
New Features
Refactor
Documentation