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

Implement shell PATH fixes via UnixShell trait #2387

Merged
merged 30 commits into from
Aug 8, 2020
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
4451be6
Adds shell.rs and UnixShell trait
workingjubilee Jun 24, 2020
bfacec9
Thinly integrate shell.rs into self_update
workingjubilee Jun 24, 2020
f7296b7
std::env::var() -> rustup::process().var()
workingjubilee Jun 26, 2020
86c97e4
Add idempotent PATH update shell script
workingjubilee Jun 27, 2020
c300af2
Fix do_remove_from_path in windows.rs
workingjubilee Jun 27, 2020
850adef
Move to source env strategy
workingjubilee Jun 27, 2020
13bd1f2
Completely remove PathUpdateMethod type
workingjubilee Jul 3, 2020
de4bf2f
Simplify zsh logic to always write to .zshenv
workingjubilee Jul 3, 2020
e138ce8
fixup under review
workingjubilee Jul 12, 2020
353ee5d
Correctly write UTF8 paths
workingjubilee Jul 13, 2020
bcbbfd5
Simplify Bash logic
workingjubilee Jul 13, 2020
7887b3a
Fix env_on_Unix test enough to build
workingjubilee Jul 13, 2020
60491a3
Fixes zsh $ZDOTDIR detection
workingjubilee Jul 13, 2020
1b61dec
Use cfg(attrs) instead of if-else for formatting messages
workingjubilee Jul 13, 2020
5625974
Move env to env.sh and do associated cleanup
workingjubilee Jul 13, 2020
7a0c72d
Add remove_legacy_paths function
workingjubilee Jul 13, 2020
3cc0ef3
Pull out path tests into new file
workingjubilee Jul 14, 2020
29483af
Adds testing for cleaning up legacy paths
workingjubilee Jul 14, 2020
d1746a0
Simplify Bash logic
workingjubilee Jul 15, 2020
0862366
Find and fix legacy path test bugs
workingjubilee Jul 15, 2020
db11263
Deduplicate path tests
workingjubilee Jul 15, 2020
e45a1ec
Improve whitespace handling somewhat
workingjubilee Jul 15, 2020
c10b410
Fix an incorrect comparison in impl Zsh
workingjubilee Jul 17, 2020
72a2d5e
Fixups: no functional changes
workingjubilee Jul 17, 2020
1ae7ae8
Remove/replace tests in cli-self-upd/paths
workingjubilee Jul 17, 2020
914007c
Remove extraneous unpacking
workingjubilee Jul 18, 2020
da957b2
Fix message to match script name
workingjubilee Jul 18, 2020
9488dac
Remove unpacking from one last test
workingjubilee Jul 18, 2020
a66e4e1
Read env.sh locally but write env
workingjubilee Jul 19, 2020
cc10149
Remove superfluous test
workingjubilee Jul 19, 2020
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
49 changes: 19 additions & 30 deletions src/cli/self_update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ use crate::utils::Notification;
use crate::{Cfg, UpdateStatus};
use crate::{DUP_TOOLS, TOOLS};

mod path_update;
use path_update::PathUpdateMethod;
#[cfg(unix)]
mod shell;
#[cfg(unix)]
mod unix;
#[cfg(windows)]
Expand Down Expand Up @@ -122,6 +122,7 @@ these changes will be reverted.
};
}

#[cfg(unix)]
macro_rules! pre_install_msg_unix {
() => {
pre_install_msg_template!(
Expand All @@ -133,6 +134,7 @@ modifying the profile file{plural} located at:
};
}

#[cfg(windows)]
macro_rules! pre_install_msg_win {
() => {
pre_install_msg_template!(
Expand Down Expand Up @@ -322,7 +324,7 @@ pub fn install(
let install_res: Result<utils::ExitCode> = (|| {
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(
Expand All @@ -336,9 +338,6 @@ pub fn install(
quiet,
)?;

#[cfg(unix)]
write_env()?;

Ok(utils::ExitCode(0))
})();

Expand Down Expand Up @@ -514,23 +513,14 @@ fn pre_install_msg(no_modify_path: bool) -> Result<String> {
let rustup_home = utils::rustup_home()?;

if !no_modify_path {
if cfg!(unix) {
let add_path_methods = get_add_path_methods();
let rcfiles = add_path_methods
.into_iter()
.filter_map(|m| {
if let PathUpdateMethod::RcFile(path) = m {
Some(format!("{}", path.display()))
} else {
None
}
})
// Brittle code warning: some duplication in unix::do_add_to_path
#[cfg(unix)]
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
{
let rcfiles = shell::get_available_shells()
.flat_map(|sh| sh.update_rcs().into_iter())
.map(|rc| format!(" {}", rc.display()))
.collect::<Vec<_>>();
let plural = if rcfiles.len() > 1 { "s" } else { "" };
let rcfiles = rcfiles
.into_iter()
.map(|f| format!(" {}", f))
.collect::<Vec<_>>();
let rcfiles = rcfiles.join("\n");
Ok(format!(
pre_install_msg_unix!(),
Expand All @@ -540,14 +530,14 @@ fn pre_install_msg(no_modify_path: bool) -> Result<String> {
rcfiles = rcfiles,
rustup_home = rustup_home.display(),
))
} else {
Ok(format!(
pre_install_msg_win!(),
cargo_home = cargo_home.display(),
cargo_home_bin = cargo_home_bin.display(),
rustup_home = rustup_home.display(),
))
}
#[cfg(windows)]
Ok(format!(
pre_install_msg_win!(),
cargo_home = cargo_home.display(),
cargo_home_bin = cargo_home_bin.display(),
rustup_home = rustup_home.display(),
))
} else {
Ok(format!(
pre_install_msg_no_modify_path!(),
Expand Down Expand Up @@ -846,8 +836,7 @@ pub fn uninstall(no_prompt: bool) -> Result<utils::ExitCode> {
info!("removing cargo home");

// Remove CARGO_HOME/bin from PATH
let remove_path_methods = get_remove_path_methods()?;
do_remove_from_path(&remove_path_methods)?;
do_remove_from_path()?;

// Delete everything in CARGO_HOME *except* the rustup bin

Expand Down
11 changes: 11 additions & 0 deletions src/cli/self_update/env.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#!/bin/sh
# rustup shell setup
# affix colons on either side of $PATH to simplify matching
case ":${PATH}:" in
*:"{cargo_bin}":*)
;;
*)
# Prepending path in case a system-installed rustc must be overwritten
export PATH="{cargo_bin}:$PATH"
;;
esac
8 changes: 0 additions & 8 deletions src/cli/self_update/path_update.rs

This file was deleted.

208 changes: 208 additions & 0 deletions src/cli/self_update/shell.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,208 @@
//! Paths and Unix shells
//!
//! MacOS, Linux, FreeBSD, and many other OS model their design on Unix,
//! 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

use super::*;
use crate::process;
use error_chain::bail;
use std::path::PathBuf;

pub type Shell = Box<dyn UnixShell>;

#[derive(Debug, PartialEq)]
pub struct ShellScript {
content: &'static str,
name: &'static str,
}

impl ShellScript {
pub fn write(&self) -> Result<()> {
let home = utils::cargo_home()?;
let cargo_bin = format!("{}/bin", cargo_home_str()?);
let env_name = home.join(self.name);
let env_file = self.content.replace("{cargo_bin}", &cargo_bin);
utils::write_file(self.name, &env_name, &env_file)?;
Ok(())
}
}

// TODO: Update into a bytestring.
rbtcollins marked this conversation as resolved.
Show resolved Hide resolved
pub fn cargo_home_str() -> Result<Cow<'static, str>> {
let path = utils::cargo_home()?;

let default_cargo_home = utils::home_dir()
.unwrap_or_else(|| PathBuf::from("."))
.join(".cargo");
Ok(if default_cargo_home == path {
"$HOME/.cargo".into()
} else {
match path.to_str() {
Some(p) => p.to_owned().into(),
None => bail!("Non-Unicode path!"),
}
})
}

// 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
fn enumerate_shells() -> Vec<Shell> {
vec![Box::new(Posix), Box::new(Bash), Box::new(Zsh)]
kinnison marked this conversation as resolved.
Show resolved Hide resolved
}

pub fn get_available_shells() -> impl Iterator<Item = Shell> {
enumerate_shells().into_iter().filter(|sh| sh.does_exist())
}

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;

// Gives all rcfiles of a given shell that rustup is concerned with.
// Used primarily in checking rcfiles for cleanup.
fn rcfiles(&self) -> Vec<PathBuf>;
Copy link
Contributor

Choose a reason for hiding this comment

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

See below - but I think rcfiles vs update_rcs is an unneeded distinction, because we can be pretty certain we won't be pure as far as updating bash's profile files is concerned, because of the intersection with the posix shell definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mm, the evolution of the zsh implementation suggests it is going to be a concern anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking through this, I cannot see rcfiles() being called on the trait; only from within trait implementations. All I'm suggesting is that it be removed from the trait unless-and-until something that is calling methods on the trait needs to call it.


// Gives rcs that should be written to.
fn update_rcs(&self) -> Vec<PathBuf>;

// Writes the relevant env file.
fn env_script(&self) -> ShellScript {
ShellScript {
name: "env.sh",
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
content: include_str!("env.sh"),
}
}

fn source_string(&self) -> Result<String> {
Ok(format!(r#"source "{}/env.sh""#, cargo_home_str()?))
}
}

struct Posix;
impl UnixShell for Posix {
fn does_exist(&self) -> bool {
true
}

fn rcfiles(&self) -> Vec<PathBuf> {
match utils::home_dir() {
Some(dir) => vec![dir.join(".profile")],
_ => vec![],
}
}

fn update_rcs(&self) -> Vec<PathBuf> {
// Write to .profile even if it doesn't exist.
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
self.rcfiles()
Copy link
Contributor

Choose a reason for hiding this comment

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

With the suggestion above, rcfiles here could be inlined into update_rcs for this impl.

Copy link
Member Author

Choose a reason for hiding this comment

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

unix.sh, line 65:

        for rc in sh.rcfiles().iter().filter(|rc| rc.is_file()) {

Copy link
Member Author

Choose a reason for hiding this comment

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

I know it's basically that one place. ^^;
But it is a method that has a reason for existing.

}
}

struct Bash;

impl UnixShell for Bash {
fn does_exist(&self) -> bool {
self.update_rcs().len() > 0
}

fn rcfiles(&self) -> Vec<PathBuf> {
[".bash_profile", ".bash_login", ".bashrc"]
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
.iter()
.filter_map(|rc| utils::home_dir().map(|dir| dir.join(rc)))
.collect()
}

fn update_rcs(&self) -> Vec<PathBuf> {
self.rcfiles()
.into_iter()
.filter(|rc| rc.is_file())
.collect()
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
}
}

struct Zsh;

impl Zsh {
fn zdotdir() -> Result<PathBuf> {
use std::ffi::OsStr;
use std::os::unix::ffi::OsStrExt;

if matches!(process().var("SHELL"), Ok(sh) if sh.contains("zsh")) {
match process().var("ZDOTDIR") {
Ok(dir) if dir.len() > 0 => Ok(PathBuf::from(dir)),
_ => bail!("Zsh setup failed."),
}
} else {
match std::process::Command::new("zsh")
.args(&["-c", "'echo $ZDOTDIR'"])
.output()
{
Ok(io) if io.stdout.len() > 0 => Ok(PathBuf::from(OsStr::from_bytes(&io.stdout))),
_ => bail!("Zsh setup failed."),
}
}
}
}

impl UnixShell for Zsh {
fn does_exist(&self) -> bool {
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
// zsh has to either be the shell or be callable for zsh setup.
matches!(process().var("SHELL"), Ok(sh) if sh.contains("zsh"))
|| matches!(utils::find_cmd(&["zsh"]), Some(sh) if sh.contains("zsh"))
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
}

fn rcfiles(&self) -> Vec<PathBuf> {
[Zsh::zdotdir().ok(), utils::home_dir()]
.iter()
.filter_map(|dir| dir.as_ref().map(|p| p.join(".zshenv")))
.collect()
}

fn update_rcs(&self) -> Vec<PathBuf> {
// zsh can change $ZDOTDIR both _before_ AND _during_ reading .zshenv,
// so we: write to $ZDOTDIR/.zshenv if-exists ($ZDOTDIR changes before)
// OR write to $HOME/.zshenv if it exists (change-during)
// if neither exist, we create it ourselves, but using the same logic,
// because we must still respond to whether $ZDOTDIR is set or unset.
// In any case we only write once.
self.rcfiles()
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
.into_iter()
.filter(|env| env.is_file())
.chain(self.rcfiles().into_iter())
.take(1)
.collect()
}
}

pub fn legacy_paths() -> impl Iterator<Item = PathBuf> {
let zprofiles = Zsh::zdotdir()
.into_iter()
.chain(utils::home_dir())
.map(|d| d.join(".zprofile"));
let profiles = [".bash_profile", ".profile"]
.iter()
.filter_map(|rc| utils::home_dir().map(|d| d.join(rc)));

profiles.chain(zprofiles)
}
Loading