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()?;