Replace rustup with running command #972
Conversation
|
@birkenfeld, |
|
@brson, |
src/rustup/command.rs
Outdated
| fn run(mut command: Command, arg0: &str) -> Result<()> { | ||
| use std::os::unix::process::CommandExt; | ||
| let error = command.exec(); | ||
| Err(error).chain_err(|| rustup_utils::ErrorKind::RunningCommand { |
There was a problem hiding this comment.
You’ll probably want to chain the error after calling run, like this:
run(cmd, arg0).chain_err(|| ...)
instead. That greatly cuts down on the amount of code needed.
There was a problem hiding this comment.
See for example similar code in cargo-fuzz
|
Hi @KalitaAlexey. Thanks for the fixes, and sorry for the delay. This does solve the problem, but I want to solve these two issues a different way, and the main reason is that rustup wants to be able to perform work after the invoked subcommand is run (e.g. recording telemetry about the results). I am not prepared to give up that capability yet. I'd instead prefer to do something more along the lines described in the op of #806, installing a signal handler and capturing Ctrl+C. Likewise the signal-swallowing can be handled by examining how the subprocess terminated. |
|
☔ The latest upstream changes (presumably #968) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@brson, |
|
Theres another good reason to exec that is proper status reporting and
debugability. Namely currently the exit code is always 1 regardless of
rustc editting with code 101 or sogsegving. You cannot use gbd through the
wrapper that shadows the name of the rustc either which makes the use case
a pain. Exec solves all of these.
…On Mar 17, 2017 6:53 AM, "Alexey" ***@***.***> wrote:
@brson <https://github.com/brson>,
If you know how to do that, you should do it because I don't know.
I need to think about it.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#972 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AApc0qHiaLxdzfoJW6UJvlCMpGbxtMjAks5rmhG4gaJpZM4MOpoZ>
.
|
|
I agree with @nagisa - especially in that rustup-rustc/rustup-cargo should behave the same as normal rustc/cargo. This is otherwise very confusing to users who shouldn't have to care about how their toolchain was installed. |
|
I'll close this, since we're not planning on using exec - further discussion about whether this is the right decision can happen on the issue (#31) |
I copied the implementation from Cargo.
I don't know if I can add any tests for that.
I checked on Ubuntu and Windows.
Fixes #806
Fixes #845