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

feat: add binding crate integrations #89

Merged
merged 7 commits into from
May 20, 2024

Conversation

rvolosatovs
Copy link
Member

@rvolosatovs rvolosatovs commented May 13, 2024

This is just the first step in adding integrations with existing crates in Rust ecosystem to bindings already provided by this crate. I intentionally kept this PR small to gather some feedback on the approach and the direction here.

In particular, this PR:

  • Slightly reorganizes the structure adding a new ext module, which now contains the stdlib integration (Implement Error for wasi::io::error::Error #86 and the newly-added std::io::{Write,Read} implementations)
  • Adds opt-in rand crate integration
  • Adds stdlib std::io::{Write, Read} implementations on streams, enabled by default, which is now also used in the examples
  • Split examples into std and no_std versions. This is probably an overkill - I think most users of this crate will use std and will compile to wasm32-wasi, but I did not want to remove the existing no_std examples.

Example follow-up for e.g. http crate integration could look like: #90

@sunfishcode
Copy link
Member

For the rand integration, would it make sense for the code to eventually live in the getrandom crate? And if so, would the code here cause a circular dependency?

@alexcrichton
Copy link
Member

I'm also wondering how best to place these extensions, either in the wasi crate itself or in a wasi-ext crate. The upside of within wasi itself is that it's easy to reach for and use plus coherence allows trait impls on the defined types. The downside though is that as @sunfishcode said we can't reach the "desired end state" as an interim transitionary state because it'd introduce circular dependencies which might be hard to break.

The alternative though of a wasi-ext crate has the downside of native trait impls would no longer work. That's ok for rand but wouldn't be suitable for the http crate integration.

Despite that though I think I'd personally lean a bit more towards an external crate. That feels more along the trajectory of what we eventually want in terms of integrating this crate into other crates (like getrandom)

@rvolosatovs
Copy link
Member Author

rvolosatovs commented May 14, 2024

It seems that we have consensus on the following two points:

  1. std integration belongs in wasi crate (at least that's the approach that was already taken in Implement Error for wasi::io::error::Error #86 and extended in this PR to Read and Write traits)
  2. third-party crates should be able to directly depend on wasi crate and introduce an absolute minimum of transitive dependencies into their projects by doing so (ideally - none)

Assuming that the we agree on the above, here's the scenario I was envisioning for the third-party crate integrations:
(I'll use http integration as an example, as it's most useful with traits implemented directly on the generated types)

  1. wasi crate includes opt-in http crate integration gated by http feature flag in version X - note that this integration would only be guaranteed to work with http crate version Y, dependent upon by wasi X
    • This includes {Try,}From implementations for generated structs/enums etc.
  2. http crate version Z is released, which depends on wasi X to pull in the generated code without the http integration (http integration is opt-in gated by a feature flag, meaning that there's no circular dependency) and provides its own integration, e.g. by simply duplicating the implementation from this crate or rolling a custom one
  3. (possible approach) wasi crate version X+1 is released, in which:
    • http crate is updated to version Z
    • all http trait implementations are removed (since they now exist in http Z), effectively making http feature on the crate a no-op
  4. wasi crate version X+2 removes the http feature altogether and the optional dependency

From downstream user perspective, update from wasi X to wasi X+1 is non-breaking, that is because all the traits dependent upon that were implemented in wasi crate are now implemented in http.

If, however, the http crate integration lived in wasi_ext::http, for example, it would contain a collection of conversion functions like method_from_http etc. (or perhaps we could create FromWasi, TryFromWasi etc. trait alternatives to std ones to provide a more familiar "idiomatic" experience) - regardless of what we'd do here, the downstream consumers would eventually have to modify their implementation to address the breaking changes they'd experience on update, as opposed to simply removing the feature in Cargo.toml.

To reiterate, the proposal is to gate every single crate integration with a feature flag - the default wasi crate import would only contain the generated code and std trait implementations.

With this approach, I'm struggling to see the circular dependency in third-party crates.
I don't mind maintaining all of these integrations in a separate crate, if that's what everyone prefers, but I would like to understand the issue we're trying to avoid by doing so at expense of downstream UX and maintenance cost.

@sunfishcode @alexcrichton could you help me understand the concern here?

@alexcrichton
Copy link
Member

Definitely agree on the first two points, but the situation I'm worried about is:

  • Currently we have http v1.1.0 and wasi v0.13.0
  • Let's assume wasi v0.13.1 adds an off-by-default http feature using http v1.*.*
  • You have an app that depends on wasi v0.13.* and http v1.*.* and enables the wasi feature
  • In the future http v1.2.0 adds a dependency on wasi v0.13.*
  • Your app can no longer run cargo update because of the circular dependency

The main assumption here is that http seems pretty unlikely to release a semver-incompatible (e.g. 2.0) change with WASI support. Any change would likely be in the 1.x semver track meaning that applications using wasi-with-http-feature are now mixed up with http-using-wasi.

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs rvolosatovs force-pushed the feat/ext branch 3 times, most recently from b77749a to 6b2b28f Compare May 20, 2024 14:19
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
Copy link
Member Author

@sunfishcode @alexcrichton rebased on top of latest main and added the wasi-ext crate to the workspace as discussed, PTAL

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, thanks!

I'll hold off on merging for @sunfishcode as well though

@sunfishcode sunfishcode merged commit 4c9ff82 into bytecodealliance:main May 20, 2024
5 checks passed
@sunfishcode
Copy link
Member

Looks good to me too!

@sunfishcode
Copy link
Member

wasi-ext is now published on crates.io.

@rvolosatovs rvolosatovs deleted the feat/ext branch May 22, 2024 15:34
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.

3 participants