Skip to content

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

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.

Sign up for GitHub

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

Merged
rami3l merged 2 commits into from
Mar 16, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
822a13f
fix(fish): fix definition of `Fish::update_rcs`
rami3l Mar 13, 2024
d4efebc
fix(shell): create parent dir before appending to rcfiles
rami3l Mar 13, 2024
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
6 changes: 5 additions & 1 deletion src/cli/self_update/shell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,11 @@ impl UnixShell for Fish {
}

fn update_rcs(&self) -> Vec<PathBuf> {
self.rcfiles()
// The first rcfile takes precedence.
Copy link
Copy Markdown
Contributor

@djc djc Mar 13, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@rami3l rami3l Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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()

Copy link
Copy Markdown
Contributor

@djc djc Mar 13, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

@rami3l rami3l Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

@rami3l rami3l Mar 13, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@rami3l rami3l Mar 14, 2024

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@rami3l rami3l Mar 15, 2024

Choose a reason for hiding this comment

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

We also originally did not handle fish at all.

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

Copy link
Copy Markdown
Member

@workingjubilee workingjubilee Mar 19, 2024

Choose a reason for hiding this comment

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

Why would it be singular?

Copy link
Copy Markdown
Contributor

@djc djc Mar 19, 2024

Choose a reason for hiding this comment

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

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.)

Copy link
Copy Markdown
Member

@workingjubilee workingjubilee Mar 19, 2024

Choose a reason for hiding this comment

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

@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.

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

fn env_script(&self) -> ShellScript {
Expand Down
7 changes: 7 additions & 0 deletions src/cli/self_update/unix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,13 @@ pub(crate) fn do_add_to_path() -> Result<()> {
_ => &source_cmd,
};

let rc_dir = rc.parent().with_context(|| {
format!(
"parent directory doesn't exist for rcfile path: `{}`",
rc.display()
)
})?;
utils::ensure_dir_exists("rcfile dir", rc_dir, &|_: Notification<'_>| ())?;
utils::append_file("rcfile", &rc, cmd_to_write)
.with_context(|| format!("could not amend shell profile: '{}'", rc.display()))?;
}
Expand Down
⚡