Find GeoModelXml in Ubuntu24 and include tomlplusplus as a prerequisite#40
Find GeoModelXml in Ubuntu24 and include tomlplusplus as a prerequisite#40antonioiuliano2 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe ChangesUbuntu24 GeoModelXml Build Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (1)
README.md (1)
40-40: ⚡ Quick winAdd distro package hints for
tomlplusplusin prerequisites.Line 40 confirms the dependency, but adding package names (e.g., Ubuntu:
libtomlplusplus-dev) would make setup faster and aligns with this PR’s Ubuntu 24 fix context.🤖 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 `@README.md` at line 40, The tomlplusplus dependency listed at line 40 in the prerequisites section lacks distro-specific package installation hints. Add package name references for popular Linux distributions (such as Ubuntu with libtomlplusplus-dev) to help users quickly install the required package without having to search for the correct package name for their system. This should be added inline or as a parenthetical note next to the tomlplusplus link to maintain consistency with the PR's goal of improving setup documentation for Ubuntu 24.
🤖 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/TimingDetector/CMakeLists.txt`:
- Around line 37-43: The DTD lookup using fixed-level path traversals in the
get_filename_component call with _gt_prefix is layout-sensitive and fails for
standard installs. Replace the hardcoded 4-level and 3-level relative path
traversals (using "../../../..") with a more robust approach that uses
GeoModelTools-provided CMake variables or package hints to determine the correct
prefix path, or use a layout-agnostic discovery mechanism that works across
different install directory structures. Ensure the resulting _geomodel_dtd path
is correctly resolved and properly copied to ${GMX_OUT_DIR}/geomodel.dtd for
downstream GMX parsing.
---
Nitpick comments:
In `@README.md`:
- Line 40: The tomlplusplus dependency listed at line 40 in the prerequisites
section lacks distro-specific package installation hints. Add package name
references for popular Linux distributions (such as Ubuntu with
libtomlplusplus-dev) to help users quickly install the required package without
having to search for the correct package name for their system. This should be
added inline or as a parenthetical note next to the tomlplusplus link to
maintain consistency with the PR's goal of improving setup documentation for
Ubuntu 24.
🪄 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: 34cb64d8-6b50-402d-a568-320274177f7d
📒 Files selected for processing (3)
CHANGELOG.mdREADME.mdsubsystems/TimingDetector/CMakeLists.txt
| get_filename_component( | ||
| _gt_prefix | ||
| "${GeoModelTools_DIR}/../../../.." | ||
| ABSOLUTE | ||
| ) | ||
| set(_geomodel_dtd "${_gt_prefix}/share/GeoModelXml/geomodel.dtd") | ||
| if(EXISTS "${_geomodel_dtd}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Demonstrate path resolution sensitivity for common GeoModelTools_DIR layouts.
python - <<'PY'
from pathlib import Path
examples = [
"/usr/lib/cmake/GeoModelTools",
"/usr/lib/x86_64-linux-gnu/cmake/GeoModelTools",
]
for p in examples:
up4 = str((Path(p) / "../../../..").resolve())
up3 = str((Path(p) / "../../..").resolve())
print(f"{p}\n up4={up4}\n up3={up3}\n")
PYRepository: ShipSoft/Geometry
Length of output: 183
🏁 Script executed:
# Verify the actual code at lines 37-43 in subsystems/TimingDetector/CMakeLists.txt
cat -n subsystems/TimingDetector/CMakeLists.txt | sed -n '35,50p'Repository: ShipSoft/Geometry
Length of output: 631
🏁 Script executed:
# Search for other GeoModelXml references in the codebase
rg "GeoModelXml|geomodel\.dtd" --type cmakeRepository: ShipSoft/Geometry
Length of output: 912
🏁 Script executed:
# Check for other DTD lookup logic related to GeoModelTools
rg "GeoModelTools_DIR" --type cmake -A 5Repository: ShipSoft/Geometry
Length of output: 1035
🏁 Script executed:
# Verify timing_detector.gmx exists and inspect how it uses the DTD
find . -name "timing_detector.gmx" -o -name "*.gmx" | head -5Repository: ShipSoft/Geometry
Length of output: 114
🏁 Script executed:
head -20 subsystems/TimingDetector/data/timing_detector.gmxRepository: ShipSoft/Geometry
Length of output: 1959
Make DTD lookup robust across GeoModelTools install layouts.
Lines 32 and 39 attempt fallback lookups using 3-level and 4-level path traversals from GeoModelTools_DIR. Both approaches are layout-sensitive: for standard installs like /usr/lib/cmake/GeoModelTools or /usr/lib/x86_64-linux-gnu/cmake/GeoModelTools, both traversals overshoot and resolve to incorrect prefixes (/usr or /), causing the DTD lookup to fail. When the DTD is not found, it is not copied to ${GMX_OUT_DIR}/geomodel.dtd, and downstream GMX parsing (which expects geomodel.dtd by filename) fails at runtime.
Suggested fix
- get_filename_component(
- _gt_prefix
- "${GeoModelTools_DIR}/../../../.."
- ABSOLUTE
- )
- set(_geomodel_dtd "${_gt_prefix}/share/GeoModelXml/geomodel.dtd")
- if(EXISTS "${_geomodel_dtd}")
+ set(_geomodel_dtd "")
+ foreach(_cand
+ "${GeoModelTools_DIR}/../../../../share/GeoModelXml/geomodel.dtd"
+ "${GeoModelTools_DIR}/../../../share/GeoModelXml/geomodel.dtd"
+ )
+ if(EXISTS "${_cand}")
+ set(_geomodel_dtd "${_cand}")
+ break()
+ endif()
+ endforeach()
+ if(_geomodel_dtd)
configure_file(
"${_geomodel_dtd}"
"${GMX_OUT_DIR}/geomodel.dtd"
COPYONLY
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get_filename_component( | |
| _gt_prefix | |
| "${GeoModelTools_DIR}/../../../.." | |
| ABSOLUTE | |
| ) | |
| set(_geomodel_dtd "${_gt_prefix}/share/GeoModelXml/geomodel.dtd") | |
| if(EXISTS "${_geomodel_dtd}") | |
| set(_geomodel_dtd "") | |
| foreach(_cand | |
| "${GeoModelTools_DIR}/../../../../share/GeoModelXml/geomodel.dtd" | |
| "${GeoModelTools_DIR}/../../../share/GeoModelXml/geomodel.dtd" | |
| ) | |
| if(EXISTS "${_cand}") | |
| set(_geomodel_dtd "${_cand}") | |
| break() | |
| endif() | |
| endforeach() | |
| if(_geomodel_dtd) |
🤖 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/TimingDetector/CMakeLists.txt` around lines 37 - 43, The DTD
lookup using fixed-level path traversals in the get_filename_component call with
_gt_prefix is layout-sensitive and fails for standard installs. Replace the
hardcoded 4-level and 3-level relative path traversals (using "../../../..")
with a more robust approach that uses GeoModelTools-provided CMake variables or
package hints to determine the correct prefix path, or use a layout-agnostic
discovery mechanism that works across different install directory structures.
Ensure the resulting _geomodel_dtd path is correctly resolved and properly
copied to ${GMX_OUT_DIR}/geomodel.dtd for downstream GMX parsing.
|
Thanks @antonioiuliano2, I'll have a look. If this doesn't interfere with the pixi/conda build, fine. But conda is the recommended way of defining the environment, and other packaging systems (e.g. apt) can not be supported. |
2 participants
Dear all,
This pull request follows a build test of this repository in my Ubuntu24.
All prerequisites, CMake, C++ and GeoModel are installed system-wide via standard apt-get install commands.
I have encountered two errors:
Could not find a package configuration file provided by "tomlplusplus" -> solved with sudo apt-get install libtomlplusplus-dev. Added as a prerequisite in the README.
geomodel.dtd not found at /usr/lib/share/GeoModelXml/geomodel.dtd; The file is actually in /usr/share/GeoModelXml/geomodel.dtd. There is no /usr/lib/share in my Ubuntu24 laptop. Solved by adding an if condition in the TimingDetector CMakeFile
All the best,
Antonio
Summary by CodeRabbit
Bug Fixes
Documentation
tomlpluspluslibrary to the list of required dependencies in prerequisites, with project URL reference.Chores