Skip to content
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
1 change: 1 addition & 0 deletions src/cli/self_update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ pub fn install(

let install_res: Result<utils::ExitCode> = (|| {
install_bins()?;
do_write_env_files()?;
if !opts.no_modify_path {
do_add_to_path()?;
}
Expand Down
2 changes: 1 addition & 1 deletion src/cli/self_update/env.sh
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ case ":${PATH}:" in
*:"{cargo_bin}":*)
;;
*)
# Prepending path in case a system-installed rustc must be overwritten
# Prepending path in case a system-installed rustc needs to be overridden
export PATH="{cargo_bin}:$PATH"
;;
esac
24 changes: 15 additions & 9 deletions src/cli/self_update/unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,6 @@ pub fn do_remove_from_path() -> Result<()> {
}

pub fn do_add_to_path() -> Result<()> {
let mut written = vec![];

for sh in shell::get_available_shells() {
let source_cmd = sh.source_string()?;
for rc in sh.update_rcs() {
Expand All @@ -92,13 +90,6 @@ pub fn do_add_to_path() -> Result<()> {
path: rc.to_path_buf(),
}
})?;
let script = sh.env_script();
// Only write scripts once.
// TODO 2021: remove this code if Rustup adds 0 shell scripts.
if !written.contains(&script) {
script.write()?;
written.push(script);
}
}
}
}
Expand All @@ -108,6 +99,21 @@ pub fn do_add_to_path() -> Result<()> {
Ok(())
}

pub fn do_write_env_files() -> Result<()> {
let mut written = vec![];

for sh in shell::get_available_shells() {
let script = sh.env_script();
// Only write each possible script once.
if !written.contains(&script) {
script.write()?;
written.push(script);
}
}

Ok(())
}

/// Tell the upgrader to replace the rustup bins, then delete
/// itself. Like with uninstallation, on Windows we're going to
/// have to jump through hoops to make everything work right.
Expand Down
5 changes: 5 additions & 0 deletions src/cli/self_update/windows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,11 @@ pub fn do_add_to_path() -> Result<()> {
_apply_new_path(new_path)
}

// Nothing to do for Windows for now
pub fn do_write_env_files() -> Result<()> {
Ok(())
}

Comment on lines +147 to +151
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a literally useless function i.e. it does nothing except further enmesh the two OS' APIs.

I submitted 3 PRs with the express intention in each one of reducing the frequency of these in the codebase so that Linux and Windows devs would not have to consult enmeshed OS APIs, and I was hardly given even 24 hours to review this, whereas at least one of my PRs was held up for weeks in review for not being pretty enough or not using APIs that had no explanation for their usage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a fair assessment. It does increase coupling in an API you worked very hard to decouple. I can only apologise for the timing and the presentation of this change, I was working under (perhaps imagined) pressure to get a fix out for users because 1.23.0 broke a behaviour people were reasonably relying on and it had broken a number of CIs (not just this, but also the macos 10.13 problem).

I will work on a PR to decouple this again, I apologise for rushing through something less than ideal.

fn _apply_new_path(new_path: Option<String>) -> Result<()> {
use std::ptr;
use winapi::shared::minwindef::*;
Expand Down
1 change: 1 addition & 0 deletions tests/cli-inst-interactive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ Rust is installed now. Great!
);
if cfg!(unix) {
assert!(!config.homedir.join(".profile").exists());
assert!(config.cargodir.join("env").exists());
}
});
}
Expand Down