Fix: Only install git hooks if .git is a directory (submodule-safe) #954
Conversation
…submodule-safe) Signed-off-by: Alan Jowett <[email protected]>
WalkthroughCMakeLists.txt: replaced Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: vbpf/prevail PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T13:34:52.142Z
Learning: Applies to src/test/**/*.{cc,cpp,cxx} : Place Catch2 unit/integration tests under src/test and add focused tests when modifying verifier behaviour
Learnt from: CR
Repo: vbpf/prevail PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T13:34:52.142Z
Learning: Applies to src/{cfg,crab,linux,arith,asm_*}/**/* : Add new verifier logic under the matching subsystem directory (cfg/, crab/, linux/, arith/, asm_*) to keep separation of concerns
📚 Learning: 2025-10-15T13:34:52.142Z
Learnt from: CR
Repo: vbpf/prevail PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T13:34:52.142Z
Learning: Applies to src/test/**/*.{cc,cpp,cxx} : Place Catch2 unit/integration tests under src/test and add focused tests when modifying verifier behaviour
Applied to files:
CMakeLists.txt
📚 Learning: 2025-10-15T13:34:52.142Z
Learnt from: CR
Repo: vbpf/prevail PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T13:34:52.142Z
Learning: Applies to src/{cfg,crab,linux,arith,asm_*}/**/* : Add new verifier logic under the matching subsystem directory (cfg/, crab/, linux/, arith/, asm_*) to keep separation of concerns
Applied to files:
CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: build_ubuntu (Debug)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_ubuntu (Release, library)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: Analyze (cpp)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_ubuntu (Debug, library)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_ubuntu (Debug, tests)
🔇 Additional comments (1)
CMakeLists.txt (1)
14-15: IS_DIRECTORY check correctly addresses submodule safety.The condition
IS_DIRECTORY "${PROJECT_SOURCE_DIR}/.git" AND NOT CMAKE_CROSSCOMPILINGproperly guards the hook installation, preventing CMake errors when the repository is used as a submodule (where.gitis a file, not a directory).
… compatibility Signed-off-by: Alan Jowett <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T13:34:52.142Z
Learnt from: CR
Repo: vbpf/prevail PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T13:34:52.142Z
Learning: Applies to src/test/**/*.{cc,cpp,cxx} : Place Catch2 unit/integration tests under src/test and add focused tests when modifying verifier behaviour
Applied to files:
CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build_ubuntu (Debug, library)
- GitHub Check: build_ubuntu (Release, library)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_ubuntu (Debug)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (2)
CMakeLists.txt (2)
14-15: Main fix looks solid — IS_DIRECTORY check handles submodules correctly.The change from existence check to
IS_DIRECTORYappropriately handles submodule scenarios where.gitis a file (symlink) rather than a directory. The additionalAND NOT CMAKE_CROSSCOMPILINGcondition correctly prevents host file modifications during cross-compilation.
16-18: Review comment is incorrect.The variable
prevail_ENABLE_TESTSis defined consistently at lines 16–18 and referenced uniformly across the file at lines 21, 82, 88, and 158. No rename toVERIFIER_ENABLE_TESTShas occurred, and CMake will evaluate the variable without issues. There is no inconsistency or undefined variable causing a build failure.Likely an incorrect or invalid review comment.
Pull Request Test Coverage Report for Build 19340838864Details
💛 - Coveralls |
3 participants
Summary
This PR fixes an issue in the CMakeLists.txt where git hooks were always installed, even when the repository is used as a submodule. This caused CMake errors in parent projects that include this repo as a submodule, since the .git directory may not exist (it may be a file or missing).
Details
.gitis a directory and not cross-compiling.Example Error Fixed
When used as a submodule, CMake would fail with:
Solution
By checking
if (IS_DIRECTORY "${PROJECT_SOURCE_DIR}/.git" ...), the script only attempts to copy hooks when appropriate.Signed-off-by: Alan Jowett [email protected]
Summary by CodeRabbit
Chores
Style