Make cargo-miri run documentation tests

[teryror]

This would resolve #584.

It Works On My Machine. Specifically, it needs a version of rustdoc with this change, which was only merged yesterday, i.e. it needs the most recent nightly build. With that, I can run the entire coca test suite successfully, which is what I used for testing this feature. Verbose output and/or purposefully changing a doc-test to fail confirm this is working as intended, and not just reported incorrectly.

However, updating rustc-version breaks several tests from Miri's own test suite, so merging this will have to wait. Still, I'm sure this would benefit a lot from code review and testing on other projects, in other build environments.

[teryror]

I had completely forgotten about test-cargo-miri since I'd been testing on my own project the whole time. This fixes an oversight of mine in phase_cargo_runner, and updates the tests to expect working doc-tests.

Given this still requires a newer rustdoc, I don't expect this to pass CI though.

[RalfJung]

Thanks a lot for this PR! I have this in my queue of things to do. Unfortunately, I am pretty busy currently and reviewing these PRs is but one of many things competing for my spare time, so it might take a week or more until I get around to taking a closer look at this. Sorry for that.

[teryror]

No worries, take all the time you need!

[RalfJung]

Has the rustdoc change landed in the rustc repository in the mean time? Can you rebase this PR over Miri master? That should now work with latest rustc.

[teryror]

Has the rustdoc change landed in the rustc repository in the mean time? Can you rebase this PR over Miri master? That should now work with latest rustc.

This has been merged for about a week now, yes.

I rolled up all of your suggestions into a single commit, then rebased and force-pushed, we should be good to go. Let's see what CI has to say about it.

[RalfJung]

I rolled up all of your suggestions into a single commit, then rebased and force-pushed, we should be good to go.

I'd prefer if you could avoid squashing while we are still doing review. That makes my job much easier as it means I do not have to re-read all your code. :)

Let's see what CI has to say about it.

Is the version in rust-version new enough to contain that fix?

[teryror]

Argh, sorry about that. Good thing this was mostly about adding more comments.

The rust_version is new enough, the CI check fails because doc-tests aren't run for all targets, so we'd need a different test.default.stdout.ref depending on the platform, it would seem. I find that a bit strange, wouldn't it be better to enable that somehow? I'm not quite sure how, though. What's your preference here?

[RalfJung]

The rust_version is new enough, the CI check fails because doc-tests aren't run for all targets, so we'd need a different test.default.stdout.ref depending on the platform, it would seem. I find that a bit strange, wouldn't it be better to enable that somehow? I'm not quite sure how, though. What's your preference here?

Looks like cargo by default does not run doctests in cross-mode, but an experimental feature exists to fix that: rust-lang/cargo#7040. Maybe our test suite could use that?

[bors]

☔ The latest upstream changes (presumably #1675) made this pull request unmergeable. Please resolve the merge conflicts.

[teryror]

That should do it, I think. I'll get around to merging this with #1675 later today, maybe tomorrow.

Edit: Oops, forgot to save after adding a comment.

[teryror]

Getting this to work locally was actually pretty easy, but the CI setup really doesn't like the -Zdoctest-xcompile option. With no toolchain explicitly set, or with +master, it complains about unstable options requiring nightly, with +nightly it says the nightly toolchain is not installed. Once again, I don't know how to proceed here, and trial-and-error gets old fast with this turnaround time. Right now it seems easier to just switch out the test.*.ref files depending on the target.

Furthermore, I don't think phase_cargo_rustdoc even works right in cross-mode, which I hadn't really considered before. Since -Zdoctest-xcompile actually makes cargo pass its own --runtool to rustdoc, which I currently just overwrite, while forwarding any --runtool-args. Miri proper may not actually like those, but that also means the way I select the phase to enter from main when MIRI_CALLED_FROM_RUSTDOC is set is broken.

Soo, I'll have to revise this some more, I guess.

[RalfJung]

Not supporting cross-running of doctests at first is also fine, if it turns out to be hard (and it seems like it does^^).

You can use the existing is_foreign variable to swap out the ref file. But it would be good if we didn't have to do this for each test. Maybe pass --no-doc for most of them or so?

[teryror]

Sounds good to me. I should be able to finish up by tomorrow in that case :)

If no one else picks this up, I should be able to set some time aside to investigate cross-running starting around mid-March (I have exams coming up until then).

[RalfJung]

If no one else picks this up, I should be able to set some time aside to investigate cross-running starting around mid-March (I have exams coming up until then).

Awesome. :) Once this lands, it's probably best to create an issue and leave such notes there. And good call on not letting Miri distract you too much from your exams 👍

[RalfJung]

@teryror I'm sorry I lost track... is this PR waiting for me to review it, or do you have further changes planned?

[teryror]

This is ready for review, yes, probably should have pinged you about that, sorry. I didn't rebase in a week or so, no changes planned besides that.

[RalfJung]

Can you change one of these tests to also pass --no-doc, just to make sure that that actually has the intended effect?

[RalfJung]

This looks great, I just have some minor nits. :)

[ghost]

This PR does not pass a test that will be introduced in #1709:

Details

## Running `cargo miri` tests
A libstd for Miri is now available in `/home/user/.cache/miri/HOST`.
Testing `cargo miri run` (no isolation)...
Testing `cargo miri run` (with arguments and target)...
Testing `cargo miri run` (subcrate, no ioslation)...
Testing `cargo miri test`...
--- BEGIN stdout ---

running 1 test
.
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out


running 7 tests
..i....
test result: ok. 6 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out

--- END stdout ---
--- BEGIN stderr ---
error[E0460]: found possibly newer version of crate `std` which `issue_1691` depends on
 --> /home/user/miri/test-cargo-miri/src/lib.rs:6:5
  |
6 |     issue_1691::use_me()
  |     ^^^^^^^^^^
  |
  = note: perhaps that crate needs to be recompiled?
  = note: the following crate versions were found:
          crate `std`: /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-6f5658153d127ddd.rlib
          crate `std`: /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-6f5658153d127ddd.so
          crate `issue_1691`: /home/user/miri/test-cargo-miri/target/x86_64-unknown-linux-gnu/debug/deps/libissue_1691-30200c4d8cd6fa63.rmeta

error: aborting due to previous error

error: test failed, to rerun pass '--doc'
--- END stderr ---

TEST FAIL: exit code was 1

This can fix it (feel free to cherry-pick it, but it duplicates some --sysroot code, so probably needs to be deduplicated before landing, also note that it contains some unrelated whitespace diff because my editor trimmed trailing whitespaces automatically).

[bors]

☔ The latest upstream changes (presumably #1710) made this pull request unmergeable. Please resolve the merge conflicts.

[RalfJung]

@teryror I haven't heard from you in a while, so I assume you moved on to other things. Thanks a lot for this PR! I will take over and finish it so that it can hopefully land soon. :)

[RalfJung]

Closing in favor of #1757.