perf(rust): additionally check rustup default for faster result.
#3354
Conversation
After checking directory overrides we were directly falling back to the relatively slow call to `rustc --version`. Inserting a call to `rustup default` leads to a quicker response.
|
I see I need to fix some rustfmt, also I just saw that the default might be missing, which I will handle. Related rustup section: |
There was a problem hiding this comment.
This is a really nice speed up have left a suggestion but other than that that this LGTM.
One thing to note is that from the docs it implies that rustup default might try to install the toolchain if it is not already installed but after testing with a fresh install it isn't the case if we are just checking what the default is
❯ rustup default --help
rustup-default
Set the default toolchain
USAGE:
rustup default [toolchain]
FLAGS:
-h, --help Prints help information
ARGS:
<toolchain> Toolchain name, such as 'stable', 'nightly', or '1.8.0'. For more information see `rustup help
toolchain`
DISCUSSION:
Sets the default toolchain to the one specified. If the toolchain
is not already installed then it is installed first.
src/modules/rust.rs
Outdated
| fn execute_rustup_default() -> Option<String> { | ||
| let Output { stdout, .. } = create_command("rustup") | ||
| .ok()? | ||
| .args(&["default"]) | ||
| .output() | ||
| .ok()?; | ||
| let stdout = String::from_utf8(stdout).ok()?; | ||
|
|
||
| stdout.split_whitespace().next().map(str::to_owned) | ||
| } |
There was a problem hiding this comment.
Not a blocker but if possible can you use the exec_cmd here since it will also provide some additional logging and automatically kill the command if it takes too long e.g
| fn execute_rustup_default() -> Option<String> { | |
| let Output { stdout, .. } = create_command("rustup") | |
| .ok()? | |
| .args(&["default"]) | |
| .output() | |
| .ok()?; | |
| let stdout = String::from_utf8(stdout).ok()?; | |
| stdout.split_whitespace().next().map(str::to_owned) | |
| } | |
| fn execute_rustup_default(context: &Context) -> Option<String> { | |
| context | |
| .exec_cmd("rustup", &["default"])? | |
| .stdout | |
| .split_whitespace() | |
| .next() | |
| .map(str::to_owned) | |
| } |
src/modules/rust.rs
Outdated
| @@ -76,6 +77,7 @@ fn get_module_version(context: &Context, config: &RustConfig) -> Option<String> | |||
| if let Some(toolchain) = env_rustup_toolchain(context) | |||
| .or_else(|| execute_rustup_override_list(&context.current_dir)) | |||
| .or_else(|| find_rust_toolchain_file(context)) | |||
| .or_else(execute_rustup_default) | |||
There was a problem hiding this comment.
Related to the below suggestion
| .or_else(execute_rustup_default) | |
| .or_else(|| execute_rustup_default(context)) |
missing default makes Also, |
syphar
force-pushed
the
use-rustup-default
branch
from
e5b8b16
to
6b4d1a1
Compare
src/modules/rust.rs
Outdated
| extract_toolchain_from_rustup_override_list(&stdout, cwd) | ||
| fn execute_rustup_override_list(context: &Context) -> Option<String> { | ||
| extract_toolchain_from_rustup_override_list( | ||
| &context.exec_cmd("rustup", &["default"])?.stdout, |
There was a problem hiding this comment.
Should this not be
| &context.exec_cmd("rustup", &["default"])?.stdout, | |
| &context.exec_cmd("rustup", &["override", "list"])?.stdout, |
There was a problem hiding this comment.
thanks for the reminder, copy/paste errors still happen, no matter the experience :)
There was a problem hiding this comment.
fixed it, in the same commit.
Or should I squash?
syphar
force-pushed
the
use-rustup-default
branch
from
6b4d1a1
to
392a524
Compare
Description
After checking directory overrides we were directly falling back to the
relatively slow call to
rustc --version.Inserting a call to
rustup defaultleads to a quicker response when usingrustup.The only way to make this even faster would be to try to read
.rustup/settings.toml, though this is not part of the officialrustupAPI. Also we might safe the second call to
rustupwhen we find out it doesn't exist in the first call.I'm not sure about the classification (
perf/fix), I'm happy to change it ifneeded. Same goes for any other implementation parts, or when I might have
missed something that made
rustup defaultgive us the wrong result? I checked#426 and didn't find any reasoning
why we cannot use
rustup default.From looking at the current rust-module tests and how command-output mocking is built inside
utils::exec_cmdI don't see how we can write a clean test of the adaptions (or the untested rest of the rust module). I would propose to leave this as a later task with a more flexible command-output mocking.Motivation and Context
This should help with (or fix?) #2004.
Fixes #2004
I found myself generally calling
rustup override stablefor any new rust repo because it was faster.screenshots
before:
after:
How Has This Been Tested?
I did a manual test with mac OS and rustup, and tested if
rustup defaultleads to a toolchain install in a fresh debian docker container (it doesn't).Checklist