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

Implement checks to identify dependencies that do not support no_std #1250

Merged
merged 2 commits into from
Aug 9, 2021

Conversation

soareschen
Copy link
Contributor

Part of #1158

Description

This PR adds new check scripts in the no-std-check sub-project to check for no_std and Substrate compliance for ibc-rs. Through the checks, I have identified the following dependencies that are not supported on no_std or Substrate:

  • tonic
  • socket2
  • getrandom
  • serde
  • serde_json
  • ics23
  • regex

Check Scripts

I have implemented the following check scripts in Makefile to check for different modes of no_std compliance:

  • check-panic-conflict - Check for no_std compliance by installing a panic handler, and any other crate importing std will cause a conflict. Runs on default target.

  • check-cargo-build-std - Check for no_std compliance using Cargo nightly's build-std feature. Runs on the target x86_64-unknown-linux-gnu.

  • check-wasm - Check for WebAssembly and no_std compliance by building on the target wasm32-unknown-unknown and installing a panic handler.

  • check-substrate - Check for Substrate, WebAssembly, and no_std compliance by importing Substrate crates and building on wasm32-unknown-unknown. Any crate using std will cause a conflict on the panic and out-of-memory (OOM) handlers installed by sp-io.

In particular, if we want to support ibc-rs on Substrate, then the check-substrate script must pass. There is additional complication that the check is done on Wasm target, and not all crate supports Wasm. In particular, the serde crate passes the no_std check when built on x86 and Wasm targets, but curiously fails the check when the Substrate runtime is linked.

With the check scripts in place, our goal is fix the dependencies issue in several ways:

  • Have the no_std support fixed upstream and update the dependency version.
  • Find alternative crates that support the same features and also support no_std.
  • Put the use of unsupported dependencies behind feature flags so that they are disabled in no_std mode.

@DaviRain-Su if possible, we prefer the no_std support to be fixed upstream so that minimal changes need to be done on our side. Other than tonic, can you check with each of the listed projects and find out if there is any way to have those packages supported on no_std as well as Substrate?


For contributor use:

  • Added a changelog entry, using unclog.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@soareschen soareschen added the E: no-std External: support for no_std compatibility label Jul 29, 2021
@soareschen
Copy link
Contributor Author

Update for ics23: the no_std support is being handled here: cosmos/ics23#42.

@DaviRain-Su
Copy link
Contributor

DaviRain-Su commented Jul 30, 2021

error[E0432]: unresolved import crate::sys
--> /Users/davirain/.cargo/registry/src/github.com-1ecc6299db9ec823/socket2-0.4.1/src/sockaddr.rs:5:12
|
5 | use crate::sys::{
| ^^^ could not find sys in the crate root
error[E0432]: unresolved imports crate::sys, crate::sys
--> /Users/davirain/.cargo/registry/src/github.com-1ecc6299db9ec823/socket2-0.4.1/src/socket.rs:21:12
|
21 | use crate::sys::{self, c_int, getsockopt, setsockopt, Bool};
| ^^^ ^^^^ no sys in the root
| |
| could not find sys in the crate root
error[E0432]: unresolved import sys
--> /Users/davirain/.cargo/registry/src/github.com-1ecc6299db9ec823/socket2-0.4.1/src/lib.rs:129:5

scoket2 does not support no_std.

@DaviRain-Su
Copy link
Contributor

Compiling sp-io v3.0.0 (/Users/davirain/workspace/substrate/primitives/io)
error[E0152]: found duplicate lang item panic_impl
--> /Users/davirain/workspace/substrate/primitives/io/src/lib.rs:1440:1
|
1440 | pub fn panic(info: &core::panic::PanicInfo) -> ! {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: the lang item is first defined in crate std (which tracing depends on)
= note: first definition in std loaded from /Users/davirain/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/wasm32-unknown-unknown/lib/libstd-ccdba070b604d7bc.rlib
= note: second definition in the local crate (sp_io)

error[E0152]: found duplicate lang item oom
--> /Users/davirain/workspace/substrate/primitives/io/src/lib.rs:1451:1
|
1451 | pub fn oom(_: core::alloc::Layout) -> ! {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: the lang item is first defined in crate std (which tracing depends on)
= note: first definition in std loaded from /Users/davirain/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/wasm32-unknown-unknown/lib/libstd-ccdba070b604d7bc.rlib
= note: second definition in the local crate (sp_io)

For more information about this error, try rustc --explain E0152.
error: could not compile sp-io due to 2 previous errors

Adding toinc also seems to be blocked by sp_io.

@DaviRain-Su
Copy link
Contributor

https://github.com/octopus-network/substrate/tree/dv-check-support-no-std

I tested it here by switching to dv-check-support-no-std branch and running cargo check -p node-template

@romac
Copy link
Member

romac commented Aug 9, 2021

@soareschen What's the next step here?

@soareschen
Copy link
Contributor Author

I think we can merge this so that we can track the support in the main branch.

@romac romac merged commit 4c90044 into master Aug 9, 2021
@romac romac deleted the soares/no-std-check-deps branch August 9, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E: no-std External: support for no_std compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants