build: clean up stale entries in deploy path#2083
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdded a reusable CMake helper to batch copy operations, replaced per-file copy loops with the helper for AIO and shader staging, introduced Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
No actionable suggestions for changed features. |
![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 `@CMakeLists.txt`:
- Around line 433-479: The stale-file detection currently runs at configure time
(the file(GLOB_RECURSE _existing_aio_files ...) and similar for _aio_shaders) so
deleted sources aren’t detected until reconfigure; move the globbing and
stale-list computation into a build-step script executed at build time (e.g.
create a CMake script invoked via add_custom_target or add_custom_command that
runs cmake -P) and have that script perform the file(GLOB_RECURSE ...) for
_existing_aio_files and _aio_shaders, compute _stale_aio_rels, and then call
append_remove_paths to populate _prepare_aio_cmds against AIO_DIR so cleanup
runs on every build; update the CMakeLists to call this generated/packaged cmake
-P script instead of performing the globs at configure time.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5eda6332-a23e-4ba9-8dae-37986291a499
📒 Files selected for processing (1)
CMakeLists.txt
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmake/CleanupStaleEntries.cmake (2)
1-103: PR metadata tweak: use scoped conventional title + issue keyword.Suggested title:
build(cmake): clean stale deploy entries
If this fixes a tracked bug, addFixes #<issue>in the PR body.As per coding guidelines: “When reviewing PRs, please provide suggestions for Conventional Commit Titles … and Issue References …”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/CleanupStaleEntries.cmake` around lines 1 - 103, Rename the PR title to follow conventional commits (e.g., "build(cmake): clean stale deploy entries") and, if this change fixes a tracked bug, add "Fixes #<issue>" to the PR body; update the PR metadata for the commit(s touching CleanupStaleEntries.cmake (the MODE/AIO/SHADERS logic) accordingly so the title and body reflect the conventional format and include the issue reference.
29-41: Improve path matching logic for future robustness, but note: current code works correctly.The current
string(FIND)approach would fail if feature paths had overlapping substrings (e.g.,/features/Featureand/features/FeatureExtra), potentially misclassifying live files as stale. However, actual feature names in the repository contain no such overlaps, so this is not a current bug. The proposed fix usingfile(RELATIVE_PATH)with../checks is still worthwhile as a robustness improvement to future-proof the code against accidental feature name additions that could introduce substring conflicts.Suggested patch
foreach(_src IN LISTS _current_aio_sources) - if(_src MATCHES "^${SOURCE_DIR}/package/") - file(RELATIVE_PATH _rel "${SOURCE_DIR}/package" "${_src}") - list(APPEND _current_aio_rels "${_rel}") - else() - foreach(_fpath IN LISTS FEATURE_PATHS) - string(FIND "${_src}" "${_fpath}" _is_feature_path) - if(NOT _is_feature_path EQUAL -1) - file(RELATIVE_PATH _rel "${_fpath}" "${_src}") - list(APPEND _current_aio_rels "${_rel}") - break() - endif() - endforeach() - endif() + file(RELATIVE_PATH _rel "${SOURCE_DIR}/package" "${_src}") + if(NOT _rel MATCHES "^\\.\\./") + list(APPEND _current_aio_rels "${_rel}") + else() + foreach(_fpath IN LISTS FEATURE_PATHS) + file(RELATIVE_PATH _rel "${_fpath}" "${_src}") + if(NOT _rel MATCHES "^\\.\\./") + list(APPEND _current_aio_rels "${_rel}") + break() + endif() + endforeach() + endif() endforeach()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/CleanupStaleEntries.cmake` around lines 29 - 41, Replace the substring-based path detection (the string(FIND) block) with a robust relative-path check: for each _fpath in FEATURE_PATHS call file(RELATIVE_PATH _rel "${_fpath}" "${_src}") and treat the source as belonging to that feature only if _rel does not start with "../" (i.e., it's not outside the feature directory), then list(APPEND _current_aio_rels "${_rel}") and break; keep the outer handling for SOURCE_DIR/package as-is and preserve variable names _current_aio_sources, _current_aio_rels, FEATURE_PATHS, _src, and _fpath so the change is localized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/CleanupStaleEntries.cmake`:
- Around line 21-25: The FEATURE_PATHS glob is currently adding test directories
into _current_aio_sources; in the foreach over FEATURE_PATHS (variables _fpath
and _current_aio_sources, and the file(GLOB_RECURSE _tmp ...) call) filter out
any paths under a Tests directory before appending. After file(GLOB_RECURSE _tmp
...), run a list(FILTER _tmp EXCLUDE REGEX ".*/Tests/.*") (or equivalent
remove-if-contains logic) so entries matching /Tests/ are excluded, then append
the filtered _tmp into _current_aio_sources.
---
Nitpick comments:
In `@cmake/CleanupStaleEntries.cmake`:
- Around line 1-103: Rename the PR title to follow conventional commits (e.g.,
"build(cmake): clean stale deploy entries") and, if this change fixes a tracked
bug, add "Fixes #<issue>" to the PR body; update the PR metadata for the
commit(s touching CleanupStaleEntries.cmake (the MODE/AIO/SHADERS logic)
accordingly so the title and body reflect the conventional format and include
the issue reference.
- Around line 29-41: Replace the substring-based path detection (the
string(FIND) block) with a robust relative-path check: for each _fpath in
FEATURE_PATHS call file(RELATIVE_PATH _rel "${_fpath}" "${_src}") and treat the
source as belonging to that feature only if _rel does not start with "../"
(i.e., it's not outside the feature directory), then list(APPEND
_current_aio_rels "${_rel}") and break; keep the outer handling for
SOURCE_DIR/package as-is and preserve variable names _current_aio_sources,
_current_aio_rels, FEATURE_PATHS, _src, and _fpath so the change is localized.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04879247-35a3-4464-8f39-b3d55682cce3
📒 Files selected for processing (2)
CMakeLists.txtcmake/CleanupStaleEntries.cmake
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
(cherry picked from commit f4a7ecc)
2 participants
Summary by CodeRabbit