From 850adefdd7f2aba2c0d49e7407a12f7a686e3c17 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 26 Jun 2020 17:01:00 -0700 Subject: [PATCH] Move to source env strategy Prior to this commit, Rustup used a strategy of modifying PATH directly in a shell's run-commands (rc) files. This moves all shell support in Rustup over to a new strategy for changing PATH. A set of "env" scripts (only one right now) are sourced from shell rcs to add Rustup's PATH idempotently (only once). PATH should no longer be modified except through those scripts! --- src/cli/self_update.rs | 12 ++- src/cli/self_update/shell.rs | 155 +++++++++++++++++++++++++++++------ src/cli/self_update/unix.rs | 107 ++++++++---------------- tests/cli-self-upd.rs | 53 ++++++++++-- 4 files changed, 216 insertions(+), 111 deletions(-) diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index 9159dc9bdd..c1ec854a01 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -322,7 +322,7 @@ pub fn install( let install_res: Result = (|| { install_bins()?; if !opts.no_modify_path { - do_add_to_path(&get_add_path_methods())?; + do_add_to_path()?; } utils::create_rustup_home()?; maybe_install_rust( @@ -336,9 +336,6 @@ pub fn install( quiet, )?; - #[cfg(unix)] - write_env()?; - Ok(utils::ExitCode(0)) })(); @@ -515,9 +512,10 @@ fn pre_install_msg(no_modify_path: bool) -> Result { if !no_modify_path { if cfg!(unix) { - let shells = shell::get_available_shells(); - let rcfiles = shells - .filter_map(|sh| sh.rcfile().map(|rc| format!(" {}", rc.display()))) + // Brittle code warning: some duplication in unix::do_add_to_path + let rcfiles = shell::get_available_shells() + .flat_map(|sh| sh.update_rcs().into_iter()) + .map(|rc| format!(" {}", rc.display())) .collect::>(); let plural = if rcfiles.len() > 1 { "s" } else { "" }; let rcfiles = rcfiles.join("\n"); diff --git a/src/cli/self_update/shell.rs b/src/cli/self_update/shell.rs index dc4312a35d..940f614581 100644 --- a/src/cli/self_update/shell.rs +++ b/src/cli/self_update/shell.rs @@ -4,16 +4,51 @@ //! so handling them is relatively consistent. But only relatively. //! POSIX postdates Unix by 20 years, and each "Unix-like" shell develops //! unique quirks over time. +//! +//! +//! Windowing Managers, Desktop Environments, GUI Terminals, and PATHs +//! +//! Duplicating paths in PATH can cause performance issues when the OS searches +//! the same place multiple times. Traditionally, Unix configurations have +//! resolved this by setting up PATHs in the shell's login profile. +//! +//! This has its own issues. Login profiles are only intended to run once, but +//! changing the PATH is common enough that people may run it twice. Desktop +//! environments often choose to NOT start login shells in GUI terminals. Thus, +//! a trend has emerged to place PATH updates in other run-commands (rc) files, +//! leaving Rustup with few assumptions to build on for fulfilling its promise +//! to set up PATH appropriately. +//! +//! Rustup addresses this by: +//! 1) using a shell script that updates PATH if the path is not in PATH +//! 2) sourcing this script in any known and appropriate rc file -// TODO: Nushell, PowerShell -// Cross-platform non-POSIX shells were not assessed for integration yet. - +use super::canonical_cargo_home; use super::*; use crate::process; use std::path::PathBuf; pub type Shell = Box; +#[derive(Debug, PartialEq)] +pub struct ShellScript { + content: &'static str, + name: &'static str, +} + +impl ShellScript { + pub fn write(&self) -> Result<()> { + let cargo_bin = format!("{}/bin", canonical_cargo_home()?); + let env_name = utils::cargo_home()?.join(self.name); + let env_file = self.content.replace("{cargo_bin}", &cargo_bin); + utils::write_file(self.name, &env_name, &env_file)?; + Ok(()) + } +} + +#[allow(dead_code)] // For some reason. +const POSIX_ENV: &str = include_str!("env"); + macro_rules! support_shells { ( $($shell:ident,)* ) => { fn enumerate_shells() -> Vec { @@ -22,6 +57,9 @@ macro_rules! support_shells { } } +// TODO: Tcsh (BSD) +// TODO?: Make a decision on Ion Shell, Power Shell, Nushell +// Cross-platform non-POSIX shells have not been assessed for integration yet support_shells! { Posix, Bash, @@ -33,17 +71,27 @@ pub fn get_available_shells() -> impl Iterator { } pub trait UnixShell { + // Detects if a shell "exists". Users have multiple shells, so an "eager" + // heuristic should be used, assuming shells exist if any traces do. fn does_exist(&self) -> bool; - fn rcfile(&self) -> Option; + // Gives all rcfiles of a given shell that rustup is concerned with. + // Used primarily in checking rcfiles for cleanup. + fn rcfiles(&self) -> Vec; + + // Gives rcs that should be written to. + fn update_rcs(&self) -> Vec; + + // Writes the relevant env file. + fn env_script(&self) -> ShellScript { + ShellScript { + name: "env", + content: POSIX_ENV, + } + } - fn export_string(&self) -> Result { - // The path is *prepended* in case there are system-installed - // rustc's that need to be overridden. - Ok(format!( - r#"export PATH="{}/bin:$PATH""#, - canonical_cargo_home()? - )) + fn source_string(&self) -> Result { + Ok(format!(r#"source "{}/env""#, canonical_cargo_home()?)) } } @@ -53,42 +101,99 @@ impl UnixShell for Posix { true } - fn rcfile(&self) -> Option { - utils::home_dir().map(|dir| dir.join(".profile")) + fn rcfiles(&self) -> Vec { + match utils::home_dir() { + Some(dir) => vec![dir.join(".profile")], + _ => vec![], + } + } + + fn update_rcs(&self) -> Vec { + // Write to .profile even if it doesn't exist. + self.rcfiles() } } struct Bash; + +// Bash is the source of many complications because it loads ONLY 1 rc, +// either a login profile or .bashrc, not both. +impl Bash { + // Bash will load only one of these, the first one that exists, when + // a login shell starts. BUT not all shells start inside a login shell! + fn profiles() -> Vec { + [".bash_profile", ".bash_login", ".profile"] + .iter() + .filter_map(|rc| utils::home_dir().map(|dir| dir.join(rc))) + .collect() + } + + // Bash will load only .bashrc on the start of most GUI terminals. + fn rc() -> Option { + utils::home_dir().map(|dir| dir.join(".bashrc")) + } +} impl UnixShell for Bash { fn does_exist(&self) -> bool { - self.rcfile().map_or(false, |rc| rc.is_file()) + matches!(process().var("SHELL"), Ok(sh) if sh.contains("bash")) + || self.rcfiles().iter().any(|rc| rc.is_file()) || matches!(utils::find_cmd(&["bash"]), Some(_)) } - fn rcfile(&self) -> Option { - // .bashrc is normative, in spite of a few weird Mac versions. - utils::home_dir().map(|dir| dir.join(".bashrc")) + fn rcfiles(&self) -> Vec { + // .bashrc first so it gets skipped in update_rcs + let mut profiles = Bash::profiles(); + if let Some(rc) = Bash::rc() { + profiles.push(rc); + } + profiles + } + + fn update_rcs(&self) -> Vec { + Bash::profiles() + .into_iter() + .filter(|rc| rc.is_file()) + // bash only reads one "login profile" so pick the one that exists + .take(1) + // Always pick .bashrc as well for GUI terminals + .chain(Bash::rc().filter(|rc| rc.is_file())) + .collect() } } struct Zsh; impl UnixShell for Zsh { fn does_exist(&self) -> bool { - self.rcfile().map_or(false, |rc| rc.is_file()) + matches!(process().var("SHELL"), Ok(sh) if sh.contains("zsh")) + || matches!(process().var("ZDOTDIR"), Ok(dir) if dir.len() > 0) + || self.rcfiles().iter().any(|rc| rc.is_file()) || matches!(utils::find_cmd(&["zsh"]), Some(_)) } - fn rcfile(&self) -> Option { + fn rcfiles(&self) -> Vec { + // FIXME: if zsh exists but is not in the process tree of the shell + // on install, $ZDOTDIR may not be loaded and give the wrong result. let zdotdir = match process().var("ZDOTDIR") { Ok(dir) => Some(PathBuf::from(dir)), _ => utils::home_dir(), }; - // .zshenv is preferred for path mods but not all zshers use it, - // zsh always loads .zshrc on interactive, unlike bash's weirdness. - zdotdir.map(|dir| match dir.join(".zshenv") { - rc if rc.is_file() => rc, - _ => dir.join(".zshrc"), - }) + // Don't bother with .zprofile/.zlogin because .zshrc will always be + // modified and always be sourced. + [".zshenv", ".zshrc"] + .iter() + .filter_map(|rc| zdotdir.as_ref().map(|dir| dir.join(rc))) + .collect() + } + + fn update_rcs(&self) -> Vec { + // .zshenv is preferred for path mods but not all zshers prefer it, + // zsh always loads .zshrc on interactive, unlike bash's xor-logic. + // So picking one will always work for this. + self.rcfiles() + .into_iter() + .filter(|rc| rc.is_file()) + .take(1) + .collect() } } diff --git a/src/cli/self_update/unix.rs b/src/cli/self_update/unix.rs index 0557a12e8a..43063a5bd0 100644 --- a/src/cli/self_update/unix.rs +++ b/src/cli/self_update/unix.rs @@ -2,15 +2,12 @@ use std::path::{Path, PathBuf}; use std::process::Command; use super::super::errors::*; -use super::path_update::PathUpdateMethod; -use super::{canonical_cargo_home, install_bins}; +use super::install_bins; +use super::shell; use crate::process; use crate::utils::utils; use crate::utils::Notification; -use super::shell; -use super::shell::Shell; - // If the user is trying to install with sudo, on some systems this will // result in writing root-owned files to the user's home directory, because // sudo is configured not to change $HOME. Don't let that bogosity happen. @@ -61,61 +58,49 @@ pub fn complete_windows_uninstall() -> Result { } pub fn do_remove_from_path() -> Result<()> { - for rcpath in get_remove_path_methods().filter_map(|sh| sh.rcfile()) { - let file = utils::read_file("rcfile", &rcpath)?; - let addition = format!("\n{}\n", shell_export_string()?); - - let file_bytes = file.into_bytes(); - let addition_bytes = addition.into_bytes(); - - let idx = file_bytes - .windows(addition_bytes.len()) - .position(|w| w == &*addition_bytes); - if let Some(i) = idx { - let mut new_file_bytes = file_bytes[..i].to_vec(); - new_file_bytes.extend(&file_bytes[i + addition_bytes.len()..]); - let new_file = String::from_utf8(new_file_bytes).unwrap(); - utils::write_file("rcfile", &rcpath, &new_file)?; - } else { - // Weird case. rcfile no longer needs to be modified? + for sh in shell::get_available_shells() { + let source_cmd = format!("\n{}\n", sh.source_string()?); + let source_bytes = source_cmd.into_bytes(); + + // Check more files for cleanup than normally are updated. + for rc in sh.rcfiles().iter().filter(|rc| rc.is_file()) { + let file = utils::read_file("rcfile", &rc)?; + let file_bytes = file.into_bytes(); + if let Some(idx) = file_bytes + .windows(source_bytes.len()) + .position(|w| w == source_bytes.as_slice()) + { + // Here we rewrite the file without the offending line. + let mut new_bytes = file_bytes[..idx].to_vec(); + new_bytes.extend(&file_bytes[idx + source_bytes.len()..]); + let new_file = String::from_utf8(new_bytes).unwrap(); + utils::write_file("rcfile", &rc, &new_file)?; + } } } Ok(()) } -pub fn write_env() -> Result<()> { - let env_file = utils::cargo_home()?.join("env"); - let env_str = format!("{}\n", shell_export_string()?); - utils::write_file("env", &env_file, &env_str)?; - Ok(()) -} +pub fn do_add_to_path() -> Result<()> { + let mut scripts = vec![]; -pub fn shell_export_string() -> Result { - let path = format!("{}/bin", canonical_cargo_home()?); - // The path is *prepended* in case there are system-installed - // rustc's that need to be overridden. - Ok(format!(r#"export PATH="{}:$PATH""#, path)) -} - -pub fn do_add_to_path(methods: &[PathUpdateMethod]) -> Result<()> { - for method in methods { - if let PathUpdateMethod::RcFile(ref rcpath) = *method { - let file = if rcpath.exists() { - utils::read_file("rcfile", rcpath)? - } else { - String::new() - }; - let addition = format!("\n{}", shell_export_string()?); - if !file.contains(&addition) { - utils::append_file("rcfile", rcpath, &addition).chain_err(|| { + for sh in shell::get_available_shells() { + let source_cmd = format!("\n{}", sh.source_string()?); + for rc in sh.update_rcs() { + if !rc.is_file() || !utils::read_file("rcfile", &rc)?.contains(&source_cmd) { + utils::append_file("rcfile", &rc, &source_cmd).chain_err(|| { ErrorKind::WritingShellProfile { - path: rcpath.to_path_buf(), + path: rc.to_path_buf(), } })?; + let script = sh.env_script(); + // Only write scripts once. + if !scripts.contains(&script) { + script.write()?; + scripts.push(script); + } } - } else { - unreachable!() } } @@ -152,27 +137,3 @@ pub fn self_replace() -> Result { Ok(utils::ExitCode(0)) } - -/// Decide which rcfiles we're going to update, so we -/// can tell the user before they confirm. -pub fn get_add_path_methods() -> Vec { - shell::get_available_shells() - .filter_map(|sh| sh.rcfile()) - .map(PathUpdateMethod::RcFile) - .collect() -} - -/// Decide which rcfiles we're going to update, so we -/// can tell the user before they confirm. -fn get_remove_path_methods() -> impl Iterator { - shell::get_available_shells().filter(|sh| { - if let (Some(rc), Ok(export)) = (sh.rcfile(), sh.export_string()) { - rc.is_file() - && utils::read_file("rcfile", &rc) - .unwrap_or_default() - .contains(&export) - } else { - false - } - }) -} diff --git a/tests/cli-self-upd.rs b/tests/cli-self-upd.rs index 5398158b85..d4bea51f62 100644 --- a/tests/cli-self-upd.rs +++ b/tests/cli-self-upd.rs @@ -311,7 +311,7 @@ fn install_adds_path_to_rc(rcfile: &str) { expect_ok(config, &["rustup-init", "-y"]); let new_rc = fs::read_to_string(&rc).unwrap(); - let addition = format!(r#"export PATH="{}/bin:$PATH""#, config.cargodir.display()); + let addition = format!("source \"{}/env\"", config.cargodir.display()); let expected = format!("{}\n{}\n", my_rc, addition); assert_eq!(new_rc, expected); }); @@ -329,6 +329,24 @@ fn install_adds_path_to_bashrc() { install_adds_path_to_rc(".bashrc"); } +#[test] +#[cfg(unix)] +fn install_adds_path_to_bash_profile() { + install_adds_path_to_rc(".bash_profile"); +} + +#[test] +#[cfg(unix)] +fn install_adds_path_to_zshenv() { + install_adds_path_to_rc(".zshenv"); +} + +#[test] +#[cfg(unix)] +fn install_adds_path_to_zshrc() { + install_adds_path_to_rc(".zshrc"); +} + #[test] #[cfg(unix)] fn install_does_not_add_path_to_bash_profile_that_doesnt_exist() { @@ -367,7 +385,7 @@ fn install_with_zsh_adds_path_to_zshrc() { assert!(cmd.output().unwrap().status.success()); let new_rc = fs::read_to_string(&rc).unwrap(); - let addition = format!(r#"export PATH="{}/bin:$PATH""#, config.cargodir.display()); + let addition = format!(r#"source "{}/env""#, config.cargodir.display()); let expected = format!("{}\n{}\n", my_rc, addition); assert_eq!(new_rc, expected); }); @@ -391,7 +409,7 @@ fn install_with_zsh_adds_path_to_zdotdir_zshrc() { assert!(cmd.output().unwrap().status.success()); let new_rc = fs::read_to_string(&rc).unwrap(); - let addition = format!(r#"export PATH="{}/bin:$PATH""#, config.cargodir.display()); + let addition = format!(r#"source "{}/env""#, config.cargodir.display()); let expected = format!("{}\n{}\n", my_rc, addition); assert_eq!(new_rc, expected); }); @@ -408,7 +426,7 @@ fn install_adds_path_to_rcfile_just_once() { expect_ok(config, &["rustup-init", "-y"]); let new_profile = fs::read_to_string(&profile).unwrap(); - let addition = format!(r#"export PATH="{}/bin:$PATH""#, config.cargodir.display()); + let addition = format!(r#"source "{}/env""#, config.cargodir.display()); let expected = format!("{}\n{}\n", my_profile, addition); assert_eq!(new_profile, expected); }); @@ -440,6 +458,24 @@ fn uninstall_removes_path_from_bashrc() { uninstall_removes_path_from_rc(".bashrc"); } +#[test] +#[cfg(unix)] +fn uninstall_removes_path_from_bash_profile() { + uninstall_removes_path_from_rc(".bash_profile"); +} + +#[test] +#[cfg(unix)] +fn uninstall_removes_path_from_zshenv() { + uninstall_removes_path_from_rc(".zshenv"); +} + +#[test] +#[cfg(unix)] +fn uninstall_removes_path_from_zshrc() { + uninstall_removes_path_from_rc(".zshrc"); +} + #[test] #[cfg(unix)] fn uninstall_doesnt_touch_rc_files_that_dont_contain_cargo_home() { @@ -475,7 +511,7 @@ fn when_cargo_home_is_the_default_write_path_specially() { assert!(cmd.output().unwrap().status.success()); let new_profile = fs::read_to_string(&profile).unwrap(); - let expected = format!("{}\nexport PATH=\"$HOME/.cargo/bin:$PATH\"\n", my_profile); + let expected = format!("{}\nsource \"$HOME/.cargo/env\"\n", my_profile); assert_eq!(new_profile, expected); let mut cmd = clitools::cmd(config, "rustup", &["self", "uninstall", "-y"]); @@ -912,7 +948,12 @@ fn produces_env_file_on_unix() { assert!(cmd.output().unwrap().status.success()); let envfile = config.homedir.join(".cargo/env"); let envfile = fs::read_to_string(&envfile).unwrap(); - assert!(envfile.contains(r#"export PATH="$HOME/.cargo/bin:$PATH""#)); + let path_string = "export PATH=\"$HOME/.cargo/bin:${PATH}\""; + let (_, envfile_export) = envfile.split_at(match envfile.find("export PATH") { + Some(idx) => idx, + None => 0, + }); + assert_eq!(&envfile_export[..path_string.len()], path_string); }); }