Skip to content

Conversation

@PetrilloAtWork
Copy link
Member

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.
sbnobj did not fulfil that requirement; one side effect is that attempting to load sbn::crt::CRTHit or other objects from sbnobj/Common/CRT fails:

root [0] sbn::crt::CRTHit hit;                                                                                                    /cvmfs/larsoft.opensciencegrid.org/products/root/v6_28_12/Linux64bit+3.10-2.17-e26-p3915-prof/etc/cling/std.modulemap:312:10: erro
r: module 'std.span' requires feature 'cplusplus20'                                                                                 module "span" {                                                                                                                 
         ^                                                                                                                        
/cvmfs/larsoft.opensciencegrid.org/products/range/v3_0_12_0/include/range/v3/range/concepts.hpp:25:10: note: submodule of top-leve
l module 'std' implicitly imported here                                                                                           
#include <span>                                                                                                                   
         ^                                                                                                                        
/cvmfs/larsoft.opensciencegrid.org/products/root/v6_28_12/Linux64bit+3.10-2.17-e26-p3915-prof/etc/cling/std.modulemap:312:10: erro
r: module 'std.span' requires feature 'cplusplus20'                                                                               
  module "span" {                                                                                                                 
         ^                                                                                                                        
/cvmfs/larsoft.opensciencegrid.org/products/range/v3_0_12_0/include/range/v3/range/access.hpp:27:10: note: submodule of top-level 
module 'std' implicitly imported here                                                                                             
#include <span>                                                                                                                   
         ^                                                                                                                        

The reason is ROOT Cling interpreter trying to load algorithm code, which pulls ranve-v3 library in, and Cling can't parse it (yet — maybe enabling C++20 it could).

The changes in this commit conform sbnobj to that requirement.

This PR requires lardataobj v10_04_00, first merged in LArSoft v10_15_00 (because of LArSoft issue #30090).

Reviewers: the changes are of technical nature, no actual change is expected.
However, dependencies unnecessary to sbnobj have 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.

Minor modifications to the code were required.
Copy link

Copilot AI left a 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_t with std::ptrdiff_t to 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)
Copy link

Copilot AI Dec 22, 2025

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.

Suggested change
find_package( canvas REQUIRED EXPORT)
find_package(canvas REQUIRED EXPORT)

Copilot uses AI. Check for mistakes.
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant