fix(shell): create parent dir before appending to rcfiles

[rami3l]

Fixes #3706, addressing https://rust-lang.github.io/rust-clippy/master/#/ineffective_open_options at the same time (superseded by #3713).

Concerns

• It's better to test the CI build in a container or something before actually merging this PR. (Tested on Ubuntu 22.04.4 LTS x86_64.)

[djc]

This doesn't obviously make sense to me. Why is this necessary? At the very least deserves some more comments on why we're doing this.

[rami3l]

@djc Sure! The snippet below explains the differences between .rcfiles() and .update_rcs().

rustup/src/cli/self_update/shell.rs

Lines 91 to 96 in 3fcd562

	// Gives all rcfiles of a given shell that Rustup is concerned with.
	// Used primarily in checking rcfiles for cleanup.
	fn rcfiles(&self) -> Vec<PathBuf>;
	// Gives rcs that should be written to.
	fn update_rcs(&self) -> Vec<PathBuf>;

The problem with the current code is that, we have two rc paths in fish, one (p1) being the fallback of the other (p0).

rustup/src/cli/self_update/shell.rs

Lines 218 to 230 in 3fcd562

	// > "$XDG_CONFIG_HOME/fish/conf.d" (or "~/.config/fish/conf.d" if that variable is unset) for the user
	// from <https://github.com/fish-shell/fish-shell/issues/3170#issuecomment-228311857>
	fn rcfiles(&self) -> Vec<PathBuf> {
	let p0 = process().var("XDG_CONFIG_HOME").ok().map(|p| {
	let mut path = PathBuf::from(p);
	path.push("fish/conf.d/rustup.fish");
	path
	});
	let p1 = utils::home_dir().map(|mut path| {
	path.push(".config/fish/conf.d/rustup.fish");
	path
	});

The issue here is, according to the current implementation, if both paths are valid, then we'll create and append to both of them. (It's okay to check both paths for cleanup, but that's another story.)

rustup/src/cli/self_update/unix.rs

Lines 88 to 93 in f6dbe2e

	for rc in sh.update_rcs() {
	let cmd_to_write = match utils::read_file("rcfile", &rc) {
	Ok(contents) if contents.contains(&source_cmd) => continue,
	Ok(contents) if !contents.ends_with('\n') => &source_cmd_with_newline,
	_ => &source_cmd,
	};

This is the expected behavior in some cases, but clearly not for fish due to this fallback relationship. I've found some similar code in the Zsh support (note the .take(1)):

rustup/src/cli/self_update/shell.rs

Lines 186 to 191 in f6dbe2e

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

rustup/src/cli/self_update/shell.rs

Lines 193 to 205 in f6dbe2e

	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()
	.into_iter()
	.filter(|env| env.is_file())
	.chain(self.rcfiles())
	.take(1)
	.collect()

[djc]

So should we change update_rcs() to yield PathBuf instead of Vec<PathBuf>? It seems conceptually confusing to do this only for some shells -- do we update all rcs for Bash?

Anyway, I guess this change is fine, but let's add some comments to explain the logic.

This way of writing it avoids the cloned(), and thus seems cleaner to me:

match self.rcfiles().into_iter().next() {
    Some(path) => vec![path],
    None => vec![],
}

What do you think?

[rami3l]

So should we change update_rcs() to yield PathBuf instead of Vec<PathBuf>? It seems conceptually confusing to do this only for some shells -- do we update all rcs for Bash?

@djc I'm not a bash expert, but at least the implementation is a bit more complex with Bash indeed, which seems to consider what exists on the current file system as a file and what doesn't.

rustup/src/cli/self_update/shell.rs

Lines 138 to 152 in f6dbe2e

	fn rcfiles(&self) -> Vec<PathBuf> {
	// Bash also may read .profile, however Rustup already includes handling
	// .profile as part of POSIX and always does setup for POSIX shells.
	[".bash_profile", ".bash_login", ".bashrc"]
	.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()
	}

... a quick blame points to #2387. @workingjubilee maybe you can provide more context on this one?

[rami3l]

This way of writing it avoids the cloned(), and thus seems cleaner to me:

match self.rcfiles().into_iter().next() {
    Some(path) => vec![path],
    None => vec![],
}

What do you think?

@djc Done.

[rami3l]

@workingjubilee Thanks for your quick response! Apart from what you said above, we are particularly curious about why update_rcs is plural 👀

[rami3l]

We also originally did not handle fish at all.

... and that's exactly why we need this context to evaluate our fish support implementation :]

[workingjubilee]

Why would it be singular?

[djc]

It was just my intuition that it wouldn't make sense to "update" multiple RC files per shell? It seems like if we want to make the rustup proxies work in fish, it would be enough if we update one file? So in that sense update_rcs() -> Vec<PathBuf> should be more like update_rc() -> PathBuf?

(I'm not big on shell customization so my understanding might be wrong.)

[workingjubilee]

@djc While imprecise on technical terms, my remarks in #3256 (comment) remain an accurate summary of the actual situation. In short: No. Not at all. That requires knowledge of not only the OS, not only the shell (and we don't actually know that), but the specific version and distribution of the software.

[rami3l]

@djc I guess we shouldn't be blocking the inclusion on a reply. I'm merging this now.