-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: Show cargo check failures to the user #10517
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
Conversation
bors r+ |
Cargo will always output something on success: ``` $ cargo check --message-format=json {"reason":"compiler-artifact", ... snipped ... } {"reason":"build-finished","success":true} ``` However, rustc does not output anything on success: ``` $ rustc --error-format=json main.rs $ echo $? 0 ``` Restore the behaviour prior to rust-lang#10517, where an exit code of 0 is considered good even if nothing is written to stdout. This enables custom overrideCommand values that use rustc rather than cargo.
10756: Allow the check command to terminate without output r=Veykril a=Wilfred Cargo will always output something on success: ``` $ cargo check --message-format=json {"reason":"compiler-artifact", ... snipped ... } {"reason":"build-finished","success":true} ``` However, rustc does not output anything on success: ``` $ rustc --error-format=json main.rs $ echo $? 0 ``` Restore the behaviour prior to #10517, where an exit code of 0 is considered good even if nothing is written to stdout. This enables custom overrideCommand values that use rustc rather than cargo. Co-authored-by: Wilfred Hughes <[email protected]>
Cargo will always output something on success: ``` $ cargo check --message-format=json {"reason":"compiler-artifact", ... snipped ... } {"reason":"build-finished","success":true} ``` However, rustc does not output anything on success: ``` $ rustc --error-format=json main.rs $ echo $? 0 ``` Restore the behaviour prior to rust-lang#10517, where an exit code of 0 is considered good even if nothing is written to stdout. This enables custom overrideCommand values that use rustc rather than cargo.
@@ -258,53 +250,36 @@ impl FlycheckActor { | |||
} | |||
|
|||
struct CargoHandle { | |||
child: JodChild, |
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.
@Veykril this removal of JodChild
feels very suspicious. The job of JodChild
is to kill the underlying cargo
process, when the CargoHandle
is dropped.
Notably, when we receive a Restart
event from the caller, we want to cancel the currently-running cargo check
. We can't just directly cancel it (there's no way to break out of read
syscall nicely), but, if we kill the whole process (which we can do from a different thread).
If we no longer want to terminate the process early, I think we can save one thread here (IIRC, the reason why we need both CargoHandle and FlycheckHandle is exactly to allow killing the 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.
Note that the previous logic was slightly suboptimal. We want self.cargo_handle.take()
just before while let Ok(Restart)
loop.
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.
I see what you mean, that's worth to describe in a comment I think. I'll try to reintroduce the JodChild here.
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.
Yeah, `` (drop) is not the most readable way to spell important stuff
Fixes #10515