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 8 commits into
Conversation
📝 WalkthroughWalkthroughThe PR updates TimingDetector’s ChangesGeometry Build and Workflow Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. |
|
@antonioiuliano2 Could you please address the review comment? |
Calls the org-level reusable workflow to run `pixi update`, render a markdown diff, and open a PR against main. Runs on the 1st of each month and on workflow_dispatch.
GeoModelCoreConfig.cmake calls find_dependency(Eigen3 5.0.1 CONFIG) and conda-forge eigen ships Eigen3Config.cmake at share/eigen3/cmake/, a path CMake does not search by default; eigen-abi-devel exposes the Config in a discoverable location (eigen-feedstock README case B). nlohmann_json is added alongside since GeoModelToolsConfig.cmake calls find_dependency(nlohmann_json 3.12.0) and the package is only pulled transitively today. Mirrors the equivalent host-dep additions in the shipgeometry conda recipe.
Adds a 'geometry' pixi task that runs build_geometry into build/ship_geometry.db, and wires it into the Build and Test workflow along with the new artifact-name / artifact-path inputs from the shared pixi-cmake-build reusable workflow. Each push to main and each PR now exposes the resulting GeoModel SQLite snapshot as a downloadable artefact tagged with the commit SHA. Running 'geometry' after 'test' ensures a failing test still blocks the upload.
Of course! I can also test the pixi/conda build, to see if it works even without this pull request. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/update-lock-files.yml:
- Line 13: The reusable workflow reference is using a floating branch name, so
update the `uses` entry for
`ShipSoft/.github/.github/workflows/pixi-lock-update.yml` to point to an
immutable commit SHA instead of `@main`. Keep the same workflow path and only
replace the ref with a pinned SHA so the `update-lock-files` job always runs
against a reviewed version.
🪄 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: 07706734-13d0-4892-b2d1-e13ed5c2167a
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/build-test.yml.github/workflows/update-lock-files.ymlpixi.tomlsubsystems/TimingDetector/CMakeLists.txt
|
|
||
| jobs: | ||
| pixi-update: | ||
| uses: ShipSoft/.github/.github/workflows/pixi-lock-update.yml@main |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin the reusable workflow to a commit SHA.
Line 13 references @main, so this job can pick up unreviewed upstream workflow changes on any run. Please pin the reusable workflow to an immutable commit SHA instead.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 13-13: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/update-lock-files.yml at line 13, The reusable workflow
reference is using a floating branch name, so update the `uses` entry for
`ShipSoft/.github/.github/workflows/pixi-lock-update.yml` to point to an
immutable commit SHA instead of `@main`. Keep the same workflow path and only
replace the ref with a pinned SHA so the `update-lock-files` job always runs
against a reviewed version.
Source: Linters/SAST tools
The pixi workflow should work out of the box on Ubuntu |
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
geomodel.dtddiscovery and installation for Ubuntu 24 to prevent GMX parsing issues.Documentation
tomlplusplus.New Features
geometrytask to generateship_geometry.db, and expanded CI to run geometry along with tests.Chores