Skip to content

Migrate back to std:: stylistically #554

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

Merged
merged 3 commits into from
Nov 19, 2019

Conversation

alexcrichton
Copy link
Member

This commit moves away from idioms such as alloc:: and core:: as
imports of standard data structures and types. Instead it migrates all
crates to uniformly use std:: for importing standard data structures
and types. This also removes the std and core features from all
crates to and removes any conditional checking for feature = "std"

All of this support was previously added in #407 in an effort to make
wasmtime/cranelift "no_std compatible". Unfortunately though this
change comes at a cost:

  • The usage of alloc and core isn't idiomatic. Especially trying to
    dual between types like HashMap from std as well as from
    hashbrown causes imports to be surprising in some cases.
  • Unfortunately there was no CI check that crates were no_std, so none
    of them actually were. Many crates still imported from std or
    depended on crates that used std.

It's important to note, however, that this does not mean that wasmtime
will not run in embedded environments
. The style of the code today and
idioms aren't ready in Rust to support this degree of multiplexing and
makes it somewhat difficult to keep up with the style of wasmtime.
Instead it's intended that embedded runtime support will be added as
necessary. Currently only std is necessary to build wasmtime, and
platforms that natively need to execute wasmtime will need to use a
Rust target that supports std. Note though that not all of std needs
to be supported, but instead much of it could be configured off to
return errors, and wasmtime would be configured to gracefully handle
errors.

The goal of this PR is to move wasmtime back to idiomatic usage of
features/std/imports/etc and help development in the short-term.
Long-term when platform concerns arise (if any) they can be addressed by
moving back to no_std crates (but fixing the issues mentioned above)
or ensuring that the target in Rust has std available.

alexcrichton added a commit to alexcrichton/wasmparser.rs that referenced this pull request Nov 12, 2019
This commit performs a similar transformation as
bytecodealliance/wasmtime#554, with similar motivation listed out there
as well.
@alexcrichton
Copy link
Member Author

Ok I've rebased this on the current master branch, and @sunfishcode want to look at the documentation I've added as well? I've opted to fill out this page and I've put down some basic words about what's currently supported and then also filled out how wasmtime is interested in running basically everywhere, but support is in the works and if it doesn't work concretely yet users should reach out.

This commit moves away from idioms such as `alloc::` and `core::` as
imports of standard data structures and types. Instead it migrates all
crates to uniformly use `std::` for importing standard data structures
and types. This also removes the `std` and `core` features from all
crates to and removes any conditional checking for `feature = "std"`

All of this support was previously added in bytecodealliance#407 in an effort to make
wasmtime/cranelift "`no_std` compatible". Unfortunately though this
change comes at a cost:

* The usage of `alloc` and `core` isn't idiomatic. Especially trying to
  dual between types like `HashMap` from `std` as well as from
  `hashbrown` causes imports to be surprising in some cases.
* Unfortunately there was no CI check that crates were `no_std`, so none
  of them actually were. Many crates still imported from `std` or
  depended on crates that used `std`.

It's important to note, however, that **this does not mean that wasmtime
will not run in embedded environments**. The style of the code today and
idioms aren't ready in Rust to support this degree of multiplexing and
makes it somewhat difficult to keep up with the style of `wasmtime`.
Instead it's intended that embedded runtime support will be added as
necessary. Currently only `std` is necessary to build `wasmtime`, and
platforms that natively need to execute `wasmtime` will need to use a
Rust target that supports `std`. Note though that not all of `std` needs
to be supported, but instead much of it could be configured off to
return errors, and `wasmtime` would be configured to gracefully handle
errors.

The goal of this PR is to move `wasmtime` back to idiomatic usage of
features/`std`/imports/etc and help development in the short-term.
Long-term when platform concerns arise (if any) they can be addressed by
moving back to `no_std` crates (but fixing the issues mentioned above)
or ensuring that the target in Rust has `std` available.
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

This looks good to me.

As additional background for folks following along, I wrote many of the no_std changes that the PR here removes. Independently of this PR, I've been thinking for a while now that it'd be nice if we could factor out all our usual no_std tricks into a helper crate, so that we don't need quite so many changes throughout the tree. Just recently, I discovered there's already a no-std-compat crate which does exactly that.

So, converting the bulk of the casebase back to the std style, as this PR does, feels like the right thing to do in any case. If do we reintroduce no_std support (and as the docs in the PR say, please talk to us if you're interested!), I expect we'd redo it anyway, in terms of something like no-std-compat so that it's much less invasive.


* Linux x86\_64
* macOS x86\_64
* Windows x86\_64
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it's worth mentioning here, but at the moment there are a handful of WASI tests which aren't currently supported on Windows yet, though we are working on addressing these.

@sunfishcode sunfishcode merged commit 39e57e3 into bytecodealliance:master Nov 19, 2019
@alexcrichton alexcrichton deleted the back-to-std branch November 19, 2019 15:23
alexcrichton added a commit to alexcrichton/wasmparser.rs that referenced this pull request Dec 4, 2019
This commit performs a similar transformation as
bytecodealliance/wasmtime#554, with similar motivation listed out there
as well.
yurydelendik pushed a commit to bytecodealliance/wasmparser that referenced this pull request Dec 4, 2019
This commit performs a similar transformation as
bytecodealliance/wasmtime#554, with similar motivation listed out there
as well.
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.

2 participants