-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
base: master
Are you sure you want to change the base?
Bye bye as command mut #143354
Conversation
The job Click to see the possible cause of the failure (guessed by this bot)
|
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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.
This PR adds streaming capabilities to BootstrapCommand and migrate existing command streaming scenario's used in bootstrap.
r? @Kobzol