Skip to content

Bye bye as command mut #143354

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Shourya742
Copy link
Contributor

This PR adds streaming capabilities to BootstrapCommand and migrate existing command streaming scenario's used in bootstrap.

r? @Kobzol

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jul 3, 2025
@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] build_helper test:false 0.685
error: variables can be used directly in the `format!` string
   --> src/bootstrap/src/utils/exec.rs:657:27
    |
657 |                 Err(e) => panic!("failed to execute command: {:?}\nERROR: {e}", command),
    |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args
    = note: `-D clippy::uninlined-format-args` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::uninlined_format_args)]`
help: change this to
    |
657 -                 Err(e) => panic!("failed to execute command: {:?}\nERROR: {e}", command),
657 +                 Err(e) => panic!("failed to execute command: {command:?}\nERROR: {e}"),
    |

[RUSTC-TIMING] bootstrap test:false 9.444
error: could not compile `bootstrap` (lib) due to 1 previous error
Build completed unsuccessfully in 0:09:56
Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Thanks! Left a few comments.

@@ -449,6 +451,12 @@ enum CommandState<'a> {
},
}

pub struct CommandStreaming {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe StreamingCommand? To keep it consistent with DeferredCommand.

{
command.mark_as_executed();

let child = match process {
Copy link
Member

Choose a reason for hiding this comment

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

We should explicitly panic here and let the user know that stream cannot be combined with caching and that they should use do_not_cache. Actually, it would be even better if stream was a method on BoostrapCommand, that would disable caching and return the streaming command directly, rather than going through DeferredCommand.

if builder.config.dry_run() {
let streaming_command = cmd.start_capture_stdout(&builder.config.exec_ctx).stream();

if streaming_command.is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

let Some(mut streaming_command) = streaming_command else { return true; };
@@ -2564,13 +2562,14 @@ pub fn stream_cargo(
}

// Make sure Cargo actually succeeded after we read all of its stdout.
let status = t!(child.wait());
let status = t!(streaming_command.wait());
Copy link
Member

Choose a reason for hiding this comment

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

This is good enough for now, but for profiling we'll want to have a unified way of finishing even the streaming commands in the execution context. But for now we can keep it like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
4 participants