rust-lang / rustup

The Rust toolchain installer
https://rust-lang.github.io/rustup/
Apache License 2.0
6.18k stars 891 forks source link

Installs binaries into $PATH that may not work #2390

Open bk2204 opened 4 years ago

bk2204 commented 4 years ago

Problem When using rustup, a binary is installed into ~/.cargo/bin regardless of whether there is a suitable implementation for that binary. As a consequence, the standard Unix approach of checking for a binary before using it of running which rustfmt or command -v rustfmt falsely succeeds, leading the user to think the binary is installed, when executing it only fails with an error. This differs from the way almost all other tools and package managers (e.g., rvm) behave and makes scripting difficult.

Apple does the same thing with their git binary, and this has caused untold angst and confusion among programmers and scripters.

rustup should ensure that a binary exists in PATH (i.e., in ~/.cargo/bin) if and only if it is fully functional. If the user switches to a different toolchain (or installs a toolchain) that does not contain the component in question, ~/.cargo/bin should not contain a binary.

Steps

  1. curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
  2. command -v cargo-miri && cargo-miri
  3. Notice command fails with an error message.

Possible Solution(s) rvm handles this by installing binaries into a different location and creating symlinks, which should be fine on Unix. Windows may be trickier due to symlinks requiring either developer mode or administrator privileges, but copying files or installing and removing a stub could be a possibility.

Notes

Output of rustup --version:

rustup 1.21.1 (7832b2ebe 2019-12-20)

Output of `rustup show`:

Default host: x86_64-unknown-linux-gnu rustup home: /home/bmc/.rustup

stable-x86_64-unknown-linux-gnu (default) rustc 1.44.1 (c7087fe00 2020-06-17)

rbtcollins commented 4 years ago

I do acknowledge that this is the behaviour we have, and that it is not entirely ideal, however: I propose to close this WONTFIX, at least for now, until or unless someone can propose a design that addresses the intersecting requirements we have below, of which this one ('which binary shows the binary is fully functional') is arguably the least important.

If you can detail what you are trying to script, you may find that we have an interface you can use (e.g. rustup run) that is suitable for your needs.

Firstly a small side note: binaries that exist on PATH may be executable, suitable for the host, and still fail for numerous other reasons, including missing data files of various sorts, mismatched dynamic libraries etc; the job of a package manager, which rustup kindof-is is to try and ensure that the reasons for failure are all reasons under the callers control; so that the caller can correct them.

Our requirements:

Specific implmementations that are ruled out by these requirements:

bk2204 commented 4 years ago

The problem with using rustup directly is that I'm supporting Rust versions that are not packaged with rustup. My long-standing support policy includes supporting certain OS distributions, including their compiler toolchains (such as rustc and cargo). This policy precedes the existence of Rust as a language and is not likely to change. Therefore, I need some way to know definitively whether a binary exists so I can invoke it or not.

If, for example, Debian does not ship rustfmt, then it doesn't exist in PATH, and I don't invoke it. With rustup, the binary does exist in PATH, I invoke it, and it fails, causing the build to break. I assume, as most software authors do, that the user has not broken their dynamic libraries or otherwise corrupted their system, and if they have done so, I let them fix the problem.

My proposal is that you make ~/.cargo/bin a symlink to the default toolchain if symlinks work, and a directory with stubs otherwise. If cargo invokes a non-default toolchain, it looks it up in the appropriate directory itself or via an alternative directory (e.g., ~/.cargo/stubs) that has the stubs in it.

This is, I think, the best that can be done given the unfortunate situation with Windows and symlinks. I appreciate the desire for a solution that works the same way everywhere, but I don't think one vendor's improvident decision should prevent tooling for every other OS working in a sane way. Portability, after all, is about more than providing the lowest common denominator which meets nobody's needs.

My proposal functions properly even on Windows 10 in development mode and has all the same benefits there as it does on Unix; it's only on Windows systems that are older or lack proper permissions that the stubs are needed. It is portable, since it's possible to rename one item over another even on Windows.

Alternatively, rustup could use a different path for the symlinked directory (e.g., ~/.cargo/toolchain) instead and fail silently if something goes wrong. Then users who would like the standard system behavior use that alternate directory, and users who are constrained by Windows symlink handling use ~/.cargo/bin since the symlinked directory doesn't exist. This does work the same way everywhere, just with different results when symlinks are not available.

I am, of course, open to other proposals which meet everyone's needs.

rbtcollins commented 4 years ago

I don't understand how what we're doing isn't meeting your needs : if you're supporting rust that is packaged directly, then there is no issue. If you're supporting rust installed via rustup, then call rustup run (or give us a narrower bug that explains the problem, rather than this vague 'it might not work' case).

Your proposal would involve making everything using rust depend on calling through cargo or some other new abstraction layer - lets call it 'rustup-compat'. IDEs, CLI tooling, everything, would need to then know about this tool, so that it could intercept toolchain configuration parameters and switch in the right paths etc.

This is exactly what the exact proxies do, and frankly they do it with less disruption and more systematically and pervasively.

This approach is also used by goenv, pyenv, rbenv, to name a few other similar tools.

Making the bin dir a symlink will fail the concurrent invocation use case, as previously mentioned.

I rather suspect your tool, whatever it is, is making some overly concrete assumptions about users machines, given that what rustup does is both common in this space, and entirely legitimate from a POSIX perspective, and possible within e.g. dpkg itself via dpkg-divert, or the alternatives system.

That said, we're happy to ensure some consistent signalling about the status of a missing backend binary to help interoperability, though really it is no different to someone that happens to have done a dpkg-divert of rustfmt to a wrapper shell script that performs some custom wrapping code and fails from time to time.

Obviously, we can't ensure that your process gets an ENOENT to the fork() syscall, but - see above - I would have said it isn't safe to rely on that anyway, unless you restrict your domain to systems where no dpkg-diversion has taken place, where no custom PATH entries exist, and where the alternatives system isn't in use. But we could make sure that we exit(ENOENT) in the case where the binary does not exist (if we don't already).

rbtcollins commented 4 years ago

Oh, btw, as far as I'm aware you can't rename on top of executables that are in use on windows - but here is some truely horrifying code for coming close.

https://github.com/cygwin/cygwin/blob/master/winsup/cygwin/syscalls.cc#L2545 and https://github.com/cygwin/cygwin/blob/master/winsup/cygwin/syscalls.cc#L2728

https://www.osr.com/nt-insider/2017-issue1/windows-10-whats-new-for-fs-filters/ notes the requirement that files be opened with FILE_SHARE_DELETE , which I'm reasonably sure executables are not. And it is documented here explicitly: https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information "Even if ReplaceIfExists is set to TRUE, the rename operation will still fail if a file with the same name already exists and is a directory, a read-only file, or a currently executing file. "

rbtcollins commented 4 years ago

I'm going to close this as this point as there has been no further feedback from the OP - if you have more context to give, please feel free to add to the bug and we can re-open it.

bk2204 commented 4 years ago

Sorry, I've gotten tied up with other things.

My particular use case is trying to run rustfmt and cargo clippy in a CI job. It needs to (a) exit unsuccessfully if the file is not formatted correctly or has clippy errors, (b) exit successfully if it is "correct", and (c) exit successfully if the program does not exist.

So if rustup did not install nonfunctional executables, this would look like this:

#!/bin/sh -e

if command -v cargo-clippy
then
    cargo clippy
fi

How is one to write this with rustup's executables? Similar tools, like rbenv, don't install nonfunctional binaries or symlinks (e.g., they won't add jruby if that's not installed). And note that, as I mentioned before, I use rustup in some cases, but not all cases, and it necessarily needs to work without modification no matter how Rust was installed.

rbtcollins commented 4 years ago

Thank you for expanding on your use case.

You can detect rustup binaries by the path; the exit code for a rustup proxy that doesn't exist is ENOENT; you should be able to generate a shell function that will abstract over that for covering all cases.

Alternatively, while I can understand the desire to work without modification regardless of the installation method of rust, but I will note that that is a self imposed desire, not a necessary condition: CI jobs are hermetic entirely controlled environments, so you have complete deterministic control over what code runs, and should be able to easily influence the logic for your matrix depending on how rust was installed.

As I said before, we don't have a design, nor one proposed, for dynamically adding and removing proxies in a concurrent usage safe fashion, and concurrent usage is a primary use case for rustup: users commonly run multiple instances of e.g. vscode with different toolchains in each instance, expecting rustup to simultaneously broker access to the right version including dealing with newer versions having more components like rls and rust-analyzer.

I'm willing to leave this open for a while to see if a design can be proposed, but I'd much rather the cost of accomodation here be worn by systems that are amenable to it - automation and CI contexts - than end user systems, where we have a higher support cost and less ability to fix things at scale.

bk2204 commented 4 years ago

While my code does indeed run on CI, it also runs on user systems. This is a generic linting rule which users will be expected to run to check their code and will also run on CI. It's a generic Makefile target, essentially, so it does indeed need to work in all environments, whether the user has installed their toolchain from Debian, rustup, asdf, Homebrew, or elsewhere, without any special knowledge.

ENOENT is actually a bad code for this case because it's impossible to know what that is on the system. For example, can you tell me what ENOENT is on Windows just with a standard POSIX environment? Can you tell me what it is on Linux, FreeBSD, or VxWorks? Does it wrap around if the value is greater than 256, and if so, how do we distinguish those cases? What if the value is 256? I agree that this would be quite a reasonable solution if it were a fixed value, like 4 or 255, but since it's ENOENT, it's really hard to use this in any practical way without intricate and unportable knowledge about the system.

I want to emphasize that my goal here is to produce code that generally just works and is buildable and able to be developed responsibly on every system, or at least every Unix system, without much more than a simple Rust toolchain, a standard POSIX environment, and GNU make. So it's not possible for me to have any sort of preconceived knowledge about what the user has installed or the configuration of the system. It needs to just work in a portable way.

I think my alternative proposal hasn't been shot down yet: create a ~/.cargo/toolchain symlink that points to the default toolchain directory, and fail silently if it can't be created. Then users who would prefer missing binaries over functionless binaries can simply adjust PATH by filtering out ~/.cargo/bin if it exists and adding ~/.cargo/toolchain. Granted, the user may not be able to change the symlink on Windows if they are running a binary, but it will work for everyone with symlink support otherwise, and fail gracefully for those who lack it.

rbtcollins commented 4 years ago

Toolchains are contextual. The 'default toolchain' depends on the CWD, environment variables, and the contents of the toolchain file if one is present. A symlink like you propose won't work when someone runs vscode from two different directory somultaneously.

bk2204 commented 4 years ago

In this case, I'm referring to what the user has set via rustup default, which is documented to set the default toolchain. I believe my proposed behavior is well defined within that scope.

rbtcollins commented 4 years ago

execution of a toolchain at any point in time depends on much more than just the default though, which is the problem.