Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Added CI back for check_no_std with compilation to wasm32 #9347

Closed
wants to merge 2 commits into from

Conversation

puma314
Copy link

@puma314 puma314 commented Jul 6, 2024

Previously the CI check for check_no_std was commented out, as the riscv32imac-unknown-none-elf target doesn't come with std and there are many dependencies in reth that don't support no_std, making it difficult to compile to this target.

However, there are several embedded environments (namely zkVMs like SP1) that do support std, but do not (easily) support crates with embedded C++ dependencies (like secp256k1, or blst or z-std) and also do not support networking crates like socket. The zkVM embedded environment is therefore similar to a target like wasm32-wasi which comes with std, but also does not support crates like secp256k1, blst and does not support networking crates.

This PR adds back the no_std CI check but with the wasm32-wasi target, which should be significantly easier to support within Reth and also will ensure that all relevant packages that might be used in a zkVM context can compile.

With this PR, reth-codecs and reth-primitives-traits compiles with no-default-features to wasm32-wasi. reth-primitives almost compiles with no-default-features, but I'm running into this weird error that I was hoping @JackG-eth could help with since I saw he did the replacement of thiserror_no_std::Error everywhere.

Compiling reth-primitives v1.0.0 (/Users/umaroy/Documents/reth/crates/primitives)
error[E0433]: failed to resolve: use of undeclared crate or module `std`
 --> crates/primitives/src/transaction/error.rs:6:1
  |
6 | pub enum InvalidTransactionError {
  | ^^^ use of undeclared crate or module `std`
  |
help: consider importing this module
  |
1 + use core::error;

error[E0433]: failed to resolve: use of undeclared crate or module `std`
  --> crates/primitives/src/transaction/error.rs:59:1
   |
59 | pub enum TransactionConversionError {
   | ^^^ use of undeclared crate or module `std`
   |
help: consider importing this module
   |
1  + use core::error;
   |

error[E0433]: failed to resolve: use of undeclared crate or module `std`
  --> crates/primitives/src/transaction/error.rs:70:1
   |
70 | pub enum TryFromRecoveredTransactionError {
   | ^^^ use of undeclared crate or module `std`
   |
help: consider importing this module
   |
1  + use core::error;

@puma314
Copy link
Author

puma314 commented Jul 6, 2024

@mattsse after this PR is merged, I can work on making reth-primitives in the workspace have default-features=false and then importing default features only in packages that need them. Then, it should be straightforward to add back reth-consensus, reth-db, reth-errors and all the other commented out packages in the check_no_std.sh CI test.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this currently does not compile without default features because we need to use this combo:

thiserror-no-std = { workspace = true, default-features = false }

std = ["thiserror-no-std/std"]

@@ -33,7 +33,8 @@ secp256k1 = { workspace = true, features = [
"global-context",
"recovery",
"rand",
] }
], optional = true }
k256 = { version = "0.13.3", default-features = false, features = ["ecdsa"] }
Copy link
Collaborator

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 if we're not using it?

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

This is cool! It would be great to have this in CI

# reth-revm
)

for package in "${no_std_packages[@]}"; do
cmd="cargo +stable build -p $package --target riscv32imac-unknown-none-elf --no-default-features"
cmd="cargo +stable build -p $package --target wasm32-wasi --no-default-features"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmd="cargo +stable build -p $package --target wasm32-wasi --no-default-features"
cmd="cargo +stable build -p $package --target wasm32-wasip1 --no-default-features"

wasm32-wasi is being renamed to wasm32-wasip1 and will be removed
rust-lang/compiler-team#607

@shekhirin
Copy link
Collaborator

Superseded by #9430 and #9982

@shekhirin shekhirin closed this Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants