Implement wasi-runtime-config #8950
Conversation
iawia002
force-pushed
the
wasi-runtime-config
branch
2 times, most recently
from
db8c1a4 to
81842a5
Compare
| /// Capture the state necessary for use in the `wasi-runtime-config` API implementation. | ||
| pub struct WasiRuntimeConfigCtx { | ||
| runtime_config: HashMap<String, String>, | ||
| } | ||
|
|
||
| impl WasiRuntimeConfigCtx { | ||
| /// Create a new context. | ||
| pub fn new<S, I>(config: I) -> Self | ||
| where | ||
| S: Into<String>, | ||
| I: IntoIterator<Item = (S, S)>, | ||
| { | ||
| Self { | ||
| runtime_config: config | ||
| .into_iter() | ||
| .map(|(k, v)| (k.into(), v.into())) | ||
| .collect(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// A wrapper capturing the needed internal `wasi-runtime-config` state. | ||
| pub struct WasiRuntimeConfigView<'a> { | ||
| ctx: &'a WasiRuntimeConfigCtx, | ||
| } | ||
|
|
||
| impl<'a> WasiRuntimeConfigView<'a> { | ||
| /// Create a new view into the `wasi-runtime-config` state. | ||
| pub fn new(ctx: &'a WasiRuntimeConfigCtx) -> Self { | ||
| Self { ctx } | ||
| } | ||
| } |
There was a problem hiding this comment.
Is the idea here to separate the configured variables from the per-Store state, so that the variables can be reused across many Stores?
Can you document that fact?
There was a problem hiding this comment.
Is the idea here to separate the configured variables from the per-
Storestate, so that the variables can be reused across manyStores?
The idea here is very simple: it's just passing some configuration variables from the runtime to the component. I did add some documentation, but it is not described this way. Please check if the current description is reasonable. Feel free to add more comments.
| /// Add all the `wasi-runtime-config` world's interfaces to a [`wasmtime::component::Linker`]. | ||
| pub fn add_to_linker<T>( | ||
| l: &mut wasmtime::component::Linker<T>, | ||
| f: impl Fn(&mut T) -> WasiRuntimeConfigView + Send + Sync + Copy + 'static, |
There was a problem hiding this comment.
Isn't this ... + 'static bound going to mean that you can effectively only use WasiRuntimeConfigView<'static>?
If so -- and if the 'static bound is necessary, which I think it is -- then instead of using lifetimes and borrows between WasiRuntimeConfigVariables and WasiRuntimeConfig, we should make WasiRuntimeConfig a newtype of Arc<WasiRuntimeConfigVariables>.
There was a problem hiding this comment.
The 'static bound is necessary, but this made me think further, since the wasi-runtime-config API is very simple, so I reduced some excessive encapsulation by removing WasiRuntimeConfigVariables/WasiRuntimeConfigCtx and keeping only WasiRuntimeConfig. Now, this problem should no longer exists.
There was a problem hiding this comment.
The + 'static doesn't require <'static> in this case since the + 'static only applies to the lifetime of the captures of the closure itself. This I think was the desired signature which allows borrowing the config without cloning it on import calls.
iawia002
force-pushed
the
wasi-runtime-config
branch
from
81842a5 to
d68819a
Compare
I made some changes based on all the comments except this one, because I plan to add support for the And I think all comments have been addressed, PTAL. |
| /// Add all the `wasi-runtime-config` world's interfaces to a [`wasmtime::component::Linker`]. | ||
| pub fn add_to_linker<T>( | ||
| l: &mut wasmtime::component::Linker<T>, | ||
| f: impl Fn(&mut T) -> WasiRuntimeConfig + Send + Sync + Copy + 'static, |
There was a problem hiding this comment.
I don't think that this is the signature that you want because it would imply that every time an import is called it would .clone() the entire WasiRuntimeConfig structure which could be pretty expensive.
There was a problem hiding this comment.
Yeah, you are right, I did not understand it correctly last time. I made some changes (99b6a11) and returned to the original WasiRuntimeConfigVariables + WasiRuntimeConfig style. Hope I have understood it correctly this time.
| /// Add all the `wasi-runtime-config` world's interfaces to a [`wasmtime::component::Linker`]. | ||
| pub fn add_to_linker<T>( | ||
| l: &mut wasmtime::component::Linker<T>, | ||
| f: impl Fn(&mut T) -> WasiRuntimeConfigView + Send + Sync + Copy + 'static, |
There was a problem hiding this comment.
The + 'static doesn't require <'static> in this case since the + 'static only applies to the lifetime of the captures of the closure itself. This I think was the desired signature which allows borrowing the config without cloning it on import calls.
|
Thanks! |
|
Thanks for the review! |
Part of #8929, the integration of
wasmtime-cliwill be implemented in the next separate PR.close #8939
close #8928
cc @alexcrichton