Skip to content

Add cargo-hack to CI to check crate features#8927

Merged
mergify[bot] merged 1 commit intosigp:unstablefrom
macladson:check-crate-features
Apr 7, 2026
Merged

Add cargo-hack to CI to check crate features#8927
mergify[bot] merged 1 commit intosigp:unstablefrom
macladson:check-crate-features

Conversation

@macladson
Copy link
Copy Markdown
Member

Issue Addressed

#8926

Proposed Changes

Add a step to CI which runs cargo check across all combinations of features for certain crates using cargo-hack

Additional Info

I have only implemented checks for eth2 and types since these are our most "user-facing" crates but we can consider other candidates as well.

The number of checks it makes scales at $2^N$ where N = number of features. So adding a single new feature effectively doubles the number of checks required. So when adding more features we should try to identify which features we can exclude if possible.

channel: stable
- uses: taiki-e/install-action@cargo-hack
- name: Check types feature powerset
run: cargo hack check -p types --feature-powerset --no-dev-deps --exclude-features arbitrary-fuzz,portable
Copy link
Copy Markdown
Member Author

@macladson macladson Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I am excluding arbitrary-fuzz (since it is merely an alias of arbitrary) and portable (since it just enables a bls feature which should have no impact on our own compilation).

@macladson macladson changed the title Add cargo-hack to CI Add cargo-hack to CI to check crate features Mar 2, 2026
@macladson macladson added the ready-for-review The code is ready for review label Mar 2, 2026
@macladson
Copy link
Copy Markdown
Member Author

macladson commented Mar 2, 2026

We could also consider testing even more crates but using --depth 2 to only test pairs of features. This would probably cover the vast majority of possible feature conflict scenarios (this is what tokio does)

Copy link
Copy Markdown
Member

@chong-he chong-he left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I have tested removing #[cfg(feature = "events")] in the eth2 crates and it will error. Just one comment below.

Comment on lines +430 to +431
cargo-hack:
name: cargo-hack
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having a new check at the top-level (which makes the total check increases), I am wondering if we could combine all dependencies-related checks (cargo-udeps, cargo-sort and the current cargo-hack) into one top-level check, and then have these 3 cargo checks under it. We can name the top-level check something like "dependency-check". That will also reduce some boilerplate when defining the name etc required for a new top-level check. Just my 2 cents

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Apr 7, 2026
@mergify mergify Bot added the queued label Apr 7, 2026
@mergify
Copy link
Copy Markdown

mergify Bot commented Apr 7, 2026

Merge Queue Status

This pull request spent 29 minutes 28 seconds in the queue, including 27 minutes 8 seconds running CI.

Required conditions to merge

mergify Bot added a commit that referenced this pull request Apr 7, 2026
@mergify mergify Bot merged commit 243eecc into sigp:unstable Apr 7, 2026
39 checks passed
@mergify mergify Bot removed the queued label Apr 7, 2026
@macladson macladson deleted the check-crate-features branch April 7, 2026 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-quality ready-for-merge This PR is ready to merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants