fix(build): Make trackbase build possible without Geant3 #1611
Conversation
📝 WalkthroughWalkthroughThe change moves the inclusion of the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fairroot/CMakeLists.txt (1)
36-37: Minor ordering nit – keep core unconditional directories groupedFor readability the unconditional core components appear in one block (
geobase,parbase,generators, …). Consider moving the newadd_subdirectory(trackbase)right after the
generatorsblock to preserve that grouping.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fairroot/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 Learning: `package_project_cmakemod_dir` is defined by the call to `configure_package_config_file()` in the cm...
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:
fairroot/CMakeLists.txt
🔇 Additional comments (1)
fairroot/CMakeLists.txt (1)
32-37: Validate thattrackbasereally builds when Geant3 is absentMaking
trackbaseunconditional is the right direction for #1609, but please confirm that nothing intrackbase/CMakeLists.txt(or its sources)• links against
Geant3::G3VMCLibrary/ usesGEANT3_INCLUDE_DIRS
• includes headers that live exclusively in thegeanesub-tree
• depends on targets produced bygeaneIf any such coupling still exists, wrap the offending bits with
if(Geant3_FOUND)guards insidetrackbaseinstead of failing at configure/link time.
ChristianTackeGSI
left a comment
ChristianTackeGSI
left a comment
There was a problem hiding this comment.
Generally this looks like the right thing to do.
If we want to support building without Geant3, do we have a testcase for this? (not sure, I haven't touched FairRoot for too long now).
P.S.: We probably should revive our CI one way or another?
|
@ChristianTackeGSI . Agree on everything. |
|
@ChristianTackeGSI can you have a look at the PR again? |
ChristianTackeGSI
left a comment
ChristianTackeGSI
left a comment
There was a problem hiding this comment.
Please remember to change the copyright year on all files that you changed.
Moved the add_subdirectory(trackbase) outside of if(Geant3_FOUND) condition. Fixes issue FairRootGroup#1609.
…sive` The ConstField magnetic field is used by few examples: Tutorial1, Tutorial3, propagator, By moving it to `common` the examples become more independent.
The build of `FairTutorialDet4PointDraw.cxx` dependend on `opengl`, while it should depend on `TARGET FairRoot::EventDisplay`. Fixed this.
…Display` Since only few classes really depend on `EventDisplay`, made only these few really depend on the exsistance of `EventDisplay`. Added `ExPassive` to link FairConstField.
The macro depended on Geant3, because it uses `FairGeane` and `FairGeanePro` inside an if function. Added preprocessor directives to remove this dependency.
Made order in the dependcies of many of the examples. Most were placed in the `if(Geant3_FOUND)` which is not needed. Moved `propagator` out of `if(TARGET FairRoot::EventDisplay` following previous commit. That increased the number of tests from 40 to 68 for the no-Geant3 case.
ChristianTackeGSI
left a comment
ChristianTackeGSI
left a comment
There was a problem hiding this comment.
Assuming you were able to compile things / run tests…
hopefully our CI will come back sometime.
|
I have tested on Mac/Sep25 and Ubuntu/May25. |
2 participants
Moved the add_subdirectory(trackbase) outside of if(Geant3_FOUND) condition.
Fixes issue #1609.
Checklist: