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.

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

Correct Nushell source command #4230

Closed
wants to merge 2 commits into from
Closed

Correct Nushell source command #4230

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a1f493
Correct Nushell source command
Yethal Mar 7, 2025
a4e468e
Correct Nushell source command
Yethal Mar 7, 2025
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: 3 additions & 3 deletions src/cli/self_update.rs
View file
Original file line number Diff line number Diff line change
@@ -382,9 +382,9 @@ macro_rules! post_install_msg_unix_source_env {
the corresponding `env` file under {cargo_home}.

This is usually done by running one of the following (note the leading DOT):
. "{cargo_home}/env" # For sh/bash/zsh/ash/dash/pdksh
source "{cargo_home}/env.fish" # For fish
source "{cargo_home}/env.nu" # For nushell
. "{cargo_home}/env" # For sh/bash/zsh/ash/dash/pdksh
source "{cargo_home}/env.fish" # For fish
source $"($nu.home-path)/.cargo/env.nu" # For nushell
Copy link
Member

Choose a reason for hiding this comment

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

Note that this is not necessary the $CARGO_HOME! It's just its default value...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a scenario where $CARGO_HOME is set to a value different than default on a system and a human user will actually read this prompt? Or is it something that's only overriden in CI and such?

Copy link
Member

@rami3l rami3l Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@Yethal A human user will actually read the prompt. We allow $CARGO_HOME to be set to something else beforehand, actually IIRC we have suggested in an earlier note (before the actual installation, see https://github.com/search?q=repo%3Arust-lang%2Frustup+%22This+can+be+modified+with+the%22&type=code) that if you want it to be elsewhere you can set that and return to the installer.

Copy link
Contributor Author

@Yethal Yethal Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So will it suffice to add a note that if $CARGO_HOME was overriden then the path will need to be adjusted accordingly?

Copy link
Member

@rami3l rami3l Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@Yethal I am still inclined to doing this properly.

For example, you probably can get inspired from this logic:

rustup/src/cli/self_update.rs

Lines 469 to 487 in 21d4b27

fn canonical_cargo_home(process: &Process) -> Result<Cow<'static, str>> {
let path = process.cargo_home()?;
let default_cargo_home = process
.home_dir()
.unwrap_or_else(|| PathBuf::from("."))
.join(".cargo");
Ok(if default_cargo_home == path {
cfg_if! {
if #[cfg(windows)] {
r"%USERPROFILE%\.cargo".into()
} else {
"$HOME/.cargo".into()
}
}
} else {
path.to_string_lossy().into_owned().into()
})
}

... and figure out a proper way to print the exact path for env.nu for nushell. Maybe instead of using {cargo_home}, you can add an extra format argument {cargo_home_nushell}.

The only detail I don't know is how $nu.home-path is defined in nushell. However the spirit stays the same: if dirname is ($nu.home-path)/.cargo you can print source $"($nu.home-path)/.cargo/env.nu", otherwise just print out the full path.

Copy link
Member

Choose a reason for hiding this comment

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

Might this be considered a nushell bug? I think they could use std::env::home_dir since the implementation was fixed in 1.85 (though it won't be undeprecated until 1.87).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What, the fact that environment variables are not parse-time safe? To the best of my knowledge this is by design.

Copy link
Member

@rami3l rami3l Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What, the fact that environment variables are not parse-time safe? To the best of my knowledge this is by design.

I believe @ChrisDenton meant that nushell relying on dirs/fn.home_dir.html's behavior could be considered a bug in this case... So you are basically free to ignore the "%USERPROFILE set to something else" case for now.

Copy link
Member

Choose a reason for hiding this comment

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

No, I mean the fact that it only uses {FOLDERID_Profile} and doesn't try %USERPROFILE% first. That would at least mean the behaviour is consistent.

Copy link
Contributor Author

@Yethal Yethal Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If the behavior was fixed in 1.85 then it might be too early for Nushell since the policy is to stay two versions behind latest stable

"#
};
}
3 changes: 2 additions & 1 deletion src/cli/self_update/shell.rs
View file
Original file line number Diff line number Diff line change
@@ -299,7 +299,8 @@ impl UnixShell for Nu {
}

fn source_string(&self, process: &Process) -> Result<String> {
Ok(format!(r#"source "{}/env.nu""#, cargo_home_str(process)?))
let cargo_home_nushell = (cargo_home_str(process)?).replace("$HOME", "($nu.home-path)");
Ok(format!(r#"source $"{}/env.nu""#, cargo_home_nushell))
}
}