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

Allow installing alongside existing Rust (with warning) #2214

Merged
merged 8 commits into from
Feb 27, 2020
14 changes: 14 additions & 0 deletions src/cli/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -522,3 +522,17 @@ pub fn report_error(e: &Error) {
}
}
}

pub fn ignorable_error(error: crate::errors::Error, no_prompt: bool) -> Result<()> {
report_error(&error);
if no_prompt {
sollyucko marked this conversation as resolved.
Show resolved Hide resolved
warn!("continuing (because the -y flag is set and the error is ignorable)");
Ok(())
} else {
if confirm("\nContinue? (y/N)", false).unwrap_or(false) {
Ok(())
} else {
Err(error)
}
}
}
44 changes: 27 additions & 17 deletions src/cli/self_update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
//! Deleting the running binary during uninstall is tricky
//! and racy on Windows.

use crate::common::{self, Confirm};
use crate::common::{self, ignorable_error, Confirm};
use crate::errors::*;
use crate::markdown::md;
use crate::term2;
Expand Down Expand Up @@ -235,9 +235,16 @@ fn canonical_cargo_home() -> Result<String> {
/// `CARGO_HOME`/bin, hard-linking the various Rust tools to it,
/// and adding `CARGO_HOME`/bin to PATH.
pub fn install(no_prompt: bool, verbose: bool, quiet: bool, mut opts: InstallOpts) -> Result<()> {
do_pre_install_sanity_checks()?;
if !env::var_os("RUSTUP_INIT_SKIP_EXISTENCE_CHECKS").map_or(false, |s| s == "yes") {
do_pre_install_sanity_checks(no_prompt)?;
}

do_pre_install_options_sanity_checks(&opts)?;
check_existence_of_rustc_or_cargo_in_path(no_prompt)?;

if !env::var_os("RUSTUP_INIT_SKIP_EXISTENCE_CHECKS").map_or(false, |s| s == "yes") {
check_existence_of_rustc_or_cargo_in_path(no_prompt)?;
}

#[cfg(unix)]
do_anti_sudo_check(no_prompt)?;

Expand Down Expand Up @@ -378,25 +385,25 @@ fn check_existence_of_rustc_or_cargo_in_path(no_prompt: bool) -> Result<()> {
// Only the test runner should set this
let skip_check = env::var_os("RUSTUP_INIT_SKIP_PATH_CHECK");

// Ignore this check if called with no prompt (-y) or if the environment variable is set
if no_prompt || skip_check == Some("yes".into()) {
// Skip this if the environment variable is set
if skip_check == Some("yes".into()) {
kinnison marked this conversation as resolved.
Show resolved Hide resolved
return Ok(());
}

if let Err(path) = rustc_or_cargo_exists_in_path() {
err!("it looks like you have an existing installation of Rust at:");
err!("{}", path);
err!("rustup should not be installed alongside Rust. Please uninstall your existing Rust first.");
err!("Otherwise you may have confusion unless you are careful with your PATH");
err!("If you are sure that you want both rustup and your already installed Rust");
err!("then please restart the installation and pass `-y' to bypass this check.");
Err("cannot install while Rust is installed".into())
} else {
Ok(())
warn!("it looks like you have an existing installation of Rust at:");
warn!("{}", path);
warn!("rustup should not be installed alongside Rust. Please uninstall your existing Rust first.");
warn!("Otherwise you may have confusion unless you are careful with your PATH");
warn!("If you are sure that you want both rustup and your already installed Rust");
warn!("then please reply `y' or `yes' or set RUSTUP_INIT_SKIP_PATH_CHECK to yes");
warn!("or pass `-y' to ignore all ignorable checks.");
ignorable_error("cannot install while Rust is installed".into(), no_prompt)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Errata: this is not ignorable if the location of the existing rust is in the CARGO_BIN directory that rustup's proxies will be written to. We're making this easier to run into, so perhaps we should check for that at this point.

Copy link
Contributor Author

@sollyucko sollyucko Feb 5, 2020

Choose a reason for hiding this comment

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

@rbtcollins Should it be possible to skip it in any way (e.g. environment variable) or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently the code will give a warning later about "tool X is already installed", and not touch those binaries. This would mean that rustup isn't actually installed. I don't think it makes sense to allow that to be bypassed, but would like @kinnison 's input. This might be something to do as a separate PR for instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

The cases this PR covers are to do with rustc on the PATH or about detecting an old-style rustup.sh install as I understand the change. If it happens that it's in CARGO_BIN then the user is going to get warnings as you say, and if they ignore those then they get to keep all the pieces. I don't think we can stop all the footguns -- this is simply a way to allow another footgun for someone who really can't install rustup otherwise due to immutable underlying layers.

Copy link
Contributor

Choose a reason for hiding this comment

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

The concern I have is that this is escalating it from a read-the-docs situation to a y/n prompt, so perhaps a lot easier to perform targeted foot removal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly so. I shall check the feel of that in my testing which I'm about to undertake.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay so the issue here is that if the rustc/cargo is in $CARGO_BIN then the only real pain is that rustup will replace them with proxies without prompting. The only tools you get the warnings about are rustfmt and cargo-fmt because those used to be provided via cargo install rather than always being proxied via rustup. I think that the footgun is permissible in this case because it's exceedingly unlikely that the user will have set up this kind of thing without also being aware of what rustup-init will end up doing

}
Ok(())
}

fn do_pre_install_sanity_checks() -> Result<()> {
fn do_pre_install_sanity_checks(no_prompt: bool) -> Result<()> {
let rustc_manifest_path = PathBuf::from("/usr/local/lib/rustlib/manifest-rustc");
let uninstaller_path = PathBuf::from("/usr/local/lib/rustlib/uninstall.sh");
let rustup_sh_path = utils::home_dir().unwrap().join(".rustup");
Expand All @@ -412,7 +419,7 @@ fn do_pre_install_sanity_checks() -> Result<()> {
"run `{}` as root to uninstall Rust",
uninstaller_path.display()
);
return Err("cannot install while Rust is installed".into());
ignorable_error("cannot install while Rust is installed".into(), no_prompt)?;
}

if rustup_sh_exists {
Expand All @@ -422,7 +429,10 @@ fn do_pre_install_sanity_checks() -> Result<()> {
warn!("or, if you already have rustup installed, you can run");
warn!("`rustup self update` and `rustup toolchain list` to upgrade");
warn!("your directory structure");
return Err("cannot install while rustup.sh is installed".into());
ignorable_error(
"cannot install while rustup.sh is installed".into(),
no_prompt,
)?;
}

Ok(())
Expand Down
116 changes: 116 additions & 0 deletions tests/cli-inst-interactive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ use crate::mock::clitools::{
};
use crate::mock::{get_path, restore_path};
use lazy_static::lazy_static;
use rustup::utils::raw;
use std::fs;
use std::io::Write;
use std::process::Stdio;
use std::sync::Mutex;
Expand Down Expand Up @@ -39,9 +41,22 @@ pub fn setup(f: &dyn Fn(&Config)) {
}

fn run_input(config: &Config, args: &[&str], input: &str) -> SanitizedOutput {
run_input_with_env(config, args, input, &[])
}

fn run_input_with_env(
config: &Config,
args: &[&str],
input: &str,
env: &[(&str, &str)],
) -> SanitizedOutput {
let mut cmd = clitools::cmd(config, args[0], &args[1..]);
clitools::env(config, &mut cmd);

for (key, value) in env.iter() {
cmd.env(key, value);
}

cmd.stdin(Stdio::piped());
cmd.stdout(Stdio::piped());
cmd.stderr(Stdio::piped());
Expand Down Expand Up @@ -266,3 +281,104 @@ fn test_warn_if_complete_profile_is_used() {
);
});
}

fn create_rustup_sh_metadata(config: &Config) {
let rustup_dir = config.homedir.join(".rustup");
fs::create_dir_all(&rustup_dir).unwrap();
let version_file = rustup_dir.join("rustup-version");
raw::write_file(&version_file, "").unwrap();
}

#[test]
fn test_prompt_fail_if_rustup_sh_already_installed_reply_nothing() {
setup(&|config| {
create_rustup_sh_metadata(&config);
let out = run_input(config, &["rustup-init"], "\n");
assert!(!out.ok);
assert!(out
.stderr
.contains("warning: it looks like you have existing rustup.sh metadata"));
assert!(out
.stderr
.contains("error: cannot install while rustup.sh is installed"));
assert!(out.stdout.contains("Continue? (y/N)"));
})
}

#[test]
fn test_prompt_fail_if_rustup_sh_already_installed_reply_no() {
setup(&|config| {
create_rustup_sh_metadata(&config);
let out = run_input(config, &["rustup-init"], "no\n");
assert!(!out.ok);
assert!(out
.stderr
.contains("warning: it looks like you have existing rustup.sh metadata"));
assert!(out
.stderr
.contains("error: cannot install while rustup.sh is installed"));
assert!(out.stdout.contains("Continue? (y/N)"));
})
}

#[test]
fn test_prompt_succeed_if_rustup_sh_already_installed_reply_yes() {
setup(&|config| {
create_rustup_sh_metadata(&config);
let out = run_input(config, &["rustup-init"], "yes\n\n\n");
assert!(out.ok);
assert!(out
.stderr
.contains("warning: it looks like you have existing rustup.sh metadata"));
assert!(out
.stderr
.contains("error: cannot install while rustup.sh is installed"));
assert!(out.stdout.contains("Continue? (y/N)"));
assert!(!out.stdout.contains(
"warning: continuing (because the -y flag is set and the error is ignorable)"
))
})
}

#[test]
fn test_warn_succeed_if_rustup_sh_already_installed_y_flag() {
setup(&|config| {
create_rustup_sh_metadata(&config);
let out = run_input(config, &["rustup-init", "-y"], "");
assert!(out.ok);
assert!(out
.stderr
.contains("warning: it looks like you have existing rustup.sh metadata"));
assert!(out
.stderr
.contains("error: cannot install while rustup.sh is installed"));
assert!(out.stderr.contains(
"warning: continuing (because the -y flag is set and the error is ignorable)"
));
assert!(!out.stdout.contains("Continue? (y/N)"));
})
}

#[test]
fn test_succeed_if_rustup_sh_already_installed_env_var_set() {
setup(&|config| {
create_rustup_sh_metadata(&config);
let out = run_input_with_env(
config,
&["rustup-init", "-y"],
"",
&[("RUSTUP_INIT_SKIP_EXISTENCE_CHECKS", "yes")],
);
assert!(out.ok);
assert!(!out
.stderr
.contains("warning: it looks like you have existing rustup.sh metadata"));
assert!(!out
.stderr
.contains("error: cannot install while rustup.sh is installed"));
assert!(!out.stderr.contains(
"warning: continuing (because the -y flag is set and the error is ignorable)"
));
assert!(!out.stdout.contains("Continue? (y/N)"));
})
}
4 changes: 2 additions & 2 deletions tests/cli-misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,7 @@ fn install_stops_if_rustc_exists() {
.contains("it looks like you have an existing installation of Rust at:"));
assert!(out
.stderr
.contains("restart the installation and pass `-y'"));
.contains("If you are sure that you want both rustup and your already installed Rust"));
});
}

Expand Down Expand Up @@ -652,7 +652,7 @@ fn install_stops_if_cargo_exists() {
.contains("it looks like you have an existing installation of Rust at:"));
assert!(out
.stderr
.contains("restart the installation and pass `-y'"));
.contains("If you are sure that you want both rustup and your already installed Rust"));
});
}

Expand Down
2 changes: 1 addition & 1 deletion tests/cli-self-upd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1166,7 +1166,7 @@ fn install_but_rustup_sh_is_installed() {
fs::create_dir_all(&rustup_dir).unwrap();
let version_file = rustup_dir.join("rustup-version");
raw::write_file(&version_file, "").unwrap();
expect_err(
expect_stderr_ok(
config,
&["rustup-init", "-y"],
"cannot install while rustup.sh is installed",
Expand Down