Resolve CMake warnings#910
Open
PetrilloAtWork wants to merge 9 commits into
Open
Conversation
Library linking has been turned into private. Header-only library `icaruscode::TPC_Utilities_tools` added to host the tool interfaces and some definitions.
SignalShapingICARUSService source was split into an implementation (source and header) and a registration (service macros) part. The header and implementation have lost the `_service` in the name, so code including the header needs to be updated. So now the idea is that: * icaruscode::TPC_Utilities_SignalShapingICARUSService_service contains `SignalShapingICARUSService.cc` and it is what user code should link to * icaruscode::TPC_Utilities_SignalShapingICARUSService_service_reg contains `SignalShapingICARUSService_service.cc` with the service "registration" macros; we don't bother with it, it's framework business * `icaruscode::TPC_Utilities` actually "contains" `SignalShapingICARUSService.h`; this is needed for the compiler to find the header, even if no library code is generated out of it. However, `icaruscode::TPC_Utilities_SignalShapingICARUSService_service` has a PUBLIC dependency on `icaruscode::TPC_Utilities`, so linking to the former enables the header discovery. Not sure if this is a good strategy, but so far it seems to work. Note that this naming pattern is the one expected by `cet_build_plugin()` in `CMakeLists.txt`.
The new layout is: * service provider and service interface (`ICARUSChannelMap.h`) in the library * implementation of the service in `ICARUSChannelMap.cc` * registration of the service in `ICARUSChannelMap_service.cc`
This service has no public interface. The header and implementation are kept in `icaruscode::TPC_Utilities_FileCatalogMetadataICARUS_service` and the registration library learns about the service class by linking to that library (mostly to get access to the header).
To automatise the versioning of the package, this file now requires the version to be always set in the same place (`icaruscode_CMAKE_PROJECT_VERSION_STRING`) and then takes care of communicating it to CMake if the version number follows the standard CMake version pattern. Also, removed some old debugging and commented out obsolete lines.
There was a problem hiding this comment.
Pull request overview
This PR focuses on reducing/clearing CMake warnings across icaruscode by tightening link interfaces (mostly switching plugin dependency lists to PRIVATE) and by cleaning up a couple of service implementations/registrations.
Changes:
- Update many
cet_build_plugin(...)invocations to useLIBRARIES PRIVATE ...(and related target-name modernizations) to reduce transitive link-interface warnings. - Refactor
SignalShapingICARUSServiceimplementation into a separate.ccand update includes to the non-_serviceheader. - Introduce a legacy
ICARUSChannelMapwrapper service class (.h/.cc) and simplify its_service.ccto registration macros; update top-level project version handling inCMakeLists.txt.
Reviewed changes
Copilot reviewed 66 out of 66 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| icaruscode/Utilities/CMakeLists.txt | Switch plugin link interfaces to PRIVATE; add note for DuplicateEventTracker. |
| icaruscode/TPC/Utilities/tools/CMakeLists.txt | Convert to cet_make_library(INTERFACE ...) and tighten tool plugin link interfaces. |
| icaruscode/TPC/Utilities/SignalShapingICARUSService.h | Update Doxygen \\file path. |
| icaruscode/TPC/Utilities/SignalShapingICARUSService.cc | New: moved service implementation code into its own translation unit. |
| icaruscode/TPC/Utilities/SignalShapingICARUSService_service.cc | Reduce service registration TU to DEFINE_ART_SERVICE(...) only. |
| icaruscode/TPC/Utilities/CMakeLists.txt | Add ROOT::Hist; adjust service plugin link interface/export. |
| icaruscode/TPC/Tracking/MCS/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/TPC/Tracking/ICARUSPandora/CMakeLists.txt | Mark tool plugin dependencies as PRIVATE. |
| icaruscode/TPC/Tracking/cluster3D/CMakeLists.txt | Mark module/tool plugin dependencies as PRIVATE. |
| icaruscode/TPC/SPAna/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/TPC/Simulation/SpaceChargeServices/CMakeLists.txt | Mark service plugin dependencies as PRIVATE. |
| icaruscode/TPC/Simulation/Overlay/tools/CMakeLists.txt | Mark tool plugin dependencies as PRIVATE. |
| icaruscode/TPC/Simulation/Overlay/OverlayICARUS_module.cc | Update include to SignalShapingICARUSService.h. |
| icaruscode/TPC/Simulation/Overlay/CMakeLists.txt | Update service target name and mark module deps PRIVATE. |
| icaruscode/TPC/Simulation/DetSim/tools/CoherentNoiseFactor_service.cc | Fix incorrect file header comment. |
| icaruscode/TPC/Simulation/DetSim/tools/CMakeLists.txt | Mark tool/service plugin dependencies as PRIVATE. |
| icaruscode/TPC/Simulation/DetSim/SimWireICARUS_module.cc | Update include to SignalShapingICARUSService.h. |
| icaruscode/TPC/Simulation/DetSim/SimReadoutBoardICARUS_module.cc | Update include to SignalShapingICARUSService.h. |
| icaruscode/TPC/Simulation/DetSim/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/TPC/SignalProcessing/RecoWire/SimTestPulse/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/TPC/SignalProcessing/RecoWire/ROITools/CMakeLists.txt | Mark tool plugin dependencies as PRIVATE. |
| icaruscode/TPC/SignalProcessing/RecoWire/RecoWireROI_module.cc | Update include to SignalShapingICARUSService.h. |
| icaruscode/TPC/SignalProcessing/RecoWire/DeconTools/ROIFinderStandard_tool.cc | Update include to SignalShapingICARUSService.h. |
| icaruscode/TPC/SignalProcessing/RecoWire/DeconTools/ROIDeconvolution_tool.cc | Update include to SignalShapingICARUSService.h. |
| icaruscode/TPC/SignalProcessing/RecoWire/DeconTools/FullWireDeconvolution_tool.cc | Update include to SignalShapingICARUSService.h. |
| icaruscode/TPC/SignalProcessing/RecoWire/DeconTools/CMakeLists.txt | Mark tool plugin dependencies as PRIVATE. |
| icaruscode/TPC/SignalProcessing/RecoWire/DeconTools/BaselineStandard_tool.cc | Update include to SignalShapingICARUSService.h. |
| icaruscode/TPC/SignalProcessing/RecoWire/DeconTools/BaselineMostProbAve_tool.cc | Update include to SignalShapingICARUSService.h. |
| icaruscode/TPC/SignalProcessing/RecoWire/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/TPC/SignalProcessing/RawDigitFilter/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/TPC/SignalProcessing/RawDigitFilter/Algorithms/CMakeLists.txt | Update service target name; mark tool deps PRIVATE. |
| icaruscode/TPC/SignalProcessing/HitFinder/HitFinderTools/CMakeLists.txt | Mark tool plugin dependencies as PRIVATE. |
| icaruscode/TPC/SignalProcessing/HitFinder/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/TPC/Compression/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/TPC/Calorimetry/CMakeLists.txt | Mark tool plugin dependencies as PRIVATE. |
| icaruscode/Timing/CMakeLists.txt | Normalize target names; mark plugin deps PRIVATE. |
| icaruscode/PMT/Trigger/CMakeLists.txt | Mark module/tool plugin dependencies as PRIVATE. |
| icaruscode/PMT/OpticalTools/CMakeLists.txt | Add missing dependency; mark tool deps PRIVATE. |
| icaruscode/PMT/OpReco/FlashFinder/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/PMT/OpReco/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/PMT/LibraryMappingTools/CMakeLists.txt | Mark tool plugin dependencies as PRIVATE. |
| icaruscode/PMT/CMakeLists.txt | Mark many module/tool plugin dependencies as PRIVATE. |
| icaruscode/PMT/Calibration/CMakeLists.txt | Normalize target names; mark deps PRIVATE; switch to cet_make_exec. |
| icaruscode/Overlays/CMakeLists.txt | Collapse LIBRARIES PRIVATE formatting and keep dependencies private. |
| icaruscode/Geometry/CMakeLists.txt | Mark tool/service plugin dependencies as PRIVATE. |
| icaruscode/Generators/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/Filters/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/Decode/DecoderTools/FakeParticle_tool.cc | Update include to SignalShapingICARUSService.h. |
| icaruscode/Decode/DecoderTools/CMakeLists.txt | Mark tool plugin dependencies as PRIVATE. |
| icaruscode/Decode/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/Decode/ChannelMapping/Legacy/ICARUSChannelMap.h | New: legacy wrapper service class declaration and documentation. |
| icaruscode/Decode/ChannelMapping/Legacy/ICARUSChannelMap.cc | New: legacy wrapper service implementation and run-based loading. |
| icaruscode/Decode/ChannelMapping/Legacy/ICARUSChannelMap_service.cc | Reduce to service-interface registration macros. |
| icaruscode/Decode/ChannelMapping/Legacy/CMakeLists.txt | Adjust library/plugin link interfaces; exclude wrapper impl from library. |
| icaruscode/Decode/ChannelMapping/CMakeLists.txt | Swap larcorealg::CoreUtils to larcorealg::headers; mark service deps PRIVATE. |
| icaruscode/CRT/CRTLegacyCode/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/CRT/CRTDecoder/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/CRT/CMakeLists.txt | Mark module plugin dependencies as PRIVATE; modernize one plugin macro usage. |
| icaruscode/Analysis/trigger/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/Analysis/tools/MCTruth/CMakeLists.txt | Mark tool/module plugin dependencies as PRIVATE. |
| icaruscode/Analysis/tools/CMakeLists.txt | Add missing deps; mark tool plugin dependencies as PRIVATE. |
| icaruscode/Analysis/tools/BasicWireAnalysis_tool.cc | Update include to SignalShapingICARUSService.h. |
| icaruscode/Analysis/tools/BasicRawDigitAnalysis_tool.cc | Update include to SignalShapingICARUSService.h. |
| icaruscode/Analysis/overburden/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| icaruscode/Analysis/CMakeLists.txt | Mark module plugin dependencies as PRIVATE. |
| CMakeLists.txt | Rework version-string assignment and include version in configuration banner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cet_build_plugin(Filter art::tool LIBRARIES ${TOOL_LIBRARIES}) | ||
| cet_build_plugin(Response art::tool LIBRARIES ${TOOL_LIBRARIES}) | ||
| cet_build_plugin(ElectronicsResponseBesselApprox art::tool LIBRARIES PRIVATE ${TOOL_LIBRARIES}) | ||
| cet_build_plugin(ElectronicsResponse art::tool LIBRARIES PRIVATE LIBRARIES PRIVATE ${TOOL_LIBRARIES}) |
| cet_build_plugin( PMTTimingCorrectionService art::service LIBRARIES PRIVATE ${SERVICE_LIBRARIES}) | ||
|
|
||
| cet_build_plugin( PMTBeamSignalsExtractor art::producer LIBRARIES PUBLIC ${MODULE_LIBRARIES}) | ||
| cet_build_plugin( PMTBeamSignalsExtractor art::Producer LIBRARIES PRIVATE ${MODULE_LIBRARIES}) |
| /** | ||
| * @file icaruscode/Decode/ChannelMapping/Legacy/ICARUSChannelMap_service.cc | ||
| * @brief Wrapper service for `icarusDB::ICARUSChannelMapProvider`. | ||
| * @file icaruscode/Decode/ChannelMapping/Legacy/ICARUSChannelMap.cc |
Comment on lines
+5
to
+6
| /// \brief Service to provide ICARUS-specific signal shaping for | ||
| /// simulation (convolution) and reconstruction (deconvolution)./Users/Lsrea/newSim/SignalShapingICARUSService.h |
Comment on lines
+33
to
+35
| # NOTE: DuplicateEventTracker exposes an interface, | ||
| # but PRIVATE linking makes that inaccessible to modules. | ||
| cet_build_plugin(DuplicateEventTracker art::service LIBRARIES PRIVATE |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These changes attempt to address all CMake warnings from
icaruscodeexcept one about Json which we have no control on.It also proposes a change in how the version is assigned to
icaruscodemainCMakeLists.txt, to be reviewed by the release managers.My grasp of what causes CMake to complain about one-definition-rule (ODR) violations is tenuous, but I have received some guidance¹ and tried to understand the recipes that avoid them. In general it appears that linking library dependencies as
PRIVATEhelps avoiding the violation.Special thanks to @hgreenlee for it — you are welcome to review these changes if you feel like.
[1] That guidance is about migrating the build to Spack, but I have not really followed it, just learned workarounds from it, so I doubt this will help much when we need to migrate for real.
Reviewers
CMakeLists.txt: while my pattern seems to work, I don't know if there are build scripts it is not compatible with.Testing
This PR should be tested with the integration tests. Unit tests are definitely not enough to guarantee it works. In fact, the FHiCL unit test fails because of inherited issues that other pull requests surely fix already.
Notes on the changes
The following notes are mostly for future documentation on the motivations of the choices in this PR. It is highly likely that the result will be scorn at these badly informed choices.
Negative examples are still examples.
art modules
art modules are end points of the code: they do not require a public interface, nor a header, nor they are linked to at build time. Declaring their libraries as
PRIVATEis enough to address violation warnings.art tools
Tools are more complicate beasts: they have a shared interface (not necessarily, but a tool with no interface is no better than a library) that needs to be included by some other units, they may be base for more derivative tool classes, they have an implementation, and they also need some "registration" with the framework.
The recommended pattern is to put the registration (art-provided preprocessor macros) in its own library (a source file
<tool name>_tool.cc) and the implementation in another one (a source file ending in<tool name>.cc). I know of no prescription about the headers. While a tool would normally not need a header (the callers use the interface header, not the tool one), having two source files about the same class means they need to share the definition of that class, that is a header. Also the interface header needs to be available in some CMake "target" so that using code is correctly configured to look into the appropriate header directory. My approach here wasPRIVATE, which should avoid ODR violation;if there is no code in that directory beside the tools,
cet_make_library(HEADERS_TARGET_ONLYwill force the creation of a CMake target.An interesting side effect of separating the registration from the implementation is that in some cases the latter can become fully framework-independent and be used in more general code.
art services
Services are more similar to tools, with the exception that many of them do have a public interface and a header (exceptions are the implementations of a service interface — in the art sense — which do behave like tools). They present the same choices: in which CMake target to put the interface/header, in which the implementation, in which the registration code. Ideally users will have to specify a single target to link, and that target should be the interface one (all plugins are dynamically linked at run time anyway). One possible pattern is:
HEADERS_TARGET_ONLY, e.g.icaruscode::ICoherentNoiseFactor; if the interface is not shared, this is the service header instead;icaruscode::TPC_Simulation_DetSim_tools_CoherentNoiseFactor_service);icaruscode::TPC_Simulation_DetSim_tools_CoherentNoiseFactor_service_reg).I tried the cheaper way instead:
icaruscode::TPC_Simulation_DetSim_tools_CoherentNoiseFactor_service)cet_make_library(), and callers will link to this one instead.I got away with this most of the times. Two exceptions:
icaruscode::Decode_ChannelMapping_Legacy_ICARUSChannelMap_service, I did split the source in three (implementation, registration and header), but shoved all in the same library and linked asPUBLICthe target where the header is included (icaruscode::Decode_ChannelMapping_Legacy), so that linking units can access that library and header when linking to the service target.icaruscode::TPC_Utilities_SignalShapingICARUSService_service, the same, except that I had to explicitly create theHEADERS_TARGET_ONLYlibraryicaruscode::TPC_Utilitiesbecause none existed.Differently from tools, services are bound to the framework by the callback registry, so they are not framework independent (even if there is no registered callback).