Skip to content

Use real exec on cfg(unix) targets #2426

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 28, 2022
Merged

Use real exec on cfg(unix) targets #2426

merged 1 commit into from
Jul 28, 2022

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Jul 23, 2022 •
edited
Loading

Closes #2421

The standard library has a platform extension trait that lets us get the behavior we want on cfg(unix), so why not use it?

I tried this out and it produces the correct behavior in concert with nextest.

@saethlin
Copy link
Member Author

Hunh, didn't know about the tests for cargo-miri. Definitely not the right behavior here...

@saethlin saethlin marked this pull request as draft July 23, 2022 18:21
RalfJung
RalfJung reviewed Jul 23, 2022
View reviewed changes
@RalfJung RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jul 23, 2022
@saethlin
Copy link
Member Author

Well I now know how to use pipe2, though I'm really not sure if this the implementation we really want to go with. I dislike that I need to resort to using libc here, as far as I can tell there is no way to work with pipes in the standard library.

@saethlin
Copy link
Member Author

Ahhhh that's why the standard library doesn't just pipe2 for all cfg(unix). I'll switch it to just pipe later.

@RalfJung
Copy link
Member

Well I now know how to use pipe2, though I'm really not sure if this the implementation we really want to go with. I dislike that I need to resort to using libc here, as far as I can tell there is no way to work with pipes in the standard library.

Yeah, I came to about the same conclusion. OTOH finding a location for a temporary file could also be annoying. And who's going to clean up that file?

@saethlin saethlin force-pushed the unix-exec branch 2 times, most recently from 74d719c to 9525719 Compare July 24, 2022 18:49
@saethlin saethlin marked this pull request as ready for review July 24, 2022 20:45
@saethlin
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Jul 24, 2022
RalfJung
RalfJung reviewed Jul 24, 2022
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good. :)

(We really should split this into multiple files... another time.)

@RalfJung
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Jul 24, 2022
@saethlin
Copy link
Member Author

By the way, I've now read every piece of documentation on pipes that I could find and oh boy there is not very much. I was reminded that pipes have a finite capacity which is not particularly huge. MacOS is probably 16 kB by default? Linux is probably 64 kB by default? I'm just extra wary of these limits now, because the "actually exec" implementation doesn't have the child process running so if the file contents don't fit in the pipe... 🤷 we just panic from an expect I think.

My Macbook reports that its pipe size is 512 bytes via ulimit -a which seems awfully small but this branch can run doctests on it just fine 🤷

@RalfJung
Copy link
Member

Yeah I was wondering about pipe size issues... maybe the more robust solution is to store the input in a separate file next to the JSON, and then pass that file as stdin? Though that does make me wonder about who's going to clean up those files. (Well I already wonder that with the new *_miri files this PR starts creating.)

@saethlin
Copy link
Member Author

Hunh. The rust_out_miri files are getting cleaned up. It looks like rustdoc creates them using the tempfile crate. I need to sleep, but maybe that suggests a way to clean up a stdin file?

@RalfJung
Copy link
Member

On Unix you can open a file and delete it and it'll get actually deleted when it gets closed... but I am not sure if that is possible on Windows.

@saethlin
Copy link
Member Author

Good thing we don't need that sort of pattern on Windows then?

@RalfJung
Copy link
Member

RalfJung commented Jul 25, 2022 •
edited
Loading

Oh yeah I guess for now we're only trying to real-exec on Unix, fair.

@sunshowers sunshowers mentioned this pull request Jul 25, 2022
Closed
@saethlin saethlin force-pushed the unix-exec branch 2 times, most recently from f1a0615 to c92423b Compare July 26, 2022 04:25
@saethlin
Copy link
Member Author

@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jul 26, 2022
@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Jul 26, 2022
RalfJung
RalfJung reviewed Jul 27, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jul 27, 2022
View reviewed changes
@RalfJung
Copy link
Member

Just one more comment, then please squash it all and we're good to go. :)

@saethlin
Use real exec on cfg(unix) targets
622613f 
When cargo-miri is executed as a cargo test runner or rustdoc runtool,
external tools expect what they launch as the runner/runtool to be the
process actually running the test. But in the implementation, we launch
the Miri interpreter as a subprocess using std::process::Command. This
tends to confuse other tools (like nextest) and users (like the author).
What we really want is to call POSIX exec so that the cargo-miri process
becomes the interpreter.

So this implements just that; we call execve via a cfg(unix) extension
trait. Windows has no such mechanism, but it also doesn't have POSIX
signals, which is the primary tripping hazard this change fixes.
@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 28, 2022

📌 Commit 622613f has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 28, 2022

⌛ Testing commit 622613f with merge a719c05...

@bors
Copy link
Contributor

bors commented Jul 28, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing a719c05 to master...

@bors bors merged commit a719c05 into rust-lang:master Jul 28, 2022
@saethlin saethlin deleted the unix-exec branch July 28, 2022 22:38
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees
No one assigned
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nextest timeouts don't work because cargo-miri doesn't propagate signals?
4 participants
@saethlin @RalfJung @bors @rustbot