Skip to content

feat: add support for stream with no <T>#1130

Merged
dicej merged 2 commits intobytecodealliance:mainfrom
rvolosatovs:feat/stream-unit
Jan 22, 2025
Merged

feat: add support for stream with no <T>#1130
dicej merged 2 commits intobytecodealliance:mainfrom
rvolosatovs:feat/stream-unit

Conversation

@rvolosatovs
Copy link
Copy Markdown
Member

@rvolosatovs rvolosatovs commented Jan 17, 2025

@rvolosatovs rvolosatovs mentioned this pull request Jan 17, 2025
Closed
@rvolosatovs rvolosatovs marked this pull request as ready for review January 17, 2025 16:05
@rvolosatovs
Copy link
Copy Markdown
Member Author

rvolosatovs commented Jan 17, 2025

I'm not sure what the policy on pulling in bleeding-edge changes from https://github.com/bytecodealliance/wasm-tools is, I feel like I'll probably have to wait for a new release, but I'm also not sure how soon that would happen, so marking the PR ready for review with the git deps

@alexcrichton
Copy link
Copy Markdown
Member

alexcrichton commented Jan 17, 2025

Ah yes to merge this it'll need to be a crates.io-based dependency. There's some documentation on releases for wasm-tools but the tl;dr; is that it's on-demand.

I personally prefer to spread out releases where possible, so if it's ok I'd prefer that this were integrated into other in-development systems first to make sure it works and then once it's reasonably confident no major changes are needed here the cranks can be turned.

@dicej
Copy link
Copy Markdown
Collaborator

dicej commented Jan 17, 2025

@rvolosatovs I'm about to update my Wasmtime async branch to use both wasm-tools and wit-bindgen main branches to pull in other fixes, so we can use that branch as a staging area to make sure everything works together before we cut crates.io releases.

@dicej
Copy link
Copy Markdown
Collaborator

dicej commented Jan 17, 2025

@rvolosatovs I'm about to update my Wasmtime async branch to use both wasm-tools and wit-bindgen main branches to pull in other fixes, so we can use that branch as a staging area to make sure everything works together before we cut crates.io releases.

Actually, I'll use the wasm-tools main branch plus your wit-bindgen branch, since I'll need these changes to line up with the wasm-tools changes.

@dicej
Copy link
Copy Markdown
Collaborator

dicej commented Jan 17, 2025

@rvolosatovs I'm about to update my Wasmtime async branch to use both wasm-tools and wit-bindgen main branches to pull in other fixes, so we can use that branch as a staging area to make sure everything works together before we cut crates.io releases.

Actually, I'll use the wasm-tools main branch plus your wit-bindgen branch, since I'll need these changes to line up with the wasm-tools changes.

Actually, actually, I used the feat/stream-unit branch on my fork, which rebases your changes onto main so as to include #1129. And I've updated my async branch to support unit streams.

dicej added a commit to dicej/wasmtime that referenced this pull request Jan 17, 2025
@dicej
async/stream/future support for wasmtime-wit-bindgen
b52eddd 
I've split this out of bytecodealliance#9582 to make review easier.

This patch adds async/stream/future/error-context support to the host binding
generator, along with placeholder type and function definitions in the
`wasmtime` crate which the generated bindings can refer to.  See
https://github.com/dicej/rfcs/blob/component-async/accepted/component-model-async.md#componentbindgen-updates
for the design and rationale.

Note that I've added temporary `[patch.crates-io]` overrides in Cargo.toml until
bytecodealliance/wit-bindgen#1130 and
bytecodealliance/wasm-tools#1978 have been released.

Also note that we emit a `T: 'static` bound for `AsContextMut<Data = T>` when
generating bindings with `concurrent_imports: true`.  This is only because
`rustc` insists that the closure we're passing to
`LinkerInstance::func_wrap_concurrent` captures the lifetime of `T` despite my
best efforts to convince it otherwise.  Alex and I suspect this is a limitation
in the compiler, and I asked about it on the rust-lang Zulip, but we haven't
been able to determine a workaround so far.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@dicej dicej mentioned this pull request Jan 17, 2025
Merged
@rvolosatovs @dicej
feat: add support for stream with no <T>
43ba064 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

test: return `future` in future return test WIT

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

build: update wasm-tools

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

avoid divide-by-zero in `StreamReader::poll_next`

I hit this when testing "unit streams" (i.e. streams with no payload type)
end-to-end.

BTW, I can't wait to get some runtime tests going in this repo so we can catch
this kind of thing in CI.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@dicej
update to latest wasm-tools
e8c5732 
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@dicej dicej enabled auto-merge January 22, 2025 00:29
@dicej dicej added this pull request to the merge queue Jan 22, 2025
Merged via the queue into bytecodealliance:main with commit 9808631 Jan 22, 2025
dicej added a commit to dicej/wasmtime that referenced this pull request Jan 22, 2025
@dicej
async/stream/future support for wasmtime-wit-bindgen
f04b932 
I've split this out of bytecodealliance#9582 to make review easier.

This patch adds async/stream/future/error-context support to the host binding
generator, along with placeholder type and function definitions in the
`wasmtime` crate which the generated bindings can refer to.  See
https://github.com/dicej/rfcs/blob/component-async/accepted/component-model-async.md#componentbindgen-updates
for the design and rationale.

Note that I've added temporary `[patch.crates-io]` overrides in Cargo.toml until
bytecodealliance/wit-bindgen#1130 and
bytecodealliance/wasm-tools#1978 have been released.

Also note that we emit a `T: 'static` bound for `AsContextMut<Data = T>` when
generating bindings with `concurrent_imports: true`.  This is only because
`rustc` insists that the closure we're passing to
`LinkerInstance::func_wrap_concurrent` captures the lifetime of `T` despite my
best efforts to convince it otherwise.  Alex and I suspect this is a limitation
in the compiler, and I asked about it on the rust-lang Zulip, but we haven't
been able to determine a workaround so far.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

remove obsolete TODO comment

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

make `futures` dep optional

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

update `wasm-tools` and `wit-bindgen`

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
github-merge-queue bot pushed a commit to bytecodealliance/wasmtime that referenced this pull request Jan 22, 2025
@dicej
async/stream/future support for wasmtime-wit-bindgen (#10044)
f6aee6f 
I've split this out of #9582 to make review easier.

This patch adds async/stream/future/error-context support to the host binding
generator, along with placeholder type and function definitions in the
`wasmtime` crate which the generated bindings can refer to.  See
https://github.com/dicej/rfcs/blob/component-async/accepted/component-model-async.md#componentbindgen-updates
for the design and rationale.

Note that I've added temporary `[patch.crates-io]` overrides in Cargo.toml until
bytecodealliance/wit-bindgen#1130 and
bytecodealliance/wasm-tools#1978 have been released.

Also note that we emit a `T: 'static` bound for `AsContextMut<Data = T>` when
generating bindings with `concurrent_imports: true`.  This is only because
`rustc` insists that the closure we're passing to
`LinkerInstance::func_wrap_concurrent` captures the lifetime of `T` despite my
best efforts to convince it otherwise.  Alex and I suspect this is a limitation
in the compiler, and I asked about it on the rust-lang Zulip, but we haven't
been able to determine a workaround so far.



remove obsolete TODO comment



make `futures` dep optional



update `wasm-tools` and `wit-bindgen`

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
github-merge-queue bot pushed a commit to bytecodealliance/wasmtime that referenced this pull request Jan 22, 2025
@dicej
async/stream/future support for wasmtime-wit-bindgen (#10044)
636435f 
* async/stream/future support for wasmtime-wit-bindgen

I've split this out of #9582 to make review easier.

This patch adds async/stream/future/error-context support to the host binding
generator, along with placeholder type and function definitions in the
`wasmtime` crate which the generated bindings can refer to.  See
https://github.com/dicej/rfcs/blob/component-async/accepted/component-model-async.md#componentbindgen-updates
for the design and rationale.

Note that I've added temporary `[patch.crates-io]` overrides in Cargo.toml until
bytecodealliance/wit-bindgen#1130 and
bytecodealliance/wasm-tools#1978 have been released.

Also note that we emit a `T: 'static` bound for `AsContextMut<Data = T>` when
generating bindings with `concurrent_imports: true`.  This is only because
`rustc` insists that the closure we're passing to
`LinkerInstance::func_wrap_concurrent` captures the lifetime of `T` despite my
best efforts to convince it otherwise.  Alex and I suspect this is a limitation
in the compiler, and I asked about it on the rust-lang Zulip, but we haven't
been able to determine a workaround so far.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

remove obsolete TODO comment

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

make `futures` dep optional

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

update `wasm-tools` and `wit-bindgen`

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

* run cargo vet

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

---------

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dicej dicej dicej approved these changes

Assignees

@rvolosatovs rvolosatovs

Labels

None yet

Projects

Ship WASIp3
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rvolosatovs @alexcrichton @dicej