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.

Sign up for GitHub

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

Jump to bottom

Cargo env vars aren't set #1515

Closed
alecmocatta opened this issue Aug 16, 2020 · 8 comments
Closed

Cargo env vars aren't set #1515

alecmocatta opened this issue Aug 16, 2020 · 8 comments

Comments

@alecmocatta
Copy link

alecmocatta commented Aug 16, 2020 •
edited
Loading

Cargo's docs specify various env vars that are passed to binaries run with cargo run/test/bench. cargo miri does not currently pass them, so this fails under miri:

fn main() {
    println!("{}", std::env::var("CARGO_PKG_NAME").unwrap());
}

(Playground)
@bjorn3
Copy link
Member

bjorn3 commented Aug 16, 2020 •
edited
Loading

You need to pass -Zmiri-disable-isolation to miri. For example cargo miri run -- -Zmiri-disable-isolation. Otherwise things like env var access and filesystem access are disabled.

@alecmocatta
Copy link
Author

Sorry, I thought I'd repro'd this with -Zmiri-disable-isolation but I was mistaken. Closing!

@RalfJung RalfJung mentioned this issue Sep 8, 2020
Merged
10 tasks
@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2020

@alecmocatta Also you should be able to use env! I think, which is evaluated at build-time rather than at run-time. That's advisable anyway as it will mean that this works even if you distribute the binary and run it outside cargo. (For tests that seems unlikely, for the main binaries more likely.)

@alecmocatta
Copy link
Author

Thanks @RalfJung! I bumped into this when I changed env::current_dir() to PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap()) to workaround #1514. env! could plausibly be outdated whereas env::var_os is in theory guaranteed to be correct. Regardless, looking forward to removing once #1540 lands!

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2020 •
edited
Loading

Not sure what you mean by "outdated"? With env! the file should be rebuilt if the env var changes (that used to not happen but was fixed recently I think).

@alecmocatta
Copy link
Author

e.g. on moving the manifest directory, but now I think about it you're probably right that would trigger a rebuild on the next cargo test invocation.

@alecmocatta
Copy link
Author

Actually I've just confirmed that moving the manifest directory does not trigger a rebuild, thus env! and env::var can disagree. This seems dubious behaviour to me, I'll file an issue rust-lang/rust.

@alecmocatta
Copy link
Author

https://github.com/rust-lang/rust/issues/76511

bors added a commit that referenced this issue Sep 12, 2020
@bors
Auto merge of #1540 - RalfJung:cargo-miri-redone, r=<try>
3960d8b 
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
@bors
Auto merge of #1540 - RalfJung:cargo-miri-redone, r=<try>
7de98a0 
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
@bors
Auto merge of #1540 - RalfJung:cargo-miri-redone, r=<try>
835e31b 
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 added a commit that referenced this issue Sep 17, 2020
@bors
Auto merge of #1540 - RalfJung:cargo-miri-redone, r=oli-obk
ce29fbf 
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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@RalfJung @alecmocatta @bjorn3