Skip to content

Conversation

@teor2345
Copy link
Contributor

@teor2345 teor2345 commented Mar 10, 2025

PR #3411 removed default features that should have been included in some dependencies (but weren't included in every dependency). This PR restores those default features.

It also adds two extra jobs to PR CI, so this doesn't happen again:~~

  • subspace-runtime snapshot build (covering the most important no_std build)
  • check all crates individually (covering all possible missed features, including tokio/rt and test runtimes)

Code contributor checklist:

@teor2345 teor2345 added bug Something isn't working execution Subspace execution labels Mar 10, 2025
@teor2345 teor2345 self-assigned this Mar 10, 2025
@teor2345 teor2345 enabled auto-merge March 10, 2025 03:06
Copy link
Contributor

@vedhavyas vedhavyas left a comment

Choose a reason for hiding this comment

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

Which crate was brining the std that lead to runtime builds being failed ?

cargo -Zgitoxide -Zgit nextest run --locked
# This job checks all crates individually, including no_std and other featureless builds
cargo-check-individually:
Copy link
Contributor

Choose a reason for hiding this comment

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

technically not required if we can just clippy consensus runtime and domain runtimes with no-std

Copy link
Contributor Author

@teor2345 teor2345 Mar 10, 2025

Choose a reason for hiding this comment

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

That would only discover some issues. The runtimes all build with commit 94bff8b, but there are fixes to 10+ other crates that are only covered by this check (see the fix commits after that commit).

It only takes 2.5 minutes (edit: still working out how long it takes) to run these checks, so it seems worth doing them.

Copy link
Contributor Author

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Which crate was brining the std that lead to runtime builds being failed ?

Before PR #3411, a significant number of crates had std features activated, but bug mainmatter/cargo-autoinherit#40 in the automated workspace dependency tool disabled those features.

This PR restores the previous std dependencies as they were before PR #3411.

If we want to enable/disable std in subspace-runtime, let’s do that in another PR?
Commit 94bff8b (and maybe earlier commits) are the ones to look at to make that change.

Edit: I also need to fix the CI cache keys to avoid conflicts.

cargo -Zgitoxide -Zgit nextest run --locked
# This job checks all crates individually, including no_std and other featureless builds
cargo-check-individually:
Copy link
Contributor Author

@teor2345 teor2345 Mar 10, 2025

Choose a reason for hiding this comment

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

That would only discover some issues. The runtimes all build with commit 94bff8b, but there are fixes to 10+ other crates that are only covered by this check (see the fix commits after that commit).

It only takes 2.5 minutes (edit: still working out how long it takes) to run these checks, so it seems worth doing them.

@teor2345 teor2345 force-pushed the fix-runtime-builds branch from bd896da to fb6542e Compare March 10, 2025 21:44
teor2345

This comment was marked as outdated.

@teor2345 teor2345 requested a review from vedhavyas March 10, 2025 21:45
@vedhavyas

This comment was marked as resolved.

rand = { workspace = true, features = ["min_const_gen"] }

# test-ethereum dependencies
ethereum.workspace = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this to be duplicated in test deps and actual deps ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it is required by the tests, but optional for production code.

@teor2345
Copy link
Contributor Author

I've resolved the conversations around CI by moving those commits to another PR, so that we can get this PR merged and the runtime snapshots built.

Would have preferred them in the same commit so that CI would run those actions as well rather merging this and that seperately.

I merged that branch into this PR, and updated the names and documentation based on your review in PR #3427.

@teor2345 teor2345 requested a review from vedhavyas March 11, 2025 03:30
@teor2345 teor2345 added this pull request to the merge queue Mar 11, 2025
Merged via the queue into main with commit 5c8cfa8 Mar 11, 2025
12 checks passed
@teor2345 teor2345 deleted the fix-runtime-builds branch March 11, 2025 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working execution Subspace execution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants