fix(rootegpythia6): patch absolute paths in dict autoload annotations#221
Open
fix(rootegpythia6): patch absolute paths in dict autoload annotations#221
Conversation
…ations
Absolute source paths passed to ROOT_GENERATE_DICTIONARY get embedded in
the shared library's $clingAutoload$ annotations. At runtime, cling
tries to resolve these paths, but the source directory no longer exists
on CVMFS, producing errors like:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Missing FileEntry for .../SOURCES/ROOTEGPythia6/.../inc/TMCParticle.h
Patch the CMakeLists.txt at build time to pass bare header names instead
and add an include_directories() call so rootcling can still find them.
olantwin
requested review from
a team and
webbjm
and removed request for
a team
📝 WalkthroughWalkthroughA Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rootegpythia6.sh`:
- Around line 28-31: The sed insertion is non-idempotent and the substitution is
unscoped: modify the script that edits CMakeLists.txt so it first checks for an
existing include_directories(${CMAKE_CURRENT_LIST_DIR}/inc) and only inserts it
if absent (guard the insertion around the pattern ROOT_GENERATE_DICTIONARY), and
narrow the header-path rewrite to only operate inside the
ROOT_GENERATE_DICTIONARY block rather than globally; update the logic that
currently uses the sed commands (the two -e expressions) to perform a
conditional insert and a block-scoped replacement targeting the
ROOT_GENERATE_DICTIONARY section in CMakeLists.txt so repeated runs do not
duplicate the include_directories line and unrelated .h paths are not rewritten.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+28
to
+31
| sed -i \ | ||
| -e 's|${CMAKE_CURRENT_LIST_DIR}/inc/\(.*\.h\)|\1|g' \ | ||
| -e '/^ROOT_GENERATE_DICTIONARY/i include_directories(${CMAKE_CURRENT_LIST_DIR}/inc)' \ | ||
| "$SOURCEDIR/CMakeLists.txt" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSL https://raw.githubusercontent.com/luketpickering/ROOTEGPythia6/main/CMakeLists.txt > "$tmp/CMakeLists.txt"
cp "$tmp/CMakeLists.txt" "$tmp/patched_once.txt"
cp "$tmp/CMakeLists.txt" "$tmp/patched_twice.txt"
# Apply current patch once
sed -i \
-e 's|${CMAKE_CURRENT_LIST_DIR}/inc/\(.*\.h\)|\1|g' \
-e '/^ROOT_GENERATE_DICTIONARY/i include_directories(${CMAKE_CURRENT_LIST_DIR}/inc)' \
"$tmp/patched_once.txt"
# Apply current patch twice
sed -i \
-e 's|${CMAKE_CURRENT_LIST_DIR}/inc/\(.*\.h\)|\1|g' \
-e '/^ROOT_GENERATE_DICTIONARY/i include_directories(${CMAKE_CURRENT_LIST_DIR}/inc)' \
"$tmp/patched_twice.txt"
sed -i \
-e 's|${CMAKE_CURRENT_LIST_DIR}/inc/\(.*\.h\)|\1|g' \
-e '/^ROOT_GENERATE_DICTIONARY/i include_directories(${CMAKE_CURRENT_LIST_DIR}/inc)' \
"$tmp/patched_twice.txt"
echo "=== ROOT_GENERATE_DICTIONARY lines (with indentation check) ==="
rg -n '^[[:space:]]*ROOT_GENERATE_DICTIONARY' "$tmp/CMakeLists.txt"
echo "=== include_directories count after 1x and 2x patch ==="
echo -n "once: "
rg -n 'include_directories\(\${CMAKE_CURRENT_LIST_DIR}/inc\)' "$tmp/patched_once.txt" | wc -l
echo -n "twice: "
rg -n 'include_directories\(\${CMAKE_CURRENT_LIST_DIR}/inc\)' "$tmp/patched_twice.txt" | wc -l
echo "=== Remaining absolute inc paths after patch ==="
rg -n '\$\{CMAKE_CURRENT_LIST_DIR\}/inc/[^[:space:])"]+\.h' "$tmp/patched_once.txt" || trueRepository: ShipSoft/shipdist
Length of output: 403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
# Fetch the target CMakeLists.txt
curl -fsSL https://raw.githubusercontent.com/luketpickering/ROOTEGPythia6/main/CMakeLists.txt > "$tmp/CMakeLists.txt"
echo "=== Original file: ROOT_GENERATE_DICTIONARY context ==="
rg -B2 -A5 'ROOT_GENERATE_DICTIONARY' "$tmp/CMakeLists.txt" | head -30
echo ""
echo "=== Original file: Check for indentation and existing include_directories ==="
rg -n 'include_directories.*CMAKE_CURRENT_LIST_DIR' "$tmp/CMakeLists.txt" || echo "(no matches found)"
echo ""
echo "=== Original file: Paths with CMAKE_CURRENT_LIST_DIR/inc pattern ==="
rg -n 'CMAKE_CURRENT_LIST_DIR.*inc' "$tmp/CMakeLists.txt" | head -20
# Test current patch
cp "$tmp/CMakeLists.txt" "$tmp/patched_once.txt"
cp "$tmp/CMakeLists.txt" "$tmp/patched_twice.txt"
echo ""
echo "=== Applying current patch (once) ==="
sed -i \
-e 's|${CMAKE_CURRENT_LIST_DIR}/inc/\(.*\.h\)|\1|g' \
-e '/^ROOT_GENERATE_DICTIONARY/i include_directories(${CMAKE_CURRENT_LIST_DIR}/inc)' \
"$tmp/patched_once.txt"
echo "=== After 1st patch: include_directories entries ==="
rg -n 'include_directories' "$tmp/patched_once.txt" | grep CMAKE_CURRENT_LIST_DIR || echo "(no matches found)"
echo ""
echo "=== Applying current patch second time ==="
sed -i \
-e 's|${CMAKE_CURRENT_LIST_DIR}/inc/\(.*\.h\)|\1|g' \
-e '/^ROOT_GENERATE_DICTIONARY/i include_directories(${CMAKE_CURRENT_LIST_DIR}/inc)' \
"$tmp/patched_twice.txt"
sed -i \
-e 's|${CMAKE_CURRENT_LIST_DIR}/inc/\(.*\.h\)|\1|g' \
-e '/^ROOT_GENERATE_DICTIONARY/i include_directories(${CMAKE_CURRENT_LIST_DIR}/inc)' \
"$tmp/patched_twice.txt"
echo "=== After 2nd patch (run twice): include_directories entries ==="
rg -n 'include_directories' "$tmp/patched_twice.txt" | grep CMAKE_CURRENT_LIST_DIR || echo "(no matches found)"
echo ""
echo "=== Diff between 1st and 2nd patch runs ==="
diff "$tmp/patched_once.txt" "$tmp/patched_twice.txt" | head -40 || echo "(no differences)"
echo ""
echo "=== Line 29 scope test: Unrelated paths changed? ==="
echo "Original unrelated PATH patterns (not in ROOT_GENERATE_DICTIONARY context):"
rg -n 'CMAKE_CURRENT_LIST_DIR' "$tmp/CMakeLists.txt" | grep -v ROOT_GENERATE | head -10 || echo "(none found)"
echo ""
echo "After patch, same search:"
rg -n 'CMAKE_CURRENT_LIST_DIR' "$tmp/patched_once.txt" | grep -v ROOT_GENERATE | head -10 || echo "(none found)"Repository: ShipSoft/shipdist
Length of output: 2728
Fix non-idempotent include_directories insertion.
At Line 30, the insert pattern /^ROOT_GENERATE_DICTIONARY/i adds a new line before every match. Running the patch twice inserts include_directories(${CMAKE_CURRENT_LIST_DIR}/inc) twice (verified: 1 entry after first run, 2 entries after second run). Wrap the insertion in a guard to prevent duplication.
At Line 29, the global s|${CMAKE_CURRENT_LIST_DIR}/inc/\(.*\.h\)|\1|g pattern is unscoped. While it correctly rewrites paths within the ROOT_GENERATE_DICTIONARY block in this file, it would match any .h file containing that path pattern elsewhere if present—making it fragile for future changes.
Proposed hardening patch
-sed -i \
- -e 's|${CMAKE_CURRENT_LIST_DIR}/inc/\(.*\.h\)|\1|g' \
- -e '/^ROOT_GENERATE_DICTIONARY/i include_directories(${CMAKE_CURRENT_LIST_DIR}/inc)' \
- "$SOURCEDIR/CMakeLists.txt"
+if ! grep -qF 'include_directories(${CMAKE_CURRENT_LIST_DIR}/inc)' "$SOURCEDIR/CMakeLists.txt"; then
+ sed -i \
+ -e '0,/^[[:space:]]*ROOT_GENERATE_DICTIONARY/s//include_directories(${CMAKE_CURRENT_LIST_DIR}\/inc)\n&/' \
+ "$SOURCEDIR/CMakeLists.txt"
+fi
+
+sed -i \
+ -e '/ROOT_GENERATE_DICTIONARY/,/)/ s|${CMAKE_CURRENT_LIST_DIR}/inc/\([^[:space:])"]*\.h\)|\1|g' \
+ "$SOURCEDIR/CMakeLists.txt"📝 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.
Suggested change
| sed -i \ | |
| -e 's|${CMAKE_CURRENT_LIST_DIR}/inc/\(.*\.h\)|\1|g' \ | |
| -e '/^ROOT_GENERATE_DICTIONARY/i include_directories(${CMAKE_CURRENT_LIST_DIR}/inc)' \ | |
| "$SOURCEDIR/CMakeLists.txt" | |
| if ! grep -qF 'include_directories(${CMAKE_CURRENT_LIST_DIR}/inc)' "$SOURCEDIR/CMakeLists.txt"; then | |
| sed -i \ | |
| -e '0,/^[[:space:]]*ROOT_GENERATE_DICTIONARY/s//include_directories(${CMAKE_CURRENT_LIST_DIR}\/inc)\n&/' \ | |
| "$SOURCEDIR/CMakeLists.txt" | |
| fi | |
| sed -i \ | |
| -e '/ROOT_GENERATE_DICTIONARY/,/)/ s|${CMAKE_CURRENT_LIST_DIR}/inc/\([^[:space:])"]*\.h\)|\1|g' \ | |
| "$SOURCEDIR/CMakeLists.txt" |
🧰 Tools
🪛 GitHub Check: alidistlint
[notice] 30-30:
Expressions don't expand in single quotes, use double quotes for that. [SC2016]
[notice] 29-29:
Expressions don't expand in single quotes, use double quotes for that. [SC2016]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rootegpythia6.sh` around lines 28 - 31, The sed insertion is non-idempotent
and the substitution is unscoped: modify the script that edits CMakeLists.txt so
it first checks for an existing
include_directories(${CMAKE_CURRENT_LIST_DIR}/inc) and only inserts it if absent
(guard the insertion around the pattern ROOT_GENERATE_DICTIONARY), and narrow
the header-path rewrite to only operate inside the ROOT_GENERATE_DICTIONARY
block rather than globally; update the logic that currently uses the sed
commands (the two -e expressions) to perform a conditional insert and a
block-scoped replacement targeting the ROOT_GENERATE_DICTIONARY section in
CMakeLists.txt so repeated runs do not duplicate the include_directories line
and unrelated .h paths are not rewritten.
Contributor
Author
|
@webbjm Sorry, I have no idea why Github keeps assigning you for review, maybe because nobody else from @ShipSoft/core-developers has contributed to shipdist recently? |
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
1 participant
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.
Absolute source paths passed to ROOT_GENERATE_DICTIONARY get embedded in the shared library's$clingAutoload$ annotations. At runtime, cling tries to resolve these paths, but the source directory no longer exists on CVMFS, producing errors like:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Missing FileEntry for .../SOURCES/ROOTEGPythia6/.../inc/TMCParticle.h
Patch the CMakeLists.txt at build time to pass bare header names instead and add an include_directories() call so rootcling can still find them.
Summary by CodeRabbit
Note: This release contains internal build system improvements with no user-facing changes.