Skip to content
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

Update wasi-common to 17.0 #941

Closed
wants to merge 3 commits into from
Closed

Conversation

Manishearth
Copy link

We're on 2.0, the latest version is 18.0 though it's not yet available for wasi-cap-std-sync. 2.0 is more than a year old and still uses syn 1.0 among other older dependencies.

They greatly changed how errors work in bytecodealliance/wasmtime#5149, but hopefully the new error code here is roughly equivalent.

(Would appreciate a release if this gets merged)

@Robbepop
Copy link
Member

@Manishearth thanks a lot for the PR!

I was under the impression that newer versions post v2.0 were fixed to Wasmtime internals (or similar) and thus incompatible with other runtimes such as Wasmi. Maybe I got something wrong or the maintainers of those libraries resolved those issues some time ago again.

Happy to be back on track if we get your PR merged. :)
The clippy CI job failure is not related to your PR and the formatting should be easily fixed.

@Manishearth
Copy link
Author

Yes, I believe the versions are linked, but it's fine if you upgrade a bunch of crates together

@Robbepop
Copy link
Member

It seems like wasi-common and wiggle do have a dependency on wasmtime still but it is just optional and we do not enable it so it should be fine. What I want to avoid is having a hard dependency on Wasmtime from Wasmi because that would not make sense.

wasi-common = "2.0"
wasi-cap-std-sync = "2.0"
wiggle = { version = "2.0", default-features = false, features = ["wiggle_metadata"] }
wasi-common = "17.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at Cargo.toml of the wasi-common crate's most recent version it has the wasmtime crate feature enabled as default feature. In order to avoid building Wasmtime via Wasmi we must disable this crate feature by using default-features = false and features = ["wiggle_metadata", "trace_log", "sync"].

wiggle = { version = "2.0", default-features = false, features = ["wiggle_metadata"] }
wasi-common = "17.0"
wasi-cap-std-sync = "17.0"
wiggle = { version = "17.0", default-features = false, features = ["wiggle_metadata"] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same applies as in https://github.com/wasmi-labs/wasmi/pull/941/files#r1503430182.

We need to use default-features = false and features = ["wiggle_metadata"]

@Manishearth
Copy link
Author

Hmm, looks like wasi-cap-std-sync still brings it in

@Manishearth
Copy link
Author

I don't understand how this stuff all ties in together if you could take a look into what's going on there. We may need to add a feature toggle upstream?

@Robbepop
Copy link
Member

Robbepop commented Feb 27, 2024

Hmm, looks like wasi-cap-std-sync still brings it in

Indeed, wasi-cap-std-sync itself depends on wasi-common with default features which depends on wasmtime. So the situation is still the same as since version v3.0 which is why we did not upgrade in Wasmi. However, maybe it can nowadays be resolved (more easily) on the WASI side by introducing yet another crate feature as you have suggested.

On further inspection wasi-common/sync crate feature also pulls in Wasmtime. This is in parts because it re-exports wasi-std-cap-sync crate APIs.

Finally, wasi-common is deprecated by its maintainers stating that it is the implementation for WASI preview 1 but WASI preview 2 is already available and work on preview 3 has started.

It would be really nice if there was a Wasm engine agnostic WASI implementation but it seems we are not there, yet.

@Robbepop
Copy link
Member

@Manishearth Do you see a proper way forward? If no, can we close this again?

@Manishearth
Copy link
Author

Manishearth commented Feb 28, 2024

@Robbepop The proper way forward is to work upstream with wasi-cap-std-sync. I don't have time to do this right now but probably can at some point. It doesn't seem hard, I'm just less familiar with the bits of the wasm ecosystem.

I don't see a need to close this: others can help out with this if I don't get the time.

@Robbepop
Copy link
Member

@Robbepop The proper way forward is to work upstream with wasi-cap-std-sync. I don't have time to do this right now but probably can at some point. It doesn't seem hard, I'm just less familiar with the bits of the wasm ecosystem.

I don't see a need to close this: others can help out with this if I don't get the time.

I feel that an issue might be the best with a link to this PR which we can re-open once someone willing to work on the issue and with time to do so is found. We could tag the issue as "help needed" to hopefully attracted volunteers. How do you feel about this?

@Manishearth
Copy link
Author

Seems fine

@Robbepop
Copy link
Member

Closed in favor of #943.

@Robbepop Robbepop closed this Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants