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

Don't add toolchain bin to PATH on Windows #3178

Merged
merged 2 commits into from
Mar 21, 2023

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jan 23, 2023

This removes the addition of the <toolchain>/bin directory to PATH on Windows. This fixes it so that recursive invocations of tools like cargo will use the rustup proxies instead of executing the original <toolchain>/bin executable, allowing those recursive invocations to do things like use +toolchain shorthands.

There is a bit of a long history for this behavior:

  1. Originally, rustup would put .cargo/bin and <toolchain>/bin prepended to PATH.
  2. Recursive tool invocations should invoke the proxy, not the tool dire… #812 changed it to remove the <toolchain>/bin
  3. The <toolchain>/bin was added again in Add the rust lib dir (containing std-<hash>.dll) to the path on windows #1093, trying to fix cargo test incompatible with latest rustup cargo#3394. There, cargo test of the cargo project itself was not working. The reason isn't listed in the bug. I don't think it is an issue anymore, as cargo's testsuite now explicitly circumvents the rustup wrappers.
    • There were some other issues such as cargo install clippy. This would put a cargo-clippy binary in ~/.cargo/bin. That binary needed to launch and load clippy-driver, which needed to load rustc-driver. Without PATH munging, this would fail. I don't think this is an issue anymore, since clippy is no longer distributed like that.
  4. Don't prepend CARGO_HOME/bin unnecessarily #2978 changed it so that the order of PATH was preserved. However, that introduced a problem now that ~/.cargo/bin ended up behind the <toolchain>/bin, re-introducing the problem where recursive calls don't see the proxies.

Fixes #3036

Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

@Rustin170506
Copy link
Member

@rbtcollins Could you please take a look? Thanks!

@rbtcollins
Copy link
Contributor

rbtcollins commented Jan 30, 2023

[edited, I found the bits I was confused on]

maybe_do_cargo_fallback does the selection of toolchain. But it doesn't add +nightly etc - it directly picks the executable to use. The recursive call with +nightly in the command is not being done by rustup as far as I can tell. And #3031 (comment) agrees with me on that.

Now some of the history also has to do with Windows binary search order. I think.

we do this thing where we make a temp directory for custom toolchains that are missing binaries, and in toolchain.rs

In particular, the presence of the toolchain bin dir in PATH shouldn't affect whether cargo sees rustc from the same toolchain, or a rustc proxy, unless an explicitly overridden RUSTC_WRAPPER is in use, or cargo is calculating the specific binary to run itself. (is it?).

I'm really hesitant to permute this again without a functional test case that exercises the end to end behaviour we want (e.g. cargo -> build.rs -> cargo-but-must-be-rustup-proxy, rather than just the particular implementation we think is right today.

But then that raises a second question - is such an end to end behaviour reasonable? @jonhoo was working (from memory - could be the wrong person) on ensuring that once rustup calls into a command, nothing bounces out through the proxies again. This would make process invocation faster by removing unneeded CreateProcess, which is slow on Windows, but also avoid the manifest processing code in rustup for all platforms. That seems a bit incompatible with the end to end behaviour being discussed - can we get some convergence on our desires here? Then the code to support it should be easier to reason about.

For instance, if we could say: "Rustup figures out the right installed binary to run, and then sets variables {RUSTC, ...} pointing specifically at that binary, improving compile speed. Code that wants to recursively use rustup[1] should unset [list of variables] to disable this feature." then that might help both that performance work and this reentrant use case to work. That would also suggest that removing the toolchain dir would be sensible, because we'd be saying that our interface for finding binaries is

  1. PATH to find rustup proxies, then the proxy calculates the exact binary
  2. overrides via environment variables like RUSTC, to let toolchain components avoid the overhead of (1) inside inner loops

What are your thoughts?

Copy link
Contributor

@rbtcollins rbtcollins left a comment

Choose a reason for hiding this comment

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

The new test is failing on FreeBSD, I haven't looked into why, but it doesn't look like a FreeBSD build flake.

@ehuss ehuss force-pushed the no-windows-bin-path branch 2 times, most recently from 01aec90 to 4d7311f Compare January 31, 2023 00:54
@ehuss
Copy link
Contributor Author

ehuss commented Jan 31, 2023

Thanks for taking a look!

I understand this is a risky change. I have done a fair amount of local testing to try to shake out any possible concerns, but it's possible there are some cases that this might cause issues.

The new test is failing on FreeBSD, I haven't looked into why, but it doesn't look like a FreeBSD build flake.

Oh, sorry, didn't catch that. I fixed it by changing a hard-link of the mock cargo to a copy. current_exe() was for some reason reporting the other hard-link file. I couldn't reproduce in a VM, but it seems to be fixed now.

I'm really hesitant to permute this again without a functional test case that exercises the end to end behaviour we want (e.g. cargo -> build.rs -> cargo-but-must-be-rustup-proxy, rather than just the particular implementation we think is right today.

Maybe I am uncertain what this means, but I believe the recursive_cargo test is intended to exercise this exact scenario. It does cargo (proxy) -> cargo (mock) -> cargo-foo -> cargo (proxy) -> cargo (mock), where "cargo-foo" is simulating a build-script or third-party cargo command.

is such an end to end behaviour reasonable?

I can understand the hesitation here. For the scenario of improving the performance, that is related to #2958 (which was reverted in #3034), tracked in #3035. The solution I think for that is for rustup to set an environment variable to points to the toolchain bin directory (like ~/.rustup/toolchains/nightly-aarch64-apple-darwin/bin). Cargo can then take that environment variable and use it to find rustc and rustdoc and execute them directly (instead of via the proxies in PATH). That should avoid the problems that were encountered with #2958. I don't think setting RUSTC directly will work well, and I think this is an alternative that should be relatively easy. I can perhaps look at implementing that if you are interested.

@rbtcollins
Copy link
Contributor

rbtcollins commented Jan 31, 2023 via email

@ehuss
Copy link
Contributor Author

ehuss commented Feb 4, 2023

could we make this feature flagged, so that we have a release with it off, but set an option / env variable to turn it on, and get more feedback?

Sure, I went ahead and pushed an update the includes an opt-in. I'm a little concerned that it is very difficult to get people to test things like this, but it seems safer to at least have some kind of escape hatch.

Due to the uncertainty around whether or not this change will
cause problems, this introduces an opt-in for removing the PATH
change on Windows.
@ehuss
Copy link
Contributor Author

ehuss commented Mar 16, 2023

@rbtcollins Did you have any more questions about this?

@rbtcollins
Copy link
Contributor

@ehuss How do you see the several related things tying together? e.g. if cargo assumes the toolchain path is predictable as we discussed, is this PR still relevant?

@ehuss
Copy link
Contributor Author

ehuss commented Mar 19, 2023

Yea, the optimization for cargo directly invoking the executables will help avoid the performance loss on Windows as a consequence of this change. This change is still necessary because that optimization will only apply to invoking rustc or rustdoc directly from within cargo. That optimization won't fix the issue here where there is:

  1. User runs cargo subcommand which launches the cargo proxy.
  2. Proxy sets PATH to include ~/.rustup/toolchains/stable-x86_64-pc-windows-msvc/bin.
  3. Proxy executes the real cargo.exe
  4. cargo invokes cargo-subcommand from ~/.cargo/bin
  5. cargo-subcommand executes rustc +nightly foo.rs. This fails because the PATH modification in step 2 circumvented the proxies.

This PR's opt-in changes it to:

  1. User runs cargo subcommand which launches the cargo proxy.
  2. Proxy executes the real cargo.exe
  3. cargo invokes cargo-subcommand from ~/.cargo/bin
  4. cargo-subcommand executes rustc +nightly foo.rs, which launches the rustc proxy from ~/.cargo/bin.
  5. The proxy finds the correct rustc to run (nightly).

The direct execution optimization (which I will hopefully post soon) will help with the following (which is a different scenario):

Today's behavior (on Windows only):

  1. User runs cargo build which launches the cargo proxy.
  2. Proxy sets PATH to include ~/.rustup/toolchains/stable-x86_64-pc-windows-msvc/bin.
  3. Proxy executes the real cargo.exe
  4. Cargo executes rustc from PATH which is directly in ~/.rustup/toolchains/stable-x86_64-pc-windows-msvc/bin.

With the opt-in from this PR, that would become (matching the behavior of non-Windows platforms):

  1. User runs cargo build which launches the cargo proxy.
  2. Proxy executes the real cargo.exe
  3. Cargo executes rustc from PATH which is the ~/.cargo/bin proxy.
  4. The proxy launches the real rustc from ~/.rustup/toolchains/stable-x86_64-pc-windows-msvc/bin

The optimization will bring it to the following on all platforms:

  1. User runs cargo build which launches the cargo proxy.
  2. Proxy executes the real cargo.exe
  3. Cargo detects it is running under rustup and executes rustc directly from ~/.rustup/toolchains/stable-x86_64-pc-windows-msvc/bin.

@rbtcollins
Copy link
Contributor

ok got it. So yes, lets merge this - we're mid release right now, but straight after the release I'll bring this in.

@rbtcollins
Copy link
Contributor

Merging now since we have to re-stage after the GPG fixups.

@rbtcollins rbtcollins merged commit 10abe8f into rust-lang:master Mar 21, 2023
bors added a commit to rust-lang/cargo that referenced this pull request May 4, 2023
Optimize usage under rustup.

Closes #10986

This optimizes cargo when running under rustup to circumvent the rustup proxies. The rustup proxies introduce overhead that can make a noticeable difference.

The solution here is to identify if cargo would normally run `rustc` from PATH, and the current `rustc` in PATH points to something that looks like a rustup proxy (by comparing it to the `rustup` binary which is a hard-link to the proxy). If it detects this situation, then it looks for a binary in `$RUSTUP_HOME/toolchains/$TOOLCHAIN/bin/$TOOL`. If it finds the direct toolchain executable, then it uses that instead.

## Considerations

There have been some past attempts in the past to address this, but it has been a tricky problem to solve. This change has some risk because cargo is attempting to guess what the user and rustup wants, and it may guess wrong. Here are some considerations and risks for this:

* Setting `RUSTC` (as in rust-lang/rustup#2958) isn't an option. This makes the `RUSTC` setting "sticky" through invocations of different toolchains, such as a cargo subcommand or build script which does something like `cargo +nightly build`.

* Changing `PATH` isn't an option, due to issues like rust-lang/rustup#3036 where cargo subcommands would be unable to execute proxies (so things like `+toolchain` shorthands don't work).

* Setting other environment variables in rustup (as in rust-lang/rustup#3207 which adds `RUSTUP_TOOLCHAIN_DIR` the path to the toolchain dir) comes with various complications, as there is risk that the environment variables could get out of sync with one another (like with `RUSTUP_TOOLCHAIN`), causing tools to break or become confused.

  There was some consideration in that PR for adding protections by using an encoded environment variable that could be cross-checked, but I have concerns about the complexity of the solution.

  We may want to go with this solution in the long run, but I would like to try a short term solution in this PR first to see how it turns out.

* This won't work for a `rustup-toolchain.toml` override with a [`path`](https://rust-lang.github.io/rustup/overrides.html#path) setting. Cargo will use the slow path in that case. In theory it could try to detect this situation, which may be an exercise for the future.

* Some build-scripts, proc-macros, or custom cargo subcommands may be doing unusual things that interfere with the assumptions made in this PR. For example, a custom subcommand could call a `cargo` executable that is not managed by rustup. Proc-macros may be executing cargo or rustc, assuming it will reach some particular toolchain. It can be difficult to predict what unusual ways cargo and rustc are being used. This PR (and its tests) tries to make extra sure that it is resilient even in unusual circumstances.

* The "dev" fallback in rustup can introduce some complications for some solutions to this problem. If a rustup toolchain does not have cargo, such as with a developer "toolchain link", then rustup will automatically call either the nightly, beta, or stable cargo if they are available. This PR should work correctly, since rustup sets the correct `RUSTUP_TOOLCHAIN` environment variable for the *actual* toolchain, not the one where cargo was executed from.

* Special care should be considered for dynamic linking. `LD_LIBRARY_PATH` (linux), `DYLD_LIBRARY_PATH` (macos), and `PATH` (windows) need to be carefully set so that `rustc` can find its shared libraries. Directly executing `rustc` has some risk that it will load the wrong shared libraries. There are some mitigations for this. macOS and Linux use rpath, and Windows looks in the same directory as `rustc.exe`. Also, rustup configures the dyld environment variables from the outer cargo. Finally, cargo also configures these (particularly for the deprecated compiler plugins).

* This shouldn't impact installations that don't use rustup.

* I've done a variety of testing on the big three platforms, but certainly nowhere exhaustive.
    * One of many examples is making sure Clippy's development environment works correctly, which has special requirements for dynamic linking.

* There is risk about future rustup versions changing some assumptions made here. Some assumptions:
    * It assumes that if `RUSTUP_TOOLCHAIN` is set, then the proxy *always* runs exactly that toolchain and no other. If this changes, cargo could execute the wrong version. Currently `RUSTUP_TOOLCHAIN` is the highest priority [toolchain override](https://rust-lang.github.io/rustup/overrides.html) and is fundamental to how toolchain selection becomes "sticky", so I think it is unlikely to change.
    * It assumes rustup sets `RUSTUP_TOOLCHAIN` to a value that is exactly equal to the name of the toolchain in the `toolchains` directory. This works for user shorthands like `RUSTUP_TOOLCHAIN=nightly`, which gets converted to the full toolchain name. However, it does not work for `path` overrides (see above).
    * It assumes the `toolchains` directory layout is always `$RUSTUP_HOME/toolchains/$TOOLCHAIN`. If this changes, then I think the only consequence is that cargo will go back to the slow path.
    * It assumes downloading toolchains is not needed (since cargo running from the toolchain means it should already be downloaded).
    * It assumes there is no other environment setup needed (such as the dyld paths mentioned above).

  My hope is that if assumptions are no longer valid that the worst case is that cargo falls back to the slow path of running the proxy from PATH.

## Performance

This change won't affect the performance on Windows because rustup currently alters PATH to point to the toolchain directory. However, rust-lang/rustup#3178 is attempting to remove that, so this PR will be required to avoid a performance penalty on Windows. That change is currently opt-in, and will likely take a long while to roll out since it won't be released until after the next release, and may be difficult to get sufficient testing.

I have done some rough performance testing on macOS, Windows, and Linux on a variety of different kinds of projects with different commands. The following attempts to summarize what I saw.

The timings are going to be heavily dependent on the system and the project. These are the values I get on my systems, but will likely be very different for everyone else.

The Windows tests were performed with a custom build of rustup with rust-lang/rustup#3178 applied and enabled (stock rustup shows no change in performance as explained above).

The data is summarized in this spreadsheet: https://docs.google.com/spreadsheets/d/1zSvU1fQ0uSELxv3VqWmegGBhbLR-8_KUkyIzCIk21X0/edit?usp=sharing

`hello-world` has a particularly large impact of about 1.68 to 2.7x faster. However, a large portion of this overhead is related to running `rustc` at the start to discover its version and querying it for information. This is cached after the first run, so except for first-time builds, the effect isn't as noticeable. The "check with info" row is an benchmark that removes `target/debug/deps` but keeps the `.rustc_info.json` file.

Incremental builds are a bit more difficult to construct since it requires customizing the commands for each project. I only did an incremental test for cargo itself, running `touch src/cargo/lib.rs` and then `cargo check --lib`.

These measurements excluded the initial overhead of launching the rustup proxy to launch the initial cargo process. This was done just for simplicity, but it makes the test a little less characteristic of a typical usage, which will have some constant overhead for running the proxy.

These tests were done using [`hyperfine`](https://crates.io/crates/hyperfine) version 1.16.1. The macOS system was an M2 Max (12-thread). The Windows and Linux experiments were run on a AMD Ryzen Threadripper 2950X (32-thread). Rust 1.68.2 was used for testing. I can share the commands if people want to see them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants