badeend requested elliottt for a review on PR #9276.
badeend requested wasmtime-core-reviewers for a review on PR #9276.
badeend opened PR #9276 from badeend:network-error-code
to bytecodealliance:main
:
Implements https://github.com/WebAssembly/wasi-sockets/pull/105
This function downcasts a stream error to a network-related error.
Similar to the existingfilesystem-error-code
andhttp-error-code
functions.Unit test is missing because I couldn't figure out a reliable way to fail a TCP stream mid-connection. Suggestions are welcome.
badeend requested wasmtime-default-reviewers for a review on PR #9276.
badeend updated PR #9276.
alexcrichton commented on PR #9276:
Thanks for this! Before I think too hard about a unit test though this PR is the unfortunate victim of "first proposal to add WITs with
@unstable
". As implemented this provides unrestricted access to the@unstable
version of the API which is not the intention of the annotation. The theory is that@unstable
would translate to some sort of-S
flag such as-Snetwork-error-code
or something like that. That would be there during development and then when@unstable
is turned to stable then-Snetwork-error-code
becomes a noop effectively.I say that this is an "unfortunate victim" because this infrastructure hasn't been built yet. Ideally what needs to happen I think is that the calls to
add_to_linker
all need to be parameterized by feature flags where the default is the empty set. So in a sense we need a set of flags for all WASI proposals which are passed during the original call toadd_to_linker
and then that set is built up on the CLI.From a technical perspective though this PR otherwise looks good to me. Would you be up for helping out with the infrastructure for plumbing flags through?
badeend commented on PR #9276:
I'll give it a try
badeend commented on PR #9276:
Do you have any preference on how to thread through the feature names down into the generated code? I have three suggestions so far:
- Add
component_model_unstable_features
setting towasmtime::Config
- Add
unstable_features
setting towasmtime::component::Linker
- Add
features
parameter to generatedadd_to_linker_*
methods and all layers below and above it.
alexcrichton commented on PR #9276:
I'd recommend the
add_to_linker_*
route and it would automatically be used whenfeatures
is specified in thebindgen!
macro (it's a WASI-specific or bindings-specific thing, less about engine state)
elliottt requested alexcrichton for a review on PR #9276.
badeend commented on PR #9276:
I'm attempting to only generate the new parameter on add_to_linker _if_ the bindgen config specifies unstable features. I'm running into a problem when the
with
option is used to forward to existing interfaces. For example:Within the bindgen macro I don't know for which of those interfaces I need to forward the
features
parameter to their respectiveadd_to_linker
call.One way to make it work is to simply unconditionally add the parameter to every generated add_to_linker function, regardless of wheter its needed or not. But that will be a backwards incompatible change.
Suggestions?
alexcrichton commented on PR #9276:
IIRC there are per-interface
add_to_linker
functions plus one large "add everything to the linker" function. I'd expect that each per-interface function has a flags/options struct as appropriate for the contents of the interface, and then the one large function would take the union of all the options of all the other interfaces. Would that be reasonable to implement?Even when
with
is used we could say that the detection of whether or not the flags are used is independent ofwith
configuration and if the flags type itself lived in the module generated it would be able to reuse the type thatwith
points to I think.
badeend commented on PR #9276:
If every interface defines its own fresh options type, then a bindgen! using
with
must generate a separate parameter for each "with"ed interface, right?So the example from above would produce
add_to_linker( linker: Linker, clocks_options: crate::bindings::clocks::Options, random_options: crate::bindings::random::Options, cli_options: crate::bindings::cli::Options, etc... )
alexcrichton commented on PR #9276:
I might bikeshed to a
struct WorldOptions { /* all the fields here */ }
to avoid needing to remember what order everything is in, but otherwise yeah the top-level function would take all the flags for sure
badeend commented on PR #9276:
For the world that could work. But I'm specifically concerned here about the
with
-included interfaces, which come from a different code generation and don't share the options type of the currently generating world type.
alexcrichton commented on PR #9276:
Could you detail a bit more how
with
complicates this? From my perspective there's no extra complications, so I'm not sure what you're meaning. Thebindgen!
usingwith
still has full WIT type information so should be able to know, even without knowing what was generated, that some of the interfaces on the other end are unstable-gated. That means that it should know that there's anOptions
type already generated and thewith
says how to go find that type. Given that and the fact thatbindgen!
would always generate a fresh the-whole-world's-options-type it seems naively to me like it would work, but I fear I'm missing something.One possible piece perhaps is that I'm thinking now that Wasmtime might want to unconditionally always use
all_features
and remove that macro option entirely. It would then always generate theOptions
structs for unstable-gated interfaces no matter what, sofeatures
/all_features
in the macro config would go away.
badeend commented on PR #9276:
The bindgen! using
with
still has full WIT type information so should be able to know, even without knowing what was generated, that some of the interfaces on the other end are unstable-gated.You're right. I've got it working in: https://github.com/bytecodealliance/wasmtime/pull/9381
badeend updated PR #9276.
badeend commented on PR #9276:
I've updated the PR with the latest main
alexcrichton submitted PR review:
Thanks!
alexcrichton merged PR #9276.
Last updated: Dec 23 2024 at 12:05 UTC