[RalfJung]

View all comments

We currently run some subset of std tests in Miri, skipping some because Miri doesn't support the operations they need. That is based on filters in the makefile. People don't know what is actually being tested and add random cfg(miri) in places to disable tests that were never run. This makes it quite hard to track which tests are being skipped and which operations we should add in Miri to get 100% std coverage. So let's replace that string-based filter with a cfg flag, skip_unsupported_miri_tests, that can be annotated at the tests.

Unfortunately I can't figure out how to disable all doctests in a module based on a cfg flag, so this entirely disables std doctests being run in Miri in our CI. (We still run them in https://github.com/rust-lang/miri-test-libstd.)

See prior discussion on Zulip.
Docs PR: rust-lang/std-dev-guide#86

r? @tgross35
try-job: test-x86_64-gnu-aux

[rustbot]

Any special-casing of Miri in the standard library requires review.

cc @rust-lang/miri

[rustbot]

tgross35 is currently at their maximum review capacity.
They may take a while to respond.

[RalfJung]

@bors try

[tgross35]

I'm pretty okay with this, @rust-lang/libs-ping any thoughts? See the Zulip discussion linked in the top post for context

[rust-bors]

☀️ Try build successful (CI)
Build commit: ea60bf0 (ea60bf0be95c912404a2821f1c21a97850d94d6e)
Base parent: ce37c4f (ce37c4ff837c5c5dc45cddf9e1e3704e42ca67ea)

[RalfJung]

I screwed this up in #164963...

View changes since the review

[RalfJung]

I'm pretty okay with this, @rust-lang/libs-ping any thoughts? See the Zulip discussion linked in the top post for context

It's been a week -- seems like nobody has major concerns?

[maxdexh]

Yeah I'd rather split that into a separate PR (or r? infra after I'm done here)

[maxdexh]

r+ from my side, the changes to src/bootstrap/mk/Makefile.in and src/ci/docker/host-x86_64/test-x86_64-gnu-miri/check-miri.sh i don't feel confident approving

View changes since this review

[maxdexh]

r? infra

[RalfJung]

src/bootstrap/mk/Makefile.in

Are you not comfortable because you don't understand what the diff does, or because you don't feel "in charge"?
Those changes are kind of driven by libs, specifically the request to keep testing libstd in Miri.

src/ci/docker/host-x86_64/test-x86_64-gnu-miri/check-miri.sh

I split out most of that change.
What remains is a fairly simple change to compensate for library/std/tests/pipe_subprocess.rs not having a cfg(miri) any more. It's just adding the new --cfg flag. Should be uncontroversial.

[maxdexh]

Are you not comfortable because you don't understand what the diff does

This. I don't undestand what BOOTSTRAP_SKIP_TARGET_SANITY does or why it's needed, and why skipping doctests is fine here.

I split out most of that change.

Oh, I didn't see that. Yeah that file is fine

[RalfJung]

I don't undestand what BOOTSTRAP_SKIP_TARGET_SANITY does,

Usually bootstrap complains when you use --target <windows> and you don't actually have link.exe. This tells it to skip that check because Miri doesn't actually need link.exe to run code for that target.

and why skipping doctests is fine here.

Well... I am asking libs to please say that it should be fine. :)
The alternative is to find a way to skip all the unsupported doctests when the cfg flag is set, and that does not seem to be easy. I couldn't find a way to make it work. (Well, I know how to edit each test to make it work, but that's terrible IMO. I'd like something that can be done on a module level.)
So yeah this PR does reduce test coverage for std doctests in bors CI. We're still testing them in https://github.com/rust-lang/miri-test-libstd so, with a delay, we'll notice if they have UB.

[maxdexh]

Hmm, alright, I think this should be fine then.

Sorry I tend to compensate inexperience with thoroughness here ^^

@bors r+

Just one more question:

The alternative is to find a way to skip all the unsupported doctests when the cfg flag is set, and that does not seem to be easy. I couldn't find a way to make it work. (Well, I know how to edit each test to make it work, but that's terrible IMO. I'd like something that can be done on a module level.)

If we have unsupported doctests, that implies to me that we're not running them (otherwise CI would fail). So why does disabling them reduce coverage?

[rust-bors]

📌 Commit d4c13db has been tentatively approved by maxdexh

It will be put into the queue for this repository once PR CI succeeds.

[RalfJung]

Sorry I tend to compensate inexperience with thoroughness here ^^

Completely fine. :) I still remember how overwhelmed and insecure I was when I reviewed the first PRs here.

If we have unsupported doctests, that implies to me that we're not running them (otherwise CI would fail). So why does disabling them reduce coverage?

Before this PR we filter tests in the makefile, that's the --skip fs:: --skip net:: --skip process:: --skip sys::. But that is crude and also people patching std don't know about it so they add cfg(miri) in random places. This PR moves us away from such ad-hoc string-based filters.

[rust-bors]

⚠️ A new commit 3cf3c878a2fb6e3667187622096d944190eda03a was pushed.

This pull request was unapproved.

[RalfJung]

Fixed the commit message.
@bors r=maxdexh

[rust-bors]

📌 Commit 3cf3c87 has been tentatively approved by maxdexh

It will be put into the queue for this repository once PR CI succeeds.

[maxdexh]

Before this PR we filter tests in the makefile, that's the --skip fs:: --skip net:: --skip process:: --skip sys::

Yikes. I thought there was no way those cfgs were enough to cover that.

If we come up with a way to make the doctest filtering work in-file, can we enable them again?

[RalfJung]

If we come up with a way to make the doctest filtering work in-file, can we enable them again?

Yes.

Also FWIW at least on Linux we can actually run almost all doctests. (Many in net/fs are no-run as they need more setup than a doctest can muster.) So maybe in a few months we can just use the annoying per-test way to disable the remaining few...

Full list of doctests that fail:

      std/src/os/unix/net/addr.rs - os::unix::net::addr::SocketAddr (line 81)
      std/src/os/unix/net/addr.rs - os::unix::net::addr::SocketAddr::as_pathname (line 240)
      std/src/os/unix/net/addr.rs - os::unix::net::addr::SocketAddr::is_unnamed (line 202)
      std/src/os/unix/net/datagram.rs - os::unix::net::datagram::UnixDatagram::read_timeout (line 806)
      std/src/os/unix/net/datagram.rs - os::unix::net::datagram::UnixDatagram::set_nonblocking (line 852)
      std/src/os/unix/net/datagram.rs - os::unix::net::datagram::UnixDatagram::set_read_timeout (line 719)
      std/src/os/unix/net/datagram.rs - os::unix::net::datagram::UnixDatagram::set_write_timeout (line 766)
      std/src/os/unix/net/datagram.rs - os::unix::net::datagram::UnixDatagram::write_timeout (line 829)
      std/src/os/unix/process.rs - os::unix::process::parent_id (line 819)
      std/src/process.rs - process::Command (line 538)
      std/src/process.rs - process::ExitStatus::exit_ok (line 1947)
      std/src/process.rs - process::ExitStatusError (line 2050)
      std/src/process.rs - process::ExitStatusError::code (line 2094)
      std/src/process.rs - process::ExitStatusError::code_nonzero (line 2118)
      std/src/process.rs - process::Output::exit_ok (line 1498)
      std/src/process.rs - process::Stdio::from (line 1832)
      std/src/process.rs - process::Stdio::from (line 1863)

[maxdexh]

I'll look into that ^^