Hey all, been working on support for the new WIT feature gating functionality -- it's required a bunch of updates (even further than what was added just last week), and I'm trying to add a test, etc.
Would love anyone with opinions/comments to take a look at the draft PR:
https://github.com/bytecodealliance/jco/pull/459
Thanks for working on this! I've left some comments in the PR.
Hey all -- made some progress here, but I think I'm stuck.
I have two tests that do what they're supposed to, and one that doesn't -- in particular @since(0.2.2)
is not being removed from a package that is at @0.2.1
-- and the Stability
values I'm getting from the functions are all actually Unknown
when I'd expect them to be marked with versions
The PR is updated (it seems to be steadily growing...) but the relevant wit is:
package test:feature-gates@0.2.1;
interface foo {
a: func();
@since(version = 0.2.1)
b: func();
@since(version = 0.2.2)
c: func();
@unstable(feature = fancier-foo)
d: func();
}
world import-and-export {
import foo;
export foo;
}
I've also added printlns so if you comment out the tests that aren't witTest
you'll see output coming from ts_bindgen.rs
that looks like this:
│> @bytecodealliance/jco@1.3.1 test │
│> node --stack-trace-limit=100 node_modules/mocha/bin/mocha.js -u tdd test/test.js --timeout 30000 │
│ │
│ │
│ │
│ WIT │
│[TS_BINDGEN] generating TS func: [a] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [b] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [c] (stability: Unknown) │
│ │
│ │
│[TEST] interfaces file content? export namespace TestFeatureGatesFoo { │
│ export function a(): void; │
│ export function b(): void; │
│ export function c(): void; │
│} │
│ │
│ 1) feature-gates: since should exclude future versions │
│[TS_BINDGEN] generating TS func: [a] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [b] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [c] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [d] (stability: Unknown) │
│ ✔ feature-gates: unstable should be present with all features enabled │
│[TS_BINDGEN] generating TS func: [a] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [b] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [c] (stability: Unknown) │
│ ✔ feature-gates: unstable should not be present without all features enabled
Just want to make sure I'm not misunderstanding the feature, but I'd expect to not see c()
because it's @since(0.2.2)
and the package is @0.2.1
.
I also tried with 0.3.0
just to be sure I wasn't running into a semver compat smoothing thing.
Weirdly enough, unstable
works just fine w/ all features specified (those are the passing tests)
@Victor Adossi this looks amazing. Given you're seeing information being lost, I'm wondering if the issue here is just lower-level then? There seems to have been a bit of movement in this code in wasm-tools. Perhaps this is an issue in the Wasmtime machinery itself? I wonder if the unreleased wasm-tools code here will fix it?
Appreciate all the help so far :bow:
I think you're right, I'm going to take a look at the lower level wasm-tools again -- I'm already on 0.212.0
, but it really feels like they shouldn't all be Unknown
, there may have been some movement there I haven't pulled down... Looking at the upstream code I'm also fairly certain I shouldn't even see the import on the to begin with (like how unstable
worked)...
Yesterday I encountered a panic in wit-parser because Wasi-cli's since isn't merged, yet. This was due to a mismatching since and unknown on the same interface.
Perhaps upgrading cli to Joel's version already fixes things.
mismatching since & unknown on the same interface is a use case I don't even have in the test yet -- I should add it.
Would you mind sharing a snippet of the WIT, @Christof Petig ?
I will need to gather the details, just combining Wasi-cli main with main on the other Wasi dependencies (io, fs, clocks) using wit-deps created the problem for us.
Ahh thanks, let me take a look and see those changes, maybe no need to waste cycles gathering the details just yet, though I'm going to add that test
:thinking: it was far more difficult to reproduce than I thought. Here it is: take this wit/deps.toml and wit/test.wit combo, run wit-deps update
and then wit-bindgen markdown wit
.
It crashes with
thread 'main' panicked at …/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wit-parser-0.212.0/src/resolve.rs:2350:21:
assertion `left == right` failed
left: Interface { id: Id { idx: 1 }, stability: Unknown }
right: Interface { id: Id { idx: 1 }, stability: Stable { since: Version { major: 0, minor: 2, patch: 0 }, feature: None } }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
If you replace either clocks with v0.2.0 or cli with the version by JoelYoshua the crash is gone.
wasi-problem.tgz
Version by Yoshua at https://github.com/WebAssembly/wasi-cli/pull/43
But to be honest I haven't yet figured out which interface is referenced both with and without since.
Here is the full version (no wit-deps update
required to retrieve the deps)
wasi-problem.full.tgz
Thanks for providing the tarballs -- I'm not sure I'm completely right (maybe you could confirm) but the line that is being pointed to seems to be this one:
https://github.com/bytecodealliance/wasm-tools/blob/v1.212.0/crates/wit-parser/src/resolve.rs#L2350
It looks like wit parser expects stabilities to never change, and we're getting the interface in there as Unknown
at one poitn then the newer version actually has it specified?
Wonder if that assert_eq!
should be a bail!
with a more descriptive error message
Ideally it would output the name of the interface which got redefined, but due to rearranging of the structure during the merge this is a far from easy to get information. Another solution would be to ignore unknown in the comparison.
And yes this assert_eq is exactly the panicking line.
I feel filling a bug report on wit-parser would be adequate.
Filed an issue:
https://github.com/bytecodealliance/wasm-tools/issues/1666
@Yoshua Wuyts would you mind taking a look if you find some time? The thing is that the check makes sense -- the same item shouldn't have it's Stability
essentially discovered twice (and be different)... so the assert makes sense, and however we're getting the Unknown
in there in the first place is probably what's jamming me up on thejco
impl
Hmnnn, so I've updated to 0.214.0 and relevant packages but still see the issue -- all stabilities are still coming in as Unknown
.
Still looking into why this could be, maybe the right annotation is ending up in the wrong place possibly
OK so I think I've found where the issue might be -- it seems that update_stability
is not called when moving worlds, and in particular moving interfaces...
I'm wondering if there's not enough copying/moving done there -- when I print out the stabilities for the items as they pass through, they're correct (non-Unknown
)
Ah some interesting discussion on how the feature gating should work in the first place:
Found the bit that was missing -- but maybe it should be missing
Last updated: Dec 23 2024 at 12:05 UTC