iawia002 opened PR #8928 from iawia002:wit-deps
to bytecodealliance:main
:
This patch uses the wit-deps package to manage all wit dependencies (except wasi-nn for now):
https://github.com/bytecodealliance/wasmtime/blob/7caca1624a3c68dd4135b3941e7c87d94eb4c7db/ci/vendor-wit.sh#L40-L43I believe this is a more understandable and automated approach compared to the bash script, as it will automatically synchronize these wit dependencies during the build process.
iawia002 requested wasmtime-default-reviewers for a review on PR #8928.
iawia002 requested fitzgen for a review on PR #8928.
iawia002 requested wasmtime-core-reviewers for a review on PR #8928.
iawia002 updated PR #8928.
iawia002 updated PR #8928.
alexcrichton commented on PR #8928:
Thanks for the PR! While I don't disagree that this is better than bash in terms of readability this has a downside of depending on
wit-deps
at build time which is a bit of a hefty dependency to add to the build. For now I think it would be best to keep the same style of build where the wit files are checked in CI to be correct but otherwise there's no extra build dependencies for downstream users.
Thanks for the PR! While I don't disagree that this is better than bash in terms of readability this has a downside of depending on
wit-deps
at build time which is a bit of a hefty dependency to add to the build. For now I think it would be best to keep the same style of build where the wit files are checked in CI to be correct but otherwise there's no extra build dependencies for downstream users.What I’m doing in a project of mine at the moment is to use wit-deps in a test to check that everything is up to date (but to also keep the wit field checked in … well technically my solution uses git submodules to avoid checking the files in directly, but that doesn’t matter for the test) - this makes the dependency a dev-dependency only so that it should not burden any downstream users
fitzgen commented on PR #8928:
Seconding what Alex said: we should run this in CI, not on every single build of the
wasmtime
crate.
iawia002 commented on PR #8928:
Thank all for your feedback. I understand your concerns and have made some improvements. I will no longer use
build.rs
to update dependencies. The process will remain as it is now, but developers will need to install thewit-deps
CLI tool to update wit files locally.Going back to the original reason why I wanted to make this change:
I noticed that the current bash script keeps the dependencies of the
wasi
andwasi-http
crates in sync, but in fact, thewasi
crate does not need thehttp
world: https://github.com/bytecodealliance/wasmtime/blob/0e5048e20a24f6da19b3794eeb94111f052dc414/crates/wasi/src/bindings.rs#L148-L150
In the bash script, achieving this differentiation requires more code. If this patch is accepted, I will submit another PR to do some cleanup.When I tried to implement
wasi-runtime-config
, since it is still a draft version, its tag is different fromcli
andhttp
etc. To manage its wit dependencies, I had to add some code to the bash script to download files individually. If there are more different versions of wit dependencies in the future, the bash script will become very complex.PTAL if this change is reasonable.
iawia002 updated PR #8928.
iawia002 updated PR #8928.
alexcrichton commented on PR #8928:
Functionally this is much better, thanks! Personally though I'd at least personally prefer to stick to the bash script for now. I'm worried to pick up another tool as part of our build which much of us don't understand and it's not buying us much over a bash script. I don't disagree that the bash script is going to get a bit more complicated over time but I think the complexity is relatively manageable. For example it's not really an issue that HTTP WIT files are part of the
wasmtime-wasi
crate. Adding support for downloading another version of things to a different location shouldn't greatly increase the complexity of the script either.Some other reasons I'd prefer to stick to a script for now:
- Installation of
wit-deps
is yet-another-tool to manage in CI. For examplecargo install
here doesn't use--locked
to prevent errors over time with mistakes in publishing to crates.io. It also doesn't specify a version to install so we'd be susceptible to breakage in terms of ifwit-deps
changes. This is all fixable but is adding more overhead relative to a script.- We can't exclusively use
wit-deps
because wasi-nn doesn't support it. That means we still need a script to do things. Additionally the script is required to matter what to pass the right arguments towit-deps
.Overall I think it may just not be the right time to switch away from bash scripts. If the shell script could go away entirely I think that's the right time to migrate to a different tool, but I don't think that we're there yet.
alexcrichton closed without merge PR #8928.
Last updated: Dec 23 2024 at 12:05 UTC