Wasmtime GitHub notifications bot (Jul 10 2024 at 04:33): Wasmtime GitHub notifications bot (Jul 10 2024 at 04:33):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.
Wasmtime GitHub notifications bot (Jul 10 2024 at 04:33):iawia002 requested wasmtime-default-reviewers for a review on PR #8928.
Wasmtime GitHub notifications bot (Jul 10 2024 at 04:33):iawia002 requested fitzgen for a review on PR #8928.
Wasmtime GitHub notifications bot (Jul 10 2024 at 04:33):iawia002 requested wasmtime-core-reviewers for a review on PR #8928.
Wasmtime GitHub notifications bot (Jul 10 2024 at 04:42):iawia002 updated PR #8928.
Wasmtime GitHub notifications bot (Jul 10 2024 at 04:51):iawia002 updated PR #8928.
Wasmtime GitHub notifications bot (Jul 10 2024 at 15:10):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-depsat 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.
Wasmtime GitHub notifications bot (Jul 10 2024 at 15:18):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-depsat 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
Wasmtime GitHub notifications bot (Jul 10 2024 at 17:28):fitzgen commented on PR #8928:
Seconding what Alex said: we should run this in CI, not on every single build of the
wasmtimecrate.
Wasmtime GitHub notifications bot (Jul 11 2024 at 02:05):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.rsto update dependencies. The process will remain as it is now, but developers will need to install thewit-depsCLI 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
wasiandwasi-httpcrates in sync, but in fact, thewasicrate does not need thehttpworld: 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 fromcliandhttpetc. 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.
Wasmtime GitHub notifications bot (Jul 11 2024 at 02:06):iawia002 updated PR #8928.
Wasmtime GitHub notifications bot (Jul 11 2024 at 06:57):iawia002 updated PR #8928.
Wasmtime GitHub notifications bot (Jul 11 2024 at 14:47):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-wasicrate. 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-depsis yet-another-tool to manage in CI. For examplecargo installhere doesn't use--lockedto 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-depschanges. This is all fixable but is adding more overhead relative to a script.- We can't exclusively use
wit-depsbecause 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.
Wasmtime GitHub notifications bot (Jul 17 2024 at 15:17):alexcrichton closed without merge PR #8928.
Last updated: Oct 23 2024 at 20:03 UTC