Update post install message to show only the source methods that are relevant to your system.#4806
Update post install message to show only the source methods that are relevant to your system.#4806ryankopf wants to merge 1 commit intorust-lang:mainfrom
Conversation
| } | ||
|
|
||
| #[cfg(not(windows))] | ||
| macro_rules! post_install_msg_unix_source_env { |
There was a problem hiding this comment.
I think this is a good idea, but I wonder if we should also keep some kind of indication that there is support for these other shells.
Maybe there should be a page in the documentation that covers this (if there isn't one already), and we could link to it here?
There was a problem hiding this comment.
I thought about this after your comment. I looked at the code and there are not many places we put "just in case, look at the docs here: [url]".
I think we already rely very heavily on the "get_supported_shells" function and it seems to have served well over the years. If they have a shell that isn't discovered by "get_supported_shells" for some reason, it's .rc files or whatever it uses will not get updated by rustup in the first place, and we don't show a documentation message related to that issue. Like there's no "just in case we didn't find your shell, look here: [url]" message in what we're already doing.
So I think it's a better strategy to just make sure the "get_supported_shells" does a really good job of detecting whatever shells you have, which I think we have done since there I searched issues for "is:issue state:open get_supported_shells" and "shells missing" and there don't seem to be any complaints about it.
There was a problem hiding this comment.
@djc @ryankopf Actually a smarter way to do this would be to dump all env files in one place and tell the user "just source anything you want in this folder if you install any new shells later".
This is part of the plan (#3639), but for now I'd suggest postponing this because we are making the installation path change (#247) and the former change will be easier afterwards.
Your proposal is going exactly in the direction we originally wanted though, thanks again!
src/cli/self_update/shell.rs
Outdated
| /// shells that are available on the current system. Shells sharing the same | ||
| /// env file are grouped onto one line (e.g. sh/bash/zsh all use `env`). | ||
| pub(crate) fn build_source_env_lines(process: &Process) -> String { | ||
| let mut groups: Vec<(String, Vec<&'static str>)> = Vec::new(); |
There was a problem hiding this comment.
Style nit: instead of a type annotation, write this as let mut groups = Vec::<(String, Vec<&'static str>)>::new();.
There was a problem hiding this comment.
@djc Do we really need a complete notation here though? 🤔 The below seems enough:
let mut groups = Vec::<(_, Vec<_>)>::new();
There was a problem hiding this comment.
Thanks for working on this!
Since this patch involves no snapshot updates, it is recommended to include a screenshot/terminal output snippet in addition to the existing PR description with the original output scenario (meaning with all possible shells) for demonstration. This should be relatively easy by temporarily modifying the relevant logic.
See below. 
Perhaps for clarity, the message could be updated, although I am not certain it is necessary. Perhaps: Could say: I think this might strike a nice balance, leaving out the textual implication that "we are certain this is your shell" from the message, while providing with probably 99+% chance that it is the right command for your shell. |
There was a problem hiding this comment.
Yeah, your suggestion on shortening the help message looks good, let's do that!
Although it'd be cooler if we make the lines not super long. Something like:
To configure your current shell, you need to source
the corresponding env file under {cargo_home}.
Consider running the right command for your shell (note the leading DOT):
. "$HOME/.cargo/env" # For sh/ash/dash/pdksh/bash
Note we cannot wrap the second line because {cargo_home} can be super long.
Please don't forget to update the PR description afterwards, and once you have fixed the compilation error in the CI we are done :)
|
I think it would be more readable to keep an empty line between the help text and the potential shell invocations. |
3 participants
When installing Rust on a new system, you get this ugly block of text listing all the possible ways to source the cargo environment variables. However, we already detect which shells are available - so the message should show the ones that exist on the users system.
Instead of:
If you have just bash and pwsh, for example, it will instead just show the relevant lines: