Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add missing methods to unix ExitStatusExt #79982

Merged
merged 14 commits into from
Jan 14, 2021
Merged

Conversation

ijackson
Copy link
Contributor

@ijackson ijackson commented Dec 12, 2020

These are the methods corresponding to the remaining exit status examination macros from wait.h. WCOREDUMP isn't in SuS but is it is very standard. I have not done portability testing to see if this builds everywhere, so I may need to Do Something if it doesn't.

There is also a bugfix and doc improvement to .signal(), and an .into_raw() accessor.

This would fix #73128 and fix #73129. Please let me know if you like this direction, and if so I will open the tracking issue and so on.

If this MR goes well, I may tackle #73125 next - I have an idea for how to do it.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 12, 2020
@ijackson
Copy link
Contributor Author

@rustbot modify labels +T-libs

See also the tracking issue #73131

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 12, 2020
library/std/src/sys/unix/ext/process.rs Outdated Show resolved Hide resolved
library/std/src/sys/unix/ext/process.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

r? @dtolnay -- needs T-libs decision on the general direction. I am also not familiar enough with the details of signals and such to know whether this is correct (and my quick manpage investigation is not feeling too productive).

I did flag a concern that this is extending a non-sealed stable extension trait. I forget if we've previously decided that's acceptable, but I presume that would be a breaking change.

@jyn514
Copy link
Member

jyn514 commented Dec 12, 2020

I did flag a concern that this is extending a non-sealed stable extension trait. I forget if we've previously decided that's acceptable, but I presume that would be a breaking change.

This broke async-std in #77089, so I would expect this is a no go for that reason alone :/

@ijackson
Copy link
Contributor Author

This broke async-std in #77089, so I would expect this is a no go for that reason alone :/

Urgh. It hadn't occurred to me that anyone would implement this for their own types. ISTM that not making this trait sealed was a serious mistake.

Given where we are now, I think our options are:

  1. Add a sealed trait std::os::unix::process::ExitStatusExtExt (and maybe get rid of it again in the 2021 edition)
  2. Retrospectively make this trait sealed, presumably with a crater run to check for problems.
  3. Never extend the set of unix-specific methods available on ExitStatus

I think 3 is clearly unacceptable given the fact that, currently, this type is missing at the very least the absolutely necessary access to WCOREDUMP and the obviously desirable into_raw. Without either WCOREDUMP or into_raw it is actually impossible to run a subcommand from Rust, and report its outcome toi the user fully correctly (ie, including whether it coredumped, like the shell does), without effedtively reimplementing the whole of Command.

ExitStatusExtExt is very ugly, but ugliness is the natural consequence of trying to fix earlier API design errors in a backward compatible way :-(.

Sealing the trait may or may not be disruptive. If a crater run would suffice, it could be run on this branch as-is, since adding methods will break all other implementors.

In defence of the idea of retrospectively sealing ExitStatusExt, the situation with ExitStatus is rather different to OpenOptions. OpenOptions has the open method - substantial functionality, which async-std needed to replace while retaining the rest of the methods. ExitStatus doesn't contain any other functionality so I am hoping there wouldn't be the same kind of reason for implementing it for one's own types.

@jyn514
Copy link
Member

jyn514 commented Dec 13, 2020

Sealing the trait may or may not be disruptive. If a crater run would suffice, it could be run on this branch as-is, since adding methods will break all other implementors.

A crater run seems like a good idea to me, but we should probably get feedback from T-libs that this is functionality they want to add first.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I am on board with these. Please file a tracking issue and we can figure out how to land them.

I don't think a crater run is necessarily worth it for this. If someone has a quick way to grep all crates for the string ExitStatusExt for we can do that but otherwise I'm comfortable enough with https://github.com/search?l=Rust&o=desc&q=%22ExitStatusExt+for%22&s=indexed&type=Code showing no results outside std. I think we can land this and listen for reported breakage, and leave it to the first beta crater run to measure fallout.

However I'd like for both ExitStatusExt traits to be changed to sealed in the same PR.

library/std/src/sys/unix/ext/process.rs Outdated Show resolved Hide resolved
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
    Checking miniz_oxide v0.4.0
    Checking object v0.22.0
    Checking hashbrown v0.9.0
    Checking addr2line v0.14.0
error: expected one of `!`, `(`, `+`, `::`, `<`, `where`, or `{`, found `;`
   --> library/std/src/sys/unix/ext/process.rs:212:39
    |
212 | impl private::Sealed for ExitStatusExt;
    |                                       ^ expected one of 7 possible tokens

error[E0432]: unresolved imports `super::process::CommandExt`, `super::process::ExitStatusExt`
   |
   |
74 |     pub use super::process::{CommandExt, ExitStatusExt};
   |                              ^^^^^^^^^^  ^^^^^^^^^^^^^ no `ExitStatusExt` in `sys::unix::ext::process`
   |                              |
   |                              no `CommandExt` in `sys::unix::ext::process`

error[E0599]: no method named `as_raw_fd` found for reference `&ChildStdin` in the current scope
   --> library/std/src/sys/unix/kernel_copy.rs:364:44
    |
364 |         CopyParams(FdMeta::Pipe, Some(self.as_raw_fd()))
    |                                            ^^^^^^^^^ method not found in `&ChildStdin`
    = help: items from traits can only be used if the trait is implemented and in scope
    = help: items from traits can only be used if the trait is implemented and in scope
note: `sys::unix::ext::io::AsRawFd` defines an item `as_raw_fd`, perhaps you need to implement it
    |
    |
22  | pub trait AsRawFd {


error[E0599]: no method named `as_raw_fd` found for reference `&ChildStdout` in the current scope
   --> library/std/src/sys/unix/kernel_copy.rs:370:44
    |
370 |         CopyParams(FdMeta::Pipe, Some(self.as_raw_fd()))
    |                                            ^^^^^^^^^ method not found in `&ChildStdout`
    = help: items from traits can only be used if the trait is implemented and in scope
    = help: items from traits can only be used if the trait is implemented and in scope
note: `sys::unix::ext::io::AsRawFd` defines an item `as_raw_fd`, perhaps you need to implement it
    |
    |
22  | pub trait AsRawFd {


error[E0599]: no method named `as_raw_fd` found for reference `&ChildStderr` in the current scope
   --> library/std/src/sys/unix/kernel_copy.rs:376:44
    |
376 |         CopyParams(FdMeta::Pipe, Some(self.as_raw_fd()))
    |                                            ^^^^^^^^^ method not found in `&ChildStderr`
    = help: items from traits can only be used if the trait is implemented and in scope
    = help: items from traits can only be used if the trait is implemented and in scope
note: `sys::unix::ext::io::AsRawFd` defines an item `as_raw_fd`, perhaps you need to implement it
    |
    |
22  | pub trait AsRawFd {

error: aborting due to 5 previous errors

Some errors have detailed explanations: E0432, E0599.
Some errors have detailed explanations: E0432, E0599.
For more information about an error, try `rustc --explain E0432`.
error: could not compile `std`

To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:01:47

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
    Checking hashbrown v0.9.0
    Checking object v0.22.0
    Checking miniz_oxide v0.4.0
    Checking addr2line v0.14.0
error: expected one of `!`, `(`, `+`, `::`, `<`, `where`, or `{`, found `;`
   --> library/std/src/sys/unix/ext/process.rs:212:39
    |
212 | impl private::Sealed for ExitStatusExt;
    |                                       ^ expected one of 7 possible tokens

error[E0432]: unresolved imports `super::process::CommandExt`, `super::process::ExitStatusExt`
   |
   |
74 |     pub use super::process::{CommandExt, ExitStatusExt};
   |                              ^^^^^^^^^^  ^^^^^^^^^^^^^ no `ExitStatusExt` in `sys::unix::ext::process`
   |                              |
   |                              no `CommandExt` in `sys::unix::ext::process`

error[E0599]: no method named `as_raw_fd` found for reference `&ChildStdin` in the current scope
   --> library/std/src/sys/unix/kernel_copy.rs:364:44
    |
364 |         CopyParams(FdMeta::Pipe, Some(self.as_raw_fd()))
    |                                            ^^^^^^^^^ method not found in `&ChildStdin`
    = help: items from traits can only be used if the trait is implemented and in scope
    = help: items from traits can only be used if the trait is implemented and in scope
note: `sys::unix::ext::io::AsRawFd` defines an item `as_raw_fd`, perhaps you need to implement it
    |
    |
22  | pub trait AsRawFd {


error[E0599]: no method named `as_raw_fd` found for reference `&ChildStdout` in the current scope
   --> library/std/src/sys/unix/kernel_copy.rs:370:44
    |
370 |         CopyParams(FdMeta::Pipe, Some(self.as_raw_fd()))
    |                                            ^^^^^^^^^ method not found in `&ChildStdout`
    = help: items from traits can only be used if the trait is implemented and in scope
    = help: items from traits can only be used if the trait is implemented and in scope
note: `sys::unix::ext::io::AsRawFd` defines an item `as_raw_fd`, perhaps you need to implement it
    |
    |
22  | pub trait AsRawFd {


error[E0599]: no method named `as_raw_fd` found for reference `&ChildStderr` in the current scope
   --> library/std/src/sys/unix/kernel_copy.rs:376:44
    |
376 |         CopyParams(FdMeta::Pipe, Some(self.as_raw_fd()))
    |                                            ^^^^^^^^^ method not found in `&ChildStderr`
    = help: items from traits can only be used if the trait is implemented and in scope
    = help: items from traits can only be used if the trait is implemented and in scope
note: `sys::unix::ext::io::AsRawFd` defines an item `as_raw_fd`, perhaps you need to implement it
    |
    |
22  | pub trait AsRawFd {

error: aborting due to 5 previous errors

Some errors have detailed explanations: E0432, E0599.
Some errors have detailed explanations: E0432, E0599.
For more information about an error, try `rustc --explain E0432`.
error: could not compile `std`

To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:01:39

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
    Checking addr2line v0.14.0
error: trait objects without an explicit `dyn` are deprecated
   --> library/std/src/sys/unix/ext/process.rs:212:26
    |
212 | impl private::Sealed for ExitStatusExt { }
    |                          ^^^^^^^^^^^^^ help: use `dyn`: `dyn ExitStatusExt`
    |
    = note: `-D bare-trait-objects` implied by `-D warnings`

error[E0038]: the trait `ExitStatusExt` cannot be made into an object
   --> library/std/src/sys/unix/ext/process.rs:212:26
    |
212 | impl private::Sealed for ExitStatusExt { }
    |                          ^^^^^^^^^^^^^ `ExitStatusExt` cannot be made into an object
    |
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
   --> library/std/src/sys/unix/ext/process.rs:181:8
    |
177 | pub trait ExitStatusExt : private::Sealed {
    |           ------------- this trait cannot be made into an object...
...
181 |     fn from_raw(raw: i32) -> Self;
    |        ^^^^^^^^ ...because associated function `from_raw` has no `self` parameter
help: consider turning `from_raw` into a method by giving it a `&self` argument
    |
181 |     fn from_raw(&self, raw: i32) -> Self;
    |                 ^^^^^^
help: alternatively, consider constraining `from_raw` so it does not apply to trait objects
    |
181 |     fn from_raw(raw: i32) -> Self where Self: Sized;


error[E0277]: the trait bound `process::ExitStatus: Sealed` is not satisfied
   --> library/std/src/sys/unix/ext/process.rs:215:6
    |
177 | pub trait ExitStatusExt : private::Sealed {
    |                           --------------- required by this bound in `ExitStatusExt`
...
215 | impl ExitStatusExt for process::ExitStatus {
    |      ^^^^^^^^^^^^^ the trait `Sealed` is not implemented for `process::ExitStatus`
error: aborting due to 3 previous errors

Some errors have detailed explanations: E0038, E0277.
For more information about an error, try `rustc --explain E0038`.
For more information about an error, try `rustc --explain E0038`.
error: could not compile `std`

To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:01:29

@ijackson

This comment has been minimized.

@ijackson
Copy link
Contributor Author

ijackson commented Jan 4, 2021

@rustbot modify labels -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2021
@ijackson ijackson requested a review from dtolnay January 4, 2021 18:51
@ijackson
Copy link
Contributor Author

ijackson commented Jan 4, 2021

Hi again. Sorry for the flail on this branch while I tried to get the CI to tell my my syntax errors :-). Worked better when I did it locally.

I think this is all sorted now, and I have squashed it a bit and rebased onto current master.

NB: I made the new trait Sealed insta-stable. Please let me know if that's not right.

Thanks.

@rustbot modify labels -S-waiting-on-author +S-waiting-on-review

@bors
Copy link
Contributor

bors commented Jan 13, 2021

@ijackson: 🔑 Insufficient privileges: not in try users

@ijackson
Copy link
Contributor Author

@ijackson: key Insufficient privileges: not in try users

Oh well.

@rustbot modify labels -S-waiting-on-author +S-waiting-on-review

@ijackson
Copy link
Contributor Author

@rustbot modify labels -S-waiting-on-author +S-waiting-on-review

@ijackson
Copy link
Contributor Author

@dtolnay would you mind re-reviewing? You might want to look at my "fix fuchsia" commit in particular since the situation is suboptimal to start with and I had to make some ... decisions.

As regards the build failure, I did a local "./x.py check" targeting fuchsia which found the original build failure as detected by bors and have checked that this commit fixes it.

@dtolnay
Copy link
Member

dtolnay commented Jan 14, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jan 14, 2021

📌 Commit a8d0161 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2021
@dtolnay
Copy link
Member

dtolnay commented Jan 14, 2021

I added a note in the tracking issue to run this by a Fuchsia dev prior to stabilization.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 14, 2021
Add missing methods to unix ExitStatusExt

These are the methods corresponding to the remaining exit status examination macros from `wait.h`.  `WCOREDUMP` isn't in SuS but is it is very standard.  I have not done portability testing to see if this builds everywhere, so I may need to Do Something if it doesn't.

There is also a bugfix and doc improvement to `.signal()`, and an `.into_raw()` accessor.

This would fix rust-lang#73128 and fix rust-lang#73129.  Please let me know if you like this direction, and if so I will open the tracking issue and so on.

If this MR goes well, I may tackle rust-lang#73125 next - I have an idea for how to do it.
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 14, 2021
Add missing methods to unix ExitStatusExt

These are the methods corresponding to the remaining exit status examination macros from `wait.h`.  `WCOREDUMP` isn't in SuS but is it is very standard.  I have not done portability testing to see if this builds everywhere, so I may need to Do Something if it doesn't.

There is also a bugfix and doc improvement to `.signal()`, and an `.into_raw()` accessor.

This would fix rust-lang#73128 and fix rust-lang#73129.  Please let me know if you like this direction, and if so I will open the tracking issue and so on.

If this MR goes well, I may tackle rust-lang#73125 next - I have an idea for how to do it.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 14, 2021
Rollup of 17 pull requests

Successful merges:

 - rust-lang#79982 (Add missing methods to unix ExitStatusExt)
 - rust-lang#80017 (Suggest `_` and `..` if a pattern has too few fields)
 - rust-lang#80169 (Recommend panic::resume_unwind instead of panicking.)
 - rust-lang#80217 (Add a `std::io::read_to_string` function)
 - rust-lang#80444 (Add as_ref and as_mut methods for Bound)
 - rust-lang#80567 (Add Iterator::intersperse_with)
 - rust-lang#80829 (Get rid of `DepConstructor`)
 - rust-lang#80895 (Fix handling of malicious Readers in read_to_end)
 - rust-lang#80966 (Deprecate atomic::spin_loop_hint in favour of hint::spin_loop)
 - rust-lang#80969 (Use better ICE message when no MIR is available)
 - rust-lang#80972 (Remove unstable deprecated Vec::remove_item)
 - rust-lang#80973 (Update books)
 - rust-lang#80980 (Fixed incorrect doc comment)
 - rust-lang#80981 (Fix -Cpasses=list and llvm version print with -vV)
 - rust-lang#80985 (Fix stabilisation version of slice_strip)
 - rust-lang#80990 (llvm: Remove the unused context from CreateDebugLocation)
 - rust-lang#80991 (Fix formatting specifiers doc links)

Failed merges:

 - rust-lang#80944 (Use Option::map_or instead of `.map(..).unwrap_or(..)`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8ac21fb into rust-lang:master Jan 14, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 14, 2021
@ijackson ijackson deleted the exit-status branch August 24, 2021 17:05
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 22, 2021
…aahc

Stabilise unix_process_wait_more, extra ExitStatusExt methods

This stabilises the feature `unix_process_wait_more`.  Tracking issue rust-lang#80695, FCP needed.

This was implemented in rust-lang#79982 and merged in January.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 22, 2021
…aahc

Stabilise unix_process_wait_more, extra ExitStatusExt methods

This stabilises the feature `unix_process_wait_more`.  Tracking issue rust-lang#80695, FCP needed.

This was implemented in rust-lang#79982 and merged in January.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 23, 2021
…aahc

Stabilise unix_process_wait_more, extra ExitStatusExt methods

This stabilises the feature `unix_process_wait_more`.  Tracking issue rust-lang#80695, FCP needed.

This was implemented in rust-lang#79982 and merged in January.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

std::os::unix::process::ExitStatusExt .coredumped() method std::os::unix::process::ExitStatusExt into_raw
8 participants