Skip to content

Delete the wasmtime-wasi-c crate#844

Merged
sunfishcode merged 1 commit into
bytecodealliance:masterfrom
alexcrichton:delete-c-code
Jan 24, 2020
Merged

Delete the wasmtime-wasi-c crate#844
sunfishcode merged 1 commit into
bytecodealliance:masterfrom
alexcrichton:delete-c-code

Conversation

@alexcrichton
Copy link
Copy Markdown
Member

This commit deletes the old C implementation of the original
wasi_unstable module, instead only leaving around our single
wasmtime-wasi crate as the implementation for both
wasi_snapshot_preview1 and wasi_unstable.

This hasn't been discussed (AFAIK) up until now, so this is also a
proposal! Some thoughts in favor of this deletion I would have are:

  • This has been off-by-default for ages
  • We don't build or test any of this on CI
  • Published binaries with wasmtime do not have this possibility
    enabled
  • Future refactorings to the wasmtime-wasi crate will either need to
    work around how the C implementation is different or bring it up to
    speed.

This is motivated by the last bullet point where I was working on
getting wasmtime-wasi working purely as an implementation detail on
top of the wasmtime crate itself, but quickly ran into a case where
the CLI would need to multiplex all sorts of wasi implementations. In
any case I'm curious what others think, is this too soon? Is there
something remaining blocking this? (etc)

@sunfishcode
Copy link
Copy Markdown
Member

It is occasionally nice to have as a point of comparison, however as WASI has evolved and wasi-common has matured, that's become less important.

It was the basis of the initial WASI web polyfill, but there's progress on compiling wasi-common into a web polyfill, and that's the way forward at any rate, so I don't think we need to keep wasi-c for that.

My other idea for it is that we could theoretically do differential fuzzing between wasi-c and wasi-common to flush out subtle semantics differences. But, as other WASI implementations such as node-wasi come online, maybe those would be more interesting anyway.

This commit deletes the old C implementation of the original
`wasi_unstable` module, instead only leaving around our single
`wasmtime-wasi` crate as the implementation for both
`wasi_snapshot_preview1` and `wasi_unstable`.

This hasn't been discussed (AFAIK) up until now, so this is also a
proposal! Some thoughts in favor of this deletion I would have are:

* This has been off-by-default for ages
* We don't build or test any of this on CI
* Published binaries with `wasmtime` do not have this possibility
  enabled
* Future refactorings to the `wasmtime-wasi` crate will either need to
  work around how the C implementation is different or bring it up to
  speed.

This is motivated by the last bullet point where I was working on
getting `wasmtime-wasi` working purely as an implementation detail on
top of the `wasmtime` crate itself, but quickly ran into a case where
the CLI would need to multiplex all sorts of wasi implementations. In
any case I'm curious what others think, is this too soon? Is there
something remaining blocking this? (etc)
@alexcrichton
Copy link
Copy Markdown
Member Author

Ah right, I forgot to call out the web polyfill! I think with the upcoming PRs though I believe that will be taken care of. Additionally we can't necessarily fuzz against the C implementation easily right now in the sense that it only implements wasi_unstable, whereas all new effort is going to wasi_snapshot_preview1, so it would require a lot of updating work to get the C API working with that as well.

@kubkon
Copy link
Copy Markdown
Member

kubkon commented Jan 22, 2020

LGTM! As far as the polyfill goes, I've already started sketching some stuff out in #720, but got stuck mainly on the problems with the Emscripten in rustc which, as far as I am aware, should be fixed in nightly now. Also, as you already suggested @alexcrichton, it'd be preferable to generate much of the stubs for JS from *.witx which, when I get time, I'll look into.

@alexcrichton
Copy link
Copy Markdown
Member Author

Do we think we want to wait for more thoughts? Or go ahead and merge this?

@sunfishcode
Copy link
Copy Markdown
Member

Let's go ahead and merge this.

@sunfishcode sunfishcode merged commit 21e0a99 into bytecodealliance:master Jan 24, 2020
@alexcrichton alexcrichton deleted the delete-c-code branch February 25, 2020 18:37
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