Stream: git-wasmtime

Topic: wasmtime / PR #8950 Implement wasi-runtime-config

view this post on Zulip Wasmtime GitHub notifications bot (Jul 13 2024 at 06:21):

iawia002 opened PR #8950 from iawia002:wasi-runtime-config to bytecodealliance:main:

Part of https://github.com/bytecodealliance/wasmtime/issues/8929, the integration of wasmtime-cli will be implemented in the next separate PR.

close https://github.com/bytecodealliance/wasmtime/pull/8939
close https://github.com/bytecodealliance/wasmtime/pull/8928

cc @alexcrichton

view this post on Zulip Wasmtime GitHub notifications bot (Jul 13 2024 at 06:21):

iawia002 requested wasmtime-core-reviewers for a review on PR #8950.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 13 2024 at 06:21):

iawia002 requested fitzgen for a review on PR #8950.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 13 2024 at 06:21):

iawia002 requested wasmtime-default-reviewers for a review on PR #8950.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 13 2024 at 06:38):

iawia002 updated PR #8950.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 13 2024 at 07:03):

iawia002 updated PR #8950.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2024 at 19:11):

alexcrichton submitted PR review:

Thanks! This all looks good to me.

Could this also get hooked up to the CLI as well? Under perhaps -Sruntime-config and/or some other -S flags for the actual keys themselves?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2024 at 19:11):

alexcrichton submitted PR review:

Thanks! This all looks good to me.

Could this also get hooked up to the CLI as well? Under perhaps -Sruntime-config and/or some other -S flags for the actual keys themselves?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2024 at 19:11):

alexcrichton created PR review comment:

Could this file also have an assert to ensure that there's a test for all runtime_config tests? Something along the lines of:

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2024 at 19:13):

fitzgen submitted PR review:

Thanks! A bunch of stylistic/idiomatic/ergonomic suggestions and nitpicks below, but this looks good, and we should be able to land it shortly after those things are addressed.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2024 at 19:13):

fitzgen submitted PR review:

Thanks! A bunch of stylistic/idiomatic/ergonomic suggestions and nitpicks below, but this looks good, and we should be able to land it shortly after those things are addressed.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2024 at 19:13):

fitzgen created PR review 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?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2024 at 19:13):

fitzgen created PR review comment:

I think it would be more ergonomic and idiomatic to turn this function into a FromIterator trait implementation, derive the Default trait for this type, make new return the default, and also add a fn define(&mut self, key: impl Into<String>, value: impl Into<String>) -> &mut Self builder-style method.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2024 at 19:13):

fitzgen created PR review comment:

Can you add a top-level doc comment that describes what this crate does, links to the associated WASI proposal, and provides an example of how to use the crate?

Additionally, can you deny missing documentation for this crate?

//! top-level docs go here...

#![deny(missing_docs)]

use anyhow::Result;

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2024 at 19:13):

fitzgen created PR review comment:

And then also rename this to WasiRuntimeConfig.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2024 at 19:13):

fitzgen created PR review comment:

Soft preference to rename this to WasiRuntimeConfigVariables.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2024 at 19:13):

fitzgen created PR review 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>.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2024 at 19:13):

fitzgen created PR review comment:

And it would also be nice, for convenience, to

impl<'a> From<&'a WasiRuntimeConfigVariables> for WasiRuntimeConfig<'a> {
    // ...
}

You might also want to consider

impl WasiRuntimeConfig<'_> {
    /// Create a new, empty runtime configuration.
    pub fn empty() -> WasiRuntimeConfig<'static> {
        static EMPTY_VARS: WasiRuntimeConfigVariables { /* ... */ };
        Self::new(&EMPTY_VARS)
    }
}

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2024 at 19:13):

fitzgen created PR review comment:

Might as well make this as portable as we can while we are here:

#!/usr/bin/env bash

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2024 at 07:25):

iawia002 updated PR #8950.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2024 at 07:29):

iawia002 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2024 at 07:29):

iawia002 created PR review 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2024 at 07:34):

iawia002 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2024 at 07:34):

iawia002 created PR review 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?

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.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2024 at 07:35):

iawia002 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2024 at 07:43):

iawia002 commented on PR #8950:

Could this also get hooked up to the CLI as well? Under perhaps -Sruntime-config and/or some other -S flags for the actual keys themselves?

I made some changes based on all the comments except this one, because I plan to add support for the wasi-runtime-config API to wasmtime-cli in a separate PR. Otherwise, this PR would have too many changes, let's focus on the wasi-runtime-config implementation only in this PR.

And I think all comments have been addressed, PTAL.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2024 at 15:00):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2024 at 15:00):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2024 at 15:00):

alexcrichton created PR review 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2024 at 15:00):

alexcrichton created PR review 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2024 at 01:50):

iawia002 updated PR #8950.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2024 at 01:58):

iawia002 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2024 at 01:58):

iawia002 created PR review comment:

Yeah, you are right, I did not understand it correctly last time. I made some changes (https://github.com/bytecodealliance/wasmtime/pull/8950/commits/99b6a113c6a5e13ce3d1a016792b32948ae7d461) and returned to the original WasiRuntimeConfigVariables + WasiRuntimeConfig style. Hope I have understood it correctly this time.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2024 at 15:01):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2024 at 15:17):

alexcrichton merged PR #8950.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2024 at 16:31):

fitzgen commented on PR #8950:

Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Jul 18 2024 at 01:08):

iawia002 commented on PR #8950:

Thanks for the review!

Last updated: Nov 22 2024 at 16:03 UTC