alexcrichton commented on issue #5087:
Seems reasonable to me! Instead of a crate feature, though, could this be added to
CodegenSettings
?
github-actions[bot] commented on issue #5087:
Subscribe to Label Action
cc @kubkon
<details>
This issue or pull request has been labeled: "wasi"Thus the following users have been cc'd because of the following labels:
- kubkon: wasi
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
abrown commented on issue #5087:
Ok, pushed a version using
CodegenSettings
; the parent crates still have Cargo features, though, since this is how thewasmtime
feature is enabled.
alexcrichton commented on issue #5087:
I don't remember enough of the rationale here to remember for sure, but can the features be removed entirely? I think it's reasonable to leave
tracing
off-by-default and turn it back on forwasi-common
to preserve its current feature set.
abrown commented on issue #5087:
Not sure I understand how you want it to be turned on-off by the users of
from_witx!
andwasmtime_integration!
...
abrown commented on issue #5087:
In fact... I think there might even be an additional level of feature propagation needed up to
crates/wasi
,crates/wasi-common
, etc. Those users of wiggle are the places where I want to turn tracing on or off and the only way I see to do that is through Cargo features (just like the Wasmtime integration).
abrown commented on issue #5087:
Oh never mind me! I think I understand now that you mean use the parse-able configuration of the macro. The latest commit attempts that. Unfortunately, when I add
tracing: false
to afrom_witx!
macro, I see build errors like:wasmtime/crates/wasi-common$ cargo build Compiling wasi-common v3.0.0 (/.../wasmtime/crates/wasi-common) error[E0728]: `await` is only allowed inside `async` functions and blocks --> crates/wasi-common/src/snapshots/preview_1.rs:21:1 | 21 | / wiggle::from_witx!({ 22 | | witx: ["$WASI_ROOT/phases/snapshot/witx/wasi_snapshot_preview1.witx"], 23 | | errors: { errno => Error }, 24 | | // Note: not every function actually needs to be async, however, nearly all of them do, and ... | 29 | | wasmtime: false 30 | | }); | | ^ | | | | |__only allowed inside `async` functions and blocks | this is not `async` | = note: this error originates in the macro `wiggle::from_witx` (in Nightly builds, run with -Z macro-backtrace for more info) e ...
So somehow the macro now thinks it should async when it shouldn't?
abrown edited a comment on issue #5087:
Oh never mind me! I think I understand now that you mean use the parse-able configuration of the macro. The latest commit attempts that. Unfortunately, when I add
tracing: false
to afrom_witx!
macro, I see build errors like:wasmtime/crates/wasi-common$ cargo build Compiling wasi-common v3.0.0 (/.../wasmtime/crates/wasi-common) error[E0728]: `await` is only allowed inside `async` functions and blocks --> crates/wasi-common/src/snapshots/preview_1.rs:21:1 | 21 | / wiggle::from_witx!({ 22 | | witx: ["$WASI_ROOT/phases/snapshot/witx/wasi_snapshot_preview1.witx"], 23 | | errors: { errno => Error }, 24 | | // Note: not every function actually needs to be async, however, nearly all of them do, and ... | 29 | | wasmtime: false 30 | | }); | | ^ | | | | |__only allowed inside `async` functions and blocks | this is not `async` | = note: this error originates in the macro `wiggle::from_witx` (in Nightly builds, run with -Z macro-backtrace for more info) e ...
So somehow the macro now thinks it should async when it shouldn't or viceversa?
alexcrichton commented on issue #5087:
Unfortunately wiggle hasn't really seen much bug fixing and battle-testing over its lifespan, so what you're seeing may be a bug in wiggle. CI isn't failing, though, so is this with an individual build rather than an always-happens things?
As for the
wasmtime
feature, wiggle-of-old was used for both Lucet and Wasmtime so it had a feature to avoid using Wasmtime, but nowadays I don't think there's necessarily any need for it and we should probably eventually remove that. No need to do that here of course, mostly just that the prior design decisions of wiggle aren't necessarily great guides for what to do with it going forward since times have changed.
abrown commented on issue #5087:
Closing in favor of the rebased #5146.
Last updated: Dec 23 2024 at 12:05 UTC