-
Notifications
You must be signed in to change notification settings - Fork 11
Removed inappropriate repository dependencies #159
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: develop
Are you sure you want to change the base?
Removed inappropriate repository dependencies #159
Conversation
Minor modifications to the code were required.
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 removes inappropriate dependencies from sbnobj to ensure it depends only on "obj" packages (not "alg" or art-dependent packages), which fixes ROOT Cling interpreter issues when loading certain data objects like sbn::crt::CRTHit.
Key changes include:
- Removed dependencies on art framework, larcorealg, lardataalg, larcore, and lardata packages
- Replaced
util::quantities::tick::value_twithstd::ptrdiff_tto avoid pulling in lardataalg - Changed library dependency visibility from PRIVATE to PUBLIC where appropriate
- Cleaned up duplicate and unnecessary includes in dictionary headers
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Removed art, larcore, larcorealg, lardataalg, and lardata dependencies; added clarifying comment that package must not depend on art or *alg packages |
| sbnobj/ICARUS/TPC/CMakeLists.txt | Changed dependencies from PRIVATE to PUBLIC visibility; removed unnecessary dictionary library dependencies |
| sbnobj/ICARUS/PMT/Trigger/Data/OpticalTriggerGate.h | Replaced lardataalg tick type with std::ptrdiff_t to eliminate dependency |
| sbnobj/ICARUS/PMT/Trigger/Data/CMakeLists.txt | Removed lardataalg and larcorealg dependencies |
| sbnobj/Common/Utilities/CMakeLists.txt | Removed art framework and geometry dependencies unnecessary for data object library |
| sbnobj/Common/Reco/CMakeLists.txt | Removed larcorealg::Geometry dependency |
| sbnobj/Common/CRT/classes.h | Cleaned up duplicate includes and reordered headers |
| sbnobj/Common/CRT/CRTHitT0TaggingInfo.hh | Removed unnecessary geometry-related includes |
| sbnobj/Common/CRT/CMakeLists.txt | Replaced larcorealg::Geometry with larcoreobj::geo_vectors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| find_package(art REQUIRED EXPORT) | ||
| find_package(canvas REQUIRED EXPORT) | ||
| # this package must NOT depend on art, nor on the *alg LArSoft packages | ||
| find_package( canvas REQUIRED EXPORT) |
Copilot
AI
Dec 22, 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.
There's an inconsistency in spacing after "find_package(" compared to the line below. The original code had consistent spacing, but line 38 now has a space after the opening parenthesis while line 39 does not. Consider making the spacing consistent for better code style.
| find_package( canvas REQUIRED EXPORT) | |
| find_package(canvas REQUIRED EXPORT) |
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.
It's consistent with all the other existing lines, so it stays.
|
Thanks Gianluca! So, to be on the safe side, testing this change should (I'll reckon) involve running |
An "obj" package is designed to make ROOT understand LArSoft data objects, and it needs the minimum possible dependencies. In particular, is expected to depend only on other "obj" packages, no "alg" or art-dependent packages.
sbnobjdid not fulfil that requirement; one side effect is that attempting to loadsbn::crt::CRTHitor other objects fromsbnobj/Common/CRTfails:The reason is ROOT Cling interpreter trying to load algorithm code, which pulls
ranve-v3library in, and Cling can't parse it (yet — maybe enabling C++20 it could).The changes in this commit conform
sbnobjto that requirement.This PR requires
lardataobjv10_04_00, first merged in LArSoftv10_15_00(because of LArSoft issue #30090).Reviewers: the changes are of technical nature, no actual change is expected.
However, dependencies unnecessary to
sbnobjhave been removed, and it is possible that some packages that were (incorrectly) not declaring those dependencies will now fail to compile. In that case, those packages need to be fixed as well.So, asking a review from somebody with fingers into the build.