From df8d3698b1e33ddb85453708740c46f6b6558d1a Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 31 Aug 2023 20:12:37 +0200 Subject: [PATCH] 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 --- RELEASES.md | 7 --- src/bin/wasmtime.rs | 46 ++++++++----------- src/commands/run.rs | 102 +++++++++++++++++++++++------------------ tests/all/cli_tests.rs | 28 +++++++---- 4 files changed, 94 insertions(+), 89 deletions(-) diff --git a/RELEASES.md b/RELEASES.md index 1162d8765031..8cc2cdb13d0a 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -80,13 +80,6 @@ Unreleased. These methods do not affect the size of the pre-allocated pool. [#6835](https://github.com/bytecodealliance/wasmtime/pull/6835) -* Options to the `wasmtime` CLI for Wasmtime itself must now come before the - WebAssembly module. For example `wasmtime run foo.wasm --disable-cache` now - must be specified as `wasmtime run --disable-cache foo.wasm`. Any - argument/option after the WebAssembly module is now interpreted as an argument - to the wasm module itself. - [#6737](https://github.com/bytecodealliance/wasmtime/pull/6737) - * Builder methods for WASI contexts onw use `&mut self` instead of `self`. [#6770](https://github.com/bytecodealliance/wasmtime/pull/6770) diff --git a/src/bin/wasmtime.rs b/src/bin/wasmtime.rs index d32a0b63f373..6d425625dfa4 100644 --- a/src/bin/wasmtime.rs +++ b/src/bin/wasmtime.rs @@ -4,7 +4,7 @@ //! See `wasmtime --help` for usage. use anyhow::Result; -use clap::Parser; +use clap::{error::ErrorKind, Parser}; use wasmtime_cli::commands::{ CompileCommand, ConfigCommand, ExploreCommand, RunCommand, SettingsCommand, WastCommand, }; @@ -27,24 +27,10 @@ use wasmtime_cli::commands::{ \n\ Invoking a specific function (e.g. `add`) in a WebAssembly module:\n\ \n \ - wasmtime example.wasm --invoke add 1 2\n", - - // This option enables the pattern below where we ask clap to parse twice - // sorta: once where it's trying to find a subcommand and once assuming - // a subcommand doesn't get passed. Clap should then, apparently, - // fill in the `subcommand` if found and otherwise fill in the - // `RunCommand`. - args_conflicts_with_subcommands = true + wasmtime example.wasm --invoke add 1 2\n" )] -struct Wasmtime { - #[clap(subcommand)] - subcommand: Option, - #[clap(flatten)] - run: RunCommand, -} - -#[derive(Parser)] -enum Subcommand { +enum Wasmtime { + // !!! IMPORTANT: if subcommands are added or removed, update `parse_module` in `src/commands/run.rs`. !!! /// Controls Wasmtime configuration settings Config(ConfigCommand), /// Compiles a WebAssembly module. @@ -62,20 +48,26 @@ enum Subcommand { impl Wasmtime { /// Executes the command. pub fn execute(self) -> Result<()> { - let subcommand = self.subcommand.unwrap_or(Subcommand::Run(self.run)); - match subcommand { - Subcommand::Config(c) => c.execute(), - Subcommand::Compile(c) => c.execute(), - Subcommand::Explore(c) => c.execute(), - Subcommand::Run(c) => c.execute(), - Subcommand::Settings(c) => c.execute(), - Subcommand::Wast(c) => c.execute(), + match self { + Self::Config(c) => c.execute(), + Self::Compile(c) => c.execute(), + Self::Explore(c) => c.execute(), + Self::Run(c) => c.execute(), + Self::Settings(c) => c.execute(), + Self::Wast(c) => c.execute(), } } } fn main() -> Result<()> { - Wasmtime::parse().execute() + Wasmtime::try_parse() + .unwrap_or_else(|e| match e.kind() { + ErrorKind::InvalidSubcommand | ErrorKind::UnknownArgument => { + Wasmtime::Run(RunCommand::parse()) + } + _ => e.exit(), + }) + .execute() } #[test] diff --git a/src/commands/run.rs b/src/commands/run.rs index 3a3f080c9dca..398b9bf686dc 100644 --- a/src/commands/run.rs +++ b/src/commands/run.rs @@ -6,8 +6,11 @@ )] use anyhow::{anyhow, bail, Context as _, Error, Result}; +use clap::builder::{OsStringValueParser, TypedValueParser}; use clap::Parser; use once_cell::sync::Lazy; +use std::ffi::OsStr; +use std::ffi::OsString; use std::fs::File; use std::io::Write; use std::path::{Path, PathBuf}; @@ -35,6 +38,18 @@ use wasmtime_wasi_threads::WasiThreadsCtx; // #[cfg(feature = "wasi-http")] // use wasmtime_wasi_http::WasiHttpCtx; +fn parse_module(s: OsString) -> anyhow::Result { + // Do not accept wasmtime subcommand names as the module name + match s.to_str() { + Some("help") | Some("config") | Some("run") | Some("wast") | Some("compile") => { + bail!("module name cannot be the same as a subcommand") + } + #[cfg(unix)] + Some("-") => Ok(PathBuf::from("/dev/stdin")), + _ => Ok(s.into()), + } +} + fn parse_env_var(s: &str) -> Result<(String, Option)> { let mut parts = s.splitn(2, '='); Ok(( @@ -103,7 +118,7 @@ static AFTER_HELP: Lazy = Lazy::new(|| crate::FLAG_EXPLANATIONS.to_strin /// Runs a WebAssembly module #[derive(Parser)] -#[structopt(name = "run", after_help = AFTER_HELP.as_str())] +#[structopt(name = "run", trailing_var_arg = true, after_help = AFTER_HELP.as_str())] pub struct RunCommand { #[clap(flatten)] common: CommonOptions, @@ -177,6 +192,14 @@ pub struct RunCommand { #[clap(long = "wasi-nn-graph", value_name = "FORMAT::HOST_DIR", value_parser = parse_graphs)] graphs: Vec<(String, String)>, + /// The path of the WebAssembly module to run + #[clap( + required = true, + value_name = "MODULE", + value_parser = OsStringValueParser::new().try_map(parse_module), + )] + module: PathBuf, + /// Load the given WebAssembly module before the main module #[clap( long = "preload", @@ -220,6 +243,11 @@ pub struct RunCommand { #[clap(long = "coredump-on-trap", value_name = "PATH")] coredump_on_trap: Option, + // NOTE: this must come last for trailing varargs + /// The arguments to pass to the module + #[clap(value_name = "ARGS")] + module_args: Vec, + /// Maximum size, in bytes, that a linear memory is allowed to reach. /// /// Growth beyond this limit will cause `memory.grow` instructions in @@ -258,14 +286,6 @@ pub struct RunCommand { #[clap(long)] wmemcheck: bool, - /// The WebAssembly module to run and arguments to pass to it. - /// - /// Arguments passed to the wasm module will be configured as WASI CLI - /// arguments unless the `--invoke` CLI argument is passed in which case - /// arguments will be interpreted as arguments to the function specified. - #[clap(value_name = "WASM", trailing_var_arg = true, required = true)] - module_and_args: Vec, - /// Indicates that the implementation of WASI preview1 should be backed by /// the preview2 implementation for components. /// @@ -337,7 +357,7 @@ impl RunCommand { let engine = Engine::new(&config)?; // Read the wasm module binary either as `*.wat` or a raw binary. - let main = self.load_module(&engine, &self.module_and_args[0])?; + let main = self.load_module(&engine, &self.module)?; // Validate coredump-on-trap argument if let Some(coredump_path) = self.coredump_on_trap.as_ref() { @@ -429,12 +449,8 @@ impl RunCommand { // Load the main wasm module. match self .load_main_module(&mut store, &mut linker, &main, modules) - .with_context(|| { - format!( - "failed to run main module `{}`", - self.module_and_args[0].display() - ) - }) { + .with_context(|| format!("failed to run main module `{}`", self.module.display())) + { Ok(()) => (), Err(e) => { // Exit the process if Wasmtime understands the error; @@ -483,25 +499,27 @@ impl RunCommand { Ok(listeners) } - fn compute_argv(&self) -> Result> { + fn compute_argv(&self) -> Vec { let mut result = Vec::new(); - for (i, arg) in self.module_and_args.iter().enumerate() { - // For argv[0], which is the program name. Only include the base - // name of the main wasm module, to avoid leaking path information. - let arg = if i == 0 { - arg.components().next_back().unwrap().as_os_str() - } else { - arg.as_ref() - }; - result.push( - arg.to_str() - .ok_or_else(|| anyhow!("failed to convert {arg:?} to utf-8"))? - .to_string(), - ); + // Add argv[0], which is the program name. Only include the base name of the + // main wasm module, to avoid leaking path information. + result.push( + self.module + .components() + .next_back() + .map(|c| c.as_os_str()) + .and_then(OsStr::to_str) + .unwrap_or("") + .to_owned(), + ); + + // Add the remaining arguments. + for arg in self.module_args.iter() { + result.push(arg.clone()); } - Ok(result) + result } fn setup_epoch_handler( @@ -510,7 +528,7 @@ impl RunCommand { modules: Vec<(String, Module)>, ) -> Box)> { if let Some(Profile::Guest { path, interval }) = &self.profile { - let module_name = self.module_and_args[0].to_str().unwrap_or("
"); + let module_name = self.module.to_str().unwrap_or("
"); let interval = *interval; store.data_mut().guest_profiler = Some(Arc::new(GuestProfiler::new(module_name, interval, modules))); @@ -616,10 +634,9 @@ impl RunCommand { CliLinker::Core(linker) => { // Use "" as a default module name. let module = module.unwrap_core(); - linker.module(&mut *store, "", &module).context(format!( - "failed to instantiate {:?}", - self.module_and_args[0] - ))?; + linker + .module(&mut *store, "", &module) + .context(format!("failed to instantiate {:?}", self.module))?; // If a function to invoke was given, invoke it. let func = if let Some(name) = &self.invoke { @@ -678,7 +695,7 @@ impl RunCommand { is experimental and may break in the future" ); } - let mut args = self.module_and_args.iter().skip(1); + let mut args = self.module_args.iter(); let mut values = Vec::new(); for ty in ty.params() { let val = match args.next() { @@ -691,9 +708,6 @@ impl RunCommand { } } }; - let val = val - .to_str() - .ok_or_else(|| anyhow!("argument is not valid utf-8: {val:?}"))?; values.push(match ty { // TODO: integer parsing here should handle hexadecimal notation // like `0x0...`, but the Rust standard library currently only @@ -751,9 +765,7 @@ impl RunCommand { if !err.is::() { return err; } - let source_name = self.module_and_args[0] - .to_str() - .unwrap_or_else(|| "unknown"); + let source_name = self.module.to_str().unwrap_or_else(|| "unknown"); if let Err(coredump_err) = generate_coredump(&err, &source_name, coredump_path) { eprintln!("warning: coredump failed to generate: {}", coredump_err); @@ -990,7 +1002,7 @@ impl RunCommand { fn set_preview1_ctx(&self, store: &mut Store) -> Result<()> { let mut builder = WasiCtxBuilder::new(); - builder.inherit_stdio().args(&self.compute_argv()?)?; + builder.inherit_stdio().args(&self.compute_argv())?; for (key, value) in self.vars.iter() { let value = match value { @@ -1022,7 +1034,7 @@ impl RunCommand { fn set_preview2_ctx(&self, store: &mut Store) -> Result<()> { let mut builder = preview2::WasiCtxBuilder::new(); - builder.inherit_stdio().args(&self.compute_argv()?); + builder.inherit_stdio().args(&self.compute_argv()); for (key, value) in self.vars.iter() { let value = match value { diff --git a/tests/all/cli_tests.rs b/tests/all/cli_tests.rs index 29561129ce61..f144dd933e66 100644 --- a/tests/all/cli_tests.rs +++ b/tests/all/cli_tests.rs @@ -604,6 +604,7 @@ fn wasm_flags() -> Result<()> { let stdout = run_wasmtime(&[ "run", "tests/all/cli_tests/print-arguments.wat", + "--", "--argument", "-for", "the", @@ -619,7 +620,7 @@ fn wasm_flags() -> Result<()> { command\n\ " ); - let stdout = run_wasmtime(&["run", "tests/all/cli_tests/print-arguments.wat", "-"])?; + let stdout = run_wasmtime(&["run", "tests/all/cli_tests/print-arguments.wat", "--", "-"])?; assert_eq!( stdout, "\ @@ -627,7 +628,7 @@ fn wasm_flags() -> Result<()> { -\n\ " ); - let stdout = run_wasmtime(&["run", "tests/all/cli_tests/print-arguments.wat", "--"])?; + let stdout = run_wasmtime(&["run", "tests/all/cli_tests/print-arguments.wat", "--", "--"])?; assert_eq!( stdout, "\ @@ -648,7 +649,6 @@ fn wasm_flags() -> Result<()> { "\ print-arguments.wat\n\ --\n\ - --\n\ -a\n\ b\n\ " @@ -658,30 +658,37 @@ fn wasm_flags() -> Result<()> { #[test] fn name_same_as_builtin_command() -> Result<()> { - // a bare subcommand shouldn't run successfully + // A bare subcommand shouldn't run successfully. + // + // This is ambiguous between a missing module argument and a module named + // `run` with no other options. let output = get_wasmtime_command()? .current_dir("tests/all/cli_tests") .arg("run") .output()?; assert!(!output.status.success()); - // a `--` prefix should let everything else get interpreted as a wasm - // module and arguments, even if the module has a name like `run` + // Currently even `--` isn't enough to disambiguate, so this still is an + // error. + // + // NB: this will change in Wasmtime 14 when #6737 is relanded. let output = get_wasmtime_command()? .current_dir("tests/all/cli_tests") .arg("--") .arg("run") .output()?; - assert!(output.status.success(), "expected success got {output:#?}"); + assert!(!output.status.success(), "expected failure got {output:#?}"); - // Passing options before the subcommand should work and doesn't require - // `--` to disambiguate + // Passing `--foo` options before the module is also not enough to + // disambiguate for now. + // + // NB: this will change in Wasmtime 14 when #6737 is relanded. let output = get_wasmtime_command()? .current_dir("tests/all/cli_tests") .arg("--disable-cache") .arg("run") .output()?; - assert!(output.status.success(), "expected success got {output:#?}"); + assert!(!output.status.success(), "expected failure got {output:#?}"); Ok(()) } @@ -701,6 +708,7 @@ fn wasm_flags_without_subcommand() -> Result<()> { let output = get_wasmtime_command()? .current_dir("tests/all/cli_tests/") .arg("print-arguments.wat") + .arg("--") .arg("-foo") .arg("bar") .output()?;