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.

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

rustup suggest executing "source ...", but this command is absent in dash #3415

Closed
safinaskar opened this issue Jul 23, 2023 · 20 comments · Fixed by #3445
Closed

rustup suggest executing "source ...", but this command is absent in dash #3415

safinaskar opened this issue Jul 23, 2023 · 20 comments · Fixed by #3445

Comments

@safinaskar
Copy link

Problem

Command curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh in the end suggest running command source "$HOME/.cargo/env". But there is no command source in dash (this is Debian's default shell). So, please fix your suggestion to . instead of source.

I copied this source "$HOME/.cargo/env" from rustup output and pasted to my dockerfile. And docker build failed, because of this problem

Steps

.

Possible Solution(s)

No response

Notes

No response

Rustup version

.

Installed toolchains

.
@safinaskar safinaskar added the bug label Jul 23, 2023
@rbtcollins
Copy link
Contributor

It is true that source is a bash extension, not POSIX. I worry that the . won't be particularly clear or visible to developers that haven't spent a lot of time in the intricacies of shell programming.

Perhaps we should make this less programmatic and instead say something like 'you need to source the environment file . Some common ways to do this are '. ' and 'source ', ...' and then we can add more for zsh and ion and all the other shells that are out there.

@rami3l
Copy link
Member

rami3l commented Aug 16, 2023

@safinaskar After some research I found two heredoc strings that look suspicious. Could you please confirm whether they are the suggestions that you were seeing?

#[cfg(not(windows))]
macro_rules! post_install_msg_unix {
() => {
r#"# Rust is installed now. Great!
To get started you may need to restart your current shell.
This would reload your `PATH` environment variable to include
Cargo's bin directory ({cargo_home}/bin).
To configure your current shell, run:
source "{cargo_home}/env"
"#
};
}

#[cfg(not(windows))]
macro_rules! post_install_msg_unix_no_modify_path {
() => {
r#"# Rust is installed now. Great!
To get started you need Cargo's bin directory ({cargo_home}/bin) in your `PATH`
environment variable. This has not been done automatically.
To configure your current shell, run:
source "{cargo_home}/env"
"#
};
}

@safinaskar
Copy link
Author

@rami3l . I just executed curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh in fresh Docker container. Here is output it prints:

Rust is installed now. Great!

To get started you may need to restart your current shell.
This would reload your PATH environment variable to include
Cargo's bin directory ($HOME/.cargo/bin).

To configure your current shell, run:
source "$HOME/.cargo/env"

@rami3l
Copy link
Member

rami3l commented Aug 16, 2023

@safinaskar Thanks!

I'm looking at the right place then. Should be an easy fix...

@rami3l
Copy link
Member

rami3l commented Aug 16, 2023

It is true that source is a bash extension, not POSIX. I worry that the . won't be particularly clear or visible to developers that haven't spent a lot of time in the intricacies of shell programming.

Perhaps we should make this less programmatic and instead say something like 'you need to source the environment file . Some common ways to do this are '. ' and 'source ', ...' and then we can add more for zsh and ion and all the other shells that are out there.

@rbtcollins
Most "modern" shells support source, including csh, bash, zsh, fish, ion, etc.

Personally I use fish, but since it cannot parse the env file (cc #478, #3108), I chose to add the PATH manually, so we are probably back to POSIX vs bash grounds.

That being said, I have the same concern as yours: the dot will be easily overlooked by beginners who don't know . is a command, so I recommend that we keep the current suggestion, and add something like:

source might need to be replaced with . on some shells like ash, dash, or pdksh.

@safinaskar
Copy link
Author

When https://sh.rustup.rs link will be updated? I want to test whether this bug is fixed

@rami3l
Copy link
Member

rami3l commented Sep 10, 2023

@safinaskar Hi! The change is in the Rust part of rustup, so I'm afraid you have to wait for the next release. However, I've tested the change myself, both manually and automatically, and it works pretty well on my machine.

@safinaskar
Copy link
Author

New version of rustc just released, but when I run curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh, I still see source ...

@rami3l
Copy link
Member

rami3l commented Oct 6, 2023

@safinaskar The version of Rust (currently 1.73.0) has nothing to do with the version of rustup (currently 1.26.0). That being said, we are considering cutting a new release (possibly 1.27.0) at #3501. Please stay tuned!

FWIW, if you want to see what has been changed, you can see for yourself directly at https://github.com/rust-lang/rustup/pull/3445/files.

@rami3l
Copy link
Member

rami3l commented Oct 11, 2023

Update: The prompt has again been polished with #3506.

@safinaskar
Copy link
Author

@rami3l, thanks a lot for all this work! Unfortunately, I don't like this patch. I think many users don't know that in Debian /bin/sh is symlink to dash. They see that default login shell is bash, so they may assume that /bin/sh is bash, too.

Back to my original use case: Docker. Consider some typical user. He sees rustup output from your latest patch. He will think: "Okay, what shell I have? bash or dash?" He will spawn fresh debian docker container (using sudo docker run -it debian) and he will see bash prompt. He will conclude: "Okay, so this is bash". Then he will write dockerfile, which will be based on debian base image and will contain RUN source "$HOME/.cargo/env"; .... Yet he will be wrong, because debian docker container uses bash as entry point, and yet uses dash to execute RUN commands (!!!). Same is true for ubuntu and for official docker rust images ( https://hub.docker.com/_/rust ). Of course, this is absolutely confusing.

At this point you may say: "Okay, how to verify this?" Start fresh debian container using sudo docker run -it debian and type to it readlink -f /proc/$$/exe. You will see bash. Now create dockerfile based on debian and add to it RUN readlink -f /proc/$$/exe. You will see dash. I think most docker users don't know this and also they don't know this readlink trick.

So, everything is very complex, so I think rustup output should give as little choice as possible. Ideally one command to use in all cases. Or at least all POSIX-compatible shells, such as bash and dash, should use same command.

I propose this text:

    . "{cargo_home}/env"             # For bash/zsh/ash/dash/pdksh (note the leading DOT)
    source "{cargo_home}/env.fish"   # For fish

(I will possibly fill bug report to dash asking them to implement source)

@safinaskar
Copy link
Author

I just found that dash upstream author said ( https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=211391 ) "It's a bashism so it will never be implemented" in 2003

@rami3l
Copy link
Member

rami3l commented Oct 11, 2023

@safinaskar I understand your concerns, but I wrote these messages mostly with another kind of users in mind: those who want to try out Rust for a new kind of programming experience. I have already asked myself dozens of times why I have to put the source line first instead of using the . line for all POSIX-like shells, and the conclusion is that for those programmers they will most probably be using bash (on Linux) or zsh (on macOS), and the source line will absolutely work for them. There is a trade-off to be made here and I think the newcomers (not necessarily with a rich shell programming experience) should be prioritized, so that has been my decision.

Also, for the more advanced users that somehow cannot use the official rust Docker image and have to use rustup-init in Docker, I assume they already know what source means and won't take it literally if it does not work. Besides, sourcing an env file in Docker is a bad idea anyway IIRC (https://stackoverflow.com/a/55922307). Fortunately, my message also works in this case, however by pointing you to a different direction: using the ENV lines to modify the $PATH, which also happens to be the approach taken by the original Dockerfile.

@safinaskar
Copy link
Author

I have already asked myself dozens of times

Reminder: . works in bash, too

sourcing an env file in Docker is a bad idea anyway

Yes, it is. But it works, if you source an env file and then execute payload in the same line: RUN . env; some-cmd

Debian has dash exactly because bash is not sh, but dash is (and so is ash)

What you mean when you say that bash is not sh? You mean that bash is not POSIX-compatible? This is not true. bash is POSIX-compatible (for practical purposes, I'm not sure that it absolutely compatible). Or you mean that sh points to dash in debian? Then logical order is wrong. "Debian chose dash as system shell because sh points to dash" is wrong, "sh points to dash, because debian chose dash as system shell" is right. Actually, debian chose dash as system shell in the hope that this will make boot times smaller (you can google for this). It seems boot times actually got smaller, but this broke everything (and systemd made boot times even smaller anyway), so this was very bad technical decision.

Or you mean that dash implements nothing on top of POSIX? As well as I remember this is not true: dash has a small amount of extensions on top of POSIX. I can search for them in web.

There are even some behavioral differences between . in sh and source in bash

These differences are not relevant for our case, because we specify absolute path here, not relative

bash in this case is executed in pure POSIX mode IIRC, so it should behave exactly like dash

bash indeed executes in "more POSIX mode". I. e. when normal bash behavior and POSIX behavior differ, then POSIX behavior is chosen. But nearly all bash extensions (such as source or for((...))) are still in place.

it should behave exactly like dash

This sounds laughable. You imply that POSIX specification is small, understandable and easy to implement exactly. And that there exist shell (or even many shells) that implement POSIX spec exactly, no more and no less. Or at least some shell can be run (if needed) in fully POSIX compatible mode, no more and no less. All this is wrong. I'm not aware of any shell, which precisely implements POSIX. As I said above, dash is very similar to exact POSIX set of features, yet, as I
said above, dash has some extensions (again: I can search for them). I once found even some whole conference titled "shell". All speakers discussed shell brokenness in various ways. And how to fix it. One of speakers came up with mathematical model, which actually precisely specifies POSIX shell (unlike POSIX spec, which is not precise enough). I can try to find this conference.

But okay, if you still think that current rustup output is correct, then let it be

@rami3l
Copy link
Member

rami3l commented Oct 12, 2023

@safinaskar Thanks for your detailed explanation. I did some more research on this topic and I sincerely apologize for my very inaccurate previous reply in the late AM.

I have changed my mind thanks to your clarification and now wish to make . the default message, and I'll definitely find another appropriate way to solve the leading dot problem.

I'll let you know once this is done, and let's see what we can do more together to make rustup more user-friendly in more use cases.

As always, thank you so much for this issue and all your comments.

@rami3l
Copy link
Member

rami3l commented Oct 12, 2023

@safinaskar How about the following:

This is usually done by running one of the following (note the leading DOT):
    v------------------------------------------------------------------/
    . "{cargo_home}/env"            # For sh/bash/zsh/ash/dash/pdksh
    source "{cargo_home}/env.fish"  # For fish

We definitely don't want bash and zsh users to get wrong about this, but at the same time we also want to add support for more non-POSIX shells, in some of which . is not a command. Besides, we don't want the message to get too long either (both in terms of line width and number of lines). I think this is a good enough solution which happens to fit into 4 lines for all POSIX shells and fish.

@djc
Copy link
Contributor

djc commented Oct 12, 2023

I don't think adding the arrow makes it easier to understand. Just calling it out in the text seems like a nice touch, though.

@rami3l
Copy link
Member

rami3l commented Oct 12, 2023

@djc Thanks! I'll remove the arrow line then.

@safinaskar
Copy link
Author

How about the following

Thanks a lot for all your work! Yes, I like this text

@rami3l
Copy link
Member

rami3l commented Oct 12, 2023

@safinaskar The PR has been made: #3509. Please feel free to review these changes so we can make sure that it reaches your expectations before it gets merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants