Stream: git-wasmtime

Topic: wasmtime / issue #6770 Change preview2 builder methods to...

Wasmtime GitHub notifications bot (Jul 25 2023 at 15:33):

pchickey commented on issue #6770:

I used the same builder design pattern as the previous generation: wasi-cap-std-sync and wasi-tokio WasiCtxBuilder. The idea was that this would make it easiest for folks to upgrade. From the diff here https://github.com/fermyon/spin/pull/1678/files I'm not sure what @tschneidereit found annoying - the changes to preview2::WasiCtxBuilder across those versions was just a change to the clock setters, and in his diff he eliminated use of mem::swap and it appears to be uniform with the preview1 implementation.

Wasmtime GitHub notifications bot (Jul 25 2023 at 15:54):

jameysharp commented on issue #6770:

@pchickey, do you want to take the review on this PR?

Wasmtime GitHub notifications bot (Jul 25 2023 at 16:25):

alexcrichton commented on issue #6770:

Ah thanks for checking @pchickey, it looks like the issue is:

enum WasiCtxBuilder {
    Preview1(wasi_preview1::WasiCtx),
    Preview2(wasi_preview2::WasiCtxBuilder),
}

where one's using a builder and one isn't. If both used a builder then it'd have the same pattern, which I believe would resolve the original motivation for this PR.

That being said though I personally prefer &mut self-style builders anyway, so if you agree that it'd be ok to move I'll update the other builders I missed here in the cap-std-sync and tokio crates?

Wasmtime GitHub notifications bot (Jul 25 2023 at 16:54):

pchickey commented on issue #6770:

Understood. Yes, we got rid of all the public &mut self methods on preview 2 WasiCtx that could substitute for the builder, mostly because of interactions with the table. So, now the WasiCtxBuilder is the only way to set things up.

We can switch all the builders to &mut self if you think that is better ergonomics. I dont particularly love that .build() is &mut self, because unlike in the Command case the builder has taken ownership of various resources and I liked to see .build() transferring that ownership to the WasiCtx (in addition to making it impossible to build it twice). But at the end of the day its just syntax, so whatever.

Wasmtime GitHub notifications bot (Jul 25 2023 at 20:31):

alexcrichton commented on issue #6770:

Ok let's stick with the move-style builder and see how far it gets us, can always revisit later!

Wasmtime GitHub notifications bot (Jul 26 2023 at 09:41):

tschneidereit commented on issue #6770:

I'm not sure what @tschneidereit found annoying - the changes to preview2::WasiCtxBuilder across those versions was just a change to the clock setters, and in his diff he eliminated use of mem::swap and it appears to be uniform with the preview1 implementation.

The annoying part is having to extract the dummy ctx builder instance, and then use one more lambda to use it. It's not the end of the world, but it is somewhat unwieldy. And I think it wouldn't become too much simpler even if preview1 weren't in the mix, either.

My main concern isn't so much our use in Spin specifically, but more generally that this seems plausible to be hit by embedders more generally.

Wasmtime GitHub notifications bot (Jul 26 2023 at 14:38):

pchickey commented on issue #6770:

Ok. I'm fine with changing it to a &mut Self style builder. It would be nice to keep preview 1 consistent but also its not that important.

Wasmtime GitHub notifications bot (Jul 26 2023 at 15:58):

alexcrichton commented on issue #6770:

Ok I'll reopen but will work on getting the preview1 bits updated.

Wasmtime GitHub notifications bot (Jul 26 2023 at 16:39):

alexcrichton commented on issue #6770:

Ok I've additionally renamed methods on preview2 builders to match preview1 builder (e.g. args instead of set_args). I've additionally updated the semantics slightly where now args will append all its arguments instead of overwriting to match the behavior of preview1 and other builders like Command

Wasmtime GitHub notifications bot (Aug 02 2023 at 12:49):

github-actions[bot] commented on issue #6770:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasi", "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

• peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Last updated: Nov 22 2024 at 16:03 UTC