-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create wasmtime::Config from toml #9811
Create wasmtime::Config from toml #9811
Conversation
23b1984
to
5427eeb
Compare
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
To modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! I'm liking how this is shaping up pretty cleanly so far. Would you be up for integrating this into a CLI flag itself as well? For example wasmtime --config cfg.toml
or something like that
Thanks for looking at this. I added a --config flag. Any suggestions on how to best test this? |
For tests I'd recommend |
This is looking good to me, was there something else you wanted to address before landing though? |
Sorry for the delay @alexcrichton. I was wondering if I need to update any documentation, and if the current tests are sufficient. If not, I am ready for merge/reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to land, so I'm going to go ahead and approve-and-merge.
If you'd like to help write some docs I suspect that https://github.com/bytecodealliance/wasmtime/blob/main/docs/cli-options.md might be a good location to put this perhaps?
5bc538f
to
485bb10
Compare
1a0e73a
to
94627d2
Compare
@alexcrichton I had some problems getting the test suite with the i686 target to pass, but it should hopefully be fixed now by using |
Let's see if that's spurious... |
Ah sorry it looks like #10123 landed which causes conflicts with this. I'm happy to help out with resolving conflicts if needed. |
94627d2
to
37faba3
Compare
@alexcrichton no worries. The only question I have is about the fmt::Display impl for CommonOptions. If configured using a |
I think that's reasonable yeah, the Display implementation here is sort of best-effort so printing out |
Remove `derive(Deserialize, Serialize)` for some configuration-based enums to ideally give us more flexibility in the future as to what these are exactly called. I'm not sure of any internal users of these except for one use due to bytecodealliance#9811 which can be switched to using an option-based parser to ensure the same syntax is accepted via `--config` and on the CLI with a flag.
* Cut down on serde requirements in `wasmtime` crate Remove `derive(Deserialize, Serialize)` for some configuration-based enums to ideally give us more flexibility in the future as to what these are exactly called. I'm not sure of any internal users of these except for one use due to #9811 which can be switched to using an option-based parser to ensure the same syntax is accepted via `--config` and on the CLI with a flag. * Remove unused imports * Fix option parsing
This PR makes it possible to create a
wasmtime::Config
from a toml file and is a first step toward resolving #8784. Overall, I think the path forward is as follows:wasmtime::Config
from a toml file, by makingCommonOptions
deriveDeserialize
. For example,opt_level
is deserialized from values 0, 1, 2, "s" (same as cli flags) instead of "None", "Speed", "SpeedAndSize". Same applies toregalloc_algorithm
,compiler
andcollector
. Feedback appreciated on this.Feedback greatly appreciated, especially regarding how to deal with
nn_graph
, as wellconfig_var
andkeyvalue_in_memory_data
, which are all markedserde(skip)
for now