refactor(Calorimeter): obtain tomlplusplus via find_package#23
Conversation
📝 WalkthroughWalkthroughThis PR migrates toml++ from a pinned vendored source (FetchContent) to a system-managed dependency. The pixi configuration declares the tomlplusplus dependency, the CMake build file switches to Changestoml++ System Dependency Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)subsystems/Calorimeter/src/CalorimeterConfig.cppsubsystems/Calorimeter/src/CalorimeterConfig.cpp:27:10: fatal error: 'Calorimeter/CalorimeterConfig.h' file not found ... [truncated 2200 characters] ... in file "src/clang/cTrans.ml" (inlined), line 4765, characters 38-71 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. Comment |
Replace the FetchContent_Declare(tomlplusplus) block with a single find_package(tomlplusplus REQUIRED). This avoids fetching toml++ from GitHub at configure time and aligns with how the repo treats its other production dependencies (GeoModelCore, GeoModelIO, GeoModelTools). tomlplusplus is available on conda-forge and in most distros, so this is a small environment-setup requirement bump rather than a real friction increase. Builds without the package installed will fail at configure time with a clear CMake error pointing at the missing package.
olantwin
force-pushed
the
refactor/calorimeter-tomlplusplus-find-package
branch
from
140d6d0 to
733373b
Compare
Commit 733373b switched the Calorimeter from FetchContent to find_package(tomlplusplus REQUIRED), but the pixi environment used by CI didn't declare tomlplusplus, so configure failed with "Could not find a package configuration file provided by tomlplusplus". Add tomlplusplus to pixi.toml [dependencies]. conda-forge only ships 3.3.0, which still uses the legacy <toml++/toml.h> header (the .h -> .hpp rename landed in 3.4.0), so update CalorimeterConfig.cpp to include <toml++/toml.h>. This matches the version available in the pixi env; if a newer build is later published on the SHiP channel we can switch back to toml.hpp.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
subsystems/Calorimeter/src/CalorimeterConfig.cpp (1)
6-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale FetchContent reference.
This comment still mentions "fetched via CMake FetchContent," but the PR migrates to
find_package. Update the documentation to reflect the new system-managed dependency approach.📝 Proposed fix to update the comment
-// Calorimeter configuration parser. Reads a calo.toml file and populates -// a CalorimeterConfig struct. Built on toml++ -// (https://github.com/marzer/tomlplusplus, single-header MIT, fetched via -// CMake FetchContent), so the file format is standard TOML — no bespoke -// scanner, no semicolon quirks. +// Calorimeter configuration parser. Reads a calo.toml file and populates +// a CalorimeterConfig struct. Built on toml++ +// (https://github.com/marzer/tomlplusplus, single-header MIT, resolved via +// find_package from conda-forge/system packages), so the file format is +// standard TOML — no bespoke scanner, no semicolon quirks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@subsystems/Calorimeter/src/CalorimeterConfig.cpp` around lines 6 - 8, Update the explanatory comment about tomlplusplus in CalorimeterConfig.cpp to remove the outdated "fetched via CMake FetchContent" wording and state that tomlplusplus is now resolved/managed via find_package (system-managed dependency), e.g., replace the phrase "fetched via CMake FetchContent" with something like "discovered via CMake find_package / system-managed dependency" while keeping the rest of the note about tomlplusplus being a single-header MIT library and the TOML format standards intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@subsystems/Calorimeter/src/CalorimeterConfig.cpp`:
- Line 36: The include in CalorimeterConfig.cpp currently uses the legacy header
name <toml++/toml.h> which breaks with tomlplusplus ≥3.4.0 where the header is
<toml++/toml.hpp>; either pin tomlplusplus in pixi.toml to the older version or
change CalorimeterConfig.cpp to conditionally include the correct header (e.g.,
use preprocessor detection like __has_include to try <toml++/toml.hpp> first and
fall back to <toml++/toml.h>), ensuring any code using the toml++ symbols
remains unchanged.
---
Outside diff comments:
In `@subsystems/Calorimeter/src/CalorimeterConfig.cpp`:
- Around line 6-8: Update the explanatory comment about tomlplusplus in
CalorimeterConfig.cpp to remove the outdated "fetched via CMake FetchContent"
wording and state that tomlplusplus is now resolved/managed via find_package
(system-managed dependency), e.g., replace the phrase "fetched via CMake
FetchContent" with something like "discovered via CMake find_package /
system-managed dependency" while keeping the rest of the note about tomlplusplus
being a single-header MIT library and the TOML format standards intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a79aa9f0-d8ec-422f-8ddd-0e3b46eb79f9
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pixi.tomlsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/src/CalorimeterConfig.cpp
| #include <string> | ||
| #include <string_view> | ||
| #include <toml++/toml.hpp> | ||
| #include <toml++/toml.h> |
There was a problem hiding this comment.
Header path assumes tomlplusplus <3.4.0.
This include uses the legacy .h extension, which matches tomlplusplus 3.3.0. However, pixi.toml specifies tomlplusplus = "*", allowing any version. When tomlplusplus ≥3.4.0 becomes available on conda-forge, this build will fail because the header was renamed to toml++/toml.hpp.
Either pin the version constraint in pixi.toml (see comment on pixi.toml:17) or use conditional compilation to support both header names.
🔄 Alternative fix: support both header names
If you want to support both old and new versions, use version-detection:
-#include <toml++/toml.h>
+#if __has_include(<toml++/toml.hpp>)
+#include <toml++/toml.hpp>
+#else
+#include <toml++/toml.h>
+#endifThis allows the code to work with both v3.3.0 (.h) and v3.4.0+ (.hpp), and you can then use a wider version range like tomlplusplus = ">=3.3.0" in pixi.toml.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@subsystems/Calorimeter/src/CalorimeterConfig.cpp` at line 36, The include in
CalorimeterConfig.cpp currently uses the legacy header name <toml++/toml.h>
which breaks with tomlplusplus ≥3.4.0 where the header is <toml++/toml.hpp>;
either pin tomlplusplus in pixi.toml to the older version or change
CalorimeterConfig.cpp to conditionally include the correct header (e.g., use
preprocessor detection like __has_include to try <toml++/toml.hpp> first and
fall back to <toml++/toml.h>), ensuring any code using the toml++ symbols
remains unchanged.
1 participant
Summary
Motivation
Surfaced while writing a `shipgeometry` recipe for `ship-conda-recipes`. `FetchContent` requires network at configure time, which we'd rather avoid in packaged builds. `tomlplusplus` is available on conda-forge (and as e.g. `libtomlplusplus-dev` on Debian/Ubuntu), so requiring it as a system dep is a small environment-setup bump rather than real friction.
Behaviour change
Builds without `tomlplusplus` installed will now fail at configure time with a clear CMake error pointing at the missing package, rather than silently downloading from GitHub. For contributors, `pixi install` / `apt install libtomlplusplus-dev` / `brew install tomlplusplus` covers it.
Test plan
Summary by CodeRabbit