Safely source Cargo `env` file

[savchenko]

Describe the problem you are trying to solve rustup adds unconditional attempt to source $HOME/.cargo/env. Two problems:

1. This leads to errors if the location is unavailable
2. The line is added simultaneously to ~/.profile, ~/.bashrc and ~/.bash_profile.

Describe the solution you'd like

1. Test if the file can be accessed prior to sourcing it:

   if [ -r "$HOME/.aliases.sh" ]; then source "$HOME/.aliases.sh"; fi
   

2. Don't add in >1 place.

Notes

3. Bonus points: test if Bash is the login shell indeed, maybe the Rust equivalent of:

   getent passwd $USER | cut -d':' -f7
   

[workingjubilee]

About 1: What is the problem with it throwing errors? If it does not, you have no information that things have been misconfigured. I would be happy to add the test if I understood the situation better.

2 should not be a problem if it works correctly, because it is designed to only affect the system state once. I tried to encourage minimizing the number of additions but there are just too many edge cases in configurations to realistically omit them all, such as switching shells between login and terminal, and other nesting making it hard or impossible to guess where we actually should modify, which means 3 is not happening at all.

[savchenko]

About 1: What is the problem with it throwing errors?

The problem is not with it throwing errors, but with modifying files without explicit user's consent and doing so in multiple places which isn't how Bash was designed to operate. E.g. I don't think there is a good reason to put it in ~/.bash_profile.

[workingjubilee]

Unfortunately it doesn't actually matter how Bash was designed to operate from Rustup's perspective because there is no reason to assume that the user is using a terminal application that uses .bashrc and not .bash_profile. Several DEs open a new login shell for every single terminal.

[workingjubilee]

The Rustup installer offers an option to not modify PATH. When you ask it to, affirmatively, modify PATH, you also implicitly ask Rustup to do so in the way it thinks is best. Rustup has an issue with not being perfectly aware of what your current setup is like re: DE and such, and also not being aware of what it will be in the future, and thus we have chosen a resilient if imperfect solution by default. I really, really tried to make it so that the design minimized the number of source invocations it added, but disagreement between DEs about how terminals are supposed to work makes it impossible to do the actually graceful thing.

If memory serves, when Rustup is uninstalled deliberately, it removes the source directive from all available places correctly, so the situation with throwing errors only occurs with user intervention beyond Rustup's design. This is why I ask: given that this arranges for a situation where Rustup is effectively misconfigured and data is left behind that could be deleted, why should the errors (however trivial... I don't believe this stops existing .*rc or .*profile scripts from continuing to run) not be thrown?

[moridinga]

This issue should be raised in priority. The unprotected nature of this source statement that rustup adds is contrary to best practices (look, for example, at ALL the other source statements in these files - I checked, every single source statement is protected as the original author indicated) and is actively leading to negative consequences for our container use cases, where the user invoking the docker container has their profile info passed into the container. Because this source statement is unprotected and the user in the container does NOT have the rust install present in its home directory, we are getting random failure deep in the middle of docker pipeline:

"bash: /home/builduser/.cargo/env: No such file or directory"

So please don't try to defend or minimize this - it breaks absolutely valid use cases (but only for users who have Rust installed via rustup with paths modified).

[workingjubilee]

It's not about "defending" or "minimizing", I am simply trying to understand: why exactly do you want to leak invalid state into a Docker pipeline without receiving feedback from it?

The container should not be invoking an rcfile from outside the container's state, as this is breaking container state isolation. My understanding is that such a use case violates best practices for containerization, and so making the requested change would incorrectly silence an error that should not pass unremarked.

[moridinga]

It's not about "defending" or "minimizing", I am simply trying to understand: why exactly do you want to leak invalid state into a Docker pipeline without receiving feedback from it?

The container should not be invoking an rcfile from outside the container's state, as this is breaking container state isolation. My understanding is that such a use case violates best practices for containerization, and so making the requested change would incorrectly silence an error that should not pass unremarked.

The error is that the the added code is trying to unconditionally access a file that may or may not exist (just because you made it exist at one point in time). The error is NOT that the file does not exist. All we are asking is that you practice simple defensive programming to prevent an error. Everyone else does this, at least when it comes to these bash script files. If someone was depending on the tools to be in the path, and this file didn't exist and therefore was not sourced, the error would be caught then. Your position is that you demand that the file always exists in all circumstances and scenarios - that's not a reasonable position to take, especially when easily mitigated with a check. It is not for you to assume that it must exist in all scenarios.

[workingjubilee]

I will be more than happy to patch the behavior myself if I am confident that doing so increases the overall correctness of the systems Rustup runs on. But the reason to leave it is as-is is simple:

Rustup, when installed or removed, is supposed to be installed or removed entirely. If Rustup is only partially installed or is only [partially removed, this is an erroneous state from the view of the install and uninstall process, which is all that Rustup is. Every last trace is supposed to disappear, including these commands from the scripts in question. Allowing them to linger is reserving a needless sequence of bytes for ourselves. If another file is added later to the same path by a process that is not Rustup, it becomes a way to trigger unintentional code execution, which is a security concern.

Thus, if a user has done something to cause a misconfiguration, however trivial, letting it pass without remark is not desirable.

Normal shell function is not interrupted by this sourcing command generating an error, so I do not see why your container tool is, as you cannot realistically expect a userscript such as an rcfile to contain no errors unless you totally control the environment. It is somewhat unfortunate that Rustup was the reason the errors were introduced in this case, but it is not defective behavior. Indeed, like a good error, it has revealed a problem with your tool's assumptions, so it seems to have done its job.

[rbtcollins]

I think its fairly clear that if rustup has been asked to modify init scripts that it is doing the right thing by modifying them.

The question then is whether we want a fail-open or fail-closed system. By which I mean, should installing rustup and then removing part of the installation show up as an error which the user directly observes.

Or should it show up as rust not working, which the users will then come and consume time from multiple people here seeking assistance troubleshooting.

This is a key part of thinking about rustup as a product.

And yes, rustup has more than one use case, and we do our best to satisfy the different needs of different use cases.

This issue should be raised in priority. The unprotected nature of this source statement that rustup adds is contrary to best practices (look, for example, at ALL the other source statements in these files - I checked, every single source statement is

The standard bash_rc and bash_profile and profile files have a number of guarded callouts thats absolutely true. But those are files which are expected to be copied into place by the skeleton system when making a new home directory, and the guard is there to allow users to add in addition files that will Just Work once added. It has little bearing on whats right for a modification made by an additional tool such as rustup.

protected as the original author indicated) and is actively leading to negative consequences for our container use cases, where the user invoking the docker container has their profile info passed into the container. Because this source statement is unprotected and the user in the container does NOT have the rust install present in its home directory, we are getting random failure deep in the middle of docker pipeline:

So let me check what is happening here:

1. a user installs rustup for everyday use
2. a user builds a docker container mapping some subsections of their HOME into HOME for the user in the container
3. but they don't map .cargo in
4. bash is invoked inside the container, and due to the combination of step 2 and 3 the rustup modifications to .profile etc result in errors being shown which someone has to track down.
5. alternatively, in 4, if set -e is present, the errors are fatal and the build fails

If this is correct, then I think the immediate thing I would recommend is to map cargo into the container, which will fix the error and permit faster builds through crate source sharing.

An alternative that might be happening is:

1. rustup is installed in a CI pipeline or similar, for use during this one pipeline
2. ... 5 as above

Here I think the obvious thing to do is to not modify the PATH in step 1 - that is, pass --no-modify-path into rustup.sh / rustup-init, which will drop the .env file on disk but not alter the rc files for bash etc. This will then also avoid the problem occuring, but require explicit importing / setting of the environment when rust is to be run.

A valid question to ask is whether we should default, when custom paths are in use, to modifying the rc files. We do today, because when we don't we get lots of people asking for support who have just decided to have a different homedir layout.

I think the supposition is that CI operators and other unattended use cases are generally more savvy and likely to read the docs and --help output and find what they need, than people, often trying rust for the first time, who just happen to have a preferred disk layout.

[jimustafa]

This may also be an issue for those that maintain a set of common dotfiles and replicate them across multiple systems (eg, https://news.ycombinator.com/item?id=11071754). Some systems may not yet have cargo installed, so that the guards help avoid an error; the error should not be fatal, so maybe not really an issue.