Allow blocking `Command::output` #105458

Ayush1325 · 2022-12-08T13:40:33Z

Problem

Currently, Command::output is internally implemented using Command::spawn. This is problematic because some targets (like UEFI) do not actually support multitasking and thus block while the program is executing. This coupling does not make much sense as Command::output is supposed to block until the execution is complete anyway and thus does not need to rely on a non-blocking Child or any other intermediate.

Solution

This PR moves the implementation of Command::output to std::sys. This means targets can choose to implement only Command::output without having to implement Command::spawn.

Additional Information

This was originally conceived when working on #100316. Currently, the only target I know about that will benefit from this change is UEFI.

This PR can also be used to implement more efficient Command::output since the intermediate Process is not actually needed anymore, but that is outside the scope of this PR.

Since this is not a public API change, I'm not sure if an RFC is needed or not.

Add `read_to_end` method for `sys::{target}::pipe::AnonPipe`. This allows having a more optimized version of `read_to_end` for ChildStdout. Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>

rustbot · 2022-12-08T13:40:40Z

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2022-12-08T13:40:43Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

Mark-Simulacrum · 2022-12-10T15:15:14Z

It seems like this doesn't change or add support itself for any platforms that can block but not spawn a subprocess, right? That will come in a future PR?

I think the changes here seem fine -- but the PR description could be updated to make clear that this isn't changing anything, just shuffling some APIs to make it easier to make changes in the future.

Ayush1325 · 2022-12-10T15:17:47Z

It seems like this doesn't change or add support itself for any platforms that can block but not spawn a subprocess, right? That will come in a future PR?

I think the changes here seem fine -- but the PR description could be updated to make clear that this isn't changing anything, just shuffling some APIs to make it easier to make changes in the future.

Ok, will do that. I specifically made this change for UEFI (#100316) but it should be helpful for any target that is similar.

Ayush1325 · 2022-12-10T15:29:05Z

@rustbot ready

Mark-Simulacrum · 2022-12-10T15:32:05Z

Thanks!

@bors r+

bors · 2022-12-10T15:32:07Z

📌 Commit 1ba9790 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

…imulacrum Allow blocking `Command::output` ### Problem Currently, `Command::output` is internally implemented using `Command::spawn`. This is problematic because some targets (like UEFI) do not actually support multitasking and thus block while the program is executing. This coupling does not make much sense as `Command::output` is supposed to block until the execution is complete anyway and thus does not need to rely on a non-blocking `Child` or any other intermediate. ### Solution This PR moves the implementation of `Command::output` to `std::sys`. This means targets can choose to implement only `Command::output` without having to implement `Command::spawn`. ### Additional Information This was originally conceived when working on rust-lang#100316. Currently, the only target I know about that will benefit from this change is UEFI. This PR can also be used to implement more efficient `Command::output` since the intermediate `Process` is not actually needed anymore, but that is outside the scope of this PR. Since this is not a public API change, I'm not sure if an RFC is needed or not.

matthiaskrgr · 2022-12-10T23:29:36Z

@bors r- failed in a rollup

[RUSTC-TIMING] addr2line test:false 0.462
[RUSTC-TIMING] gimli test:false 4.967
[RUSTC-TIMING] object test:false 5.721
warning: dropping unsupported crate type `dylib` for target `wasm32-unknown-unknown`

error: unused variable: `buf`
  --> library/std/src/sys/wasm/../unsupported/pipe.rs:18:31
   |
18 |     pub fn read_to_end(&self, buf: &mut Vec<u8>) -> io::Result<usize> {
   |                               ^^^ help: if this is intentional, prefix it with an underscore: `_buf`
   |
   = note: `-D unused-variables` implied by `-D warnings`

[RUSTC-TIMING] std test:false 2.677
warning: `std` (lib) generated 1 warning
error: could not compile `std` due to previous error; 1 warning emitted
Build completed unsuccessfully in 0:12:08
== clock drift check ==
  local time: Sat Dec 10 22:55:48 UTC 2022
  network time: Sat, 10 Dec 2022 22:55:48 GMT

#105540 (comment)

This allows decoupling `Command::spawn` and `Command::output`. This is useful for targets which do support launching programs in blocking mode but do not support multitasking (Eg: UEFI). This was originally conceived when working on rust-lang#100316 Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>

Ayush1325 · 2022-12-11T05:37:06Z

@rustbot ready

Mark-Simulacrum · 2022-12-17T15:28:16Z

@bors r+ rollup

bors · 2022-12-17T15:28:17Z

📌 Commit a94793d has been approved by Mark-Simulacrum

It is now in the queue for this repository.

…imulacrum Allow blocking `Command::output` ### Problem Currently, `Command::output` is internally implemented using `Command::spawn`. This is problematic because some targets (like UEFI) do not actually support multitasking and thus block while the program is executing. This coupling does not make much sense as `Command::output` is supposed to block until the execution is complete anyway and thus does not need to rely on a non-blocking `Child` or any other intermediate. ### Solution This PR moves the implementation of `Command::output` to `std::sys`. This means targets can choose to implement only `Command::output` without having to implement `Command::spawn`. ### Additional Information This was originally conceived when working on rust-lang#100316. Currently, the only target I know about that will benefit from this change is UEFI. This PR can also be used to implement more efficient `Command::output` since the intermediate `Process` is not actually needed anymore, but that is outside the scope of this PR. Since this is not a public API change, I'm not sure if an RFC is needed or not.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#104854 (Symlink `build/host` -> `build/$HOST_TRIPLE`) - rust-lang#105458 (Allow blocking `Command::output`) - rust-lang#105559 (bootstrap: Allow installing `llvm-tools`) - rust-lang#105789 (rustdoc: clean up margin CSS for scraped examples) - rust-lang#105792 (docs: add long error explanation for error E0320) - rust-lang#105814 (Support call and drop terminators in custom mir) - rust-lang#105829 (Speed up tidy) - rust-lang#105836 (std::fmt: Use args directly in example code) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Add Process support for UEFI UEFI does not have an actual process. However, it does provide methods to launch and execute another UEFI image. Having process support is important since it is possible to run rust test suit using `Command::output` and is the first step towards being able to run it for UEFI. Here is an overview of how the support is implemented. - We create a copy of the SystemTable. This is required since at least OVMF seems to crash if the original system table is modified. - Stdout and Stderr pipe works by registering a new `simple_text_output` Protocol and pointing the child system table to use those. - `Stdio::Inherit` just points the console to the current running image console which seems to work with even 3 levels of process. - `spawn` is left unimplemented since it does not make sense for UEFI architecture. Additionally, since rust-lang#105458 was merged, the `spawn` and `output` implementations are completely independent.

Rollup merge of rust-lang#123196 - Ayush1325:uefi-process, r=joboet Add Process support for UEFI UEFI does not have an actual process. However, it does provide methods to launch and execute another UEFI image. Having process support is important since it is possible to run rust test suit using `Command::output` and is the first step towards being able to run it for UEFI. Here is an overview of how the support is implemented. - We create a copy of the SystemTable. This is required since at least OVMF seems to crash if the original system table is modified. - Stdout and Stderr pipe works by registering a new `simple_text_output` Protocol and pointing the child system table to use those. - `Stdio::Inherit` just points the console to the current running image console which seems to work with even 3 levels of process. - `spawn` is left unimplemented since it does not make sense for UEFI architecture. Additionally, since rust-lang#105458 was merged, the `spawn` and `output` implementations are completely independent.

Add read_to_end for AnonPipe

5479fe5

Add `read_to_end` method for `sys::{target}::pipe::AnonPipe`. This allows having a more optimized version of `read_to_end` for ChildStdout. Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>

rustbot assigned Mark-Simulacrum Dec 8, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 8, 2022

Mark-Simulacrum 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 Dec 10, 2022

Ayush1325 changed the title ~~Implement blocking output for Command~~ Allow blocking Command::output Dec 10, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 10, 2022

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 Dec 10, 2022

matthiaskrgr mentioned this pull request Dec 10, 2022

Rollup of 7 pull requests #105540

Closed

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 Dec 10, 2022

Ayush1325 force-pushed the blocking_spawn branch from 1ba9790 to a94793d Compare December 11, 2022 04:52

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 11, 2022

Ayush1325 mentioned this pull request Dec 11, 2022

Add batch flag to remote-test-server #105145

Merged

Ayush1325 mentioned this pull request Dec 14, 2022

Add Rust std support for x86_64-unknown-uefi #100316

Closed

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 Dec 17, 2022

This was referenced Dec 17, 2022

Rollup of 5 pull requests #105841

Closed

Rollup of 8 pull requests #105849

Merged

bors merged commit 6d1cdca into rust-lang:master Dec 18, 2022

rustbot added this to the 1.68.0 milestone Dec 18, 2022

Ayush1325 deleted the blocking_spawn branch December 18, 2022 04:11

Ayush1325 mentioned this pull request Mar 29, 2024

Add Process support for UEFI #123196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow blocking `Command::output` #105458

Allow blocking `Command::output` #105458

Ayush1325 commented Dec 8, 2022 •

edited

Loading

rustbot commented Dec 8, 2022

rustbot commented Dec 8, 2022

Mark-Simulacrum commented Dec 10, 2022

Ayush1325 commented Dec 10, 2022

Ayush1325 commented Dec 10, 2022

Mark-Simulacrum commented Dec 10, 2022

bors commented Dec 10, 2022

matthiaskrgr commented Dec 10, 2022

Ayush1325 commented Dec 11, 2022

Mark-Simulacrum commented Dec 17, 2022

bors commented Dec 17, 2022

Allow blocking Command::output #105458

Allow blocking Command::output #105458

Conversation

Ayush1325 commented Dec 8, 2022 • edited Loading

Problem

Solution

Additional Information

rustbot commented Dec 8, 2022

rustbot commented Dec 8, 2022

Mark-Simulacrum commented Dec 10, 2022

Ayush1325 commented Dec 10, 2022

Ayush1325 commented Dec 10, 2022

Mark-Simulacrum commented Dec 10, 2022

bors commented Dec 10, 2022

matthiaskrgr commented Dec 10, 2022

Ayush1325 commented Dec 11, 2022

Mark-Simulacrum commented Dec 17, 2022

bors commented Dec 17, 2022

Allow blocking `Command::output` #105458

Allow blocking `Command::output` #105458

Ayush1325 commented Dec 8, 2022 •

edited

Loading