Skip to content

unset RUSTC_WRAPPER for xargo invocation #1426

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

Merged
merged 1 commit into from
May 24, 2020
Merged

unset RUSTC_WRAPPER for xargo invocation #1426

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/bin/cargo-miri.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,6 @@ path = "lib.rs"
command.current_dir(&dir);
command.env("XARGO_HOME", &dir);
command.env("XARGO_RUST_SRC", &rust_src);
command.env_remove("RUSTFLAGS"); // Make sure external `RUSTFLAGS` do not influence the build.
// Use Miri as rustc to build a libstd compatible with us (and use the right flags).
// However, when we are running in bootstrap, we cannot just overwrite `RUSTC`,
// because we still need bootstrap to distinguish between host and target crates.
Expand All @@ -347,6 +346,12 @@ path = "lib.rs"
command.env("RUSTC", find_miri());
}
command.env("MIRI_BE_RUSTC", "1");
// Make sure there are no other wrappers or flags getting in our way
// (Cc https://github.com/rust-lang/miri/issues/1421).
// This is consistent with normal `cargo build` that does not apply `RUSTFLAGS`
// to the sysroot either.
command.env_remove("RUSTC_WRAPPER");
Copy link

Choose a reason for hiding this comment

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

Maybe it shouldn't be done in invisible to a user, manner? I.e. throw an error if there are certain variables that conflict with miri?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, good point. Throwing an error might be too extreme though. What about printing a warning, similar to what we do on MIRI_SYSROOT=something cargo miri setup?

Copy link

Choose a reason for hiding this comment

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

well, before I faced some errors in miri setup with the caching I didn't even know it removes certain variables. It would be at least nice to add the warnings that "x and y are being overriden".

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we have such a warning for RUSTC_WRAPPER now.

Copy link

Choose a reason for hiding this comment

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

Yeah, I've noticed it right after this comment. Thanks a bunch for this!

command.env_remove("RUSTFLAGS");
Copy link

Choose a reason for hiding this comment

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

Isn't it too invasive? Many flags wouldn't interfere with miri and could be required in certain circumstances.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is preexisting. Also note that this just affects the libstd build, not the build of the local crate. When you do RUSTFLAGS=foo cargo build, your libstd also is not affected by the flags you set. So I would say this is consistent with non-Miri operation.

// Finally run it!
if command.status().expect("failed to run xargo").success().not() {
show_error(format!("Failed to run xargo"));
Expand Down Expand Up @@ -446,6 +451,9 @@ fn in_cargo_miri() {
// Set `RUSTC_WRAPPER` to ourselves. Cargo will prepend that binary to its usual invocation,
// i.e., the first argument is `rustc` -- which is what we use in `main` to distinguish
// the two codepaths. (That extra argument is why we prefer this over setting `RUSTC`.)
if env::var_os("RUSTC_WRAPPER").is_some() {
println!("WARNING: Ignoring existing `RUSTC_WRAPPER` environment variable, Miri does not support wrapping.");
}
let path = std::env::current_exe().expect("current executable path invalid");
cmd.env("RUSTC_WRAPPER", path);
if verbose {
Expand Down