Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change wasmtime serve to apply -S common by default #8086

Open
tschneidereit opened this issue Mar 12, 2024 · 8 comments
Open

Change wasmtime serve to apply -S common by default #8086

tschneidereit opened this issue Mar 12, 2024 · 8 comments

Comments

@tschneidereit
Copy link
Member

People keep running into problems with wasmtime serve because by default it only exposes wasi:proxy, nothing else. I think we should change the behavior here to imply -S common to make default toolchains and in particular the normal Preview1 adapter work.

One could see it as a downside that wasmtime serve would be a bit less specifically about HTTP, but I can't really come up with compelling arguments for why that'd be a problem.

@alexcrichton
Copy link
Member

The reason this wasn't done by default originally was because it was seen as desirable to have wasmtime serve by-default work with the wasi:http/proxy world. By enabling -S common by default it means that, I suspect, no provider works with wasi:http/proxy by default.

Given that I'd personally see two possible routes from here:

  1. Improve tooling/errors/etc to better guide developers towards success through the proxy adapter. Unsure what this would mean for various tools and such, but there's probably at least something to improve here.
  2. See this as rationale and pushback for having such a small wasi:http/proxy world and add more imports there (e.g. -S common)

If possible I think it'd be good to hold the line where wasmtime serve matches wasi:http/proxy by default and the problems with the adapter serve as motivation for rejiggering something else in the system. (although I'm not necessarily 100% opposed to just turning on -S common, I mostly think it'd be good to try to keep this line if we can, but not at all costs)

@tschneidereit
Copy link
Member Author

I agree that in principle it'd be good to keep this minimal. I'm not convinced that we'll be able to provide a developer experience here that'll not have people continue to run into roadblocks immediately. Yes, we can make those roadblocks easier to understand (and should, independently, of this particular issue!)

I think there are two things making this challenging to address: one is that perhaps the reality is that most use cases really do want more of WASI for running a server than is currently contained in wasi:http/proxy. The other though is that most tooling still uses the WASIp1 adapter, and that that adapter just pulls in more requirements than we provide. If there's a way to address the latter somehow, that'd go a long way to a solution. I do not think that forcing people to understand the concept of adapters and making them choose between multiple is a good answer, fwiw.

If we can't solve this, then that to me indicates that we should make wasmtime serve work with the adapter out of the box, independently from our stance on what should and shouldn't be in wasi:http/proxy.

@alexcrichton
Copy link
Member

To clarify though I'm not arguing for minimality, I agree that we should turn -S common on by default. This was discussed a bit while ago as well for some added context.

My point is that adding -S common by default is, in my opinion, not the best place to fix this. If we did that I think it's admitting that when facing reality the proxy world is too small, and I'd prefer if the wasi:http/proxy world itself admitted to said reality and added more interfaces.

Or perhaps put another way, can you expand a bit more on why you think it's ok wasmtime serve and wasi:http/proxy diverge? That's what I'm trying to keep aligned, but it sounds like you think it's ok if they diverge.

@tschneidereit
Copy link
Member Author

Or perhaps put another way, can you expand a bit more on why you think it's ok wasmtime serve and wasi:http/proxy diverge? That's what I'm trying to keep aligned, but it sounds like you think it's ok if they diverge.

Yeah, that is a great question, and I agree that it needs an answer if we want to change anything here (that doesn't involve changes to wasi:http/proxy.

My take is twofold: one part is very pragmatically that, as mentioned above, it's very difficult right now to target a pure wasi:http/proxy environment. If we can truly address that in a reasonable timeframe, I'm all for it. I don't really know what that'd look like, though.

The second part is that serve indicates something more than proxy does. In fact, I think we should also add the ability to map directories to preopens, as for wasmtime run.

I guess more generally I'm in this case not too concerned about the availability of more functionality leading developers to expect those to be available in all environments implementing wasi:http/proxy. That's an argument based on intuition of course, so it might well be wrong.

@alexcrichton
Copy link
Member

Reading over WebAssembly/wasi-http#79 again I think I'm probably the only one advocating for wasi:http/proxy and wasmtime serve being the same thing, and I additionally don't disagree with anything you say. I'll send a PR to update this.

alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Mar 14, 2024
This commit enables more WASI interfaces by default with `wasmtime
serve`, notably through the use of the `-Scommon` flag. This means that
the interfaces supported by `wasmtime serve` are the same as `wasmtime
run` by default.

To fully implement this there's a number of refactorings here:

* More flags like `--dir` and `--env` are moved into `RunCommon` to be
  shared between `wasmtime serve` and `wasmtime run`, meaning that the
  `serve` command can now configure environment variables.

* A small test has been added as well as infrastructure for running
  tests with `wasmtime serve` itself. Previously there were no tests
  that executed `wasmtime serve`.

* The `test_programs` crate had a small refactoring to avoid
  double-generation of http bindings.

Closes bytecodealliance#8086
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Mar 14, 2024
This commit enables more WASI interfaces by default with `wasmtime
serve`, notably through the use of the `-Scommon` flag. This means that
the interfaces supported by `wasmtime serve` are the same as `wasmtime
run` by default.

To fully implement this there's a number of refactorings here:

* More flags like `--dir` and `--env` are moved into `RunCommon` to be
  shared between `wasmtime serve` and `wasmtime run`, meaning that the
  `serve` command can now configure environment variables.

* A small test has been added as well as infrastructure for running
  tests with `wasmtime serve` itself. Previously there were no tests
  that executed `wasmtime serve`.

* The `test_programs` crate had a small refactoring to avoid
  double-generation of http bindings.

Closes bytecodealliance#8086
@sunfishcode
Copy link
Member

Thinking about this more, it seems desirable to avoid a situation where filesystems/sockets come to be seen as "common" functionality that's implicitly included in everything.

One potential problem here is that -S common is confusingly named. My understanding is that the name "common" came from "wasi-common" which came from having code in common between wasmtime and lucet. But it sounds like "common functionality". I've now submitted #8166 to rename it.

Another potential problem is that -S common might be too coarse-grained. Perhaps we should split it up so that users that just need environment variables don't need to pull in the rest.

Even with the rename, and even if we provide finer-grained flags, there still a question of, what do we want the proxy world to be? And, if we start bundling files/sockets/etc. with it by default, will it be harder in the future t disable it by default again, if we improve the ergonomics and want to start promoting platforms based on the proxy world?

What if we added a diagnostic to wasmtime for when a program has unsatisfied imports that suggests users add -S flags that would satisfy their imports? Would that be enough, to justify keeping wasmtime serve the same as wasi:http/proxy? There'd still be a papercut when users get the error, and have to add the flag. But at least it'd be clear to users what they need to do.

@tschneidereit
Copy link
Member Author

What if we added a diagnostic to wasmtime for when a program has unsatisfied imports that suggests users add -S flags that would satisfy their imports? Would that be enough, to justify keeping wasmtime serve the same as wasi:http/proxy? There'd still be a papercut when users get the error, and have to add the flag. But at least it'd be clear to users what they need to do.

I think having that would to some degree alleviate my concerns here. I'm not sure it would have all that much value though: I'm skeptical that users would take much more away from this than "oh ok, I guess the way to run these things in the Wasmtime CLI involves adding -S [something].

I also don't really think that wasmtime serve is the right place to teach people to target wasi:http/proxy: there are lots of use cases for running a server that have nothing to do with an L7 proxy.

@sunfishcode
Copy link
Member

For example, on crates.io dependencies can include their own wit-bindgen bindings, which then get silently added to the implied world of the final component. It's possible for developers to pick up additional component-model-level dependencies without being aware of it.

If we want to have small and simple works like wasi-http's proxy, but also things like wasi-cloud-core's http-service and more, and not have filesystems and sockets and environment variables become de-facto dependencies in all the places, where's the time and place to push back?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants