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 1.25: On Windows, nested cargo invocation with a toolchain specified fails #3036

Closed
nicholasbishop opened this issue Jul 12, 2022 · 12 comments · Fixed by #3178 or #3703
Closed
Labels

Comments

@nicholasbishop
Copy link
Contributor

nicholasbishop commented Jul 12, 2022

Problem

This is a regression in rustup 1.25 on Windows. If a program run via cargo run tries to run a command like cargo +nightly ..., it fails even if RUSTC is unset in the environment.

Splitting this off from #3031 (comment) because #3031 (comment) indicates it's a separate problem.

Steps

https://github.com/nicholasbishop/rustup-nested-bug-repro#rustup-nested-bug-repro contains a repro of this bug.

Here's the main.rs of that repo:

use std::process::Command;
use std::env;

fn main() {
    // Modify the path if any argument is passed in.
    let modify_path = env::args().len() == 2;

    let mut cmd = Command::new("cargo");
    cmd.env_remove("RUSTC");
    cmd.env_remove("RUSTDOC");
    cmd.args(&["+nightly", "version"]);

    if modify_path {
        let orig_path = env::var_os("PATH").unwrap_or_default();
        let modified_split_paths = env::split_paths(&orig_path).filter(|path| {
            !path
                .components()
                .any(|component| component.as_os_str() == ".rustup")
        });
        let modified_path = env::join_paths(modified_split_paths).expect("invalid PATH");
        cmd.env("PATH", modified_path);
    }

    println!("running command: {:?}", cmd);
    let s = cmd.status().unwrap();
    assert!(s.success());
}

On Linux this command succeeds:

$ cargo run
   Compiling rustup-nested-bug-repro v0.1.0 (/var/home/nbishop/src/rustup-nested-bug-repro)
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
     Running `target/debug/rustup-nested-bug-repro`
running command: "cargo" "+nightly" "version"
cargo 1.64.0-nightly (b1dd22e66 2022-07-09)

On Windows it fails:

$ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.18s
     Running `target\debug\rustup-nested-bug-repro.exe`
running command: "cargo" "+nightly" "version"
error: no such subcommand: `+nightly`
thread 'main' panicked at 'assertion failed: s.success()', src\main.rs:26:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `target\debug\rustup-nested-bug-repro.exe` (exit code: 101)

But it succeeds if the PATH is modified to remove toolchain entries
added by rustup:

$ cargo run modify
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
     Running `target\debug\rustup-nested-bug-repro.exe modify`
running command: "cargo" "+nightly" "version"
cargo 1.64.0-nightly (b1dd22e66 2022-07-09)

Possible Solution(s)

No response

Notes

No response

Rustup version

$ rustup --version
rustup 1.25.0 (90365aa81 2022-06-15)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.62.0 (a8314ef7d 2022-06-27)`

Installed toolchains

$ rustup show
Default host: x86_64-pc-windows-msvc
rustup home:  C:\Users\nicholasbishop\.rustup

installed toolchains
--------------------

stable-x86_64-pc-windows-msvc (default)
nightly-x86_64-pc-windows-msvc

active toolchain
----------------

stable-x86_64-pc-windows-msvc (default)
rustc 1.62.0 (a8314ef7d 2022-06-27)
@jyn514
Copy link
Member

jyn514 commented Jul 12, 2022

@nicholasbishop can you change your program to print RUSTUP_TOOLCHAIN, PATH, and the arguments, and run it under both 1.24.1 and 1.25.0 so we can see the difference?

@nicholasbishop
Copy link
Contributor Author

Sure thing. What's the recommended way to install an older version of rustup?

@jyn514
Copy link
Member

jyn514 commented Jul 12, 2022

Hmm, I'm not sure we keep around older versions - you may need to build from source: https://github.com/rust-lang/rustup/releases/tag/1.24.3

@pietroalbini
Copy link
Member

pietroalbini commented Jul 12, 2022

@nicholasbishop you can download the installer from either URLs, depending on the extension of the OS:

  • https://static.rust-lang.org/rustup/archive/${VERSION}/${TARGET}/rustup-init
  • https://static.rust-lang.org/rustup/archive/${VERSION}/${TARGET}/rustup-init.exe

So, for example:

@nicholasbishop
Copy link
Contributor Author

Thanks. Here's what it looks like in 1.24.3:

$ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target\debug\rustup-nested-bug-repro.exe`
RUSTUP_TOOLCHAIN: "stable-x86_64-pc-windows-msvc"
Original PATH: "C:\\Users\\nicholasbishop\\src\\rustup-nested-bug-repro\\target\\debug\\deps;C:\\Users\\nicholasbishop\\src\\rustup-nested-bug-repro\\target\\debug;C:\\Users\\nicholasbishop\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib;C:\\Users\\nicholasbishop\\.cargo\\bin;C:\\Users\\nicholasbishop\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\bin;C:\\Users\\nicholasbishop\\AppData\\Local\\Programs\\Python\\Python310\\;C:\\Users\\nicholasbishop\\bin;C:\\Program Files\\Git\\mingw64\\bin;C:\\Program Files\\Git\\usr\\local\\bin;C:\\Program Files\\Git\\usr\\bin;C:\\Program Files\\Git\\usr\\bin;C:\\Program Files\\Git\\mingw64\\bin;C:\\Program Files\\Git\\usr\\bin;C:\\Users\\nicholasbishop\\bin;C:\\Program Files\\Oculus\\Support\\oculus-runtime;C:\\Program Files (x86)\\Razer Chroma SDK\\bin;C:\\Program Files\\Razer Chroma SDK\\bin;C:\\Windows\\system32;C:\\Windows;C:\\Windows\\System32\\Wbem;C:\\Windows\\System32\\WindowsPowerShell\\v1.0;C:\\WINDOWS\\system32;C:\\WINDOWS;C:\\WINDOWS\\System32\\Wbem;C:\\WINDOWS\\System32\\WindowsPowerShell\\v1.0;C:\\WINDOWS\\System32\\OpenSSH;C:\\Program Files\\Git\\cmd;C:\\Users\\nicholasbishop\\.cargo\\bin;C:\\Users\\nicholasbishop\\AppData\\Local\\Microsoft\\WindowsApps;C:\\Program Files\\Git\\usr\\bin\\vendor_perl;C:\\Program Files\\Git\\usr\\bin\\core_perl"
running command: "cargo" "+nightly" "version"
cargo 1.64.0-nightly (b1dd22e66 2022-07-09)

And in 1.25.0:

$ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
     Running `target\debug\rustup-nested-bug-repro.exe`
RUSTUP_TOOLCHAIN: "stable-x86_64-pc-windows-msvc"
Original PATH: "C:\\Users\\nicholasbishop\\src\\rustup-nested-bug-repro\\target\\debug\\deps;C:\\Users\\nicholasbishop\\src\\rustup-nested-bug-repro\\target\\debug;C:\\Users\\nicholasbishop\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib;C:\\Users\\nicholasbishop\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\bin;C:\\Users\\nicholasbishop\\AppData\\Local\\Programs\\Python\\Python310\\;C:\\Users\\nicholasbishop\\bin;C:\\Program Files\\Git\\mingw64\\bin;C:\\Program Files\\Git\\usr\\local\\bin;C:\\Program Files\\Git\\usr\\bin;C:\\Program Files\\Git\\usr\\bin;C:\\Program Files\\Git\\mingw64\\bin;C:\\Program Files\\Git\\usr\\bin;C:\\Users\\nicholasbishop\\bin;C:\\Program Files\\Oculus\\Support\\oculus-runtime;C:\\Program Files (x86)\\Razer Chroma SDK\\bin;C:\\Program Files\\Razer Chroma SDK\\bin;C:\\Windows\\system32;C:\\Windows;C:\\Windows\\System32\\Wbem;C:\\Windows\\System32\\WindowsPowerShell\\v1.0;C:\\WINDOWS\\system32;C:\\WINDOWS;C:\\WINDOWS\\System32\\Wbem;C:\\WINDOWS\\System32\\WindowsPowerShell\\v1.0;C:\\WINDOWS\\System32\\OpenSSH;C:\\Program Files\\Git\\cmd;C:\\Users\\nicholasbishop\\.cargo\\bin;C:\\Users\\nicholasbishop\\AppData\\Local\\Microsoft\\WindowsApps;C:\\Program Files\\Git\\usr\\bin\\vendor_perl;C:\\Program Files\\Git\\usr\\bin\\core_perl"
running command: "cargo" "+nightly" "version"
error: no such subcommand: `+nightly`
thread 'main' panicked at 'assertion failed: s.success()', src\main.rs:32:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `target\debug\rustup-nested-bug-repro.exe` (exit code: 101)

Seems like the significant change between them might be the order of the entries. C:\\Users\\nicholasbishop\\.cargo\\bin comes before C:\\Users\\nicholasbishop\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\bin in 1.24.3, but after it in 1.25.0.

@ChrisDenton
Copy link
Contributor

ChrisDenton commented Jul 12, 2022

Possibly related to #2978? Not sure. Edit: Hm, seems cargo\bin is listed twice in 1.24.3 but not in 1.25.0. And because 1.25.0 skips prepending it to PATH, it effectively changes the order. @jonhoo

@CAD97
Copy link

CAD97 commented Jul 25, 2022

For future visitors: rustup run toolchain cargo seems to work in the desired manner, at least w.r.t. this issue.

korejan added a commit to korejan/ALVR that referenced this issue Aug 3, 2022
Fixes xtask build script for  UWP/hololens2 builds, caused by changes in rustup 1.25

rust-lang/rustup#3036
korejan added a commit to korejan/ALVR that referenced this issue Aug 3, 2022
Fixes xtask build script for  UWP/hololens2 builds, caused by changes in rustup 1.25

rust-lang/rustup#3036
bors added a commit to rust-lang/rust-analyzer that referenced this issue Aug 6, 2022
Run stable `fmt` & `cargo` through `rustup`

`cargo test -p ide-assists` fails on Windows/x64/nightly:

```shell
> rustup self update
info: checking for self-updates
  rustup unchanged - 1.25.1

> rustup update
info: syncing channel updates for 'stable-x86_64-pc-windows-msvc'
info: syncing channel updates for 'nightly-x86_64-pc-windows-msvc'
info: checking for self-updates

   stable-x86_64-pc-windows-msvc unchanged - rustc 1.62.1 (e092d0b6b 2022-07-16)
  nightly-x86_64-pc-windows-msvc unchanged - rustc 1.64.0-nightly (affe0d3a0 2022-08-05)

info: cleaning up downloads & tmp directories

> rustup show
Default host: x86_64-pc-windows-msvc
rustup home:  C:\Users\stanc\.rustup

installed toolchains
--------------------

stable-x86_64-pc-windows-msvc
nightly-x86_64-pc-windows-msvc (default)

active toolchain
----------------

nightly-x86_64-pc-windows-msvc (default)
rustc 1.64.0-nightly (affe0d3a0 2022-08-05)

> cargo test -p ide-assists

test tests::sourcegen::sourcegen_assists_docs ... FAILED

failures:

---- tests::sourcegen::sourcegen_assists_docs stdout ----
thread 'tests::sourcegen::sourcegen_assists_docs' panicked at 'Failed to run rustfmt from toolchain 'stable'. Please run `rustup component add rustfmt --toolchain stable` to install it.', crates\sourcegen\src\lib.rs:141:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    tests::sourcegen::sourcegen_assists_docs

test result: FAILED. 1576 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.82s

error: test failed, to rerun pass '-p ide-assists --lib'
```

After some investigation it seemed that [`cmd!`](https://github.com/rust-lang/rust-analyzer/blob/51705698bd66919435e4fcbc25d96bd7fc5583f4/crates/sourcegen/src/lib.rs#L139) didn't execute the expected (stable) rustfmt.

A simple `xshell` test failed too:

```rust
use xshell::{cmd, Shell};

fn main() {
    let sh = &Shell::new().unwrap();
    sh.set_var("RUSTUP_TOOLCHAIN", "stable");
    let version = cmd!(sh, "rustfmt --version").read().unwrap_or_default();
    println!("{version}");
}
```

Bypassing `xshell` and using `Command` directly failed too:

```rust
use std::process::{Command, Stdio};

fn main() {
    let child = Command::new("rustfmt")
        .arg("--version")
        .stdin(Stdio::null())
        .stdout(Stdio::piped())
        .env("RUSTUP_TOOLCHAIN", "stable")
        .spawn()
        .expect("failed to start");
    let output = child.wait_with_output().unwrap();
    let version = String::from_utf8_lossy(&output.stdout);
    println!("{version}");
}
```

Spawning `cargo +stable fmt version` [failed too](rust-lang/rustup#3036) with `error: no such subcommand: +stable`.

Only `rustup run stable` worked fine for both `cargo` and `fmt`.

Thanks to `@lnicola` for a live investigation session, hints and tips.
@kinnison
Copy link
Contributor

Is this resolved in 1.25.1 or do we need to still find out what the right fix is?

@lnicola
Copy link
Member

lnicola commented Aug 27, 2022

Still a problem according to rust-lang/rust-analyzer#12953.

@jonhoo
Copy link
Contributor

jonhoo commented Aug 31, 2022

Hm, seems cargo\bin is listed twice in 1.24.3 but not in 1.25.0. And because 1.25.0 skips prepending it to PATH, it effectively changes the order.

This was entirely an intentional change. It's not clear that Rustup should change $PATH in the way it used to — it's effectively hoisting the priority of any Rust program the user has installed above the existing priority order of $PATH.

In my mind, the bug here is that C:\\Users\\nicholasbishop\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\bin is on $PATH in the first place and is listed before C:\\Users\\nicholasbishop\\.cargo\\bin. We should track down the origin of that, because that's the real (configuration) issue here. I think such a setup should have been broken, and just worked accidentally because Rustup was making changes to $PATH that it's not supposed to make.

epage added a commit to epage/cargo-script-mvs that referenced this issue Sep 1, 2022
This works around rust-lang/rustup#3036 but also makes it so it can work
without rustup.
epage added a commit to epage/cargo-script-mvs that referenced this issue Sep 1, 2022
@ehuss
Copy link
Contributor

ehuss commented Jan 23, 2023

I did some digging and opened #3178 as a solution for this issue.

bors added a commit to rust-lang/cargo that referenced this issue 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.
@ehuss
Copy link
Contributor

ehuss commented May 23, 2023

@rbtcollins Can you reopen this since the change is still opt-in?

@rbtcollins rbtcollins reopened this May 24, 2023
korejan added a commit to korejan/ALVR that referenced this issue Jul 2, 2023
Fixes xtask build script for  UWP/hololens2 builds, caused by changes in rustup 1.25

rust-lang/rustup#3036
korejan added a commit to korejan/ALVR that referenced this issue Jul 8, 2023
Fixes xtask build script for  UWP/hololens2 builds, caused by changes in rustup 1.25

rust-lang/rustup#3036
zalanlevai added a commit to zalanlevai/mutest-rs that referenced this issue Feb 2, 2024
On Windows, nested Cargo invocations fail if a toolchain is specified, since the nested invocations do not use the rustup proxy anymore, but use the exact executable instead, see rust-lang/rustup#3036.

To solve this issue, we now build `rustup run TOOLCHAIN cargo` commands, instead of `cargo +TOOLCHAIN`, skipping the proxies entirely.
ehuss added a commit to ehuss/cargo that referenced this issue Feb 20, 2024
On Windows, rustup has an issue with recursive cargo invocations. This
commit can be removed once
rust-lang/rustup#3036 is resolved.
stupendoussuperpowers pushed a commit to stupendoussuperpowers/cargo that referenced this issue Feb 28, 2024
On Windows, rustup has an issue with recursive cargo invocations. This
commit can be removed once
rust-lang/rustup#3036 is resolved.
@djc djc closed this as completed in #3703 Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
10 participants