-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
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.
I reckon that even with the issues listed in the PR's description, this is a much better setup than the previous one, so +1 from me.
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.
Please also remove the exclude = ["misc_testsuite/*.wasm"]
from the top-level Cargo.toml :-).
build.rs
Outdated
cmd.args(&["build", "--release", "--target=wasm32-wasi"]) | ||
.stdout(Stdio::inherit()) | ||
.stderr(Stdio::inherit()) | ||
.current_dir(testsuite); |
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.
Would it work to add a --target-dir to put this build's target somewhere inside OUT _DIR, or the outer cargo build's target dir, to fix cargo clean
?
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.
Done. I thought it would conflict with the cargo locks on build directory, but it appears it doesn't.
@sunfishcode are you happy with the PR so that we could merge it? :-) |
I thought about it for a while more, and there's one more problem with the current approach. If a test is broken, it may occur that it's impossible to make it pass in wasi-common, forcing us to do a revert. Adding a CI check here is a chicken&egg problem, so I suggest every PR to this repository should have a corresponding PR to wasi-common, bumping the revision of the submodule. (this can be a part of the PR template on GitHub) Alternatively we can keep the tests in this repo, but it will make it harder to reuse the testsuite for other projects. (actually, I think I prefer this solution as a simpler one) |
Hmm, I’m not sure I see the problem here, but maybe if I explain how I understand it, you’ll be able to point me in the right direction. So i thought that, since |
It's true that there'll be much less activity here, it's why enforcing a matching PR to wasi-common during review is also ok. |
Ah, OK, I get it now. Thanks for the explanation :-) I feel that there is option 3 that doesn’t require creating a matching PR to this repo immediately after a new test case lands in |
Yes, this looks good to me! |
Now we require the compiler toolchain to have both the host target and the
wasm32-wasi
target. Issues with this PR:cargo clean
doesn't removemisc_testsuite/target
cargo build
(cf. Add support for test build scripts rust-lang/cargo#1581)Closes CraneStation/wasi-misc-tests#18