ci: add check features matrix gate with dynamic CI fan-out#1283
Conversation
9b21fda to
001c640
Compare
check features matrix gate with dynamic CI fan-outcheck features matrix gate with dynamic CI fan-out
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Excellent! Thank you
There was a problem hiding this comment.
Pull request overview
Adds a new feature-matrix gate driven by xtask, with CI dynamically expanding one job per feature-check case.
Changes:
- Adds
cargo xtask check featureswith list, single-case, and run-all modes. - Defines curated feature checks and cargo-hack powerset groups in
xtask/src/features.rs. - Adds CI setup/fan-out jobs and wires them into the existing
successgate.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
xtask/src/main.rs |
Routes the new feature-check action and includes it in local ci. |
xtask/src/features.rs |
Defines and runs feature-matrix cases, including GitHub matrix JSON output. |
xtask/src/cli.rs |
Adds CLI help and parsing for check features. |
xtask/src/check.rs |
Installs cargo-hack as part of check tooling. |
xtask/src/bin_version.rs |
Pins the cargo-hack version. |
.github/workflows/ci.yml |
Adds feature-matrix setup/fan-out jobs and includes them in success aggregation. |
Comments suppressed due to low confidence (1)
xtask/src/features.rs:229
- The cargo-hack powerset checks also omit
--locked. Since these checks are used as a CI gate and the feature-matrix job does not run the lock-file check afterward, they should use the committed lockfile rather than allowing Cargo/cargo-hack to refresh dependency resolution inside the job.
let mut args: Vec<String> = vec![
"hack".into(),
"check".into(),
"--feature-powerset".into(),
"--no-dev-deps".into(),
"--depth".into(),
depth_str,
"--exclude-features".into(),
exclude_features,
];
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut args: Vec<String> = vec!["check".into(), "-p".into(), (*package).into()]; | ||
| if *no_default_features { | ||
| args.push("--no-default-features".into()); | ||
| } | ||
| if !features.is_empty() { | ||
| args.push("--features".into()); | ||
| args.push(features.join(",")); | ||
| } | ||
| cmd!(sh, "{CARGO}").args(&args).run()?; |
There was a problem hiding this comment.
Right. Added --locked to both invocation paths in run_one. cargo-hack's --no-dev-deps mutates Cargo.toml while it runs, which conflicts with --locked (the lockfile diverges from the modified manifest), so the powerset case also drops --no-dev-deps to keep reproducibility. Trade-off is small: dev-deps stay in the type-check graph for the powerset cases.
Added a cargo xtask check locks -v step to the feature-matrix worker as well, so any lockfile drift specific to running cargo-hack gets caught at the gate. Force-pushed 7facb020.
| /// `qoiz` is a transitive dependent of `qoi`; combinatorics are covered by the | ||
| /// `qoi` enumeration. | ||
| /// `__bench` and `__test` are private features that pull `visibility` for the | ||
| /// testsuite; the existing lints job exercises them via `--features helper,__bench`. | ||
| const EXCLUDE_FEATURES: &[&str] = &["qoiz", "__bench", "__test"]; |
There was a problem hiding this comment.
Correct. qoiz removed from EXCLUDE_FEATURES and the comment revised. ironrdp-session, ironrdp-server, and the ironrdp-connector forwarding chain do gate additional zstd/QoiZ code on qoiz that the qoi enumeration doesn't cover. Combinatorial cost stays bounded by --depth 2. Force-pushed 7facb020.
001c640 to
f2b6e33
Compare
Adds a feature-matrix CI gate per Devolutions#1123 (CI: feature matrix including `arbitrary`). The case enumeration lives in xtask so the matrix is self-documenting and locally reproducible. `cargo xtask check features` is the new subcommand. Three modes: cargo xtask check features Run every case sequentially. Mirrors what CI does in aggregate. cargo xtask check features --case <NAME> Run a single case. CI matrix workers use this; the case name doubles as the local repro command for any failure. cargo xtask check features --list [--format <FMT>] Enumerate cases. `--format github-matrix` emits a JSON include array for `strategy.matrix.include: ${{ fromJson(...) }}`. Two new CI jobs in `.github/workflows/ci.yml`: feature-matrix-setup Preflight job. Runs `cargo xtask check features --list --format github-matrix` and emits the JSON via `$GITHUB_OUTPUT`. feature-matrix [${{ matrix.case }}] Fan-out job. Consumes the preflight output via fromJson and runs `cargo xtask check features --case '${{ matrix.case }}'` per case. Both join the existing `success` job's `needs` list so branch protection keeps a single required status check. Each worker writes a one-line markdown entry to `$GITHUB_STEP_SUMMARY` so failures show in the run summary view without expanding logs. Initial case set (10): - per-crate curated invariants (ironrdp-core std/alloc, ironrdp-pdu std/arbitrary/arbitrary-alloc) - workspace powersets partitioned by layer: foundation, pdu, channels, connector-session, runtime cargo-hack pinned at 0.6.44 in xtask/src/bin_version.rs, installed via the existing `cargo xtask check install` flow. ironrdp-tls / ironrdp-client / ironrdp-mstsgu intentionally outside the initial case set. cargo-hack does not honor package.metadata.cargo-hack, so the exactly-one-of TLS-backend constraint needs `--mutually-exclusive -features`, `--at-least-one-of`, and `--exclude-no-default-features` on the invocation. The powerset also surfaces a latent bug in `extract_tls_server_public_key` that uses `x509_cert::*` unconditionally instead of gating on `rustls | native-tls`. Both are tractable but out of scope for this gate's initial landing; the regular Checks job already exercises all three crates with their default features. Closes Devolutions#1123 items 1-3 of the remediation set posted in issuecomment-4464512384. Items 4-5 (the `ironrdp-web` exclusion and the cargo-hack version pin) are folded into the per-case `extra_args` and `bin_version.rs` respectively. The TLS coverage gap is the remaining item from the original five. Signed-off-by: Greg Lamberson <greg@lamco.io> Assisted-by: Claude Code (Anthropic, Opus) Signed-off-by: Greg Lamberson <greg@lamco.io> Assisted-by: Claude Code (Anthropic, Opus)
f2b6e33 to
7facb02
Compare
d18c511
into
Devolutions:master
Closes the matrix-gate items (3-5) from #1123. The case enumeration lives in
xtaskso the matrix is self-documenting and locally reproducible; CI fans out via a preflight job emitting a JSONmatrix.includearray, per #1123 (comment).What it adds
cargo xtask check featuressubcommand with three modes:cargo xtask check featurescargo xtask check features --case <NAME>cargo xtask check features --list [--format <FMT>]--format github-matrixemits a JSON include array suitable forstrategy.matrix.include: ${{ fromJson(...) }}.Two new CI jobs in
.github/workflows/ci.yml:feature-matrix-setuppreflight runs the list step, emitsfeature-matrixvia$GITHUB_OUTPUT.feature-matrix [${{ matrix.case }}]fan-out consumes the preflight output viafromJsonand runs one case per worker.Both join the existing
successjob'sneedslist so branch protection keeps a single required status check. Each worker writes a one-line markdown entry to$GITHUB_STEP_SUMMARYso per-case results show in the run summary view without expanding logs.Initial case set (10)
Per-crate curated invariants (the
arbitraryand std/alloc discipline that the powerset alone does not pin down):ironrdp-core/alloc,ironrdp-core/stdironrdp-pdu/std,ironrdp-pdu/arbitrary,ironrdp-pdu/arbitrary-allocWorkspace powersets, partitioned by layer so each fan-out worker stays bounded:
workspace/powerset-foundation(ironrdp-core, ironrdp-error, ironrdp-str, ironrdp-bulk)workspace/powerset-pdu(ironrdp-pdu, ironrdp-graphics)workspace/powerset-channels(ironrdp-cliprdr, ironrdp-rdpdr, ironrdp-rdpsnd, ironrdp-egfx, ironrdp-dvc, ironrdp-svc, ironrdp-input, ironrdp-ainput, ironrdp-displaycontrol, ironrdp-rdpeusb)workspace/powerset-connector-session(ironrdp-connector, ironrdp-session, ironrdp-acceptor)workspace/powerset-runtime(ironrdp-async, ironrdp-blocking, ironrdp-tokio, ironrdp-server)Adding a new check is one entry in
CASESinxtask/src/features.rs; the CI matrix discovers it automatically.Mapping to the #1123 remediation set
ironrdp-bulkalloc plumbingironrdp-strToOwnedimportextra_args(see comment infeatures.rsre. TLS scope)ironrdp-webexclusionironrdp-web(the existingwebjob covers it on wasm32)TLS coverage gap (documented limitation)
ironrdp-tls,ironrdp-client,ironrdp-mstsguare intentionally outside the initial case set. Two reasons, both tractable but out of scope for this gate's initial landing:package.metadata.cargo-hack, so the exactly-one-of TLS-backend constraint needs--mutually-exclusive-features,--at-least-one-of, and--exclude-no-default-featuresre-stated on the invocation.crates/ironrdp-tls/src/lib.rs::extract_tls_server_public_keythat usesx509_cert::Certificateunconditionally instead of gating oncfg(any(feature = "rustls", feature = "native-tls")). Visible under--features stubalone.The regular
Checksjob already exercises all three crates with their default features, so the new gate's omission is not a coverage regression.Local validation
Provenance and tooling
This change originated from the #1123 workspace feature-matrix scan and the design discussion at #1123 (comment). I agree to the Developer Certificate of Origin and disclose use of Claude Code (Opus) per the LLM use policy; trailers on the commit reflect both.