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 topreview2::WasiCtxBuilder
across those versions was just a change to the clock setters, and in his diff he eliminated use ofmem::swap
and it appears to be uniform with the preview1 implementation.
jameysharp commented on issue #6770:
@pchickey, do you want to take the review on this PR?
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?
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.
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!
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 ofmem::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.
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.
alexcrichton commented on issue #6770:
Ok I'll reopen but will work on getting the preview1 bits updated.
alexcrichton commented on issue #6770:
Ok I've additionally renamed methods on preview2 builders to match preview1 builder (e.g.
args
instead ofset_args
). I've additionally updated the semantics slightly where nowargs
will append all its arguments instead of overwriting to match the behavior of preview1 and other builders likeCommand
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: Dec 23 2024 at 12:05 UTC