Print rustup version after update #614
Conversation
| @@ -1069,7 +1071,7 @@ pub fn update() -> Result<()> { | |||
| } | |||
| let setup_path = try!(prepare_update()); | |||
| if let Some(ref p) = setup_path { | |||
| info!("rustup updated successfully"); | |||
| info!("rustup updated successfully to {}", VERSION); | |||
There was a problem hiding this comment.
By printing CARGO_PKG_VERSION here rustup will be printing the version of the currently running version of rustup. Instead, what we want is to print the version we just downloaded (here represented by setup_path). So this is instead going to need to run the executable at setup_path with --version and parse out the new version number.
|
Thanks @julienXX ! |
julienXX
force-pushed
the
print-new-version-after-update
branch
from
b35bb04
to
a23c403
Compare
|
@brson Oh okay, I should have thought about that. Thanks! |
| let re = Regex::new(r"\d+.\d+.\d+").unwrap(); | ||
| let capture = re.captures(&string_version).unwrap().at(0).unwrap(); | ||
| Ok(String::from(capture)) | ||
| } |
There was a problem hiding this comment.
Since this code is on the super-important upgrade path, let's be a little more defensive. What happens if the new executable doesn't run? Seems like it could happen. What happens if it fails? What happens if the output of --version changes? Seems like we might want to do that (for example returning a version like "2.0.0-rc1"). Instead of unwraps here can they return errors instead? Then have the caller convert those errors to a placeholder string like "(unknown)".
An aside, if the executable fails to run that would be a bad sign - it might be worth thinking about turning that into a hard error and failing the upgrade. But that can be a different PR.
There was a problem hiding this comment.
Yes absolutely! I'll update the code to be more defensive. Do you feel like regexp is the way to go or could a string split be sufficient?
There was a problem hiding this comment.
Sorry for the slow response.
|
The test is good enough I think. Yeah, it could be better by having a different version number, but the effort to make that happen isn't worth it. |
julienXX
force-pushed
the
print-new-version-after-update
branch
from
a23c403
to
5ea4473
Compare
|
@brson I updated the code to be more defensive. Also it now exit the process if the executable fails to run. |
|
Looks great. Thanks @julienXX ! |
Fixes #542