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

Use .wasm extension when building for wasm in cargo-miri #2685

Merged
merged 1 commit into from
Nov 27, 2022

Conversation

Noratrieb
Copy link
Member

WASM uses the .wasm file extension for its binaries (just like how windows uses .exe), so we need to set that as well.

I'm not sure whether gating this behind the wasm target is a good idea, maybe it makes more sense to always do it just like on windows.

cargo-miri/src/phases.rs Outdated Show resolved Hide resolved
cargo-miri/src/phases.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

Thanks!

We should also test this. Usually we just add new targets in ci.sh but I don't know if enough of wasm works for that to make sense?

@Noratrieb Noratrieb force-pushed the cargo-miri-wasm branch 2 times, most recently from 26dc948 to 8e21633 Compare November 21, 2022 18:20
ci.sh Outdated
@@ -85,7 +85,7 @@ case $HOST_TARGET in
MIRI_TEST_TARGET=i686-pc-windows-msvc run_tests
MIRI_TEST_TARGET=x86_64-unknown-freebsd run_tests_minimal hello integer vec panic/panic concurrency/simple atomic data_race env/var
MIRI_TEST_TARGET=aarch64-linux-android run_tests_minimal hello integer vec panic/panic
MIRI_TEST_TARGET=wasm32-wasi run_tests
MIRI_TEST_TARGET=wasm32-wasi MIRI_NO_STD=1 run_tests_minimal # supports std but miri doesn't support it
Copy link
Member

Choose a reason for hiding this comment

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

run_tests_minimal expects as argument a filter to indicate which tests should be run. No filter means all tests.

However, run_tests_minimal also does not test cargo miri, so... it's not really helpful for this PR.

@RalfJung
Copy link
Member

Supporting wasi will take a while, that needs a bunch of wasi APIs to be implemented in Miri. We should probably find a way to land this without doing that.

I would suggest a 2-step process:

  • in a first PR we make run_tests_minimal also test cargo miri run on some minimal no-std crate.
  • then this PR can land with MIRI_TEST_TARGET=wasm32-wasi run_tests_minimal no_std (I don't think MIRI_NO_STD is needed, the sysroot can be built with std)

@RalfJung RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Nov 23, 2022
bors added a commit that referenced this pull request Nov 26, 2022
…o-miri-actually-works-kinda, r=RalfJung

Test a small cargo-miri smoke test even in `run_tests_minimal`

This makes sure that cargo-miri works on all targets.

Implements the first step of #2685 (comment) to get that PR tested.
@RalfJung
Copy link
Member

RalfJung commented Nov 26, 2022

#2690 landed, so hopefully we can test cargo-miri on wasm in that way. :) A rebase should show whether that works.

ci.sh Outdated Show resolved Hide resolved
WASM uses the `.wasm` file extension for its binaries (just like how
windows uses `.exe`), so we need to set that as well.
@Noratrieb
Copy link
Member Author

It works, CI passed!

@RalfJung
Copy link
Member

RalfJung commented Nov 27, 2022

The bad news is that the smoke test isn't actually testing what it claims to test... see #2701.

@RalfJung
Copy link
Member

All right, let's see if they still pass.
@bors r+

@bors
Copy link
Contributor

bors commented Nov 27, 2022

📌 Commit 66a88c2 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 27, 2022

⌛ Testing commit 66a88c2 with merge 582cd61...

@bors
Copy link
Contributor

bors commented Nov 27, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 582cd61 to master...

@bors bors merged commit 582cd61 into rust-lang:master Nov 27, 2022
@Noratrieb Noratrieb deleted the cargo-miri-wasm branch November 27, 2022 14:15
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Nov 27, 2022
…that-cargo-miri-actually-works-kinda, r=RalfJung

Test a small cargo-miri smoke test even in `run_tests_minimal`

This makes sure that cargo-miri works on all targets.

Implements the first step of rust-lang/miri#2685 (comment) to get that PR tested.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: Waiting for the PR author to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants