build: Add cmake helper function for header compilation #4083
Conversation
WalkthroughNew functionality for header management, introduced across the project it is. A helper function, Changes
Sequence Diagram(s)sequenceDiagram
participant CMake as "CMake Build System"
participant ACHF as "acts_compile_headers()"
participant FS as "File System"
participant Target as "Helper Target"
CMake->>ACHF: Call acts_compile_headers(target, GLOB pattern)
ACHF->>FS: Scan for header files matching the glob pattern
FS-->>ACHF: Return list of headers found
ACHF->>FS: Generate temporary source file for each header
ACHF->>Target: Create helper target with generated sources
Target-->>CMake: Link helper target to main target
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
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 - Core
![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
🔭 Outside diff range comments (1)
Tests/CommonHelpers/include/Acts/Tests/CommonHelpers/TestSpacePoint.hpp (1)
56-56:⚠️ Potential issueA disturbance in the Force, I sense. Bug in time accessor, there is.
Return m_t instead of m_z, you must. A critical error this is.
Apply this fix, you should:
- constexpr std::optional<float> t() const { return m_z; } + constexpr std::optional<float> t() const { return m_t; }
🧹 Nitpick comments (1)
Examples/Algorithms/Digitization/include/ActsExamples/Digitization/Smearers.hpp (1)
199-209: Optimize the Digital struct's operator(), we should.Cache the bin boundaries, we could. Multiple lookups of the same values, you perform.
Improve performance thus, you can:
Acts::Result<std::pair<double, double>> operator()( double value, RandomEngine& /*rnd*/) const { if (binningData.min < value && binningData.max > value) { auto bin = binningData.search(value); + const auto& boundaries = binningData.boundaries(); - auto lower = binningData.boundaries()[bin]; - auto higher = binningData.boundaries()[bin + 1]; + auto lower = boundaries[bin]; + auto higher = boundaries[bin + 1]; double svalue = 0.5 * (lower + higher); return std::pair{svalue, (higher - lower) / std::sqrt(12.)}; } return ActsFatras::DigitizationError::SmearingError; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
Core/CMakeLists.txt(1 hunks)Core/include/Acts/Propagator/detail/LoopStepperUtils.hpp(1 hunks)Examples/Algorithms/Digitization/CMakeLists.txt(1 hunks)Examples/Algorithms/Digitization/include/ActsExamples/Digitization/Smearers.hpp(1 hunks)Examples/Algorithms/MaterialMapping/CMakeLists.txt(1 hunks)Examples/Algorithms/MaterialMapping/include/ActsExamples/MaterialMapping/MappingMaterialDecorator.hpp(1 hunks)Examples/Algorithms/Propagation/CMakeLists.txt(1 hunks)Examples/Algorithms/Propagation/src/SimHitToSummaryConversion.cpp(0 hunks)Examples/Algorithms/Traccc/CMakeLists.txt(1 hunks)Examples/Algorithms/TrackFinding/CMakeLists.txt(1 hunks)Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/HoughVectors.hpp(0 hunks)Examples/Algorithms/TrackFindingML/CMakeLists.txt(1 hunks)Examples/Algorithms/TrackFitting/CMakeLists.txt(1 hunks)Examples/Detectors/ContextualDetector/CMakeLists.txt(1 hunks)Examples/Detectors/GenericDetector/CMakeLists.txt(1 hunks)Examples/Detectors/MagneticField/CMakeLists.txt(1 hunks)Examples/Detectors/TGeoDetector/CMakeLists.txt(1 hunks)Examples/Framework/CMakeLists.txt(1 hunks)Examples/Io/Csv/src/CsvOutputData.hpp(1 hunks)Examples/Io/Json/CMakeLists.txt(1 hunks)Examples/Io/Obj/CMakeLists.txt(1 hunks)Examples/Io/Obj/include/ActsExamples/Io/Obj/ObjWriterOptions.hpp(0 hunks)Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackContainer.hpp(1 hunks)Tests/CommonHelpers/CMakeLists.txt(2 hunks)Tests/CommonHelpers/include/Acts/Tests/CommonHelpers/TestSpacePoint.hpp(1 hunks)cmake/ActsFunctions.cmake(1 hunks)
💤 Files with no reviewable changes (3)
- Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/HoughVectors.hpp
- Examples/Io/Obj/include/ActsExamples/Io/Obj/ObjWriterOptions.hpp
- Examples/Algorithms/Propagation/src/SimHitToSummaryConversion.cpp
✅ Files skipped from review due to trivial changes (3)
- Examples/Algorithms/MaterialMapping/include/ActsExamples/MaterialMapping/MappingMaterialDecorator.hpp
- Examples/Io/Csv/src/CsvOutputData.hpp
- Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackContainer.hpp
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: merge-sentinel
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: docs
- GitHub Check: macos
- GitHub Check: build_debug
🔇 Additional comments (26)
cmake/ActsFunctions.cmake (4)
1-7: Well documented, this function is! Hmmmm.Clear purpose and usage instructions in comments, I see. The force is strong with this documentation.
20-36: Wise error handling, you have implemented.Prevent simultaneous use of GLOB and HEADERS, you do. Clear error message, you provide. Strong with the validation, this code is.
40-53: Thorough path validation, I sense.Check for absolute paths, non-existent files, and directories, you do. Prevent confusion and errors early, this will.
70-71: Exclude from default build, you wisely do.EXCLUDE_FROM_ALL flag, you use. Prevent build slowdown, this will. Link to original target correctly, you do.
Core/include/Acts/Propagator/detail/LoopStepperUtils.hpp (1)
11-13: Strong with types, this code has become.Added necessary includes for covariance handling and type traits, you have. Improve type safety and clarity, this will.
Examples/Algorithms/Digitization/include/ActsExamples/Digitization/Smearers.hpp (1)
141-153: Wise implementation of binning, I see.Good initialization of binningData with proper parameters, you have. Clear and maintainable, your code is.
Examples/Algorithms/Propagation/CMakeLists.txt (1)
13-14: Hmmmm, good this change is!Properly placed after target configuration, the header compilation function is. The Force, strong with this implementation it is.
Examples/Algorithms/Traccc/CMakeLists.txt (1)
17-18: Verify the use with INTERFACE library, we must.INTERFACE library this is, yet headers compile we attempt. Ensure functioning correctly with such library type, we should.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check if there are any header files to compile fd . "include" -e hppLength of output: 116
#!/bin/bash # Re-verify header files existence for the TRACCC example. # The 'acts_compile_headers' command in CMakeLists.txt uses a relative path "include/**/*.hpp". # Check in the expected directory relative to the CMakeLists.txt file. find Examples/Algorithms/Traccc/include -type f -name "*.hpp"Manual verification, please perform. The original command searched in the wrong directory, it did. Ensure that header files exist in the
Examples/Algorithms/Traccc/includedirectory for the INTERFACE library to compile correctly, you must. If header files are present and the INTERFACE target handles them properly as designed, you are. Otherwise, further investigation necessary, it might be.Examples/Io/Obj/CMakeLists.txt (1)
18-19: Balanced and harmonious, this change is!In the right place it sits, after target configuration but before installation. The path of the Jedi, this follows.
Examples/Algorithms/MaterialMapping/CMakeLists.txt (1)
18-19: Wise placement of header compilation, this is!Like a well-trained Padawan, this change follows the established pattern. Pleased with this implementation, I am.
Examples/Algorithms/TrackFindingML/CMakeLists.txt (1)
18-19: Strong with the Force, this implementation is!Perfect harmony it maintains with other changes. The way of the CMake, this follows.
Examples/Detectors/GenericDetector/CMakeLists.txt (1)
9-9: Hmmmm, good this change is!Compile the headers, it will. Help the static analysis tools, this shall. Wise placement after library definition, you have chosen.
Examples/Algorithms/Digitization/CMakeLists.txt (1)
20-20: Placed well, this change is!After library configurations, you have put it. Strong with the Force, this placement is. Compile all digitization headers, it shall.
Examples/Algorithms/TrackFitting/CMakeLists.txt (1)
20-20: Consistent with the Force, this change is!Follow the ways of other files, you do. Compile the track fitting headers, this shall. Balanced and harmonious, your changes are.
Examples/Io/Json/CMakeLists.txt (2)
21-21: A new ally, ActsExamplesTrackFinding becomes!Join forces with Json library, it does. Strong together, they shall be.
24-24: In harmony with other files, this change is!Compile the Json headers, it shall. Follow the path of consistency, you do.
Examples/Detectors/ContextualDetector/CMakeLists.txt (1)
23-23: Complete the circle, this change does!Like its siblings before it, placed well this function is. Compile the contextual detector headers, it shall. In balance with all other changes, this one is.
Examples/Detectors/MagneticField/CMakeLists.txt (1)
20-20: Approve this change, I do!Correctly implemented, the header compilation function is. With the force of proper build configuration, strong this one will be.
Tests/CommonHelpers/CMakeLists.txt (3)
7-8: Reorganized source paths, wisdom shows!To the 'src' directory, moved the source files are. Clear structure, this brings.
18-18: Include path, more specific it becomes!From root to include directory, the path now points. Better organization, this creates.
26-26: Header compilation, added it is!For Acts headers specifically, compilation configured is. Good practice, this follows.
Examples/Detectors/TGeoDetector/CMakeLists.txt (1)
25-25: Simple yet powerful, this change is!Header compilation configuration, properly added it is. With the force of consistency, strong this one remains.
Examples/Algorithms/TrackFinding/CMakeLists.txt (2)
23-23: Consolidated dependencies, clearer they become!On a single line, the public dependencies now rest. Better readability, this brings.
27-27: Header compilation, properly configured it is!Following the way of the force, header compilation added it was.
Examples/Framework/CMakeLists.txt (1)
61-61: In harmony with the force, this change is!After all configurations, header compilation properly placed it is. The way of CMake, followed it has.
Core/CMakeLists.txt (1)
115-116: Wise inclusion of ActsFunctions, this is!Help with compiler flags, it will. Clear path to better header management, I see.
|
Added the ability to turn this off via an option. |
|
Done, can you have another look @benjaminhuth? |
|
This PR adds a CMake function that will
_HEADERS.cppfile which includes that header, and adds it to the_HEADERStarget for compilationThis will help static analysis tools like clang-tidy, because the
compile_commands.jsonwill be populated.This PR also removes a leftover
ACTS_BUILD_NONCOMPILE_TESTSthat was unused.--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor / Chores
ACTS_BUILD_NONCOMPILE_TESTSoption from the configuration settings.