Stream: git-wasmtime

Topic: wasmtime / issue #6737 Require wasmtime options are first...

view this post on Zulip Wasmtime GitHub notifications bot (Aug 04 2023 at 22:38):

michelledaviest commented on issue #6737:

Hi! When you run wasmtime run --help, it still says that the way to pass arguments into the WebAssembly file you're executing is using the -- flag, ie, wasmtime run [OPTIONS] <MODULE> [--] [ARGS].... Could we change this to remove the -- from the help message?

For reference: Currently when you run wasmtime run foo.wasm -- --arg (which used to work a few weeks ago), wasmtime reports that it can't find --arg, which is super confusing behavior and it took me a long time to debug this and trace it back to this commit.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 05 2023 at 15:38):

alexcrichton commented on issue #6737:

I think you might be using different versions of wasmtime perhaps? Currently I see:

$ cargo run run -h
...
Usage: wasmtime run [OPTIONS] <WASM>...
...

whereas with 11.0.1 I see:

$ wasmtime run -h
...
USAGE:
    wasmtime run [OPTIONS] <MODULE> [--] [ARGS]...
...

so I think that with 11.0.1 the help message is accurate and with main the help message is also accurate. The main difference is that the CLI changed with this PR, which was the purpose of this PR?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2023 at 17:30):

cfallin commented on issue #6737:

As a quick data point, this change also tripped up @ssunkin-fastly just now while she was re-running some things after rebasing her summer internship branch on top of current main. It indeed does what it says on the tin; but if both of my interns independently tripped over it and burned a few hours trying to work things out, other CLI users might as well. I wonder if we should think a little bit about whether we can include some more prominent notice, or figure out some sort of migration strategy?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2023 at 18:16):

alexcrichton commented on issue #6737:

Do y'all have a place in mind for where this could be mentioned or how this could be migrated to? I do realize it's confusing coming from prior versions so I at least wanted to make sure it's in the release notes for 12.0.0 but I'm not sure how else best to mitigate this

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2023 at 19:12):

cfallin commented on issue #6737:

Yeah, I've thought about this for a bit and I'm not comping up with a good intermediate point or "you might have done something wrong" warning trigger condition. The issue I guess is that in the new design, we strictly pass through every arg past the wasm; looking at those args for wasmtime-like things or a -- or whatnot and heuristically emitting a warning would be incorrect. Maybe I'm missing someway we could detect this though -- anyone else have thoughts?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2023 at 19:15):

cfallin commented on issue #6737:

I guess an alternative would be to simply not do this -- if we believe that the details of the old syntax are hardcoded in enough places (the shell histories of two people above for sure, maybe assorted scripts that others have), and the long-tail cost of silent breakage is high, then maybe we should revert and consider the interface frozen. (I'll note that we haven't released this change yet, so it's "not too late".) In the end that's a value judgment though; the only objective data we have is "cost a half-day of debugging total" shrug

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2023 at 19:24):

cfallin commented on issue #6737:

One other option from conversation with @iximeow just now: Wasmtime could scan the WASI program's args for options that are also valid Wasmtime args, and warn, with a warning that will eventually be removed. Then add a --literal-args or --disable-arg-warning or something to silence this warning. In some future version remove both and make the current (args are just literals, no warnings) behavior the default.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2023 at 19:36):

alexcrichton commented on issue #6737:

One option would be to include both CLI parsers and compare the results of parsing. If both the old and the new CLI parser successfully parse but produce different results then a warning could be issued. I don't know how to implement that in a way that's maintainable though (aka not duplicating everything and a giant impl PartialEq<OldOpts> for NewOpts which is brittle)

Personally I'm not too much of a fan of considering things frozen. That was a conscious decision on our part to have an ever-increasing major version number to signal that while we'll do our best to not change everything all the time we still reserve the right to do so. I think it's ok to back this out and reconsider how best to land it, but I don't want to be in a state where we're afraid of landing changes. Additionally I would prefer to avoid a situation where we spend most of our time figuring out how to land changes rather than actually landing changes.

I certainly don't mean to discount the confusion and frustration though. My personal take on topics like this is typically pretty biased towards the maintenance-of-the-tool aspect since that's what I do primarily, so I know I don't do a great job of always anticipating all the fallout of changes like this.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2023 at 21:01):

cfallin commented on issue #6737:

Would it make sense to talk about this at the next Wasmtime biweekly (next week)? I'm also sympathetic to the tool-maintainer "we need to move the design forward" aspect too, FWIW. Would be curious to hear what others think.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2023 at 21:25):

alexcrichton commented on issue #6737:

Sounds good to me!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2023 at 21:30):

michelledaviest commented on issue #6737:

How about if you pass in --, wasmtime spits out a custom error that says something along the lines of "Things have changed. This is what the new behavior is"? I think it's fine that the old version of passing arguments isn't supported anymore as long as there's a way to recognize what the issue was and quickly change things. I think the reason Sophia and I spent hours debugging this was just that the error message was super confusing and not helpful.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2023 at 21:47):

alexcrichton commented on issue #6737:

Above you mentioned that the error you were seeing was that Wasmtime said it couldn't find "--arg" when executed as wasmtime run foo.wasm -- --arg, could you paste the full error you were seeing? With Wasmtime 11 that's interpreted as foo.wasm --arg but with Wasmtime 12 (and main) that's interpreted as foo.wasm -- --arg (e.g. the wasm itself receives the --). I'm not actually sure what component of the system was indicating that --arg couldn't be located, but it might have been the wasm itself because the wasm may not have been expecting -- as its first argument.

Otherwise though unfortunately that runs afoul of the disambiguation here because you may want to pass -- to the wasm module itself, so -- doesn't necessarily indicate that it's old-vs-new.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2023 at 22:09):

iximeow commented on issue #6737:

one other thought i wanted to mention here in case it helps any other readers think through the space - there are kind of two camps of ---as-a-separator:

i personally have a strong preference for the former in the interest of not guessing at what the intended program to run probably is. but that helped me understand my instinct for cargo run -- --the --arguments.

anyway, my concrete thoughts here are: i'm also curious what folks think at the wasmtime biweekly, a heuristic might be nice for a little bit if folks trip on the change, and i'd also +1 not asserting that the CLI parsing rules are frozen any more than other API changes that programs would deal with.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2023 at 22:35):

michelledaviest commented on issue #6737:

The specific error I get is, 

$ wasmtime run foo.wasm -- --file bar.txt
error: unexpected argument '--file' found

Usage: foo.wasm [OPTIONS] --file <FILE>

For more information, try '--help'.

It would be nice if there was a warning here that said,
!!! Things have changed, don't pass in arguments to wasm files with -- anymore !!!
It would have helped with debugging things.

You could have the warning displayed in wasmtime 13.0 and remove it in 14.0.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 10 2023 at 15:19):

alexcrichton commented on issue #6737:

FWIW one of the reasons cargo run is the way it is is because there's no positional argument that specifies the binary to run, so the only way to start differentiating between Cargo's arguments or the binary's arguments is via -- or a positional argument passed to the binary. For example cargo run foo -h will execute ./target/debug/rust-program foo -h but cargo run -h prints Cargo's help.

@michelledaviest ah ok thanks! I can see how that's confusing, but that's compounded because it's foo.wasm printing that error message, not Wasmtime. The meaning of wasmtime run foo.wasm -- --file bar.txt changed with this PR where before this PR that was interpreted as foo.wasm --file bar.txt but after this PR that's interpreted (intentionally) as foo.wasm -- --file bar.txt and your foo.wasm program probably wasn't ready for that -- argument.

The warning is unfortunately difficult because it's unclear whether -- is intended to be passed to the wasm program or not. The goal of this PR is to make things more consistent with other tools (e.g. perf and strace as mentioned by @iximeow in addition to things like docker that I'm aware of), but it necessarily requires breaking the meaning of previous invocations of Wasmtime. In that sense I don't know of a way to provide a warning other than what I mentioned earlier by parsing twice and seeing if the results are different.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 10 2023 at 17:39):

alexcrichton commented on issue #6737:

I realize now I may not be able to make the meeting this coming Thursday, so I've posted a revert for 12.0.0 since it'll be our last meeting before 12.0.0 goes out and there's no urgency to land this now, it can wait for the next release.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2023 at 01:15):

cardoso commented on issue #6737:

Similar discussion here: https://github.com/fermyon/spin/issues/1214

The preference was for cargo run [options] [-- args].

I think it boiled down to:

Spin got stuck on clap 3 due to clap 4 seeing the combination of flags slightly differently, so it will have to take the breaking change if it wants to update the dependency. A bunch of improvements are blocked too.

Anyway, things may be different here, but I hated dealing with this enough I had to chime in.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2023 at 01:21):

cardoso edited a comment on issue #6737:

Similar discussion here: https://github.com/fermyon/spin/issues/1214

The preference was for cargo run [options] [-- args].

I think it boiled down to:

Spin got stuck on clap 3 due to clap 4 seeing the combination of flags slightly differently, so it will have to take the breaking change if it wants to update the dependency. A bunch of improvements are blocked too.

Anyway, things may be different here, but I hated dealing with this enough I had to chime in.

Edit: more ~suffering~ context: https://github.com/clap-rs/clap/issues/1404 https://github.com/fermyon/spin/pull/1198

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2023 at 14:07):

alexcrichton commented on issue #6737:

Thanks for the link! Wasmtime may be in a bit of a different situation here though than cargo run. For Wasmtime the main demarcation of "ok here's guest arguments" is the wasm module itself. For a tool like cargo run there's no demarcation "by default" since you don't know when Cargo's flags stop and the tool's flags start, but for us the wasm module is a good point in the middle. Whether that makes auto-completion difficult though I'm not sure.

Technically the change here for args_conflicts_with_subcommands is actually somewhat orthogonal. I wanted to improve the implementation of our support for wasmtime foo.wasm which is a shortcut for wasmtime run foo.wasm. The way it's currently implemented without args_conflicts_with_subcommands can sometimes lead to confusing error messages (since we catch some errors and parse twice but errors can change over time with clap). Otherwise though that setting isn't integral to the change to where options are parsed, and I should probably have split that out to a separate change.

Regardless though I'll read up on these contexts and see what more I can lean, thanks for the links!

Last updated: Nov 22 2024 at 16:03 UTC