Run stable fmt & cargo through rustup

[stanciuadrian]

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

> 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! didn't execute the expected (stable) rustfmt.

A simple xshell test failed too:

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:

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 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.

[lnicola]

@bors r+

[bors]

📌 Commit e8a9bc0 has been approved by lnicola

It is now in the queue for this repository.

[bors]

⌛ Testing commit e8a9bc0 with merge d9e462f...

[bors]

☀️ Test successful - checks-actions
Approved by: lnicola
Pushing d9e462f to master...

[oxalica]

This broke tests in environment without rustup, eg. rust toolchain is setup via Nix.

[lnicola]

It did, but without it the tests completely break on Windows because of a rustup bug.

[oxalica]

It did, but without it the tests completely break on Windows because of a rustup bug.

For me, this PR pulls in not-required rustup dependency. It's not a bug fix. It's a (windows) workaround on a workaround (for rustup managed toolchains), which breaks plain rustc/cargo dev environment.

Personally, I think we should not intervene the setup of development environment without user consent (like calling rustup, apt, or nix), especially in tests.
Maybe providing a workspace configuration rust-toolchain could help here? This would be easier to opt-out.

[lnicola]

I think there was a time when we installed rustfmt multiple times in parallel right from the tests, which failed in interesting ways. Nobody complained except me. If running these tests don't work for you, it's fine to ignore them.

Or are you running the tests as part of building a package?

[CAD97]

This broke tests in environment without rustup,

FWIW, rustup was always assumed, just in an ignorable way using RUSTUP_TOOLCHAIN, so it only incidentally worked without rustup previously.

[oxalica]

@lnicola

If running these tests don't work for you, it's fine to ignore them. Or are you running the tests as part of building a package?

Yes and I did ignore them currently.

@CAD97

FWIW, rustup was always assumed, just in an ignorable way using RUSTUP_TOOLCHAIN, so it only incidentally worked without rustup previously.

The previous method is implemented by me in #4219 to remove this assumption. This PR kinda reverted it.
Well, decision should be made about whether or not to assume rustup by invoking in tests. If yes, the current issue should be wont-fix; otherwise, we need to find another way to workaround.

[nekopsykose]

does this not also affect runtime, because of the changes to lib.rs? as in, requiring rustup to be installed for anything using the sourcegen formatting function. i personally don't have rustup installed, nor see a reason to ever have it for myself, so this is quite a change (if so)

[lnicola]

@nekopsykose it doesn't. It's strictly about some tests in rust-analyzer itself. sourcegen is for generating parts of RA itself. You can even compile it without rustup, since the generated code is checked in

[nekopsykose]

ah, okay. thanks for the clarification!