Skip to content
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

Add From<u8> for ExitCode #93445

Merged
merged 2 commits into from
Feb 9, 2022
Merged

Add From<u8> for ExitCode #93445

merged 2 commits into from
Feb 9, 2022

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented Jan 28, 2022

This should cover a mostly cross-platform subset of supported exit codes.

We decided to stick with u8 initially since its the common subset between all platforms that we support (excluding wasm which I think only works with true or false). Posix is supposed to take i32s, but in practice many unix platforms mask out all but the low 8 bits or in some cases the 8-15th bits. Windows takes a u32 instead of an i32. Bourne-compatible shells also report signals as exitcode 128 + signal_no, so there's some ambiguity there when returning exit codes > 127, but it is possible to disambiguate them on the other side so we decided against restricting the possible codes further than to u8.

Related

@rust-highfive
Copy link
Contributor

r? @dtolnay

(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 Jan 28, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@yaahc yaahc force-pushed the exitcode-constructor branch from f2ab115 to a22bdc0 Compare January 28, 2022 23:28
@joshtriplett joshtriplett added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 29, 2022
@joshtriplett
Copy link
Member

Trait impls are insta-stable, and even though ExitCode isn't stable, it might be possible for stable code to write something like From::from and potentially depend on this indirectly. So, to be safe, I think this should go through an FCP:

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 29, 2022

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 29, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Jan 29, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 29, 2022
@bors
Copy link
Collaborator

bors commented Feb 1, 2022

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

This should cover a mostly cross-platform subset of supported exit codes.
@dtolnay dtolnay force-pushed the exitcode-constructor branch from a22bdc0 to cf4ac6b Compare February 6, 2022 20:45
@dtolnay
Copy link
Member

dtolnay commented Feb 6, 2022

@dtolnay
Copy link
Member

dtolnay commented Feb 6, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 6, 2022

📌 Commit cf4ac6b 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 Feb 6, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 6, 2022
Add From<u8> for ExitCode

This should cover a mostly cross-platform subset of supported exit codes.

We decided to stick with `u8` initially since its the common subset between all platforms that we support (excluding wasm which I think only works with `true` or `false`). Posix is supposed to take i32s, but in practice many unix platforms mask out all but the low 8 bits or in some cases the 8-15th bits. Windows takes a u32 instead of an i32. Bourne-compatible shells also report signals as exitcode 128 + `signal_no`, so there's some ambiguity there when returning exit codes > 127, but it is possible to disambiguate them on the other side so we decided against restricting the possible codes further than to `u8`.

## Related

- Detailed analysis of exit code support on various platforms: https://internals.rust-lang.org/t/mini-pre-rfc-redesigning-process-exitstatus/5426
- rust-lang#48711
- rust-lang#43301
- https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Termination.2FExit.20Status.20Stabilization
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 7, 2022
Add From<u8> for ExitCode

This should cover a mostly cross-platform subset of supported exit codes.

We decided to stick with `u8` initially since its the common subset between all platforms that we support (excluding wasm which I think only works with `true` or `false`). Posix is supposed to take i32s, but in practice many unix platforms mask out all but the low 8 bits or in some cases the 8-15th bits. Windows takes a u32 instead of an i32. Bourne-compatible shells also report signals as exitcode 128 + `signal_no`, so there's some ambiguity there when returning exit codes > 127, but it is possible to disambiguate them on the other side so we decided against restricting the possible codes further than to `u8`.

## Related

- Detailed analysis of exit code support on various platforms: https://internals.rust-lang.org/t/mini-pre-rfc-redesigning-process-exitstatus/5426
- rust-lang#48711
- rust-lang#43301
- https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Termination.2FExit.20Status.20Stabilization
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 7, 2022
Add From<u8> for ExitCode

This should cover a mostly cross-platform subset of supported exit codes.

We decided to stick with `u8` initially since its the common subset between all platforms that we support (excluding wasm which I think only works with `true` or `false`). Posix is supposed to take i32s, but in practice many unix platforms mask out all but the low 8 bits or in some cases the 8-15th bits. Windows takes a u32 instead of an i32. Bourne-compatible shells also report signals as exitcode 128 + `signal_no`, so there's some ambiguity there when returning exit codes > 127, but it is possible to disambiguate them on the other side so we decided against restricting the possible codes further than to `u8`.

## Related

- Detailed analysis of exit code support on various platforms: https://internals.rust-lang.org/t/mini-pre-rfc-redesigning-process-exitstatus/5426
- rust-lang#48711
- rust-lang#43301
- https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Termination.2FExit.20Status.20Stabilization
@m-ou-se
Copy link
Member

m-ou-se commented Feb 7, 2022

This failed in #93734:

169 |             1..255 => Self::FAILURE,
    |             ^^^^^^
    |
    = note: see issue #37854 <https://github.com/rust-lang/rust/issues/37854> for more information
    = help: add `#![feature(exclusive_range_pattern)]` to the crate attributes to enable
For more information about this error, try `rustc --explain E0658`.
[RUSTC-TIMING] std test:false 1.657
warning: `std` (lib) generated 1 warning
error: could not compile `std` due to previous error; 1 warning emitted

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 7, 2022
@dtolnay
Copy link
Member

dtolnay commented Feb 7, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 7, 2022

📌 Commit 4c5a36e 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 7, 2022
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 8, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Feb 8, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Feb 8, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#93445 (Add From<u8> for ExitCode)
 - rust-lang#93694 (rustdoc: tweak line spacing and paragraph spacing for accessibility)
 - rust-lang#93735 (Stabilize int_abs_diff in 1.60.0.)
 - rust-lang#93746 (Remove defaultness from ImplItem.)
 - rust-lang#93748 (rustc_query_impl: reduce visibility of some modules/fn's)
 - rust-lang#93751 (Drop tracking: track borrows of projections)
 - rust-lang#93781 (update `ty::TyKind` documentation)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ec2fd8a into rust-lang:master Feb 9, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 9, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 10, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 19, 2024
…atrieb

`pal::unsupported::process::ExitCode`: use an `u8` instead of a `bool`

`ExitCode` should “represents the status code the current process can return to its parent under normal termination”, but is currently represented as a `bool` on unsupported platforms, making the `impl From<u8> for ExitCode` lossy.

Fixes rust-lang#130532.

History: [IRLO thread](https://internals.rust-lang.org/t/mini-pre-rfc-redesigning-process-exitstatus/5426) (`ExitCode` as a `main` return), rust-lang#48618 (initial impl), rust-lang#93445 (`From<u8>` impl).
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2024
Rollup merge of rust-lang#130554 - ShE3py:unsupported-exitcode, r=Noratrieb

`pal::unsupported::process::ExitCode`: use an `u8` instead of a `bool`

`ExitCode` should “represents the status code the current process can return to its parent under normal termination”, but is currently represented as a `bool` on unsupported platforms, making the `impl From<u8> for ExitCode` lossy.

Fixes rust-lang#130532.

History: [IRLO thread](https://internals.rust-lang.org/t/mini-pre-rfc-redesigning-process-exitstatus/5426) (`ExitCode` as a `main` return), rust-lang#48618 (initial impl), rust-lang#93445 (`From<u8>` impl).
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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.

10 participants