search

rust-lang / miri

An interpreter for Rust's mid-level intermediate representation
Apache License 2.0
4.37k stars 337 forks source link

Run tests for all specified targets #3702

Closed Mandragorian closed 2 months ago

Mandragorian commented 3 months ago

Currently cargo-miri uses the first target specified in the command line. However, when multiple targets are specified in a cargo build invocation, cargo will build for all of them.

Miri should match this behaviour to reduce surprises.

Fixes: #3460

saethlin commented 3 months ago

I mean, we need a loop for setup

I think we actually need support in rustc-build-sysroot for passing through the full list of targets to Cargo, not a loop.

RalfJung commented 3 months ago

Why should calling setup in a loop not work?

saethlin commented 3 months ago

Ah, bad phrasing. It will work, but we don't need to use a loop. It would be much faster to let Cargo run all the sysroot builds in parallel.

RalfJung commented 3 months ago

Ah, that's what you mean. Yeah makes sense. It also complicates the copying and printing logic, though.

We can leave that as a future possibility though, it doesn't have to happen in this PR.

Mandragorian commented 3 months ago

Looking into the code more, I see that if I don't loop I get an error that error: only one --target argument is supported, which I think originates from cargo and not miri.

I also realized that the loop as I had set it up wouldn't work since exec(cmd) never returns. Multiple targets are only supported for the build subcommand. So I think that if we want to support running/testing for all targets specified in miri, we might need to spawn the command in a loop after all.

RalfJung commented 3 months ago

Multiple targets are only supported for the build subcommand.

Wait, really? We intend to mirror cargo test. If cargo test does not support multiple targets then I don't think we should, either.

But I was sure I checked this, see this comment.

Mandragorian commented 3 months ago

ah my bad. I actually run cargo miri run instead of test. sorry for the confusion.

Mandragorian commented 2 months ago

@rustbot ready

RalfJung commented 2 months ago

@rustbot author

Mandragorian commented 2 months ago

@rustbot ready

RalfJung commented 2 months ago

This looks great, thanks. :)

Can you please squash this into a single commit? (ideally while keeping the same base commit so that the force-push has an empty diff)

Mandragorian commented 2 months ago

This looks great, thanks. :)

Can you please squash this into a single commit? (ideally while keeping the same base commit so that the force-push has an empty diff)

done!

RalfJung commented 2 months ago

Great, thanks!

@bors r+

bors commented 2 months ago

:pushpin: Commit 471dcec041122b1bba8ab4fd8085f88e3bd275ff has been approved by RalfJung

It is now in the queue for this repository.

bors commented 2 months ago

:hourglass: Testing commit 471dcec041122b1bba8ab4fd8085f88e3bd275ff with merge cdaa4d595545623cce8e94ee1de4920a392b95a1...

bors commented 2 months ago

:sunny: Test successful - checks-actions Approved by: RalfJung Pushing cdaa4d595545623cce8e94ee1de4920a392b95a1 to master...