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

Reuse cargo's binary selection in cargo miri #739

Closed
RalfJung opened this issue May 21, 2019 · 20 comments · Fixed by #1540
Closed

Reuse cargo's binary selection in cargo miri #739

RalfJung opened this issue May 21, 2019 · 20 comments · Fixed by #1540
Labels
A-cargo Area: affects the cargo wrapper (cargo miri) C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement

Comments

@RalfJung
Copy link
Member

Currently, cargo miri run will run all the binaries in the project (ouch) and cargo miri test will test all binaries and libraries. This leads to issues like #700 (we run tests even when they are disabled), and it means one cannot run examples with Miri.

We should find a way to re-use cargo's selection of binaries, tests and so on.

@RalfJung RalfJung added C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement A-cargo Area: affects the cargo wrapper (cargo miri) labels May 21, 2019
@RalfJung
Copy link
Member Author

Related: #1001

@jonhoo
Copy link

jonhoo commented May 6, 2020

To add to this, I think it'd be helpful to support all the various target specifiers that cargo build (and cargo test) supports. A lightly edited list:

$ cargo help build
...
OPTIONS:
    -p, --package <SPEC>...          Package to build (see `cargo help pkgid`)
        --workspace                  Build all packages in the workspace
        --exclude <SPEC>...          Exclude packages from the build
        --lib                        Build only this package's library
        --bin <NAME>...              Build only the specified binary
        --bins                       Build all binaries
        --example <NAME>...          Build only the specified example
        --examples                   Build all examples
        --test <NAME>...             Build only the specified test target
        --tests                      Build all tests
        --bench <NAME>...            Build only the specified bench target
        --benches                    Build all benches
        --all-targets                Build all targets
        --release                    Build artifacts in release mode, with optimizations
        --features <FEATURES>...     Space-separated list of features to activate
        --all-features               Activate all available features
        --no-default-features        Do not activate the `default` feature

All packages in the workspace are built if the `--workspace` flag is supplied. The
`--workspace` flag is automatically assumed for a virtual manifest.
Note that `--exclude` has to be specified in conjunction with the `--workspace` flag.

This may just be a matter of being able to pass custom flags to cargo build, which I don't think is currently possible, no matter how many -- you pass :p

@RalfJung
Copy link
Member Author

RalfJung commented May 6, 2020

This may just be a matter of being able to pass custom flags to cargo build, which I don't think is currently possible, no matter how many -- you pass :p

Maybe... historically we used cargo rustc but that changed (fairly) recently, so maybe resolving this issue here is now easier than before. But currently we still use cargo_metadata to parse the Cargo.toml and get a list of all targets ourselves and then iterate over them.

Basically, all flags before the first -- are meant to go to cargo. But how weird would it be for cargo miri run/cargo miri test to support the flags of cargo build (and not cargo run/cargo test)?

@RalfJung
Copy link
Member Author

RalfJung commented May 6, 2020

Hm, but relying on cargo build to do the enumeration could (a) end up running multiple Miri in parallel (I think you saw that with some of your flag combinations), and (b) make #584 much harder.

@CAD97
Copy link

CAD97 commented May 10, 2020

one cannot run examples with Miri

I'm.. having the opposite problem right now. cargo +nightly miri test --all-features --all-targets is running both the tests in my examples (intended) and the examples' main (unintended). The invocation without miri, cargo +nightly test --all-features --all-targets does not run the example main. (This is important for this example, because the main is a REPL, which obviously isn't good for miri on CI.)

I'm currently using a cfg to just replace the example main under miri, but it'd be nice if I didn't have to (and still run the examples' tests under miri).

Here's an example run.

@RalfJung
Copy link
Member Author

That's odd, the match here should only cover tests. My best guess is the extra flags are confusing the cargo check call... I don't know what cargo check --bin <name> --profile test --all-features --all-targets does but it is probably weird.^^

@CAD97
Copy link

CAD97 commented May 10, 2020

IIUC, --all-targets overrides any narrowing options. IOW, in the presence of none of --lib, --bin <NAME>..., --bins, example <NAME>..., --examples, --test <NAME>..., --tests, --bench <NAME>..., --benches, or --all-targets, the default targets are built (IIUC, --lib if no bins are present, --bin <name> if a single bin is (or default bin is configured), and error if multiple bins are present (and no default bin is configured).) But if any of the above flags are present, the default targets are not included, and each listed target is added to the list of targets to [check|build|test|run].

So cargo check --lib --profile test --all-targets will add the lib target to the check list and then add all of the targets to the check list. That would also explain why I thought I saw tests multiple times -- because the library tests are actually getting added to the list of targets to compile twice.

@CAD97
Copy link

CAD97 commented May 10, 2020

OK, this gets weirder 😅

I set up a dead simple playground with

// src/lib.rs
#[test]
fn test_in_lib() {
    println!("====== test_in_lib =====");
}
// src/main.rs
#[test]
fn test_in_bin() {
    println!("====== test_in_bin =====");
}

fn main() {
    println!("====== main_in_bin =====");
}
// examples/example.rs
#[test]
fn test_in_example() {
    println!("====== test_in_example =====");
}

fn main() {
    println!("====== main_in_example =====");
}

Output:

$ cargo miri test --all-targets
# stdout
running 1 test
test test_in_example ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out


running 1 test
test test_in_example ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

# stderr
    Checking playground v0.0.0 (D:\repos\cad97\playground)
    Finished test [unoptimized + debuginfo] target(s) in 1.37s
    Checking playground v0.0.0 (D:\repos\cad97\playground)
    Finished test [unoptimized + debuginfo] target(s) in 1.37s

$ cargo miri run --all-targets
# stdout

[empty]

# stderr
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s

Rename examples/example.rs to examples/example.rs.ignored

$ cargo miri test --all-targets
# stdout
running 1 test

running 1 test
test test_in_lib ... ok

test test_in_bin ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out


test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out


running 1 test

running 1 test
test test_in_lib ... ok
test test_in_bin ... ok


test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

# stderr
    Checking playground v0.0.0 (D:\repos\cad97\playground)
    Finished test [unoptimized + debuginfo] target(s) in 1.42s
    Checking playground v0.0.0 (D:\repos\cad97\playground)
    Finished test [unoptimized + debuginfo] target(s) in 1.33s

$ cargo miri run --all-targets
# stdout
====== main_in_bin =====

running 1 test

running 1 test
test test_in_lib ... ok
test test_in_bin ... ok


test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

# stderr
    Checking playground v0.0.0 (D:\repos\cad97\playground)
    Finished dev [unoptimized + debuginfo] target(s) in 1.36s

@CAD97
Copy link

CAD97 commented May 11, 2020

One last note: here's proof that the example's main really is definitely running: it hits a panic I added to that main. I don't know why the smaller playground example doesn't seem to be running the example main.

@RalfJung
Copy link
Member Author

I'm rather surprised we do anything with examples, but for now I can only say that --all-targets should not be used with cargo-miri... it violates some assumptions that code is otherwise making.^^
I didn't even know cargo test --all-targets was a thing...

CAD97 added a commit to CAD97/sorbus that referenced this issue May 11, 2020
as per [this comment](rust-lang/miri#739 (comment))
--all-targets violates assumptions within the miri driver.
bors bot added a commit to CAD97/sorbus that referenced this issue May 11, 2020
29: Remove --all-targets from miri invocation r=CAD97 a=CAD97

as per [this comment](rust-lang/miri#739 (comment)) --all-targets violates assumptions within the miri driver. 



Co-authored-by: CAD97 <[email protected]>
@RalfJung
Copy link
Member Author

#1512 is another bug caused by using incorrectly reimplementing cargo's binary selection.

@ehuss do you think there is any way that Miri could ask cargo for which binaries it would run with a given cargo run/cargo test invocation? We cannot currently use cargo run/cargo test, not even with a custom RUSTC/RUSTC_WRAPPER, because then cargo will run the binaries which is something we do not want it to do.

@alecmocatta
Copy link

I think cargo build --message-format=json for cargo run and cargo test --no-run --message-format=json for cargo test give you the build artifacts for a particular invocation, which you can filter for binaries. For cargo run you'd also have to match against default-run in Cargo.toml and a --bin argument for disambiguation.

@RalfJung
Copy link
Member Author

Oh, I did not know about cargo test --no-run. That could be very useful indeed.

For cargo run, not having to reimplement --bin and default-run would be good, but that seems much less of a hassle than it is for tests. Note that we currently do cargo check for this, not cargo build, because we want check-only builds... but we can always mess with the build flags later so that's not a big difference. (I doubt there is a check-only cargo test --no-run so if we go for that we'll have to implement this flag patching anyway.)

@RalfJung
Copy link
Member Author

Looks like there's more logic than I was even aware of: #1514, #1516. I think it would be rather silly to reimplement all that, we really should find a way to reuse the code from cargo for that.

@ehuss
Copy link
Contributor

ehuss commented Aug 17, 2020

Unfortunately I don't think there is a way to determine what cargo run is going to run without running it.

@RalfJung
Copy link
Member Author

Assuming we can get something based on cargo test --no-run to work, would you be open to cargo run --no-run? It sounds a bit silly but I hope this is an not entirely unreasonable usecase.

@ehuss
Copy link
Contributor

ehuss commented Aug 17, 2020

I think that should be OK. An alternate solution is to set the runner like CARGO_TARGET_X86_64_APPLE_DARWIN_RUNNER=true cargo run, but a flag should probably be fine.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 17, 2020

Oh, that "runner" concept sounds very interesting! The only trouble is we'd have to figure out the "target" argument. We could store the build environment in JSON at build time and then actually run Miri when being invoked as the runner. Also presumably this is used for build scripts as well so I suppose we'd have to detect those?

@bjorn3
Copy link
Member

bjorn3 commented Aug 17, 2020

Build scripts are compiled for the host and not the target. The syntax is CARGO_TARGET_<TARGET>_RUNNER. This doesn't apply to host dependencies or when you don't pass --target to cargo.

@RalfJung
Copy link
Member Author

Oh so no runner is used for host things? I see.

@RalfJung RalfJung changed the title Proper binary selection in cargo miri Reuse cargo's binary selection in cargo miri Aug 20, 2020
@RalfJung RalfJung mentioned this issue Sep 8, 2020
10 tasks
bors added a commit that referenced this issue Sep 12, 2020
Redo cargo-miri logic

This rewrite the cargo-miri logic for running the requested crate(s) following what we outlined in #739: `cargo miri run/test $FLAGS` (almost) literally invokes `cargo run/test $FLAGS` but with some environment variables set so that we can control what happens:
* `RUSTC_WRAPPER` is set so that we get invoked instead of `rustc`. We use that power to mess with the flags being used for the build (things to be interpreted by Miri use a different sysroot), and when we are detecting a binary crate meant to be run by Miri, we grab the info we care about and put it into a JSON file for later use.
* `CARGO_TARGET_$TARGET_RUNNER` is set so what we get invoked when cargo wants to run a binary. At that point we take that JSON info from before and use it to invoke Miri.

Overall this works great! We get all sorts of cargo magic for free, and do not even need `cargo-metadata` any more. There's one annoying point though, which I have not managed to entirely work around yet: this means we are doing a full build, not just a check-build. Given that our sysroot is MIR-only, I was surprised that this even worked, but still -- this means we are doing more work than we should. So I also added some patching of arguments to make sure `--emit` does not contain `link`, and then more patching was required of the `--extern` flags for the binary because those referenced the `.rlib` files but now only `.rmeta` exists, and that is still not fully working because cargo seems to expect those `.rmeta` files and now triggers a rebuild each time as those files are still missing. My current plan is to make our wrapper create some empty dummy files with the right names, but the amount of hacks we are stacking on top of each other here is getting worrysome.^^ `@ehuss` your input would be welcome on this issue.

Due to re-using cargo more literally, this also changes flag parsing to match `cargo`. So `-Zmiri` flags now have to be passed via an environment variable (Cc #1416).

This PR is not ready yet, but the parts that are there I think can be reviewed already. TODO:
* [x] [Fix Windows](#1540 (comment)).
* [x] Figure out how we can do check-only builds without the rebuild problem above. I am also worried about cases where `build.rs` script might detect check-only builds and then do less work; I think some crates in rustc are doing that and if this is a thing in the wider ecosystem we need to find a way to support this as well.
* [x] Currently cargo runs doctests as well, and they are not run in Miri. We should at least figure out a way to not run them at all (resolving #584 is left for a future PR).
* [x] For some time, detect the old way of passing `-Zmiri` flags and warn that this will need updating. For some simple situations we can probably make it still support the old way, but I plan to remove those hacks after a bit. This is just to give people and me time to go around and send PRs to all projects that use Miri on CI, and update their use of the flags.
* [x] Add a test for stdin handling (#1505). This should work now but we should be sure.
* [x] Add a test for cargo env vars (#1515).
* [x] Check if #1516 is resolved.
* [x] Check if #1001 and #1514 are resolved.
* [x] Check if #1312 is resolved (not sure if it is wort adding a test).
* [x] Check if #1512 is resolved (not sure if it is wort adding a test).

Fixes #700.
Fixes #739.
Fixes #1001.
Fixes #1312 (without a test, as we run without cargo's stdin/stdout wrapping now, as the test for stdin confirms).
Fixes #1416.
Fixes #1505.
Fixes #1512 (without a test, as cargo now does all that handling anyway, which various other tests confirm).
Fixes #1514.
Fixes #1516.

Cc `@alecmocatta` who reported many of the bugs above; would be great if you could help with the tests e.g. by providing some small examples I could try.
r? `@oli-obk`
bors added a commit that referenced this issue Sep 12, 2020
Redo cargo-miri logic

This rewrite the cargo-miri logic for running the requested crate(s) following what we outlined in #739: `cargo miri run/test $FLAGS` (almost) literally invokes `cargo run/test $FLAGS` but with some environment variables set so that we can control what happens:
* `RUSTC_WRAPPER` is set so that we get invoked instead of `rustc`. We use that power to mess with the flags being used for the build (things to be interpreted by Miri use a different sysroot), and when we are detecting a binary crate meant to be run by Miri, we grab the info we care about and put it into a JSON file for later use.
* `CARGO_TARGET_$TARGET_RUNNER` is set so what we get invoked when cargo wants to run a binary. At that point we take that JSON info from before and use it to invoke Miri.

Overall this works great! We get all sorts of cargo magic for free, and do not even need `cargo-metadata` any more. There's one annoying point though, which I have not managed to entirely work around yet: this means we are doing a full build, not just a check-build. Given that our sysroot is MIR-only, I was surprised that this even worked, but still -- this means we are doing more work than we should. So I also added some patching of arguments to make sure `--emit` does not contain `link`, and then more patching was required of the `--extern` flags for the binary because those referenced the `.rlib` files but now only `.rmeta` exists, and that is still not fully working because cargo seems to expect those `.rmeta` files and now triggers a rebuild each time as those files are still missing. My current plan is to make our wrapper create some empty dummy files with the right names, but the amount of hacks we are stacking on top of each other here is getting worrysome.^^ `@ehuss` your input would be welcome on this issue.

Due to re-using cargo more literally, this also changes flag parsing to match `cargo`. So `-Zmiri` flags now have to be passed via an environment variable (Cc #1416).

This PR is not ready yet, but the parts that are there I think can be reviewed already. TODO:
* [x] [Fix Windows](#1540 (comment)).
* [x] Figure out how we can do check-only builds without the rebuild problem above. I am also worried about cases where `build.rs` script might detect check-only builds and then do less work; I think some crates in rustc are doing that and if this is a thing in the wider ecosystem we need to find a way to support this as well.
* [x] Currently cargo runs doctests as well, and they are not run in Miri. We should at least figure out a way to not run them at all (resolving #584 is left for a future PR).
* [x] For some time, detect the old way of passing `-Zmiri` flags and warn that this will need updating. For some simple situations we can probably make it still support the old way, but I plan to remove those hacks after a bit. This is just to give people and me time to go around and send PRs to all projects that use Miri on CI, and update their use of the flags.
* [x] Add a test for stdin handling (#1505). This should work now but we should be sure.
* [x] Add a test for cargo env vars (#1515).
* [x] Check if #1516 is resolved.
* [x] Check if #1001 and #1514 are resolved.
* [x] Check if #1312 is resolved (not sure if it is wort adding a test).
* [x] Check if #1512 is resolved (not sure if it is wort adding a test).

Fixes #700.
Fixes #739.
Fixes #1001.
Fixes #1312 (without a test, as we run without cargo's stdin/stdout wrapping now, as the test for stdin confirms).
Fixes #1416.
Fixes #1505.
Fixes #1512 (without a test, as cargo now does all that handling anyway, which various other tests confirm).
Fixes #1514.
Fixes #1516.

Cc `@alecmocatta` who reported many of the bugs above; would be great if you could help with the tests e.g. by providing some small examples I could try.
r? `@oli-obk`
bors added a commit that referenced this issue Sep 12, 2020
Redo cargo-miri logic

This rewrite the cargo-miri logic for running the requested crate(s) following what we outlined in #739: `cargo miri run/test $FLAGS` (almost) literally invokes `cargo run/test $FLAGS` but with some environment variables set so that we can control what happens:
* `RUSTC_WRAPPER` is set so that we get invoked instead of `rustc`. We use that power to mess with the flags being used for the build (things to be interpreted by Miri use a different sysroot), and when we are detecting a binary crate meant to be run by Miri, we grab the info we care about and put it into a JSON file for later use.
* `CARGO_TARGET_$TARGET_RUNNER` is set so what we get invoked when cargo wants to run a binary. At that point we take that JSON info from before and use it to invoke Miri.

Overall this works great! We get all sorts of cargo magic for free, and do not even need `cargo-metadata` any more. There's one annoying point though, which I have not managed to entirely work around yet: this means we are doing a full build, not just a check-build. Given that our sysroot is MIR-only, I was surprised that this even worked, but still -- this means we are doing more work than we should. So I also added some patching of arguments to make sure `--emit` does not contain `link`, and then more patching was required of the `--extern` flags for the binary because those referenced the `.rlib` files but now only `.rmeta` exists, and that is still not fully working because cargo seems to expect those `.rmeta` files and now triggers a rebuild each time as those files are still missing. My current plan is to make our wrapper create some empty dummy files with the right names, but the amount of hacks we are stacking on top of each other here is getting worrysome.^^ `@ehuss` your input would be welcome on this issue.

Due to re-using cargo more literally, this also changes flag parsing to match `cargo`. So `-Zmiri` flags now have to be passed via an environment variable (Cc #1416).

This PR is not ready yet, but the parts that are there I think can be reviewed already. TODO:
* [x] [Fix Windows](#1540 (comment)).
* [x] Figure out how we can do check-only builds without the rebuild problem above. ~~I am also worried about cases where `build.rs` script might detect check-only builds and then do less work; I think some crates in rustc are doing that and if this is a thing in the wider ecosystem we need to find a way to support this as well.~~ (postponed that until we have a concrete example)
* [x] Currently cargo runs doctests as well, and they are not run in Miri. We should at least figure out a way to not run them at all (resolving #584 is left for a future PR).
* [x] For some time, detect the old way of passing `-Zmiri` flags and warn that this will need updating. For some simple situations we can probably make it still support the old way, but I plan to remove those hacks after a bit. This is just to give people and me time to go around and send PRs to all projects that use Miri on CI, and update their use of the flags.
* [x] Add a test for stdin handling (#1505). This should work now but we should be sure.
* [x] Add a test for cargo env vars (#1515).
* [x] Check if #1516 is resolved.
* [x] Check if #1001 and #1514 are resolved.
* [x] Check if #1312 is resolved (not sure if it is wort adding a test).
* [x] Check if #1512 is resolved (not sure if it is wort adding a test).

Fixes #700.
Fixes #739.
Fixes #1001.
Fixes #1312 (without a test, as we run without cargo's stdin/stdout wrapping now, as the test for stdin confirms).
Fixes #1416.
Fixes #1505.
Fixes #1512 (without a test, as cargo now does all that handling anyway, which various other tests confirm).
Fixes #1514.
Fixes #1516.

Cc `@alecmocatta` who reported many of the bugs above; would be great if you could help with the tests e.g. by providing some small examples I could try.
r? `@oli-obk`
@bors bors closed this as completed in ce29fbf Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cargo Area: affects the cargo wrapper (cargo miri) C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants