CLI: Add WASI support #597
Conversation
BENCHMARKS
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov Report
@@ Coverage Diff @@
## master #597 +/- ##
==========================================
- Coverage 81.13% 80.51% -0.62%
==========================================
Files 93 94 +1
Lines 7719 7807 +88
==========================================
+ Hits 6263 6286 +23
- Misses 1456 1521 +65
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
crates/cli/src/main.rs
Outdated
| #[clap()] | ||
| func_name: Option<String>, | ||
| #[clap(long, value_parser)] | ||
| invoke: Option<String>, |
There was a problem hiding this comment.
The invoke flag needs to take a custom value of InvokeArgs which consist of the function name and its arguments since it does not make sense to have function arguments without an associated function name. The default _start function in WASI never takes arguments.
struct InvokeArgs {
func_name: Option<String>,
func_args: Vec<String>,
}As with the KeyValue type we also need a parse function for this type that is going to be used.
Maybe it is possible to simply derive clap::Parse instead of providing a custom parse_invoke_args function.
There was a problem hiding this comment.
Cool. Will Address it.
There was a problem hiding this comment.
So if clap::Parse derive work we should be fine with:
#[derive(Parser)]
struct InvokeArgs {
#[clap(value_name = "FUNC_NAME")]
func_name: Option<String>,
#[clap()]
func_args: Vec<String>,
}There was a problem hiding this comment.
The default _start function in WASI never takes arguments.
Something to consider: A user might name their exported function _start or " " and invoke with arguments.
There was a problem hiding this comment.
--invoke _start 42 true and --invoke " " 42 true may be working for those cases, right?
There was a problem hiding this comment.
Does the code now work so that function arguments can only be provided together with --invoke and a function name?
There was a problem hiding this comment.
Yes. Both " " and _start can take arguments.
Does the code now work so that function arguments can only be provided together with --invoke and a function name?
No. I just understood you. Would have to use a custom parser or use groups. I'll look into it. Maybe I don't need the new struct though. Seen the use of groups in nextest cli.
There was a problem hiding this comment.
From what I know parse groups are usually used for excluding certain options. For example: Among these flags the user may pick at most one. I think the most straight forward approach is to have InvokeArgs as a custom type and simply support it being a CLI flag such as PathBuf or SocketAddr. I am sure clap supports custom types somehow.
There was a problem hiding this comment.
I tried to execute the following command using this PR:
> cargo run --package wasmi_cli -- --invoke ./crates/wasmi/benches/wat/count_until.wat count_until 100
Finished dev [unoptimized + debuginfo] target(s) in 0.14s
Running `target\debug\wasmi_cli.exe --invoke .\crates\wasmi\benches\wat\count_until.wat count_until 100`
Error: failed to read Wasm file "count_until"
So the input path ./crates/wasmi/benches/wat/count_until.wat gets cut down to just count_until and therefore no file can be found.
|
Hey @Robbepop , please review whenever you're free |
Co-authored-by: Robin Freyler <robbepop@web.de>
Co-authored-by: Robin Freyler <robbepop@web.de>
|
Since we already are way into the review process for this PR it is no longer a draft. I will convert it now. |
There was a problem hiding this comment.
All in all looks good to me!
Thanks a ton for this amazing work @OLUWAMUYIWA ! 🚀
There are just a few minor nits but it will be vastly simpler for the whole process if I fix those after merging the PR.
The PR is good as is. :)
I also checked out the PR locally and toyed around with it a little. Seeing a wasmi "Hello, World!" was quite an experience for me. :)
Let us merge WASI support for the wasmi CLI tool. 😎
The next big milestone for us is integrating the WASI testsuite and making it pass so that we can remove the experimental flag on our WASI support. My estimation is that passing the WASI testsuite won't be too hard since we are basically building on the shoulders of giants already.
PR adds
wasisupport towasmi_cli.Notable changes:
--dirflag: allows pre-opening of directories with whichhosts protectguests from accessingonlydirectories they want. i.e.wasisandboxing in action.--envflag: allows users to set environment variables--tcplistenflag: pre-open sockets.wasisandboxing.Note
wasmi_climakesWASIavailable to all programs.