Skip to content
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

Update on install #2339

Merged
merged 3 commits into from
Jun 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions src/cli/self_update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ pub struct InstallOpts<'a> {
pub default_toolchain: Option<String>,
pub profile: String,
pub no_modify_path: bool,
pub no_update_toolchain: bool,
pub components: &'a [&'a str],
pub targets: &'a [&'a str],
}
Expand Down Expand Up @@ -328,6 +329,7 @@ pub fn install(
opts.default_toolchain.as_deref(),
&opts.profile,
opts.default_host_triple.as_deref(),
!opts.no_update_toolchain,
opts.components,
opts.targets,
verbose,
Expand Down Expand Up @@ -712,6 +714,7 @@ fn maybe_install_rust(
toolchain: Option<&str>,
profile_str: &str,
default_host_triple: Option<&str>,
update_existing_toolchain: bool,
components: &[&str],
targets: &[&str],
verbose: bool,
Expand All @@ -728,8 +731,10 @@ fn maybe_install_rust(
info!("default host triple is {}", cfg.get_default_host_triple()?);
}

let user_specified_something =
toolchain.is_some() || !targets.is_empty() || !components.is_empty();
let user_specified_something = toolchain.is_some()
|| !targets.is_empty()
|| !components.is_empty()
|| update_existing_toolchain;

// If the user specified they want no toolchain, we skip this, otherwise
// if they specify something directly, or we have no default, then we install
Expand All @@ -754,16 +759,21 @@ fn maybe_install_rust(
}
writeln!(process().stdout())?;
} else if user_specified_something || cfg.find_default()?.is_none() {
let toolchain_str = toolchain.unwrap_or("stable");
let toolchain = cfg.get_toolchain(toolchain_str, false)?;
let (toolchain_str, toolchain) = match toolchain {
Some(s) => (s.to_owned(), cfg.get_toolchain(s, false)?),
None => match cfg.find_default()? {
Some(t) => (t.name().to_owned(), t),
None => ("stable".to_owned(), cfg.get_toolchain("stable", false)?),
},
};
if toolchain.exists() {
warn!("Updating existing toolchain, profile choice will be ignored");
}
let distributable = DistributableToolchain::new(&toolchain)?;
let status = distributable.install_from_dist(true, false, components, targets)?;
cfg.set_default(toolchain_str)?;
cfg.set_default(&toolchain_str)?;
writeln!(process().stdout())?;
common::show_channel_update(&cfg, toolchain_str, Ok(status))?;
common::show_channel_update(&cfg, &toolchain_str, Ok(status))?;
} else {
info!("updating existing rustup installation - leaving toolchains alone");
writeln!(process().stdout())?;
Expand Down
7 changes: 7 additions & 0 deletions src/cli/setup_mode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ pub fn main() -> Result<utils::ExitCode> {
.multiple(true)
.use_delimiter(true),
)
.arg(
Arg::with_name("no-update-default-toolchain")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this is a bit verbose. We have --no-self-update; perhaps just --no-update ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have a ponder. --no-update feels a little odd but the terseness may be okay. I'll reconsider in the face of the rest of the args, it is a bit long yes.

.long("no-update-default-toolchain")
.help("Don't update any existing default toolchain after install"),
)
.arg(
Arg::with_name("no-modify-path")
.long("no-modify-path")
Expand All @@ -106,6 +111,7 @@ pub fn main() -> Result<utils::ExitCode> {
.value_of("profile")
.expect("Unreachable: Clap should supply a default");
let no_modify_path = matches.is_present("no-modify-path");
let no_update_toolchain = matches.is_present("no-update-default-toolchain");

let components: Vec<_> = matches
.values_of("components")
Expand All @@ -122,6 +128,7 @@ pub fn main() -> Result<utils::ExitCode> {
default_toolchain,
profile: profile.to_owned(),
no_modify_path,
no_update_toolchain,
components: &components,
targets: &targets,
};
Expand Down
24 changes: 23 additions & 1 deletion tests/cli-inst-interactive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ use std::io::Write;
use std::process::Stdio;
use std::sync::Mutex;

macro_rules! for_host {
Copy link
Contributor

@rbtcollins rbtcollins Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really feel that a used-once macro that just forms a function once, used in a single test is really pulling its weight - but if you feel it is going to be used again I'm cool with it being added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm intending, once this is merged, to file an e-easy e-mentor help-wanted to move those macros into mock/clitools.rs

($s: expr) => {
&format!($s, this_host_triple())
};
}

pub fn setup_(complex: bool, f: &dyn Fn(&Config)) {
let scenario = if complex {
Scenario::UnavailableRls
Expand Down Expand Up @@ -124,7 +130,11 @@ Rust is installed now. Great!
fn blank_lines_around_stderr_log_output_update() {
setup(&|config| {
run_input(config, &["rustup-init"], "\n\n");
let out = run_input(config, &["rustup-init"], "\n\n");
let out = run_input(
config,
&["rustup-init", "--no-update-default-toolchain"],
"\n\n",
);
println!("-- stdout --\n {}", out.stdout);
println!("-- stderr --\n {}", out.stderr);

Expand Down Expand Up @@ -407,3 +417,15 @@ fn test_succeed_if_rustup_sh_already_installed_env_var_set() {
assert!(!out.stdout.contains("Continue? (y/N)"));
})
}

#[test]
fn installing_when_already_installed_updates_toolchain() {
setup(&|config| {
run_input(config, &["rustup-init"], "\n\n");
let out = run_input(config, &["rustup-init"], "\n\n");
println!("stdout:\n{}\n...\n", out.stdout);
assert!(out
.stdout
.contains(for_host!("stable-{} unchanged - 1.1.0 (hash-stable-1.1.0)")));
})
}
2 changes: 1 addition & 1 deletion tests/cli-self-upd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -901,7 +901,7 @@ fn reinstall_exact() {
expect_ok(config, &["rustup-init", "-y"]);
expect_stderr_ok(
config,
&["rustup-init", "-y"],
&["rustup-init", "-y", "--no-update-default-toolchain"],
r"info: updating existing rustup installation - leaving toolchains alone",
);
});
Expand Down