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

testsuite now requires installing wasm32-wasi target #595

Closed
joshtriplett opened this issue Nov 19, 2019 · 9 comments
Closed

testsuite now requires installing wasm32-wasi target #595

joshtriplett opened this issue Nov 19, 2019 · 9 comments
Labels
wasi:tests Issues pertaining to WASI tests in Wasmtime

Comments

@joshtriplett
Copy link
Member

I ran into some mysterious testsuite failures, and managed to figure out that they occurred because I didn't have the wasm32-wasi target installed for my stable toolchain.

I think this needs documenting somewhere. It's not at all an unreasonable requirement, but since it doesn't occur automatically, we should document the need to install it before running the testsuite.

@sunfishcode
Copy link
Member

This is unintended -- the crate that uses the wasm32-wasi target is test-programs, and it's marked as optional = true in the top-level Cargo.toml. Can you say how you're invoking cargo?

@joshtriplett
Copy link
Member Author

@sunfishcode cargo test --all --exclude wasmtime-py --exclude lightbeam

I'm excluding wasmtime-py because of #468, and lightbeam because I'm running stable; when I test on nightly I use cargo +nightly test --all --exclude wasmtime-py.

@sunfishcode
Copy link
Member

One possible solution I am contemplating is to check in the compiled .wasm files into the repository, so if the wasm32-wasi target isn't available, we can still run the tests.

We'll likely have test programs written in C at some point too, which will need wasi-sdk or similar installed as well, and not everyone will have that either. And hopefully we'll have other languages too. Having a collection of precompiled wasms would make is easy to run all the tests regardless of what the developer has installed.

That said, I haven't decided anything yet, and am open to other ideas.

@joshtriplett
Copy link
Member Author

I personally don't have a problem with needing to install a wasm32-wasi target, but I definitely don't think a C SDK should be required. And in any case, we probably want the test suite to use the same binaries every time rather than depending on what rustc generates.

Could we put them in a separate repository instead?

@kubkon
Copy link
Member

kubkon commented Nov 19, 2019

I personally don't have a problem with needing to install a wasm32-wasi target, but I definitely don't think a C SDK should be required. And in any case, we probably want the test suite to use the same binaries every time rather than depending on what rustc generates.

Could we put them in a separate repository instead?

We've already had this setup before and it proved a nightmare for development. Keeping two repos, which change a lot and often, in sync is a proper nightmare, so I strongly disagree with separating the tests out into a separate repo at this stage. FWIW, after the tests and wasi-common matures enough to warrant first stable release, we can revisit this case as, by then, I suspect the tests being matured enough not to change too much, too often.

Until then, is there any reason why we can't have build.rs for test-programs behind a feature-gate as it was prior to merging wasi-common into wasmtime? I know it's not ideal but it works as expected, and until dev-dependencies support optional dependencies, I cannot see any other simple way around it.

kubkon pushed a commit to kubkon/wasmtime that referenced this issue Nov 19, 2019
This commit feature-gates `test-programs` as a workaround for
`cargo test` not allowing for optional dev-dependencies.

This commit addresses bytecodealliance#595.
@kubkon
Copy link
Member

kubkon commented Nov 19, 2019

OK, I've now opened #600 which re-introduces feature-gating of the test-programs. Have a look, and let me know what you guys reckon!

kubkon pushed a commit to kubkon/wasmtime that referenced this issue Nov 19, 2019
This commit feature-gates `test-programs` as a workaround for
`cargo test` not allowing for optional dev-dependencies.

This commit addresses bytecodealliance#595.
kubkon pushed a commit to kubkon/wasmtime that referenced this issue Nov 20, 2019
This commit feature-gates `test-programs` as a workaround for
`cargo test` not allowing for optional dev-dependencies.

This commit addresses bytecodealliance#595.
kubkon pushed a commit to kubkon/wasmtime that referenced this issue Nov 20, 2019
This commit feature-gates `test-programs` as a workaround for
`cargo test` not allowing for optional dev-dependencies.

This commit addresses bytecodealliance#595.
kubkon pushed a commit that referenced this issue Nov 21, 2019
This commit feature-gates `test-programs` as a workaround for
`cargo test` not allowing for optional dev-dependencies.

This commit addresses #595.
@kubkon
Copy link
Member

kubkon commented Nov 21, 2019

I've now merged #600, however, I'm inclined to leave this issue open, as it might become even more relevant as we start adding fuzzing of our WASI implementation. See #611 for relevant initial discussion.

@marmistrz
Copy link
Contributor

Another problem with keeping the tests as binaries was that it was really hard to keep track which version was actually committed and fixing merge conflicts was horrible.

We could probably just require the users to have clang installed (we probably even require this nowadays) and have just the WASI sysroot as a submodule.

@kubkon kubkon added the wasi:tests Issues pertaining to WASI tests in Wasmtime label Nov 29, 2019
@alexcrichton
Copy link
Member

Going through some older issues, this is now a requirement of Wasmtime and this is a pretty old issue as well, so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi:tests Issues pertaining to WASI tests in Wasmtime
Projects
None yet
Development

No branches or pull requests

5 participants