-
Notifications
You must be signed in to change notification settings - Fork 730
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
WASI Integration #1068
WASI Integration #1068
Conversation
Thanks, looks pretty good! I'm not sure this needs to be upstream (see for example, https://www.npmjs.com/package/wabt and https://github.com/AssemblyScript/wabt.js). One downside is that it will increase the size of the source repo, since the files are checked-in. That said, the binaries aren't too big, and it might be convenient for others to have these files readily available. I also noticed that the |
This is indeed pretty exciting. I kind of agree with ben that package manager metadata tends not to live in upstream repos, but I can also see an argument for it in this case since wabt is such a core part of wasm. Is there a reason why the binaries need to be checked in? This change would be pretty tiny without them, right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is way better now, thanks for taking the time!
Would you mind splitting this into 2 PRs: "Add wasi build" then "wapm integration"?
The first one can just be could just include CMakeLists.txt
, wasi-sdk.cmake
and scripts/build_wasi.sh? We could also consider adding build_wasi.sh to travis (assuming its not too slow).
Then the README-wapm.md
and wapm.toml
can be a followup?
scripts/build_wasi.sh
Outdated
if [[ ! -d wasi-sdk-5.0 ]]; then | ||
OS=$(uname | tr '[:upper:]' '[:lower:]') | ||
case "$OS" in | ||
darwin) WASI_URL='https://github.com/CraneStation/wasi-sdk/releases/download/wasi-sdk-5/wasi-sdk-5.0-macos.tar.gz';; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using version 6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bundled version it's only available for linux 🙁 (without macOS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it just got added: WebAssembly/wasi-sdk#59
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
@sbc100 feedback addressed! I think I can address Travis automation in another PR 🙂 (and even automatic deployment to wapm! 🎉) Note: if we can review wapm in this PR it would make my life a bit easier! I'm super happy to iterate on it though if you have any feedback :) |
The reason I suggest the split is that part of this change (adding a wasi build) is completely uncontroversial. Its the adding of distro-specific files in the top level directory that I think is a somewhat controversial. Just like we would probably wouldn't want debian or redhat packaging rules here at the top level. How about putting them under |
However I'm happy to change dirs if you think that would make more sense |
I think the difference with I'm also curious if being in the root dir is required? Perhaps we can allow wapm to accept metadata outside the root dir? |
Yeah, I agree that it's not trivial to see what's the ideal scenario. I would love to get other people thoughts 🙂
In general projects have been happy positioning the manifest config in the root. However, they have been mostly Rust/AssemblyScript projects. Most C/C++ projects we forked them and we haven't created a PR to them yet (this is the first one!).
The manifest have to be in a parent dir of the WebAssembly files exposed. Doesn't need to be the root per-se though
It will require a bit of engineering effort on the wapm cli side, but it's completely feasible. |
This dicussion is why I suggested to split the wasi-building part of the PR out, so that we could discuss that separably from the wapm metadata. I would rather see One approach (that I believe freebsd ports takes) is to put the metadata for all packages into a single repo that package maintainers then commit to. Have you considered that approach. Then the entire wapm universe could be described in a single place and you can do thinks like "make world" to build every single package. For example here is the freebsd vim package data: Interstingly |
I don’t have an opinion about whether the packaging metadata should be upstream or elsewhere, but if we do decide to have it in-tree, it seems reasonable to me to put it and the binary artifacts together in some separate subdirectory. |
I agree with @sbc100, I think it'll be nicer to have the materials required for packaging kept in a separate repo. This gives us a lot of flexibility too -- so the package can change without having to land a change in the upstream repo, and so changes to wabt don't break the packaging scripts. |
Would you mind splitting this PR so we can land the wasi builder script on its own? |
Yes! Will do that soon :) |
Here's the article analyzing wasienv! |
Since WABT is a very general set of tools, it does not make sense to me to make it depend upon wasienv. It be better to add this script to wasienv (make it depend on WABT instead). |
I'm not sure if I understand correctly. I think wasienv falls in the same category as Emscripten? (Emscripten is already used in this project) Do
|
Sorry for the delay responding here. I'm afraid I haven't had a change to take a deep look at wasienv. I think such tool could be a useful part of the ecosystem. However, I'm wary of taking a dependency on it at this point in time. I suggest the following approach, to mirror the current support for emscripten:
Developers who want to build locally can then call I'm sorry for so much back and forth on this PR. I do think useful things have already come out of it and I hope we can still land it still. I do hope to see wabt as part of wapm and hopefully this change will mean that we are doing CI to ensure that the wasi build doesn't regress. I plan on keeping an eye wasienv as it evolves and maybe it will make sense to transition to using it at later date. |
# Conflicts: # CMakeLists.txt
Closing as stale. (wasienv also appears to have gone dormant, or maybe just stable.) |
This PR brings WASI to wabt using wasienv.
Old description - wapm integration
We just released WAPM: a Package Manager for WebAssembly (announcement here).
We adapted wabt to emit .wasm files (using Emscripten) and then published to wapm so they can be used very easily (just one install command) across any OS/platform.
You can try it with:
It would be awesome if it could be integrated (maybe in the CI?) so any new releases are published automatically.
Let me know your thoughts!! :)
In order to be able to transfer you the ownership of the
wabt
package on wapm, I would need an username. You can register here: https://wapm.io/signup