Make cargo-miri run documentation tests #1671

teryror · 2021-01-14T18:02:18Z

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 · 2021-01-16T20:47:33Z

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.

test-cargo-miri/run-test.py

RalfJung · 2021-01-17T19:17:53Z

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 · 2021-01-17T19:46:55Z

No worries, take all the time you need!

RalfJung · 2021-01-23T11:16:22Z

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.

test-cargo-miri/test.stderr-rustdoc.ref

cargo-miri/bin.rs

teryror · 2021-01-23T17:28:14Z

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 · 2021-01-23T17:39:42Z

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 · 2021-01-23T17:55:25Z

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?

cargo-miri/bin.rs

README.md

RalfJung · 2021-01-24T15:31:48Z

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 · 2021-01-24T16:12:57Z

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

teryror · 2021-01-25T13:28:17Z

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 · 2021-01-27T18:02:28Z

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 · 2021-01-27T18:13:51Z

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 · 2021-01-27T18:32:44Z

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 · 2021-01-27T18:35:04Z

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 · 2021-02-02T14:45:56Z

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

teryror · 2021-02-04T08:48:12Z

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.

cargo-miri/bin.rs

RalfJung · 2021-02-06T17:50:18Z

test-cargo-miri/run-test.py

        env={'MIRIFLAGS': "-Zmiri-disable-isolation"},
    )
    test("`cargo miri test` (raw-ptr tracking)",
        cargo_miri("test"),
-        "test.default.stdout.ref", rustdoc_ref,
+        default_ref, "test.stderr-empty.ref",


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

RalfJung · 2021-02-06T17:50:26Z

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

cargo-miri/bin.rs

ghost · 2021-02-14T12:45:38Z

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

## 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 · 2021-02-15T14:21:23Z

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

RalfJung · 2021-04-05T09:37:36Z

@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 · 2021-04-05T10:52:02Z

Closing in favor of #1757.

add rustdoc support `@teryror` did all the work in #1671; I just finished things up and fixed conflicts. Also thanks to `@hyd-dev` for preemptively fixing a sysroot issue that would have taken me some time to diagnose. Fixes #584

teryror commented Jan 16, 2021

View reviewed changes

test-cargo-miri/run-test.py Outdated Show resolved Hide resolved

RalfJung reviewed Jan 23, 2021

View reviewed changes

test-cargo-miri/test.stderr-rustdoc.ref Show resolved Hide resolved

RalfJung reviewed Jan 23, 2021

View reviewed changes

cargo-miri/bin.rs Show resolved Hide resolved

RalfJung reviewed Jan 23, 2021

View reviewed changes

cargo-miri/bin.rs Show resolved Hide resolved

RalfJung reviewed Jan 23, 2021

View reviewed changes

cargo-miri/bin.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jan 23, 2021

View reviewed changes

cargo-miri/bin.rs Show resolved Hide resolved

RalfJung reviewed Jan 23, 2021

View reviewed changes

cargo-miri/bin.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jan 23, 2021

View reviewed changes

cargo-miri/bin.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jan 23, 2021

View reviewed changes

cargo-miri/bin.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jan 23, 2021

View reviewed changes

cargo-miri/bin.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jan 23, 2021

View reviewed changes

cargo-miri/bin.rs Show resolved Hide resolved

teryror and others added 5 commits January 23, 2021 18:23

Prototype: make cargo-miri run doc-tests

f7bfbe7

Add check builds to support compile_fail doc-tests

46e9dbc

Play back stdin only when called from rustdoc

11c3029

Update test-cargo-miri

75053be

Update README, add more comments, cleanup test-cargo-miri

b10a1b0

teryror force-pushed the master branch from c06cc6e to b10a1b0 Compare January 23, 2021 17:24

RalfJung reviewed Jan 24, 2021

View reviewed changes

cargo-miri/bin.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jan 24, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

Update README.md

10115cd

teryror force-pushed the master branch from c430e50 to 05e7a28 Compare January 27, 2021 17:24

Resolve merge conflicts

154fb52

teryror force-pushed the master branch from 05e7a28 to 154fb52 Compare January 27, 2021 17:36

Detect cross-mode and skip doc-tests

10345ff

teryror force-pushed the master branch from 1ea3bef to 10345ff Compare January 28, 2021 18:05

RalfJung reviewed Feb 6, 2021

View reviewed changes

cargo-miri/bin.rs Show resolved Hide resolved

RalfJung reviewed Feb 6, 2021

View reviewed changes

cargo-miri/bin.rs Show resolved Hide resolved

RalfJung reviewed Feb 6, 2021

View reviewed changes

cargo-miri/bin.rs Show resolved Hide resolved

RalfJung reviewed Feb 6, 2021

View reviewed changes

cargo-miri/bin.rs Show resolved Hide resolved

RalfJung reviewed Feb 6, 2021

View reviewed changes

cargo-miri/bin.rs Show resolved Hide resolved

RalfJung reviewed Feb 6, 2021

View reviewed changes

cargo-miri/bin.rs Show resolved Hide resolved

RalfJung reviewed Feb 6, 2021

View reviewed changes

cargo-miri/bin.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

RalfJung mentioned this pull request Feb 13, 2021

Can't find crate with hyper crate as a dependency #1705

Closed

ghost mentioned this pull request Feb 14, 2021

Patch --extern arguments in phase_cargo_rustc as well #1710

Merged

RalfJung mentioned this pull request Apr 5, 2021

add rustdoc support #1757

Merged

RalfJung closed this Apr 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make cargo-miri run documentation tests #1671

Make cargo-miri run documentation tests #1671

teryror commented Jan 14, 2021

teryror commented Jan 16, 2021

RalfJung commented Jan 17, 2021

teryror commented Jan 17, 2021

RalfJung commented Jan 23, 2021

teryror commented Jan 23, 2021

RalfJung commented Jan 23, 2021

teryror commented Jan 23, 2021

RalfJung commented Jan 24, 2021

bors commented Jan 24, 2021

teryror commented Jan 25, 2021 •

edited

Loading

teryror commented Jan 27, 2021

RalfJung commented Jan 27, 2021

teryror commented Jan 27, 2021

RalfJung commented Jan 27, 2021

RalfJung commented Feb 2, 2021

teryror commented Feb 4, 2021

RalfJung Feb 6, 2021

RalfJung commented Feb 6, 2021

This comment has been minimized.

ghost commented Feb 14, 2021

bors commented Feb 15, 2021

RalfJung commented Apr 5, 2021

RalfJung commented Apr 5, 2021

Make cargo-miri run documentation tests #1671

Make cargo-miri run documentation tests #1671

Conversation

teryror commented Jan 14, 2021

teryror commented Jan 16, 2021

RalfJung commented Jan 17, 2021

teryror commented Jan 17, 2021

RalfJung commented Jan 23, 2021

teryror commented Jan 23, 2021

RalfJung commented Jan 23, 2021

teryror commented Jan 23, 2021

RalfJung commented Jan 24, 2021

bors commented Jan 24, 2021

teryror commented Jan 25, 2021 • edited Loading

teryror commented Jan 27, 2021

RalfJung commented Jan 27, 2021

teryror commented Jan 27, 2021

RalfJung commented Jan 27, 2021

RalfJung commented Feb 2, 2021

teryror commented Feb 4, 2021

RalfJung Feb 6, 2021

Choose a reason for hiding this comment

RalfJung commented Feb 6, 2021

This comment has been minimized.

ghost commented Feb 14, 2021

bors commented Feb 15, 2021

RalfJung commented Apr 5, 2021

RalfJung commented Apr 5, 2021

teryror commented Jan 25, 2021 •

edited

Loading