Require wasmtime options are first when running modules #6737

alexcrichton · 2023-07-17T20:38:24Z

Currently the way we've configured argument parsing it's valid to execute a command such as:

wasmtime run foo.wasm -O

which is the same as:

wasmtime run -O foo.wasm

or otherwise all flags are attempted to be parsed as Wasmtime flags and an error is generated when they're not wasmtime flags. I've personally found this a bit confusing in the past and I find myself frequently executing:

wasmtime run -- foo.wasm -other -arguments

While this works my general impression is that many other "wrapper commands" don't behave this way and typically don't require -- to pass flags to the target executable. This commit reconfigures argument parsing to consider any argument after the WebAssembly module itself to be an argument to the wasm program rather than an argument to Wasmtime. This means that all Wasmtime options must come between the run command and the foo.wasm WebAssembly argument.

src/commands/run.rs

src/bin/wasmtime.rs

Currently the way we've configured argument parsing it's valid to execute a command such as: wasmtime run foo.wasm -O which is the same as: wasmtime run -O foo.wasm or otherwise all flags are attempted to be parsed as Wasmtime flags and an error is generated when they're not wasmtime flags. I've personally found this a bit confusing in the past and I find myself frequently executing: wasmtime run -- foo.wasm -other -arguments While this works my general impression is that many other "wrapper commands" don't behave this way and typically don't require `--` to pass flags to the target executable. This commit reconfigures argument parsing to consider any argument after the WebAssembly module itself to be an argument to the wasm program rather than an argument to Wasmtime. This means that all Wasmtime options must come between the `run` command and the `foo.wasm` WebAssembly argument.

Additionally use more clap features to avoid manually checking against subcommands.

cfallin · 2023-08-09T19:12:40Z

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?

cfallin · 2023-08-09T19:15:55Z

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

cfallin · 2023-08-09T19:24:57Z

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.

alexcrichton · 2023-08-09T19:36:06Z

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.

cfallin · 2023-08-09T21:01:22Z

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.

alexcrichton · 2023-08-09T21:25:00Z

Sounds good to me!

michelledaviest · 2023-08-09T21:30:37Z

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.

alexcrichton · 2023-08-09T21:47:02Z

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.

iximeow · 2023-08-09T22:08:59Z

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:

perf, strace, etc, now wasmtime, where the argument scheme is pretty stoically [command] [command-args] [target-program] [target-program-args]
cargo run, lldb, gdb kinda, etc, where the argument scheme is [command] [arguments, target program might be in the arg list somewhere] -- [target-program-args]. gdb uses --args instead of -- because sure i guess..

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.

michelledaviest · 2023-08-09T22:35:18Z

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.

alexcrichton · 2023-08-10T15:19:24Z

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.

…les (bytecodealliance#6737)" This reverts commit 8091556. The fallout of this change has been larger than expected so revert this on the 12.0.0 branch to provide more time to discuss this and figure out the best course of action.

alexcrichton · 2023-08-10T17:37:19Z

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.

…les (#6737)" (#6830) * [12.0.0] Revert "Require wasmtime options are first when running modules (#6737)" This reverts commit 8091556. The fallout of this change has been larger than expected so revert this on the 12.0.0 branch to provide more time to discuss this and figure out the best course of action. * Revert addition of one more test

This commit is a partial revert of bytecodealliance#6737. That change was reverted in bytecodealliance#6830 for the 12.0.0 release of Wasmtime and otherwise it's currently slated to get released with the 13.0.0 release of Wasmtime. Discussion at today's Wasmtime meeting concluded that it's best to couple this change with bytecodealliance#6925 as a single release rather than spread out across multiple releases. This commit is thus the revert of bytecodealliance#6737, although it's a partial revert in that I've kept many of the new tests added to showcase the differences before/after when the change lands. This means that Wasmtime 13.0.0 will exhibit the same CLI behavior as 12.0.0 and all prior releases. The 14.0.0 release will have both a new CLI and new argument passing semantics. I'll revert this revert (aka re-land bytecodealliance#6737) once the 13.0.0 release branch is created and `main` becomes 14.0.0.

* Partially revert CLI argument changes from #6737 This commit is a partial revert of #6737. That change was reverted in #6830 for the 12.0.0 release of Wasmtime and otherwise it's currently slated to get released with the 13.0.0 release of Wasmtime. Discussion at today's Wasmtime meeting concluded that it's best to couple this change with #6925 as a single release rather than spread out across multiple releases. This commit is thus the revert of #6737, although it's a partial revert in that I've kept many of the new tests added to showcase the differences before/after when the change lands. This means that Wasmtime 13.0.0 will exhibit the same CLI behavior as 12.0.0 and all prior releases. The 14.0.0 release will have both a new CLI and new argument passing semantics. I'll revert this revert (aka re-land #6737) once the 13.0.0 release branch is created and `main` becomes 14.0.0. * Update release notes

This commit implements a new behavior for the CLI of the `wasmtime` executable which will require that options for Wasmtime itself come before the wasm module being run. Currently they're allowed to come afterwards, but instead all arguments and flags coming after a module will be interpreted as arguments for the module itself. This feature has a bit of a storied history at this point, and the breadcrumbs are: * Originally landed in bytecodealliance#6737 * Reverted for 12.0.0 in bytecodealliance#6830 * Reverted for 13.0.0 in bytecodealliance#6944 This PR is intended to be landed as a sibling of bytecodealliance#6925, another independent overhaul of Wasmtime's own options on the CLI, for the Wasmtime 14.0.0 release. More information about the motivation for this change, as well as consequences of the fallout, can be found on bytecodealliance#6737.

…6950) * Enhance `async` configuration of `bindgen!` macro (#6942) This commit takes a leaf out of `wiggle`'s book to enable bindings generation for async host functions where only some host functions are async instead of all of them. This enhances the `async` key with a few more options: async: { except_imports: ["foo"], only_imports: ["bar"], } This is beyond what `wiggle` supports where either an allow-list or deny-list can be specified (although only one can be specified). This can be useful if either the list of sync imports or the list of async imports is small. * cranelift-interpreter: Fix SIMD shifts and rotates (#6939) * cranelift-interpreter: Fix SIMD `ishl`/`{s,u}`shr * fuzzgen: Enable a few more ops * cranelift: Fix tests for {u,s}shr * fuzzgen: Change pattern matching arms for shifts Co-Authored-By: Jamey Sharp <[email protected]> --------- Co-authored-by: Jamey Sharp <[email protected]> * Partially revert CLI argument changes from #6737 (#6944) * Partially revert CLI argument changes from #6737 This commit is a partial revert of #6737. That change was reverted in #6830 for the 12.0.0 release of Wasmtime and otherwise it's currently slated to get released with the 13.0.0 release of Wasmtime. Discussion at today's Wasmtime meeting concluded that it's best to couple this change with #6925 as a single release rather than spread out across multiple releases. This commit is thus the revert of #6737, although it's a partial revert in that I've kept many of the new tests added to showcase the differences before/after when the change lands. This means that Wasmtime 13.0.0 will exhibit the same CLI behavior as 12.0.0 and all prior releases. The 14.0.0 release will have both a new CLI and new argument passing semantics. I'll revert this revert (aka re-land #6737) once the 13.0.0 release branch is created and `main` becomes 14.0.0. * Update release notes * riscv64: Use `PCRelLo12I` relocation on Loads (#6938) * riscv64: Use `PCRelLo12I` relocation on Loads * riscv64: Strenghten pattern matching when emitting Load's * riscv64: Clarify some of the load address logic * riscv64: Even stronger matching * Update Rust in CI to 1.72.0, clarify Wasmtime's MSRV (#6900) * Update Rust in CI to 1.72.0 * Update CI, tooling, and docs for MSRV This commit codifies an MSRV policy for Wasmtime at "stable minus two" meaning that the latest three releases of Rust will be supported. This is enforced on CI with a full test suite job running on Linux x86_64 with the minimum supported Rust version. The full test suite will use the latest stable version. A downside of this approach is that new changes may break MSRV support on non-Linux or non-x86_64 platforms and we won't know about it, but that's deemed a minor enough risk at this time. A minor fix is applied to Wasmtime's `Cargo.toml` to support Rust 1.70.0 instead of requiring Rust 1.71.0 * Fix installation of rust * Scrape MSRV from Cargo.toml * Cranelift is the same as Wasmtime's MSRV now, more words too * Fix a typo * aarch64: Use `RegScaled*` addressing modes (#6945) This commit adds a few cases to `amode` construction on AArch64 for using the `RegScaled*` variants of `AMode`. This won't affect wasm due to this only matching the sign-extension happening before the shift, but it should otherwise help non-wasm Cranelift use cases. Closes #6742 * cranelift: Validate `iconst` ranges (#6850) * cranelift: Validate `iconst` ranges Add the following checks: `iconst.i8` immediate must be within 0 .. 2^8-1 `iconst.i16` immediate must be within 0 .. 2^16-1 `iconst.i32` immediate must be within 0 .. 2^32-1 Resolves #3059 * cranelift: Parse `iconst` according to its type Modifies the parser for textual CLIF so that V in `iconst.T V` is parsed according to T. Before this commit, something like `iconst.i32 0xffff_ffff_ffff` was valid because all `iconst` were parsed the same as an `iconst.i64`. Now the above example will throw an error. Also, a negative immediate as in `iconst.iN -X` is now converted to `2^N - X`. This commit also fixes some broken tests. * cranelift: Update tests to match new CLIF parser * Some minor fixes and features for WASI and sockets (#6948) * Use `command::add_to_linker` in tests to reduce the number of times all the `add_to_linker` are listed. * Add all `wasi:sockets` interfaces currently implemented to both the sync and async `command` functions (this enables all the interfaces in the CLI for example). * Use `tokio::net::TcpStream::try_io` whenever I/O is performed on a socket, ensuring that readable/writable flags are set/cleared appropriately (otherwise once readable a socket is infinitely readable). * Add a `with_ambient_tokio_runtime` helper function to use when creating a `tokio::net::TcpStream` since otherwise it panics due to a lack of active runtime in a synchronous context. * Add `WouldBlock` handling to return a 0-length read. * Add an `--inherit-network` CLI flag to enable basic usage of sockets in the CLI. This will conflict a small amount with #6877 but should be easy to resolve, and otherwise this targets different usability points/issues than that PR. --------- Co-authored-by: Afonso Bordado <[email protected]> Co-authored-by: Jamey Sharp <[email protected]> Co-authored-by: Timothée Jourde <[email protected]>

cardoso · 2023-09-02T01:15:39Z

Similar discussion here: fermyon/spin#1214

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

I think it boiled down to:

Args are clearly separate: CLI uses args until it sees -- and all else is forwarded. Autocompletion tools liked that.
It's clap's happy path: args_conflicts_with_subcommands and other flags can get pretty unpredictable.

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: clap-rs/clap#1404 fermyon/spin#1198

alexcrichton · 2023-09-05T14:07:04Z

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!

…ecodealliance#6944) * Partially revert CLI argument changes from bytecodealliance#6737 This commit is a partial revert of bytecodealliance#6737. That change was reverted in bytecodealliance#6830 for the 12.0.0 release of Wasmtime and otherwise it's currently slated to get released with the 13.0.0 release of Wasmtime. Discussion at today's Wasmtime meeting concluded that it's best to couple this change with bytecodealliance#6925 as a single release rather than spread out across multiple releases. This commit is thus the revert of bytecodealliance#6737, although it's a partial revert in that I've kept many of the new tests added to showcase the differences before/after when the change lands. This means that Wasmtime 13.0.0 will exhibit the same CLI behavior as 12.0.0 and all prior releases. The 14.0.0 release will have both a new CLI and new argument passing semantics. I'll revert this revert (aka re-land bytecodealliance#6737) once the 13.0.0 release branch is created and `main` becomes 14.0.0. * Update release notes

This commit implements a new behavior for the CLI of the `wasmtime` executable which will require that options for Wasmtime itself come before the wasm module being run. Currently they're allowed to come afterwards, but instead all arguments and flags coming after a module will be interpreted as arguments for the module itself. This feature has a bit of a storied history at this point, and the breadcrumbs are: * Originally landed in bytecodealliance#6737 * Reverted for 12.0.0 in bytecodealliance#6830 * Reverted for 13.0.0 in bytecodealliance#6944 This PR is intended to be landed as a sibling of bytecodealliance#6925, another independent overhaul of Wasmtime's own options on the CLI, for the Wasmtime 14.0.0 release. More information about the motivation for this change, as well as consequences of the fallout, can be found on bytecodealliance#6737.

This commit implements a new behavior for the CLI of the `wasmtime` executable which will require that options for Wasmtime itself come before the wasm module being run. Currently they're allowed to come afterwards, but instead all arguments and flags coming after a module will be interpreted as arguments for the module itself. This feature has a bit of a storied history at this point, and the breadcrumbs are: * Originally landed in #6737 * Reverted for 12.0.0 in #6830 * Reverted for 13.0.0 in #6944 This PR is intended to be landed as a sibling of #6925, another independent overhaul of Wasmtime's own options on the CLI, for the Wasmtime 14.0.0 release. More information about the motivation for this change, as well as consequences of the fallout, can be found on #6737.

) This commit implements a new behavior for the CLI of the `wasmtime` executable which will require that options for Wasmtime itself come before the wasm module being run. Currently they're allowed to come afterwards, but instead all arguments and flags coming after a module will be interpreted as arguments for the module itself. This feature has a bit of a storied history at this point, and the breadcrumbs are: * Originally landed in bytecodealliance#6737 * Reverted for 12.0.0 in bytecodealliance#6830 * Reverted for 13.0.0 in bytecodealliance#6944 This PR is intended to be landed as a sibling of bytecodealliance#6925, another independent overhaul of Wasmtime's own options on the CLI, for the Wasmtime 14.0.0 release. More information about the motivation for this change, as well as consequences of the fallout, can be found on bytecodealliance#6737.

alexcrichton requested a review from a team as a code owner July 17, 2023 20:38

alexcrichton requested review from itsrainy and removed request for a team July 17, 2023 20:38

sunfishcode reviewed Jul 17, 2023

View reviewed changes

src/commands/run.rs Show resolved Hide resolved

sunfishcode reviewed Jul 18, 2023

View reviewed changes

src/commands/run.rs Outdated Show resolved Hide resolved

src/bin/wasmtime.rs Show resolved Hide resolved

sunfishcode approved these changes Jul 18, 2023

View reviewed changes

alexcrichton added this pull request to the merge queue Jul 18, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 18, 2023

alexcrichton requested a review from a team as a code owner July 18, 2023 19:58

alexcrichton enabled auto-merge July 18, 2023 19:59

alexcrichton added this pull request to the merge queue Jul 18, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 18, 2023

alexcrichton enabled auto-merge July 18, 2023 20:27

alexcrichton added this pull request to the merge queue Jul 18, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 18, 2023

alexcrichton enabled auto-merge July 19, 2023 14:21

alexcrichton added this pull request to the merge queue Jul 19, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 19, 2023

alexcrichton enabled auto-merge July 19, 2023 15:08

alexcrichton added this pull request to the merge queue Jul 19, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 19, 2023

alexcrichton added this pull request to the merge queue Jul 19, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 19, 2023

alexcrichton added 7 commits July 19, 2023 11:42

Update wasi testsuite runner

81409d8

Support wasmtime -- run

cde6248

Additionally use more clap features to avoid manually checking against subcommands.

Remove stale comment

cd9d506

Reorder wasi-nn arguments

6dce59c

Reorder more flags

3dcceaa

Fix unused import on Windows

93ccb8c

alexcrichton mentioned this pull request Aug 10, 2023

[12.0.0] Revert "Require wasmtime options are first when running modules (#6737)" #6830

Merged

This was referenced Aug 31, 2023

Partially revert CLI argument changes from #6737 #6944

Merged

Redesign Wasmtime's CLI #6925

Merged

alexcrichton mentioned this pull request Aug 31, 2023

Require Wasmtime options come before wasm modules #6946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require wasmtime options are first when running modules #6737

Require wasmtime options are first when running modules #6737

alexcrichton commented Jul 17, 2023

cfallin commented Aug 9, 2023

cfallin commented Aug 9, 2023

cfallin commented Aug 9, 2023

alexcrichton commented Aug 9, 2023

cfallin commented Aug 9, 2023

alexcrichton commented Aug 9, 2023

michelledaviest commented Aug 9, 2023

alexcrichton commented Aug 9, 2023

iximeow commented Aug 9, 2023

michelledaviest commented Aug 9, 2023

alexcrichton commented Aug 10, 2023

alexcrichton commented Aug 10, 2023

cardoso commented Sep 2, 2023 •

edited

Loading

alexcrichton commented Sep 5, 2023

Require wasmtime options are first when running modules #6737

Require wasmtime options are first when running modules #6737

Conversation

alexcrichton commented Jul 17, 2023

cfallin commented Aug 9, 2023

cfallin commented Aug 9, 2023

cfallin commented Aug 9, 2023

alexcrichton commented Aug 9, 2023

cfallin commented Aug 9, 2023

alexcrichton commented Aug 9, 2023

michelledaviest commented Aug 9, 2023

alexcrichton commented Aug 9, 2023

iximeow commented Aug 9, 2023

michelledaviest commented Aug 9, 2023

alexcrichton commented Aug 10, 2023

alexcrichton commented Aug 10, 2023

cardoso commented Sep 2, 2023 • edited Loading

alexcrichton commented Sep 5, 2023

cardoso commented Sep 2, 2023 •

edited

Loading