x.py: Test cargo miri test for doctests

[RalfJung]

Currently we do ensure on CI that cargo miri test works, but doctests are enabled as I couldn't figure out how to get this to work within bootstrap. However, that has let rust-lang/miri#3404 slip through. So ideally we can figure out these bootstrap issues and test rustdoc inside cargo miri test on rustc CI.

[RalfJung]

Here's what currently happens:

   Doc-tests cargo-miri-test
error[E0514]: found crate `std` compiled by an incompatible version of rustc
  |
  = note: the following crate versions were found:
          crate `std` compiled by rustc 1.79.0-dev: /home/r/src/rust/rustc.3/build/x86_64-unknown-linux-gnu/miri-sysroot/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-995a88566402f52a.rmeta
          crate `std` compiled by rustc 1.79.0-dev: /home/r/src/rust/rustc.3/build/x86_64-unknown-linux-gnu/miri-sysroot/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd_detect-04ee359a7e2a7fe8.rmeta
  = help: please recompile that crate using this compiler (rustc 1.78.0-nightly (ef324565d 2024-02-27)) (consider running `cargo clean` first)

error[E0514]: found crate `issue_1760` compiled by an incompatible version of rustc
  --> src/lib.rs:33:5
   |
33 |     issue_1760::use_the_dependency!();
   |     ^^^^^^^^^^
   |
   = note: the following crate versions were found:
           crate `issue_1760` compiled by rustc 1.79.0-dev: /home/r/src/rust/rustc.3/build/x86_64-unknown-linux-gnu/stage0-tools/miri/debug/deps/libissue_1760-442b1cfe21f87d8d.so
   = help: please recompile that crate using this compiler (rustc 1.78.0-nightly (ef324565d 2024-02-27)) (consider running `cargo clean` first)

--stage 1 shows a similar error.

So... probably we're using the wrong rustdoc? I assume it's the bootstrap rustdoc, if that's a thing? I can't even see a rustdoc being built in the build log. We want the rustdoc that is produced by stage 0 here, since we are testing the Miri produced by stage 0. How do we get that?

Cc @rust-lang/bootstrap

[Mark-Simulacrum]

https://github.com/rust-lang/miri/blob/6a72dc6d24958eeab5c7c6caf4103312a5692f45/cargo-miri/src/phases.rs#L584-L586 looks to be the reason we're running a random rustdoc here.

I suspect we'll need to thread through a custom bootstrap env variable, since IIRC there's no RUSTDOC_WRAPPER that miri can use to thread state. Alternatively it might work to adjust miri such that it remembers which toolchain is running earlier, but I suspect that'll be harder. As-is though it's almost certainly semi-broken for any Rust user doing something like cargo +nightly miri test presumably, where their default toolchain isn't nightly?

[RalfJung]

As-is though it's almost certainly semi-broken for any Rust user doing something like cargo +nightly miri test presumably, where their default toolchain isn't nightly?

It seems to be working, AFAIK rustup ensures that nested calls use the +-specified toolchain. Or maybe we are just lucky...

But yes we should

• in the outermost wrapper, determine which rustdoc cargo would invoke using the same logic as cargo
• store that in a miri-private env var
• invoke that in phase_rustdoc

I assume however we'll also need further changes in how bootstrap invokes cargo-miri to tell it where the right rustdoc lives? Currently we're not even building rustdoc so something surely needs to change in bootstrap.

I see this snippet a few times, is that all we need?

cmd.env("RUSTDOC", builder.rustdoc(self.compiler))

[Mark-Simulacrum]

Yeah, I'd expect that to be enough to start with at least.

Also potentially relevant: If we're already using builder.cargo somewhere in there (we should be, probably?) then that should already get mostly setup, see here:

rust/src/bootstrap/src/core/builder.rs

Lines 1659 to 1681 in dda2372

	// Customize the compiler we're running. Specify the compiler to cargo
	// as our shim and then pass it some various options used to configure
	// how the actual compiler itself is called.
	//
	// These variables are primarily all read by
	// src/bootstrap/bin/{rustc.rs,rustdoc.rs}
	cargo
	.env("RUSTBUILD_NATIVE_DIR", self.native_dir(target))
	.env("RUSTC_REAL", self.rustc(compiler))
	.env("RUSTC_STAGE", stage.to_string())
	.env("RUSTC_SYSROOT", sysroot)
	.env("RUSTC_LIBDIR", libdir)
	.env("RUSTDOC", self.bootstrap_out.join("rustdoc"))
	.env(
	"RUSTDOC_REAL",
	if cmd == "doc" || cmd == "rustdoc" || (cmd == "test" && want_rustdoc) {
	self.rustdoc(compiler)
	} else {
	PathBuf::from("/path/to/nowhere/rustdoc/not/required")
	},
	)
	.env("RUSTC_ERROR_METADATA_DST", self.extended_error_dir())
	.env("RUSTC_BREAK_ON_ICE", "1");

,

rust/src/bootstrap/src/core/builder.rs

Lines 1703 to 1705 in dda2372

	if !(["build", "check", "clippy", "fix", "rustc"].contains(&cmd)) && want_rustdoc {
	cargo.env("RUSTDOC_LIBDIR", self.rustc_libdir(compiler));
	}

[onur-ozkan]

I assume however we'll also need further changes in how bootstrap invokes cargo-miri to tell it where the right rustdoc lives?

We do that already:

rust/src/bootstrap/src/core/builder.rs

Lines 1671 to 1679 in dda2372

	.env("RUSTDOC", self.bootstrap_out.join("rustdoc"))
	.env(
	"RUSTDOC_REAL",
	if cmd == "doc" || cmd == "rustdoc" || (cmd == "test" && want_rustdoc) {
	self.rustdoc(compiler)
	} else {
	PathBuf::from("/path/to/nowhere/rustdoc/not/required")
	},
	)

Currently we're not even building rustdoc so something surely needs to change in bootstrap.

Unless you are trying stage 0, you should be able to see rustdoc compilation in the build logs.

[RalfJung]

This is stage 0. It should still have to build rustdoc, stage 0 Miri needs stage 0 rustdoc (not bootstrap rustdoc) for its doctests. But, I also just saw this in the logs:
(...)
So I guess we're already building it, I just never noticed...

@onur-ozkan ah fun, so if we invoke RUSTDOC we'll actually invoke bootstrap's rustdoc wrapper. We'll have to see if having both Miri and that wrapper around will work.^^

[RalfJung]

I also just saw this in the logs:

Never mind, that was for a --stage 1 build. For stage 0 this doesn't happen. I don't think this can work though...

The staging with Miri is a bit weird; we need a stage 1 std as well for our stage 0 tests. Specifically we need a host sysroot that was built with a compiler that "is the same as" Miri, i.e. that is fully compatible. (The comment here is wrong, it's not the compiler that built Miri.) This was needed to make build scripts and proc macros work in cargo miri test.

[onur-ozkan]

@onur-ozkan ah fun, so if we invoke RUSTDOC we'll actually invoke bootstrap's rustdoc wrapper. We'll have to see if having both Miri and that wrapper around will work.^^

Yes, and if you want to use the actual rustdoc without wrapper, you can invoke RUSTDOC_REAL.

I tried using RUSTDOC_REAL env instead of plain "rustdoc" and this is the output:

   Doc-tests cargo-miri-test

running 5 tests
test src/lib.rs - make_true (line 3) ... FAILED
test src/lib.rs - make_true (line 9) - compile ... FAILED
test src/lib.rs - make_true (line 21) - compile fail ... ok
test src/lib.rs - make_true (line 15) - compile fail ... ok
test src/lib.rs - miri_only_fn (line 37) ... FAILED

failures:

---- src/lib.rs - make_true (line 3) stdout ----
error: Option 'sysroot' given more than once

Couldn't compile the test.
---- src/lib.rs - make_true (line 9) stdout ----
error: Option 'sysroot' given more than once

Couldn't compile the test.
---- src/lib.rs - miri_only_fn (line 37) stdout ----
error: Option 'sysroot' given more than once

Couldn't compile the test.

failures:
    src/lib.rs - make_true (line 3)
    src/lib.rs - make_true (line 9)
    src/lib.rs - miri_only_fn (line 37)

test result: FAILED. 2 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s

[onur-ozkan]

I had to manually pass RUSTDOC_REAL from the cargo miri test caller; it turns out we are not passing it by default (or it somehow gets removed later?). Anyway, this is how I tested it:

diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
index 6d3163b90b1..5190b6df9ba 100644
--- a/src/bootstrap/src/core/build_steps/test.rs
+++ b/src/bootstrap/src/core/build_steps/test.rs
@@ -637,7 +637,7 @@ fn run(self, builder: &Builder<'_>) {
         let mut cargo = prepare_cargo_test(cargo, &[], &[], "miri", compiler, target, builder);
         {
             let _time = helpers::timeit(builder);
-            builder.run(&mut cargo);
+            // builder.run(&mut cargo);
         }
 
         // Run it again for mir-opt-level 4 to catch some miscompilations.
@@ -653,7 +653,7 @@ fn run(self, builder: &Builder<'_>) {
             let mut cargo = prepare_cargo_test(cargo, &[], &[], "miri", compiler, target, builder);
             {
                 let _time = helpers::timeit(builder);
-                builder.run(&mut cargo);
+                // builder.run(&mut cargo);
             }
         }
 
@@ -680,10 +680,11 @@ fn run(self, builder: &Builder<'_>) {
             .arg("--manifest-path")
             .arg(builder.src.join("src/tools/miri/test-cargo-miri/Cargo.toml"));
         cargo.arg("--target").arg(target.rustc_target_arg());
-        cargo.arg("--tests"); // don't run doctests, they are too confused by the staging
+        // cargo.arg("--tests"); // don't run doctests, they are too confused by the staging
         cargo.arg("--").args(builder.config.test_args());
 
         // Tell `cargo miri` where to find things.
+        cargo.env("RUSTDOC_REAL", builder.rustdoc(compiler));
         cargo.env("MIRI_SYSROOT", &miri_sysroot);
         cargo.env("MIRI_HOST_SYSROOT", sysroot);
         cargo.env("MIRI", &miri);
diff --git a/src/tools/miri/cargo-miri/src/phases.rs b/src/tools/miri/cargo-miri/src/phases.rs
index 81ff68545cc..6f41a8cb7f0 100644
--- a/src/tools/miri/cargo-miri/src/phases.rs
+++ b/src/tools/miri/cargo-miri/src/phases.rs
@@ -583,7 +583,8 @@ pub fn phase_rustdoc(mut args: impl Iterator<Item = String>) {
 
     // phase_cargo_miri sets the RUSTDOC env var to ourselves, so we can't use that here;
     // just default to a straight-forward invocation for now:
-    let mut cmd = Command::new("rustdoc");
+    let t = std::env::var("RUSTDOC_REAL").unwrap();
+    let mut cmd = Command::new(t);
 
     let extern_flag = "--extern";
     let runtool_flag = "--runtool";

[RalfJung]

I tried using RUSTDOC_REAL env instead of plain "rustdoc" and this is the output:

Yeah currently this does not work due to rust-lang/miri#3404. But you have already come closer than I did. :)

Was that with --stage 0 or stage 1?

I had to manually pass RUSTDOC_REAL from the cargo miri test caller; it turns out we are not passing it by default (or it somehow gets removed later?).

Maybe self.doc_tests is not set here?

[onur-ozkan]

Yeah currently this does not work due to rust-lang/miri#3404.

I guess that needs to be resolved first before we enable doctests on cargo miri test ?

Was that with --stage 0 or stage 1?

It was stage 1.

Maybe self.doc_tests is not set here?

Yeah, it seems like.

[RalfJung]

I guess that needs to be resolved first before we enable doctests on cargo miri test ?

Yes.

It was stage 1.

x.py test miri --stage 0 must continue working. I don't want to always build rustc twice when working on Miri in the tree.^^

But if necessary we could enable doc-tests only for higher stages... that would still be better than the status quo.