Auto merge of #3798 - RalfJung:miri-script-remap-path-prefix, r=saethlin

miri-script: use --remap-path-prefix to print errors relative to the right root

Inspired by rust-lang/rust-clippy#13232, this makes it so that when cargo-miri fails to build, `./miri check` will print errors with paths like `cargo-miri/src/setup.rs`. That means we can get rid of the miri-symlink-hacks and instead tell RA to just always invoke the `./miri clippy` script just once, in the root.

This means that we can no longer share a target dir between cargo-miri and miri as the RUSTFLAGS are different to crates that are shared in the dependency tree need to be built twice with two different flags. `miri-script` hence now has to set the MIRI environment variable to tell the `cargo miri setup` invocation where to find Miri.

I also made it so that errors in miri-script itself are properly shown in RA, for which the `./miri` shell wrapper needs to set the right flags.

Author: bors

src/tools/miri/.cargo/config.toml

@@ -0,0 +1,9 @@
+[unstable]
+profile-rustflags = true
+
+# Add back the containing directory of the packages we have to refer to using --manifest-path.
+# Per-package profiles avoid adding this to build dependencies.
+[profile.dev.package."cargo-miri"]
+rustflags = ["--remap-path-prefix", "=cargo-miri"]
+[profile.dev.package."miri-script"]
+rustflags = ["--remap-path-prefix", "=miri-script"]

src/tools/miri/.github/workflows/ci.yml

@@ -60,7 +60,7 @@ jobs:
       - name: clippy (all features)
         run: ./miri clippy --all-features -- -D warnings
       - name: rustdoc
-        run: RUSTDOCFLAGS="-Dwarnings" ./miri cargo doc --document-private-items
+        run: RUSTDOCFLAGS="-Dwarnings" ./miri doc --document-private-items
 
   # These jobs doesn't actually test anything, but they're only used to tell
   # bors the build completed, as there is no practical way to detect when a

src/tools/miri/.github/workflows/setup/action.yml

@@ -35,6 +35,10 @@ runs:
         run: cargo install -f rustup-toolchain-install-master hyperfine
         shell: bash
 
+      - name: Install nightly toolchain
+        run: rustup toolchain install nightly --profile minimal
+        shell: bash
+
       - name: Install "master" toolchain
         run: |
           if [[ ${{ github.event_name }} == 'schedule' ]]; then

src/tools/miri/CONTRIBUTING.md

@@ -173,24 +173,24 @@ to `.vscode/settings.json` in your local Miri clone:
         "cargo-miri/Cargo.toml",
         "miri-script/Cargo.toml",
     ],
+    "rust-analyzer.check.invocationLocation": "root",
+    "rust-analyzer.check.invocationStrategy": "once",
     "rust-analyzer.check.overrideCommand": [
         "env",
         "MIRI_AUTO_OPS=no",
         "./miri",
-        "cargo",
         "clippy", // make this `check` when working with a locally built rustc
         "--message-format=json",
-        "--all-targets",
     ],
     // Contrary to what the name suggests, this also affects proc macros.
+    "rust-analyzer.cargo.buildScripts.invocationLocation": "root",
+    "rust-analyzer.cargo.buildScripts.invocationStrategy": "once",
     "rust-analyzer.cargo.buildScripts.overrideCommand": [
         "env",
         "MIRI_AUTO_OPS=no",
         "./miri",
-        "cargo",
         "check",
         "--message-format=json",
-        "--all-targets",
     ],
 }
 ```
@@ -309,6 +309,7 @@ anyone but Miri itself. They are used to communicate between different Miri
 binaries, and as such worth documenting:
 
 * `CARGO_EXTRA_FLAGS` is understood by `./miri` and passed to all host cargo invocations.
+  It is reserved for CI usage; setting the wrong flags this way can easily confuse the script.
 * `MIRI_BE_RUSTC` can be set to `host` or `target`. It tells the Miri driver to
   actually not interpret the code but compile it like rustc would. With `target`, Miri sets
   some compiler flags to prepare the code for interpretation; with `host`, this is not done.

src/tools/miri/cargo-miri/miri

@@ -93,12 +93,9 @@ pub fn find_miri() -> PathBuf {
     if let Some(path) = env::var_os("MIRI") {
         return path.into();
     }
+    // Assume it is in the same directory as ourselves.
     let mut path = std::env::current_exe().expect("current executable path invalid");
-    if cfg!(windows) {
-        path.set_file_name("miri.exe");
-    } else {
-        path.set_file_name("miri");
-    }
+    path.set_file_name(format!("miri{}", env::consts::EXE_SUFFIX));
     path
 }
 

src/tools/miri/cargo-miri/src/util.rs

@@ -1,8 +1,15 @@
 #!/usr/bin/env bash
 set -e
+# We want to call the binary directly, so we need to know where it ends up.
+MIRI_SCRIPT_TARGET_DIR="$(dirname "$0")"/miri-script/target
+# If stdout is not a terminal and we are not on CI, assume that we are being invoked by RA, and use JSON output.
+if ! [ -t 1 ] && [ -z "$CI" ]; then
+  MESSAGE_FORMAT="--message-format=json"
+fi
+# We need a nightly toolchain, for the `profile-rustflags` cargo feature.
+cargo +nightly build $CARGO_EXTRA_FLAGS --manifest-path "$(dirname "$0")"/miri-script/Cargo.toml \
+  -q --target-dir "$MIRI_SCRIPT_TARGET_DIR" $MESSAGE_FORMAT || \
+  ( echo "Failed to build miri-script. Is the 'nightly' toolchain installed?"; exit 1 )
 # Instead of doing just `cargo run --manifest-path .. $@`, we invoke miri-script binary directly. Invoking `cargo run` goes through
 # rustup (that sets it's own environmental variables), which is undesirable.
-MIRI_SCRIPT_TARGET_DIR="$(dirname "$0")"/miri-script/target
-cargo +stable build $CARGO_EXTRA_FLAGS -q --target-dir "$MIRI_SCRIPT_TARGET_DIR" --manifest-path "$(dirname "$0")"/miri-script/Cargo.toml || \
-  ( echo "Failed to build miri-script. Is the 'stable' toolchain installed?"; exit 1 )
 "$MIRI_SCRIPT_TARGET_DIR"/debug/miri-script "$@"

src/tools/miri/miri

@@ -22,6 +22,7 @@ const JOSH_FILTER: &str =
 const JOSH_PORT: u16 = 42042;
 
 impl MiriEnv {
+    /// Prepares the environment: builds miri and cargo-miri and a sysroot.
     /// Returns the location of the sysroot.
     ///
     /// If the target is None the sysroot will be built for the host machine.
@@ -34,12 +35,10 @@ impl MiriEnv {
             // Sysroot already set, use that.
             return Ok(miri_sysroot.into());
         }
-        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)?;
+        self.build(".", &[], quiet)?;
+        self.build("cargo-miri", &[], quiet)?;
 
         let target_flag = if let Some(target) = &target {
             vec![OsStr::new("--target"), target.as_ref()]
@@ -56,10 +55,12 @@ impl MiriEnv {
             eprintln!();
         }
 
-        let mut cmd = cmd!(self.sh,
-            "cargo +{toolchain} --quiet run {cargo_extra_flags...} --manifest-path {manifest_path} --
-             miri setup --print-sysroot {target_flag...}"
-        );
+        let mut cmd = self
+            .cargo_cmd("cargo-miri", "run")
+            .arg("--quiet")
+            .arg("--")
+            .args(&["miri", "setup", "--print-sysroot"])
+            .args(target_flag);
         cmd.set_quiet(quiet);
         let output = cmd.read()?;
         self.sh.set_var("MIRI_SYSROOT", &output);
@@ -162,8 +163,8 @@ impl Command {
             | Command::Test { .. }
             | Command::Run { .. }
             | Command::Fmt { .. }
-            | Command::Clippy { .. }
-            | Command::Cargo { .. } => Self::auto_actions()?,
+            | Command::Doc { .. }
+            | Command::Clippy { .. } => Self::auto_actions()?,
             | Command::Toolchain { .. }
             | Command::Bench { .. }
             | Command::RustcPull { .. }
@@ -177,9 +178,9 @@ impl Command {
             Command::Test { bless, flags, target } => Self::test(bless, flags, target),
             Command::Run { dep, verbose, many_seeds, target, edition, flags } =>
                 Self::run(dep, verbose, many_seeds, target, edition, flags),
+            Command::Doc { flags } => Self::doc(flags),
             Command::Fmt { flags } => Self::fmt(flags),
             Command::Clippy { flags } => Self::clippy(flags),
-            Command::Cargo { flags } => Self::cargo(flags),
             Command::Bench { target, benches } => Self::bench(target, benches),
             Command::Toolchain { flags } => Self::toolchain(flags),
             Command::RustcPull { commit } => Self::rustc_pull(commit.clone()),
@@ -433,39 +434,37 @@ impl Command {
 
     fn build(flags: Vec<String>) -> Result<()> {
         let e = MiriEnv::new()?;
-        e.build(path!(e.miri_dir / "Cargo.toml"), &flags, /* quiet */ false)?;
-        e.build(path!(e.miri_dir / "cargo-miri" / "Cargo.toml"), &flags, /* quiet */ false)?;
+        e.build(".", &flags, /* quiet */ false)?;
+        e.build("cargo-miri", &flags, /* quiet */ false)?;
         Ok(())
     }
 
     fn check(flags: Vec<String>) -> Result<()> {
         let e = MiriEnv::new()?;
-        e.check(path!(e.miri_dir / "Cargo.toml"), &flags)?;
-        e.check(path!(e.miri_dir / "cargo-miri" / "Cargo.toml"), &flags)?;
+        e.check(".", &flags)?;
+        e.check("cargo-miri", &flags)?;
         Ok(())
     }
 
-    fn clippy(flags: Vec<String>) -> Result<()> {
+    fn doc(flags: Vec<String>) -> Result<()> {
         let e = MiriEnv::new()?;
-        e.clippy(path!(e.miri_dir / "Cargo.toml"), &flags)?;
-        e.clippy(path!(e.miri_dir / "cargo-miri" / "Cargo.toml"), &flags)?;
-        e.clippy(path!(e.miri_dir / "miri-script" / "Cargo.toml"), &flags)?;
+        e.doc(".", &flags)?;
+        e.doc("cargo-miri", &flags)?;
         Ok(())
     }
 
-    fn cargo(flags: Vec<String>) -> Result<()> {
+    fn clippy(flags: Vec<String>) -> Result<()> {
         let e = MiriEnv::new()?;
-        let toolchain = &e.toolchain;
-        // We carefully kept the working dir intact, so this will run cargo *on the workspace in the
-        // current working dir*, not on the main Miri workspace. That is exactly what RA needs.
-        cmd!(e.sh, "cargo +{toolchain} {flags...}").run()?;
+        e.clippy(".", &flags)?;
+        e.clippy("cargo-miri", &flags)?;
+        e.clippy("miri-script", &flags)?;
         Ok(())
     }
 
     fn test(bless: bool, mut flags: Vec<String>, target: Option<String>) -> Result<()> {
         let mut e = MiriEnv::new()?;
 
-        // Prepare a sysroot.
+        // Prepare a sysroot. (Also builds cargo-miri, which we need.)
         e.build_miri_sysroot(/* quiet */ false, target.as_deref())?;
 
         // Forward information to test harness.
@@ -482,7 +481,7 @@ impl Command {
 
         // Then test, and let caller control flags.
         // Only in root project as `cargo-miri` has no tests.
-        e.test(path!(e.miri_dir / "Cargo.toml"), &flags)?;
+        e.test(".", &flags)?;
         Ok(())
     }
 
@@ -510,32 +509,27 @@ impl Command {
         early_flags.push("--edition".into());
         early_flags.push(edition.as_deref().unwrap_or("2021").into());
 
-        // Prepare a sysroot, add it to the flags.
+        // Prepare a sysroot, add it to the flags. (Also builds cargo-miri, which we need.)
         let miri_sysroot = e.build_miri_sysroot(/* quiet */ !verbose, target.as_deref())?;
         early_flags.push("--sysroot".into());
         early_flags.push(miri_sysroot.into());
 
         // Compute everything needed to run the actual command. Also add MIRIFLAGS.
-        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;
-        let extra_flags = &e.cargo_extra_flags;
         let quiet_flag = if verbose { None } else { Some("--quiet") };
         // This closure runs the command with the given `seed_flag` added between the MIRIFLAGS and
         // the `flags` given on the command-line.
-        let run_miri = |sh: &Shell, seed_flag: Option<String>| -> Result<()> {
+        let run_miri = |e: &MiriEnv, seed_flag: Option<String>| -> Result<()> {
             // The basic command that executes the Miri driver.
             let mut cmd = if dep {
-                cmd!(
-                    sh,
-                    "cargo +{toolchain} {quiet_flag...} test {extra_flags...} --manifest-path {miri_manifest} --test ui -- --miri-run-dep-mode"
-                )
+                e.cargo_cmd(".", "test")
+                    .args(&["--test", "ui"])
+                    .args(quiet_flag)
+                    .arg("--")
+                    .args(&["--miri-run-dep-mode"])
             } else {
-                cmd!(
-                    sh,
-                    "cargo +{toolchain} {quiet_flag...} run {extra_flags...} --manifest-path {miri_manifest} --"
-                )
+                e.cargo_cmd(".", "run").args(quiet_flag).arg("--")
             };
             cmd.set_quiet(!verbose);
             // Add Miri flags
@@ -551,14 +545,14 @@ impl Command {
         };
         // Run the closure once or many times.
         if let Some(seed_range) = many_seeds {
-            e.run_many_times(seed_range, |sh, seed| {
+            e.run_many_times(seed_range, |e, seed| {
                 eprintln!("Trying seed: {seed}");
-                run_miri(sh, Some(format!("-Zmiri-seed={seed}"))).inspect_err(|_| {
+                run_miri(e, Some(format!("-Zmiri-seed={seed}"))).inspect_err(|_| {
                     eprintln!("FAILING SEED: {seed}");
                 })
             })?;
         } else {
-            run_miri(&e.sh, None)?;
+            run_miri(&e, None)?;
         }
         Ok(())
     }
@@ -585,6 +579,6 @@ impl Command {
             .filter_ok(|item| item.file_type().is_file())
             .map_ok(|item| item.into_path());
 
-        e.format_files(files, &e.toolchain[..], &config_path, &flags)
+        e.format_files(files, &config_path, &flags)
     }
 }

src/tools/miri/miri-script/miri

@@ -48,6 +48,11 @@ pub enum Command {
         /// Flags that are passed through to `miri`.
         flags: Vec<String>,
     },
+    /// Build documentation
+    Doc {
+        /// Flags that are passed through to `cargo doc`.
+        flags: Vec<String>,
+    },
     /// Format all sources and tests.
     Fmt {
         /// Flags that are passed through to `rustfmt`.
@@ -58,9 +63,6 @@ pub enum Command {
         /// Flags that are passed through to `cargo clippy`.
         flags: Vec<String>,
     },
-    /// Runs just `cargo <flags>` with the Miri-specific environment variables.
-    /// Mainly meant to be invoked by rust-analyzer.
-    Cargo { flags: Vec<String> },
     /// Runs the benchmarks from bench-cargo-miri in hyperfine. hyperfine needs to be installed.
     Bench {
         target: Option<String>,
@@ -151,6 +153,7 @@ fn main() -> Result<()> {
     let command = match args.next_raw().as_deref() {
         Some("build") => Command::Build { flags: args.remainder() },
         Some("check") => Command::Check { flags: args.remainder() },
+        Some("doc") => Command::Doc { flags: args.remainder() },
         Some("test") => {
             let mut target = None;
             let mut bless = false;
@@ -205,7 +208,6 @@ fn main() -> Result<()> {
         }
         Some("fmt") => Command::Fmt { flags: args.remainder() },
         Some("clippy") => Command::Clippy { flags: args.remainder() },
-        Some("cargo") => Command::Cargo { flags: args.remainder() },
         Some("install") => Command::Install { flags: args.remainder() },
         Some("bench") => {
             let mut target = None;