feat(ci): check only affected Rust packages#2963
Conversation
Summary by CodeRabbit
WalkthroughIntroduces a ChangesAffected-package CI pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Replace full-workspace Rust pre-build checks with an affected-package selection flow. The new xtask maps changed Git paths to workspace packages, expands the selection to transitive workspace dependents, and falls back to the full workspace when a change is global or cannot be mapped safely. This reduces CI time for package-scoped changes while preserving coverage for shared inputs such as Cargo files, toolchain config, CI config, custom lints, xtask code, and other global build inputs. The affected check now runs: - isolated default-feature package builds - Clippy with all targets/features - carbide-lints with all targets/features Also add reviewer guidance for keeping the global-path fallback list up to date. Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/xtask/src/affected_packages.rs (1)
545-605: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrive the global-path regression cases from the fallback list.
This test only samples part of
GLOBAL_FILES/GLOBAL_DIRECTORIES, so a future addition tois_global_path()can land without matching coverage. Please derive the cases from a single shared table so the safety check and its test stay in sync. As per coding guidelines,crates/xtask/src/affected_packages.rs:Keep is_global_path() ... up to date; as per path instructions,crates/**/*.rs:Prefer findings about behavior ... and missing tests over style-only comments; based on learnings,When writing tests, prefer the table-driven style.🤖 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 `@crates/xtask/src/affected_packages.rs` around lines 545 - 605, The regression test for global path handling is only covering a hand-written subset of the fallback globals, so it can drift from is_global_path() as GLOBAL_FILES or GLOBAL_DIRECTORIES change. Refactor global_unmapped_unsafe_and_empty_changes_select_all_with_a_reason in affected_packages.rs to drive its cases from one shared table/source used by is_global_path(), and iterate that table in the test so new global paths automatically stay covered. Keep the existing assertions around select_affected and Selection::All, but make the test table-driven and shared with the production global-path matcher.Sources: Coding guidelines, Path instructions, Learnings
🤖 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.
Nitpick comments:
In `@crates/xtask/src/affected_packages.rs`:
- Around line 545-605: The regression test for global path handling is only
covering a hand-written subset of the fallback globals, so it can drift from
is_global_path() as GLOBAL_FILES or GLOBAL_DIRECTORIES change. Refactor
global_unmapped_unsafe_and_empty_changes_select_all_with_a_reason in
affected_packages.rs to drive its cases from one shared table/source used by
is_global_path(), and iterate that table in the test so new global paths
automatically stay covered. Keep the existing assertions around select_affected
and Selection::All, but make the test table-driven and shared with the
production global-path matcher.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4f042823-af75-49f5-8e78-97fe6dfa5f74
📒 Files selected for processing (7)
.github/workflows/ci.yamlAGENTS.mdMakefileMakefile.tomlcrates/xtask/src/affected_packages.rscrates/xtask/src/isolated_package_builds.rscrates/xtask/src/main.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
|
@coderabbitai full_review |
|
✅ Action performedFull review finished. |
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 `@AGENTS.md`:
- Around line 129-137: Clarify the maintenance guidance for is_global_path() in
crates/xtask/src/affected_packages.rs by replacing the vague phrase with
explicit categories of shared paths; spell out which kinds of generated or
configuration directories should be treated as global, and keep the existing
examples of root Cargo files, Cargo.lock, cargo-make files, Rust toolchain
config, .cargo/, CI config, custom lint/xtask code, and include/ aligned with
that list. Make the wording unambiguous so contributors can tell when a newly
introduced shared directory must be added, and preserve the fallback rule that
any path not safely attributable to exactly one workspace package should select
the full workspace.
🪄 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: Enterprise
Run ID: fa5e3d37-e6f3-494b-846b-2f824b5f1ca4
📒 Files selected for processing (7)
.github/workflows/ci.yamlAGENTS.mdMakefileMakefile.tomlcrates/xtask/src/affected_packages.rscrates/xtask/src/isolated_package_builds.rscrates/xtask/src/main.rs
| When reviewing changes that add, remove, rename, or repurpose shared Rust build | ||
| inputs, verify that `is_global_path()` in | ||
| `crates/xtask/src/affected_packages.rs` remains up to date. Currently matched | ||
| shared inputs include the root Cargo files and `Cargo.lock`, cargo-make files, | ||
| Rust toolchain configuration, `.cargo/`, CI configuration, custom lint and xtask | ||
| code, and `include/`. Add any newly introduced or repurposed shared generated or | ||
| configuration directories to the predicate. If a changed path cannot be mapped | ||
| safely to exactly one workspace package, affected-package selection must fall | ||
| back to the full workspace. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the directory categories in this guidance.
Line 134’s shared generated or configuration directories is ambiguous, so the is_global_path() maintenance rule is easy to misread. Please spell the categories out explicitly.
✏️ Proposed fix
- Rust toolchain configuration, `.cargo/`, CI configuration, custom lint and xtask
- code, and `include/`. Add any newly introduced or repurposed shared generated or
- configuration directories to the predicate.
+ Rust toolchain configuration, `.cargo/`, CI configuration, custom lint and xtask
+ code, and `include/`. Add any newly introduced or repurposed shared
+ generated/configuration directories to the predicate.As per path instructions, review Markdown for correctness, clarity, spelling, grammar, working links, and whether commands/examples are realistic and safe.
📝 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.
| When reviewing changes that add, remove, rename, or repurpose shared Rust build | |
| inputs, verify that `is_global_path()` in | |
| `crates/xtask/src/affected_packages.rs` remains up to date. Currently matched | |
| shared inputs include the root Cargo files and `Cargo.lock`, cargo-make files, | |
| Rust toolchain configuration, `.cargo/`, CI configuration, custom lint and xtask | |
| code, and `include/`. Add any newly introduced or repurposed shared generated or | |
| configuration directories to the predicate. If a changed path cannot be mapped | |
| safely to exactly one workspace package, affected-package selection must fall | |
| back to the full workspace. | |
| When reviewing changes that add, remove, rename, or repurpose shared Rust build | |
| inputs, verify that `is_global_path()` in | |
| `crates/xtask/src/affected_packages.rs` remains up to date. Currently matched | |
| shared inputs include the root Cargo files and `Cargo.lock`, cargo-make files, | |
| Rust toolchain configuration, `.cargo/`, CI configuration, custom lint and xtask | |
| code, and `include/`. Add any newly introduced or repurposed shared | |
| generated/configuration directories to the predicate. If a changed path cannot be mapped | |
| safely to exactly one workspace package, affected-package selection must fall | |
| back to the full workspace. |
🧰 Tools
🪛 LanguageTool
[grammar] ~134-~134: Use a hyphen to join words.
Context: ...ny newly introduced or repurposed shared generated or configuration directories t...
(QB_NEW_EN_HYPHEN)
🤖 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 `@AGENTS.md` around lines 129 - 137, Clarify the maintenance guidance for
is_global_path() in crates/xtask/src/affected_packages.rs by replacing the vague
phrase with explicit categories of shared paths; spell out which kinds of
generated or configuration directories should be treated as global, and keep the
existing examples of root Cargo files, Cargo.lock, cargo-make files, Rust
toolchain config, .cargo/, CI config, custom lint/xtask code, and include/
aligned with that list. Make the wording unambiguous so contributors can tell
when a newly introduced shared directory must be added, and preserve the
fallback rule that any path not safely attributable to exactly one workspace
package should select the full workspace.
Sources: Path instructions, Linters/SAST tools
|
We decided not to make things more complex than needed and just remove check isolated packages from lint-police job: #2984 |
1 participant
Replace full-workspace Rust pre-build checks with an affected-package selection flow. The new xtask maps changed Git paths to workspace packages, expands the selection to transitive workspace dependents, and falls back to the full workspace when a change is global or cannot be mapped safely.
This reduces CI time for package-scoped changes while preserving coverage for shared inputs such as Cargo files, toolchain config, CI config, custom lints, xtask code, and other global build inputs.
The affected check now runs:
Also add reviewer guidance for keeping the global-path fallback list up to date.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes