ci: scope Feature Unification Check to PR-affected crates#1695
ci: scope Feature Unification Check to PR-affected crates#1695gilescope wants to merge 4 commits into
Conversation
Signed-off-by: Giles Cope <gilescope@gmail.com>
gilescope
added
skip-changes-check-all
ai-assisted
Signed-off-by: Giles Cope <gilescope@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c551779b53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This is a lie. |
| | map(select($o[.] != $n[.])) | ||
| ' | ||
|
|
||
| # Workspace members ($members) reverse-reachable from $seeds in the lock |
There was a problem hiding this comment.
Are "reverse-reachable" (transitive) dependent crates?
I think this is also not true - the script checks for file changes, and runs checks for the crate that changed, and any crate that depends on the changed crate |
| ARG PACKAGES="--workspace --exclude partner-chains-demo-node --exclude partner-chains-demo-runtime" | ||
| # Pinned: an unpinned binstall here can drift from the version baked into | ||
| # the CI base image and change check behaviour between runs. | ||
| RUN cargo binstall --no-confirm --locked cargo-hack@0.6.45 |
There was a problem hiding this comment.
Will this version pin get picked up by dependabot?
There was a problem hiding this comment.
This script is pretty hard to parse - could we replace this with a typescript implementation?
Then we could read the lockfile directly as toml rather than converting it to JSON - I imagine knowledge of jq script is limited in our team, particularly at this level of complexity
Signed-off-by: Giles Cope <gilescope@gmail.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b554fba74f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const touched = files | ||
| .map((f) => owningCrate(f, crates)) | ||
| .filter((x): x is string => x !== null); |
There was a problem hiding this comment.
When a non-root Cargo.toml changes only the feature set of a shared third-party dependency, Cargo.lock does not change and the root-manifest path is not used, so these lines seed only the crate that owns that manifest. The reverse closure then checks that crate and its workspace dependents, but skips unrelated crates that use the same dependency and may now compile only because the new feature is unified in the workspace, which is exactly the class of regression this job is meant to catch.
Useful? React with 👍 / 👎.
| paths: | ||
| - "**/Cargo.toml" | ||
| - "Cargo.lock" | ||
| - "rust-toolchain.toml" | ||
| - "scripts/feature-unification-scope.ts" | ||
| - ".github/workflows/feature-unification.yml" |
There was a problem hiding this comment.
This path allowlist omits inputs that now directly affect +check-feature-unification, notably Earthfile and scripts/package*.json; a PR that changes only the Earthly target or the npm lockfile that npm ci consumes will not start this workflow at all. Since this commit also enables Dependabot for /scripts, those dependency-update PRs can merge without ever exercising the scoper install/runtime path they modify.
Useful? React with 👍 / 👎.
3 participants
Overview
Your PRs just got 42 minutes faster.
The Feature Unification Check was CI's most expensive subscription: ~42 min of serial
cargo hack checkacross 74 crates, on every PR push and every merge-group entry — fully cold each time (ephemeral runner, no warm cache anywhere in that job) — while gating nothing, since it isn't a required status check.This PR replaces brute force with a dependency graph:
**/Cargo.toml,Cargo.lock,Earthfile, the workflow itself). The.rs-only PR that dominates daily traffic: 42 min → 0.scripts/feature-unification-scope.shattributes the diff to crates and computes the reverse-dependency closure over normal+build edges, feedingcargo hack -pexactly the crates whose isolated resolution could have changed. Touch one pallet → ~5 crates, not 74. Dev-dep edges are deliberately ignored: the check strips dev-deps, so nothing can break through one.Cargo.lock, rootCargo.toml,.cargo/,Earthfile, or any file no crate claims → full--workspacerun. Files inside crate dirs always attribute (yes,docs/andres/are crates —include_str!is watching).partner-chains-demo-{node,runtime}excluded (~5 min of zero signal),cargo-hackpinned to 0.6.45 (no more version drift between runs), GHCR login dropped (the build chain only pulls the publicmidnight-node-ciimage from docker.io — one less secret in a PR-triggered job).Worst case (lock-bump PR) pays roughly the old price minus the demo crates. The common case pays nothing. Coverage for what lands on a PR is unchanged: a change can only break the per-package check of itself or its dependents — exactly the closure we check.
🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
scripts/feature-unification-scope.sh --self-test— 11 cases green (closure propagation, dev-dep edge inertness, global-input fallback, ignore list, demo-crate exclusion, whole-closure collapse to--workspace).pallets/midnight/src/lib.rs→ 5 crates;node/src/main.rs→ 1;primitives/midnight→ ~10;Cargo.lock→ full workspace.cargo hack check -p docs --no-dev-depsvalidated locally against this workspace (flag combination works with the pinned 0.6.45).shellcheckclean; workflow YAML parses;earthly lsparses the Earthfile.cargo hack, hot spotsmidnight-node6.7 min and the demo crates 5.2 min.🔱 Fork Strategy
Links
None — CI housekeeping (labels applied accordingly).