Skip to content

fix: install TimingDetector GMX and hoist GNUInstallDirs#35

Merged
olantwin merged 1 commit into
mainfrom
fix/timing-detector-install-paths
Jun 18, 2026
Merged

fix: install TimingDetector GMX and hoist GNUInstallDirs#35
olantwin merged 1 commit into
mainfrom
fix/timing-detector-install-paths

Conversation

@olantwin

@olantwin olantwin commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Two related installation fixes that together let installed deployments find subsystem data files without environment overrides.

  • Hoist include(GNUInstallDirs) above add_subdirectory(subsystems). The top-level include sat after the subsystems were processed, so \${CMAKE_INSTALL_DATADIR} was empty in their install rules. calo.toml and sbt.toml were silently being installed to absolute /SHiPGeometry/; with the hoist they land under \$PREFIX/share/SHiPGeometry/ (and so does the new TimingDetector GMX).
  • Install timing_detector.gmx + resolve path at runtime. Mirror the existing pattern from Calorimeter / DecayVolume: bake both a TIMING_DETECTOR_GMX_DEFAULT_PATH (build tree) and TIMING_DETECTOR_GMX_INSTALL_PATH (\${CMAKE_INSTALL_PREFIX}/share/SHiPGeometry/TimingDetector/...) compile definition, install the GMX and the co-located geomodel.dtd (Xerces resolves the document SYSTEM identifier against it), and resolve at runtime via a small file-scope helper that prefers build-tree → install-tree. Without this, packaged builds of libTimingDetector.so carried a build-time absolute path that didn't exist on the consumer's machine.

Test plan

  • pixi run build succeeds
  • pixi run test — all 33 tests pass (including TimingDetectorTest and BuilderTest)
  • cmake --install build --prefix=/tmp/td-install puts share/SHiPGeometry/calo.toml, share/SHiPGeometry/sbt.toml, share/SHiPGeometry/TimingDetector/timing_detector.gmx, and share/SHiPGeometry/TimingDetector/geomodel.dtd under the prefix

Summary by CodeRabbit

  • Chores
    • Improved TimingDetector geometry file resolution to reliably locate data files in both development and production environments
    • Ensured proper installation of TimingDetector geometry data files with fallback path support

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 07a8c5cc-f16f-4b85-95f3-e7fbeeb4fff0

📥 Commits

Reviewing files that changed from the base of the PR and between a29ff02 and 4ecb42c.

📒 Files selected for processing (2)
  • subsystems/TimingDetector/CMakeLists.txt
  • subsystems/TimingDetector/src/TimingDetectorFactory.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • subsystems/TimingDetector/CMakeLists.txt
  • subsystems/TimingDetector/src/TimingDetectorFactory.cpp

📝 Walkthrough

Walkthrough

Replaces the single TIMING_DETECTOR_GMX compile-time definition with two fallback path macros (TIMING_DETECTOR_GMX_DEFAULT_PATH and TIMING_DETECTOR_GMX_INSTALL_PATH), adds CMake install rules for timing_detector.gmx and geomodel.dtd, and introduces a resolveGmxPath() helper in TimingDetectorFactory.cpp that selects the correct file path at runtime.

Changes

TimingDetector install-path support

Layer / File(s) Summary
CMake path definitions and install rules
subsystems/TimingDetector/CMakeLists.txt
Replaces the single TIMING_DETECTOR_GMX compile definition with TIMING_DETECTOR_GMX_DEFAULT_PATH and TIMING_DETECTOR_GMX_INSTALL_PATH. Adds install rules deploying timing_detector.gmx and conditionally geomodel.dtd into ${CMAKE_INSTALL_DATADIR}/SHiPGeometry/TimingDetector.
Runtime GMX path resolution
subsystems/TimingDetector/src/TimingDetectorFactory.cpp
Adds <filesystem>, fallback macro defaults, and a resolveGmxPath() helper that checks the build-tree path first, then the install-time path, and otherwise returns the default. TimingDetectorFactory::build() passes the resolved path to Gmx2Geo.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • ShipSoft/Geometry#36: Moves include(GNUInstallDirs) earlier in the top-level CMake so that ${CMAKE_INSTALL_DATADIR} is defined before subdirectories (including TimingDetector) are configured, which is a direct prerequisite for the install rules added in this PR.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures both main changes: installing the TimingDetector GMX file and hoisting GNUInstallDirs, which are the core fixes described in the PR objectives.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/timing-detector-install-paths

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@olantwin

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@subsystems/TimingDetector/CMakeLists.txt`:
- Line 52: The TIMING_DETECTOR_GMX_INSTALL_PATH variable definition concatenates
CMAKE_INSTALL_PREFIX with CMAKE_INSTALL_DATADIR, which can produce invalid paths
when CMAKE_INSTALL_DATADIR is absolute. Replace the concatenation of
CMAKE_INSTALL_PREFIX and CMAKE_INSTALL_DATADIR with CMAKE_INSTALL_FULL_DATADIR,
which already contains the full path and handles both relative and absolute
cases correctly.
🪄 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: d9e31952-e231-4264-b588-4dba62c72b1a

📥 Commits

Reviewing files that changed from the base of the PR and between 6a1455d and a29ff02.

📒 Files selected for processing (3)
  • CMakeLists.txt
  • subsystems/TimingDetector/CMakeLists.txt
  • subsystems/TimingDetector/src/TimingDetectorFactory.cpp

Comment thread subsystems/TimingDetector/CMakeLists.txt Outdated
Install timing_detector.gmx (and the co-located geomodel.dtd) into
${CMAKE_INSTALL_FULL_DATADIR}/SHiPGeometry/TimingDetector. Bake both
the build-tree path and the install-tree path as compile-time defines
and pick the first one that exists at runtime, so the factory works in
both development and installed deployments.
@olantwin olantwin force-pushed the fix/timing-detector-install-paths branch from a29ff02 to 4ecb42c Compare June 18, 2026 09:07
@olantwin

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@olantwin olantwin merged commit 8a22d1c into main Jun 18, 2026
3 checks passed
@olantwin olantwin deleted the fix/timing-detector-install-paths branch June 18, 2026 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant