ci: include omitted rust packages in ci filters#3663
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR restructures GitHub Actions CI package filter configurations to enable composable anchor-based groupings, consolidating contract definitions and introducing modular SDK component anchors (signer, async, context, encryption). It simultaneously expands Swift SDK change detection to recognize additional package modifications. ChangesCI Package Filter Reorganization
Workflow Change Detection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
✅ Review complete (commit 107ffd5) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3663 +/- ##
============================================
- Coverage 88.06% 87.86% -0.21%
============================================
Files 2521 2537 +16
Lines 308995 311803 +2808
============================================
+ Hits 272122 273960 +1838
- Misses 36873 37843 +970
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR closes the immediate dispatcher gap: every Cargo workspace member is now reachable from rs-packages-no-workflows.yml, which gates rs-workspace-tests. Codex flags a remaining iOS-specific coverage hole — the new swift-sdk-changed trigger omits several crates that the unified-SDK build transitively compiles, so iOS-only compile/link regressions in dpp/drive/dapi-grpc-etc. can still skip swift-sdk-build. Claude flags one minor cross-file drift (rs-dapi missing *dpp in the no-workflows file) and two cosmetic nitpicks. No blockers.
Reviewed commit: b2c2e06
🟡 2 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
💬 nitpick: rs-dash-platform-macros not extracted to its own entry in the 'direct' variant
.github/package-filters/rs-packages-direct.yml (lines 96-101)
rs-packages-direct.yml follows a one-crate-per-entry convention (e.g., platform-version, platform-value, data-contracts are split out). rs-dash-platform-macros/** is still bundled inside the dapi-grpc: block with no top-level entry of its own, inconsistent with how other derive/value/macro crates are treated here. Cosmetic only — this file isn't consumed by any workflow yet — but it undermines the file's stated 'direct = one crate per entry' contract.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/tests.yml`:
- [SUGGESTION] lines 122-133: swift-sdk-changed trigger omits crates that unified-SDK FFI compiles transitively
The expanded `swift-sdk-changed` filter is still a strict subset of the crate graph that `packages/swift-sdk/build_ios.sh` actually compiles. `rs-sdk-ffi/Cargo.toml:18` declares a direct path dependency on `drive-proof-verifier`, and `rs-sdk/Cargo.toml:10-25` pulls in `dpp`, `dapi-grpc`, `rs-dapi-client`, `drive`, and `dash-platform-macros` — none of which appear in the trigger list. A PR touching only one of those crates will run `rs-workspace-tests` but skip `swift-sdk-build`, so iOS-target-specific compile/link regressions (the exact failure mode this gate exists to catch) can still land unnoticed. Add the missing transitive crates, or — to avoid the same drift the package-filter files now suffer from — gate `swift-sdk-build` off the same rs-packages output that already covers them.
In `.github/package-filters/rs-packages-no-workflows.yml`:
- [SUGGESTION] lines 111-114: rs-dapi filter drifts from rs-packages.yml (missing *dpp)
The `rs-dapi` block here omits `*dpp`, while the equivalent block in `rs-packages.yml:136-141` includes it. Since `rs-packages-no-workflows.yml` is the file actually consumed by `tests.yml`, the no-workflows variant is the one that needs to be correct. The functional impact is small today — `rs-workspace-tests` only checks that the output array is non-empty, and a change to `rs-dpp` already triggers via the `dpp` top-level key — but this is exactly the kind of cross-file drift the PR is trying to eliminate going forward.
PastaPastaPasta
force-pushed
the
codex/fix-rust-ci-package-filters
branch
from
b2c2e06 to
107ffd5
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR closes the previously-flagged dispatcher gaps: rs-dapi now pulls in *dpp (rs-packages-no-workflows.yml:114) and swift-sdk-changed in tests.yml lists the full transitive crate set built by build_ios.sh. Two nitpicks remain — redundant alias expansions in three blocks of rs-packages-no-workflows.yml, and three crates still bundled rather than split out in the one-crate-per-entry rs-packages-direct.yml. No blocking issues.
Reviewed commit: 107ffd5
💬 2 nitpick(s)
| @@ -121,8 +121,38 @@ jobs: | |||
| filters: | | |||
| swift-sdk-changed: | |||
There was a problem hiding this comment.
Shall we move it to a separate file as well? It's become quite big
4 participants
Issue being fixed or feature implemented
Some Rust workspace package changes did not trigger the Rust workspace CI dispatcher because
.github/package-filters/rs-packages-no-workflows.ymldid not cover every Cargo workspace member. This is why PR #3662 ran the top-levelTestsworkflow but skippedRust workspace testsfor a change underpackages/rs-platform-wallet-ffi/**.The same gap applied to other workspace crates such as
rs-platform-wallet,rs-unified-sdk-ffi,rs-platform-encryption,rs-dash-async,simple-signer,strategy-tests,data-contracts,keyword-search-contract, andwasm-drive-verify.What was done?
rs-packages.yml,rs-packages-no-workflows.yml, andrs-packages-direct.ymlfilter variants aligned so future use does not reintroduce inconsistent coverage.rs-unified-sdk-ffiandplatform-wallet-ffi.How Has This Been Tested?
YAML.load.packages/*/Cargo.tomlworkspace package path appears in all three Rust package filter files.git diff --checkThis pull request was created by Codex.
Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit