Auto merge of #3006 - RalfJung:miri-script-shell, r=RalfJung

miri-script and cargo-miri cleanups

At least in an initial test, `cmd!(Shell::new()?, "env")` does show the full environment. Let's see what CI says.

`@osiewicz` why did you add this code to forward the host environment?

Author: bors

cargo-miri/src/phases.rs

@@ -94,7 +94,7 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
     let target = target.as_ref().unwrap_or(host);
 
     // We always setup.
-    setup(&subcommand, target, &rustc_version, verbose);
+    let miri_sysroot = setup(&subcommand, target, &rustc_version, verbose);
 
     // Invoke actual cargo for the job, but with different flags.
     // We re-use `cargo test` and `cargo run`, which makes target and binary handling very easy but
@@ -159,6 +159,8 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
     // Forward all further arguments (not consumed by `ArgSplitFlagValue`) to cargo.
     cmd.args(args);
 
+    // Let it know where the Miri sysroot lives.
+    cmd.env("MIRI_SYSROOT", miri_sysroot);
     // 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`.)

cargo-miri/src/setup.rs

@@ -13,13 +13,20 @@ use crate::util::*;
 /// Performs the setup required to make `cargo miri` work: Getting a custom-built libstd. Then sets
 /// `MIRI_SYSROOT`. Skipped if `MIRI_SYSROOT` is already set, in which case we expect the user has
 /// done all this already.
-pub fn setup(subcommand: &MiriCommand, target: &str, rustc_version: &VersionMeta, verbose: usize) {
+pub fn setup(
+    subcommand: &MiriCommand,
+    target: &str,
+    rustc_version: &VersionMeta,
+    verbose: usize,
+) -> PathBuf {
     let only_setup = matches!(subcommand, MiriCommand::Setup);
     let ask_user = !only_setup;
     let print_sysroot = only_setup && has_arg_flag("--print-sysroot"); // whether we just print the sysroot path
-    if !only_setup && std::env::var_os("MIRI_SYSROOT").is_some() {
-        // Skip setup step if MIRI_SYSROOT is explicitly set, *unless* we are `cargo miri setup`.
-        return;
+    if !only_setup {
+        if let Some(sysroot) = std::env::var_os("MIRI_SYSROOT") {
+            // Skip setup step if MIRI_SYSROOT is explicitly set, *unless* we are `cargo miri setup`.
+            return sysroot.into();
+        }
     }
 
     // Determine where the rust sources are located.  The env var trumps auto-detection.
@@ -92,6 +99,8 @@ pub fn setup(subcommand: &MiriCommand, target: &str, rustc_version: &VersionMeta
             command.env("RUSTC", &cargo_miri_path);
         }
         command.env("MIRI_CALLED_FROM_SETUP", "1");
+        // Miri expects `MIRI_SYSROOT` to be set when invoked in target mode. Even if that directory is empty.
+        command.env("MIRI_SYSROOT", &sysroot_dir);
         // Make sure there are no other wrappers getting in our way (Cc
         // https://github.com/rust-lang/miri/issues/1421,
         // https://github.com/rust-lang/miri/issues/2429). Looks like setting
@@ -117,8 +126,6 @@ pub fn setup(subcommand: &MiriCommand, target: &str, rustc_version: &VersionMeta
     // the user might have set, which is consistent with normal `cargo build` that does
     // not apply `RUSTFLAGS` to the sysroot either.
     let rustflags = &["-Cdebug-assertions=off", "-Coverflow-checks=on"];
-    // Make sure all target-level Miri invocations know their sysroot.
-    std::env::set_var("MIRI_SYSROOT", &sysroot_dir);
 
     // Do the build.
     if print_sysroot {
@@ -159,4 +166,6 @@ pub fn setup(subcommand: &MiriCommand, target: &str, rustc_version: &VersionMeta
         // Print just the sysroot and nothing else to stdout; this way we do not need any escaping.
         println!("{}", sysroot_dir.display());
     }
+
+    sysroot_dir
 }

cargo-miri/src/util.rs

@@ -1,7 +1,5 @@
-use std::collections::HashMap;
 use std::env;
 use std::ffi::OsString;
-use std::fmt::Write as _;
 use std::fs::File;
 use std::io::{self, BufWriter, Read, Write};
 use std::ops::Not;
@@ -247,46 +245,10 @@ pub fn local_crates(metadata: &Metadata) -> String {
     local_crates
 }
 
-fn env_vars_from_cmd(cmd: &Command) -> Vec<(String, String)> {
-    let mut envs = HashMap::new();
-    for (key, value) in std::env::vars() {
-        envs.insert(key, value);
-    }
-    for (key, value) in cmd.get_envs() {
-        if let Some(value) = value {
-            envs.insert(key.to_string_lossy().to_string(), value.to_string_lossy().to_string());
-        } else {
-            envs.remove(&key.to_string_lossy().to_string());
-        }
-    }
-    let mut envs: Vec<_> = envs.into_iter().collect();
-    envs.sort();
-    envs
-}
-
 /// Debug-print a command that is going to be run.
 pub fn debug_cmd(prefix: &str, verbose: usize, cmd: &Command) {
     if verbose == 0 {
         return;
     }
-    // We only do a single `eprintln!` call to minimize concurrency interactions.
-    let mut out = prefix.to_string();
-    writeln!(out, " running command: env \\").unwrap();
-    if verbose > 1 {
-        // Print the full environment this will be called in.
-        for (key, value) in env_vars_from_cmd(cmd) {
-            writeln!(out, "{key}={value:?} \\").unwrap();
-        }
-    } else {
-        // Print only what has been changed for this `cmd`.
-        for (var, val) in cmd.get_envs() {
-            if let Some(val) = val {
-                writeln!(out, "{}={val:?} \\", var.to_string_lossy()).unwrap();
-            } else {
-                writeln!(out, "--unset={}", var.to_string_lossy()).unwrap();
-            }
-        }
-    }
-    write!(out, "{cmd:?}").unwrap();
-    eprintln!("{out}");
+    eprintln!("{prefix} running command: {cmd:?}");
 }

miri-script/src/commands.rs

@@ -6,7 +6,7 @@ use std::ops::Not;
 use anyhow::{anyhow, bail, Context, Result};
 use path_macro::path;
 use walkdir::WalkDir;
-use xshell::cmd;
+use xshell::{cmd, Shell};
 
 use crate::util::*;
 use crate::Command;
@@ -16,17 +16,25 @@ const JOSH_FILTER: &str =
     ":rev(75dd959a3a40eb5b4574f8d2e23aa6efbeb33573:prefix=src/tools/miri):/src/tools/miri";
 
 impl MiriEnv {
-    fn build_miri_sysroot(&mut self) -> Result<()> {
+    fn build_miri_sysroot(&mut self, quiet: bool) -> Result<()> {
         if self.sh.var("MIRI_SYSROOT").is_ok() {
             // Sysroot already set, use that.
             return Ok(());
         }
         let manifest_path = path!(self.miri_dir / "cargo-miri" / "Cargo.toml");
         let Self { toolchain, cargo_extra_flags, .. } = &self;
+
+        // Make sure everything is built. Also Miri itself.
+        self.build(path!(self.miri_dir / "Cargo.toml"), &[], quiet)?;
+        self.build(&manifest_path, &[], quiet)?;
+
         let target = &match self.sh.var("MIRI_TEST_TARGET") {
             Ok(target) => vec!["--target".into(), target],
             Err(_) => vec![],
         };
+        if !quiet {
+            eprintln!("$ (buildig Miri sysroot)");
+        }
         let output = cmd!(self.sh,
             "cargo +{toolchain} --quiet run {cargo_extra_flags...} --manifest-path {manifest_path} --
              miri setup --print-sysroot {target...}"
@@ -91,7 +99,7 @@ impl Command {
         // Make sure rustup-toolchain-install-master is installed.
         which::which("rustup-toolchain-install-master")
             .context("Please install rustup-toolchain-install-master by running 'cargo install rustup-toolchain-install-master'")?;
-        let sh = shell()?;
+        let sh = Shell::new()?;
         sh.change_dir(miri_dir()?);
         let new_commit = Some(sh.read_file("rust-version")?.trim().to_owned());
         let current_commit = {
@@ -130,7 +138,7 @@ impl Command {
     }
 
     fn rustc_pull(commit: Option<String>) -> Result<()> {
-        let sh = shell()?;
+        let sh = Shell::new()?;
         sh.change_dir(miri_dir()?);
         let commit = commit.map(Result::Ok).unwrap_or_else(|| {
             let rust_repo_head =
@@ -177,7 +185,7 @@ impl Command {
     }
 
     fn rustc_push(github_user: String, branch: String) -> Result<()> {
-        let sh = shell()?;
+        let sh = Shell::new()?;
         sh.change_dir(miri_dir()?);
         let base = sh.read_file("rust-version")?.trim().to_owned();
         // Make sure the repo is clean.
@@ -265,7 +273,7 @@ impl Command {
         let Some((command_name, trailing_args)) = command.split_first() else {
             bail!("expected many-seeds command to be non-empty");
         };
-        let sh = shell()?;
+        let sh = Shell::new()?;
         for seed in seed_start..seed_end {
             println!("Trying seed: {seed}");
             let mut miriflags = env::var_os("MIRIFLAGS").unwrap_or_default();
@@ -293,7 +301,7 @@ impl Command {
         // Make sure we have an up-to-date Miri installed and selected the right toolchain.
         Self::install(vec![])?;
 
-        let sh = shell()?;
+        let sh = Shell::new()?;
         sh.change_dir(miri_dir()?);
         let benches_dir = "bench-cargo-miri";
         let benches = if benches.is_empty() {
@@ -365,9 +373,8 @@ impl Command {
     fn test(bless: bool, flags: Vec<OsString>) -> Result<()> {
         Self::auto_actions()?;
         let mut e = MiriEnv::new()?;
-        // First build, and get a sysroot.
-        e.build(path!(e.miri_dir / "Cargo.toml"), &[], /* quiet */ true)?;
-        e.build_miri_sysroot()?;
+        // Prepare a sysroot.
+        e.build_miri_sysroot(/* quiet */ false)?;
 
         // Then test, and let caller control flags.
         // Only in root project as `cargo-miri` has no tests.
@@ -393,12 +400,11 @@ impl Command {
             let miriflags = e.sh.var("MIRIFLAGS").unwrap_or_default();
             e.sh.set_var("MIRIFLAGS", format!("{miriflags} --target {target}"));
         }
-        // First build, and get a sysroot.
-        let miri_manifest = path!(e.miri_dir / "Cargo.toml");
-        e.build(&miri_manifest, &[], /* quiet */ true)?;
-        e.build_miri_sysroot()?;
+        // Prepare a sysroot.
+        e.build_miri_sysroot(/* quiet */ true)?;
 
         // Then run the actual command.
+        let miri_manifest = path!(e.miri_dir / "Cargo.toml");
         let miri_flags = e.sh.var("MIRIFLAGS").unwrap_or_default();
         let miri_flags = flagsplit(&miri_flags);
         let toolchain = &e.toolchain;

miri-script/src/util.rs

@@ -13,21 +13,12 @@ pub fn miri_dir() -> std::io::Result<PathBuf> {
 
 /// Queries the active toolchain for the Miri dir.
 pub fn active_toolchain() -> Result<String> {
-    let sh = shell()?;
+    let sh = Shell::new()?;
     sh.change_dir(miri_dir()?);
     let stdout = cmd!(sh, "rustup show active-toolchain").read()?;
     Ok(stdout.split_whitespace().next().context("Could not obtain active Rust toolchain")?.into())
 }
 
-pub fn shell() -> Result<Shell> {
-    let sh = Shell::new()?;
-    // xshell does not propagate parent's env variables by default.
-    for (k, v) in std::env::vars_os() {
-        sh.set_var(k, v);
-    }
-    Ok(sh)
-}
-
 pub fn flagsplit(flags: &str) -> Vec<String> {
     // This code is taken from `RUSTFLAGS` handling in cargo.
     flags.split(' ').map(str::trim).filter(|s| !s.is_empty()).map(str::to_string).collect()
@@ -50,7 +41,7 @@ pub struct MiriEnv {
 impl MiriEnv {
     pub fn new() -> Result<Self> {
         let toolchain = active_toolchain()?;
-        let sh = shell()?; // we are preserving the current_dir on this one, so paths resolve properly!
+        let sh = Shell::new()?; // we are preserving the current_dir on this one, so paths resolve properly!
         let miri_dir = miri_dir()?;
 
         let sysroot = cmd!(sh, "rustc +{toolchain} --print sysroot").read()?.into();