Change preview2 builder methods to use &mut self

[alexcrichton]

This commit changes the WasiCtxBuilder for preview2 to use a builder pattern more similar to std::process::Command where methods take &mut self and return &mut Self instead of taking self and returning Self. This pattern enables more easily building up configuration over time throughout code where ownership transfer might otherwise be awkward.

A small caveat to this is that the ergonomics of this pattern only really work out well if the final "build" method takes &mut self as well. In this situation it's difficult to try to figure out what's supposed to happen if this method is called twice, so I left it to panic for now so we can more easily update it in the future possibly.

I'll also note that the original motivations for this come from the upgrade at fermyon/spin#1678

[pchickey]

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.

[jameysharp]

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

[alexcrichton]

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]

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]

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

[tschneidereit]

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.

[pchickey]

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]

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

[alexcrichton]

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

[github-actions]

Subscribe to Label Action

cc @peterhuene

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 .github/subscribe-to-label.json configuration file.

Learn more.