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

WASI Integration #1068

Closed
wants to merge 21 commits into from
Closed

WASI Integration #1068

wants to merge 21 commits into from

Conversation

syrusakbary
Copy link

@syrusakbary syrusakbary commented Apr 23, 2019

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:

# First, let's install wapm
curl https://get.wasmer.io -sSfL | sh

# Then, install wabt
wapm install wabt

# And then, you can use wasm2wat, ...
wapm run wasm2wat ...

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

@binji
Copy link
Member

binji commented Apr 23, 2019

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 .js scripts don't seem to work with node (I just get stat failed: No such file or directory). Not sure why, but we may just want to include the .wasm files.

@sbc100
Copy link
Member

sbc100 commented Apr 24, 2019

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?

@syrusakbary
Copy link
Author

@binji @sbc100 I have completely re-worked on the PR.

This PR is now much cleaner. Builds the executables directly with WASI and doesn't check the binaries in the repo.
You can build the WASI executables just by doing:

./scripts/build_wasi.sh

Looking forward hearing your thoughts!

Copy link
Member

@sbc100 sbc100 left a 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?

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';;
Copy link
Member

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?

Copy link
Author

@syrusakbary syrusakbary Sep 26, 2019

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)

Copy link
Member

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

Copy link
Author

Choose a reason for hiding this comment

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

Awesome!

@syrusakbary
Copy link
Author

@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 :)

@sbc100
Copy link
Member

sbc100 commented Sep 26, 2019

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 dist/wapm or at least under wapm subdirectory? I could be overthinking it.. what does @binji think?

@syrusakbary
Copy link
Author

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 dist/wapm or at least under wapm subdirectory?

wapm.toml is generally positioned in main dir for convenience, similarly on how CI tools (travis or appveyor), or other package managers position the manifests in the root dir (package.json, requirements.txt, Cargo.toml, ...).

However I'm happy to change dirs if you think that would make more sense

@sbc100
Copy link
Member

sbc100 commented Sep 26, 2019

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 dist/wapm or at least under wapm subdirectory?

wapm.toml is generally positioned in main dir for convenience, similarly on how CI tools (travis or appveyor), or other package managers position the manifests in the root dir (package.json, requirements.txt, Cargo.toml, ...).

However I'm happy to change dirs if you think that would make more sense

I think the difference with requirements.txt, Cargo.toml or package.json, is that they are linked to the language the project is written in. I would see that as more akin the CMakeLists.txt file here in wabt. I see wapm.toml as a description of how to distribute the program on one particular platform, more like packaging for debian/ubuntu/redhat etc or a windows nsis installer script. Perhaps I'm nitpiking too much and such files should be welcome at the top level? I'm curious what others think? I'm also curious about other projects? Have other C/C++ projects been ok with this stuff in the root?

I'm also curious if being in the root dir is required? Perhaps we can allow wapm to accept metadata outside the root dir?

@syrusakbary
Copy link
Author

Perhaps I'm nitpiking too much and such files should be welcome at the top level? I'm curious what others think?

Yeah, I agree that it's not trivial to see what's the ideal scenario. I would love to get other people thoughts 🙂

I'm also curious about other projects? Have other C/C++ projects been ok with this stuff in the root?

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!).

I'm also curious if being in the root dir is required?

The manifest have to be in a parent dir of the WebAssembly files exposed. Doesn't need to be the root per-se though

Perhaps we can allow wapm to accept metadata outside the root dir?

It will require a bit of engineering effort on the wapm cli side, but it's completely feasible.
Perhaps the first thing that we need to figure out is where to position the wapm.toml config, and then adapt the ergonomics :)

@sbc100
Copy link
Member

sbc100 commented Sep 26, 2019

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 wapm.toml continue to live downstream and be maintained by package maintainers as it would be in debian/redhat/etc. Pushing this stuff upstream to C/C++ projects seems like the difficult direction. They/We don't want to give any particular distro special status, and we can't possible support all distros upstream. I obviously can't speak to other projects but I imagine you will meet similar pushback there.

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:
https://github.com/freebsd/freebsd-ports/tree/master/editors/vim

Interstingly waby doesn't seem to be in freebsd ports .. somebody should add it :)

@tlively
Copy link
Member

tlively commented Sep 26, 2019

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.

@binji
Copy link
Member

binji commented Sep 26, 2019

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.

@sbc100
Copy link
Member

sbc100 commented Oct 11, 2019

Would you mind splitting this PR so we can land the wasi builder script on its own?

@syrusakbary
Copy link
Author

Would you mind splitting this PR so we can land the wasi builder script on its own?

Yes! Will do that soon :)

@syrusakbary syrusakbary changed the title WAPM Integration WASI Integration Oct 22, 2019
@syrusakbary
Copy link
Author

And... done! ✅

I removed all the references to WAPM in this PR, focusing only on compiling to WASI.

But... how? This PR now uses wasienv to compile easily to WebAssembly WASI (article coming up soon!)

@sbc100 @binji the PR should be ready to re-review :)

@syrusakbary
Copy link
Author

@aardappel
Copy link
Contributor

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).

@syrusakbary
Copy link
Author

syrusakbary commented Oct 24, 2019

Since WABT is a very general set of tools, it does not make sense to me to make it depend upon wasienv.

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 wabt wants WASI compilation target?

I think the main question that we have to answer is whether wabt wants to have first-class support for WASI in the main repo, and if so, what would be the path forward for that.

In the case that WASI compilation is desired in wabt, there will always be some kind of dependency that will need to be fulfilled (either downloading the SDK for the specific system/platform and having the CMake configuration apart, using wasienv which will make the process much easier, ...).

Summary

I thought using wasienv could be very useful for this project to enable WASI compilation in a very simple and easy way... but feel free to close this PR if you think that's not the case!

I'm here to help, but only if the help is desired and useful for the project (it's fine if it's not, no hard feelings!) 🙂

@sbc100
Copy link
Member

sbc100 commented Oct 29, 2019

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:

  1. Rename this script to scripts/travis-wasi.sh and have it just to the building .. no downloading of anything (see travis-emcc.sh).
  2. Add test-wasi to the build matrix that downloads the wasi-sdk (just the linux version) (as in a previous of the PR) and similar to the existing test-emcc (but without the need for docker) that runs scripts/travis-wasi.sh with $WASI_SDK_ROOT set.

Developers who want to build locally can then call scripts/travis-wasi.sh with $WASI_SDK_ROOT rather than having scripts/travis-wasi.sh do any downloading.

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.

Base automatically changed from master to main March 22, 2021 16:56
@keithw
Copy link
Member

keithw commented Aug 16, 2022

Closing as stale. (wasienv also appears to have gone dormant, or maybe just stable.)

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.

6 participants