Fixes to ExitStatus and its docs#82411
Conversation
ijackson
commented
Feb 22, 2021
ijackson
commented
- On Unix, properly display every possible wait status (and don't panic on weird values)
- In the documentation, be clear and consistent about "exit status" vs "wait status".
Currently, on Nightly, this panics:
```
use std::process::ExitStatus;
use std::os::unix::process::ExitStatusExt;
fn main() {
let st = ExitStatus::from_raw(0x007f);
println!("st = {}", st);
}
```
This is because the impl of Display assumes that if .code() is None,
.signal() must be Some. That was a false assumption, although it was
true with buggy code before
5b1316f
unix ExitStatus: Do not treat WIFSTOPPED as WIFSIGNALED
This is not likely to have affected many people in practice, because
`Command` will never produce such a wait status (`ExitStatus`).
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
|
r? @dtolnay (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
dtolnay
left a comment
dtolnay
left a comment
There was a problem hiding this comment.
LGTM once the tidy failure is resolved. Thanks!
ijackson
force-pushed
the
fix-exitstatus
branch
from
31e8907 to
a6b229e
Compare
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
The use of `ExitStatus` as the Rust type name for a Unix *wait status*, not an *exit status*, is very confusing, but sadly probably too late to change. This area is confusing enough in Unix already (and many programmers are already confuxed). We can at least document it. I chose *not* to mention the way shells like to exit with signal numbers, thus turning signal numbers into exit statuses. This is only relevant for Rust programs using `std::process` if they run shells. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
ijackson
force-pushed
the
fix-exitstatus
branch
from
a6b229e to
4bb8425
Compare
Oops. My privsep workflow means I sometimes forget the tidy check. Fixed now (and squashed the fix in). Thanks. |
dtolnay
left a comment
dtolnay
left a comment
There was a problem hiding this comment.
Thanks for the reminder. I just had way too many things stack up, sorry for letting this drop. Looks good!
|
@bors r+ |
|
📌 Commit 4bb8425 has been approved by |
bors
added
S-waiting-on-bors
No problem, I know the feeling. Thank you :-). |
Fixes to ExitStatus and its docs * On Unix, properly display every possible wait status (and don't panic on weird values) * In the documentation, be clear and consistent about "exit status" vs "wait status".
Fixes to ExitStatus and its docs * On Unix, properly display every possible wait status (and don't panic on weird values) * In the documentation, be clear and consistent about "exit status" vs "wait status".
|
Failed in rollup: #82749 (comment) |
bors
added
S-waiting-on-author
Sorry about that. I will restrict the failing test cases to Linux. |
MacOS uses a different representation. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
If different unices have different bit patterns for WIFSTOPPED and WIFCONTINUED then simply being glibc is probably not good enough for this rather ad-hoc test to work. Do it on Linux only. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
This comment has been minimized.
This comment has been minimized.
I strongly disagree with tidy in this case but AIUI there is no way to override it. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
|
OK I think I have fixed this now. @rustbot modify labels -S-waiting-on-author +S-waiting-on-review |
rustbot
added
S-waiting-on-review
|
@JohnTitor Should I be asking you to re-approve it, or ...? (Thanks for your attention and sorry for messing up your rollup.) |
|
@JohnTitor (or anyone) Also, if I may ask: if I have a branch which I suspect might have portability problems, is there a less disruptive way for me to get the full rust CI run on it than to try to get it approved, and wait for it to make a mess for you? I could ask the reviewer to mark it not for rollup, but ideally I would get the computers to find the problems before bothering a human reviewer with re-review over portability fixes. |
|
Seems reasonable to me but I'm not part of the reviewers so r? @dtolnay
If it makes sense, we can run a try build (via |
…itionally Co-authored-by: David Tolnay <dtolnay@gmail.com>
|
@bors r+ |
|
📌 Commit 11ca644 has been approved by |
bors
added
S-waiting-on-bors
Fixes to ExitStatus and its docs * On Unix, properly display every possible wait status (and don't panic on weird values) * In the documentation, be clear and consistent about "exit status" vs "wait status".
Fixes to ExitStatus and its docs * On Unix, properly display every possible wait status (and don't panic on weird values) * In the documentation, be clear and consistent about "exit status" vs "wait status".
Rollup of 10 pull requests Successful merges: - rust-lang#77511 (Add StatementKind::CopyNonOverlapping) - rust-lang#79208 (Stabilize `unsafe_op_in_unsafe_fn` lint) - rust-lang#82411 (Fixes to ExitStatus and its docs) - rust-lang#82733 (Add powerpc-unknown-openbsd target) - rust-lang#82802 (Build rustdoc for run-make tests, not just run-make-fulldeps) - rust-lang#82849 (Add Option::get_or_default) - rust-lang#82908 (:arrow_up: rust-analyzer) - rust-lang#82937 (Update README.md to use the correct cmake version number) - rust-lang#82938 (Bump tracing-tree dependency) - rust-lang#82942 (Don't hardcode the `v1` prelude in diagnostics, to allow for new preludes.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
7 participants
