Merge #4329

4329: Look for `cargo`, `rustc`, and `rustup` in standard installation path r=matklad a=cdisselkoen

Discussed in #3118.  This is approximately a 90% fix for the issue described there.

This PR creates a new crate `ra_env` with a function `get_path_for_executable()`; see docs there.  `get_path_for_executable()` improves and generalizes the function `cargo_binary()` which was previously duplicated in the `ra_project_model` and `ra_flycheck` crates.  (Both of those crates now depend on the new `ra_env` crate.)  The new function checks (e.g.) `$CARGO` and `$PATH`, but also falls back on `~/.cargo/bin` manually before erroring out.  This should allow most users to not have to worry about setting the `$CARGO` or `$PATH` variables for VSCode, which can be difficult e.g. on macOS as discussed in #3118.

I've attempted to replace all calls to `cargo`, `rustc`, and `rustup` in rust-analyzer with appropriate invocations of `get_path_for_executable()`; I don't think I've missed any in Rust code, but there is at least one invocation in TypeScript code which I haven't fixed.  (I'm not sure whether it's affected by the same problem or not.) https://github.com/rust-analyzer/rust-analyzer/blob/a4778ddb7a00f552a8e653bbf56ae9fd69cfe1d3/editors/code/src/cargo.ts#L79

I'm sure this PR could be improved a bunch, so I'm happy to take feedback/suggestions on how to solve this problem better, or just bikeshedding variable/function/crate names etc.

cc @Veetaha 

Fixes #3118.

Co-authored-by: Craig Disselkoen <[email protected]>
Co-authored-by: veetaha <[email protected]>

13 files changed, 183 insertions, 63 deletions

diff --git a/Cargo.lock b/Cargo.lock
index 264b9b7fb..36cff6402 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -465,6 +465,15 @@ dependencies = [
 ]
 
 [[package]]
+name = "home"
+version = "0.5.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "2456aef2e6b6a9784192ae780c0f15bc57df0e918585282325e8c8ac27737654"
+dependencies = [
+ "winapi 0.3.8",
+]
+
+[[package]]
 name = "idna"
 version = "0.2.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
@@ -949,6 +958,14 @@ dependencies = [
 ]
 
 [[package]]
+name = "ra_env"
+version = "0.1.0"
+dependencies = [
+ "anyhow",
+ "home",
+]
+
+[[package]]
 name = "ra_flycheck"
 version = "0.1.0"
 dependencies = [
@@ -958,6 +975,7 @@ dependencies = [
  "jod-thread",
  "log",
  "lsp-types",
+ "ra_env",
  "serde_json",
 ]
 
@@ -1162,6 +1180,7 @@ dependencies = [
  "ra_arena",
  "ra_cfg",
  "ra_db",
+ "ra_env",
  "ra_proc_macro",
  "rustc-hash",
  "serde",
diff --git a/crates/ra_env/Cargo.toml b/crates/ra_env/Cargo.toml
new file mode 100644
index 000000000..f0a401be5
--- /dev/null
+++ b/crates/ra_env/Cargo.toml
@@ -0,0 +1,9 @@
+[package]
+edition = "2018"
+name = "ra_env"
+version = "0.1.0"
+authors = ["rust-analyzer developers"]
+
+[dependencies]
+anyhow = "1.0.26"
+home = "0.5.3"
diff --git a/crates/ra_env/src/lib.rs b/crates/ra_env/src/lib.rs
new file mode 100644
index 000000000..413da1982
--- /dev/null
+++ b/crates/ra_env/src/lib.rs
@@ -0,0 +1,66 @@
+//! This crate contains a single public function
+//! [`get_path_for_executable`](fn.get_path_for_executable.html).
+//! See docs there for more information.
+
+use anyhow::{bail, Result};
+use std::env;
+use std::path::{Path, PathBuf};
+use std::process::Command;
+
+/// Return a `PathBuf` to use for the given executable.
+///
+/// E.g., `get_path_for_executable("cargo")` may return just `cargo` if that
+/// gives a valid Cargo executable; or it may return a full path to a valid
+/// Cargo.
+pub fn get_path_for_executable(executable_name: impl AsRef<str>) -> Result<PathBuf> {
+    // The current implementation checks three places for an executable to use:
+    // 1) Appropriate environment variable (erroring if this is set but not a usable executable)
+    //      example: for cargo, this checks $CARGO environment variable; for rustc, $RUSTC; etc
+    // 2) `<executable_name>`
+    //      example: for cargo, this tries just `cargo`, which will succeed if `cargo` is on the $PATH
+    // 3) `~/.cargo/bin/<executable_name>`
+    //      example: for cargo, this tries ~/.cargo/bin/cargo
+    //      It seems that this is a reasonable place to try for cargo, rustc, and rustup
+    let executable_name = executable_name.as_ref();
+    let env_var = executable_name.to_ascii_uppercase();
+    if let Ok(path) = env::var(&env_var) {
+        if is_valid_executable(&path) {
+            Ok(path.into())
+        } else {
+            bail!(
+                "`{}` environment variable points to something that's not a valid executable",
+                env_var
+            )
+        }
+    } else {
+        if is_valid_executable(executable_name) {
+            return Ok(executable_name.into());
+        }
+        if let Some(mut path) = ::home::home_dir() {
+            path.push(".cargo");
+            path.push("bin");
+            path.push(executable_name);
+            if is_valid_executable(&path) {
+                return Ok(path);
+            }
+        }
+        // This error message may also be caused by $PATH or $CARGO/$RUSTC/etc not being set correctly
+        // for VSCode, even if they are set correctly in a terminal.
+        // On macOS in particular, launching VSCode from terminal with `code <dirname>` causes VSCode
+        // to inherit environment variables including $PATH, $CARGO, $RUSTC, etc from that terminal;
+        // but launching VSCode from Dock does not inherit environment variables from a terminal.
+        // For more discussion, see #3118.
+        bail!(
+            "Failed to find `{}` executable. Make sure `{}` is in `$PATH`, or set `${}` to point to a valid executable.",
+            executable_name, executable_name, env_var
+        )
+    }
+}
+
+/// Does the given `Path` point to a usable executable?
+///
+/// (assumes the executable takes a `--version` switch and writes to stdout,
+/// which is true for `cargo`, `rustc`, and `rustup`)
+fn is_valid_executable(p: impl AsRef<Path>) -> bool {
+    Command::new(p.as_ref()).arg("--version").output().is_ok()
+}
diff --git a/crates/ra_flycheck/Cargo.toml b/crates/ra_flycheck/Cargo.toml
index 3d5093264..d0f7fb2dc 100644
--- a/crates/ra_flycheck/Cargo.toml
+++ b/crates/ra_flycheck/Cargo.toml
@@ -14,6 +14,7 @@ log = "0.4.8"
 cargo_metadata = "0.9.1"
 serde_json = "1.0.48"
 jod-thread = "0.1.1"
+ra_env = { path = "../ra_env" }
 
 [dev-dependencies]
 insta = "0.16.0"
diff --git a/crates/ra_flycheck/src/lib.rs b/crates/ra_flycheck/src/lib.rs
index f27252949..d8b727b0e 100644
--- a/crates/ra_flycheck/src/lib.rs
+++ b/crates/ra_flycheck/src/lib.rs
@@ -4,7 +4,6 @@
 mod conv;
 
 use std::{
-    env,
     io::{self, BufRead, BufReader},
     path::PathBuf,
     process::{Command, Stdio},
@@ -17,6 +16,7 @@ use lsp_types::{
     CodeAction, CodeActionOrCommand, Diagnostic, Url, WorkDoneProgress, WorkDoneProgressBegin,
     WorkDoneProgressEnd, WorkDoneProgressReport,
 };
+use ra_env::get_path_for_executable;
 
 use crate::conv::{map_rust_diagnostic_to_lsp, MappedRustDiagnostic};
 
@@ -216,7 +216,7 @@ impl FlycheckThread {
 
         let mut cmd = match &self.config {
             FlycheckConfig::CargoCommand { command, all_targets, all_features, extra_args } => {
-                let mut cmd = Command::new(cargo_binary());
+                let mut cmd = Command::new(get_path_for_executable("cargo").unwrap());
                 cmd.arg(command);
                 cmd.args(&["--workspace", "--message-format=json", "--manifest-path"]);
                 cmd.arg(self.workspace_root.join("Cargo.toml"));
@@ -337,7 +337,3 @@ fn run_cargo(
 
     Ok(())
 }
-
-fn cargo_binary() -> String {
-    env::var("CARGO").unwrap_or_else(|_| "cargo".to_string())
-}
diff --git a/crates/ra_project_model/Cargo.toml b/crates/ra_project_model/Cargo.toml
index 5e651fe70..626478468 100644
--- a/crates/ra_project_model/Cargo.toml
+++ b/crates/ra_project_model/Cargo.toml
@@ -14,8 +14,9 @@ rustc-hash = "1.1.0"
 cargo_metadata = "0.9.1"
 
 ra_arena = { path = "../ra_arena" }
-ra_db = { path = "../ra_db" }
 ra_cfg = { path = "../ra_cfg" }
+ra_db = { path = "../ra_db" }
+ra_env = { path = "../ra_env" }
 ra_proc_macro =  { path = "../ra_proc_macro" }
 
 serde = { version = "1.0.106", features = ["derive"] }
diff --git a/crates/ra_project_model/src/cargo_workspace.rs b/crates/ra_project_model/src/cargo_workspace.rs
index 16fff9f1c..eb9f33ee8 100644
--- a/crates/ra_project_model/src/cargo_workspace.rs
+++ b/crates/ra_project_model/src/cargo_workspace.rs
@@ -1,7 +1,6 @@
 //! FIXME: write short doc here
 
 use std::{
-    env,
     ffi::OsStr,
     ops,
     path::{Path, PathBuf},
@@ -12,6 +11,7 @@ use anyhow::{Context, Result};
 use cargo_metadata::{BuildScript, CargoOpt, Message, MetadataCommand, PackageId};
 use ra_arena::{Arena, Idx};
 use ra_db::Edition;
+use ra_env::get_path_for_executable;
 use rustc_hash::FxHashMap;
 
 /// `CargoWorkspace` represents the logical structure of, well, a Cargo
@@ -146,12 +146,8 @@ impl CargoWorkspace {
         cargo_toml: &Path,
         cargo_features: &CargoConfig,
     ) -> Result<CargoWorkspace> {
-        let _ = Command::new(cargo_binary())
-            .arg("--version")
-            .output()
-            .context("failed to run `cargo --version`, is `cargo` in PATH?")?;
-
         let mut meta = MetadataCommand::new();
+        meta.cargo_path(get_path_for_executable("cargo")?);
         meta.manifest_path(cargo_toml);
         if cargo_features.all_features {
             meta.features(CargoOpt::AllFeatures);
@@ -293,7 +289,7 @@ pub fn load_extern_resources(
     cargo_toml: &Path,
     cargo_features: &CargoConfig,
 ) -> Result<ExternResources> {
-    let mut cmd = Command::new(cargo_binary());
+    let mut cmd = Command::new(get_path_for_executable("cargo")?);
     cmd.args(&["check", "--message-format=json", "--manifest-path"]).arg(cargo_toml);
     if cargo_features.all_features {
         cmd.arg("--all-features");
@@ -347,7 +343,3 @@ fn is_dylib(path: &Path) -> bool {
         Some(ext) => matches!(ext.as_str(), "dll" | "dylib" | "so"),
     }
 }
-
-fn cargo_binary() -> String {
-    env::var("CARGO").unwrap_or_else(|_| "cargo".to_string())
-}
diff --git a/crates/ra_project_model/src/lib.rs b/crates/ra_project_model/src/lib.rs
index c226ffa57..88a6ffb2a 100644
--- a/crates/ra_project_model/src/lib.rs
+++ b/crates/ra_project_model/src/lib.rs
@@ -14,6 +14,7 @@ use std::{
 use anyhow::{bail, Context, Result};
 use ra_cfg::CfgOptions;
 use ra_db::{CrateGraph, CrateName, Edition, Env, ExternSource, ExternSourceId, FileId};
+use ra_env::get_path_for_executable;
 use rustc_hash::FxHashMap;
 use serde_json::from_reader;
 
@@ -569,7 +570,7 @@ pub fn get_rustc_cfg_options(target: Option<&String>) -> CfgOptions {
 
     match (|| -> Result<String> {
         // `cfg(test)` and `cfg(debug_assertion)` are handled outside, so we suppress them here.
-        let mut cmd = Command::new("rustc");
+        let mut cmd = Command::new(get_path_for_executable("rustc")?);
         cmd.args(&["--print", "cfg", "-O"]);
         if let Some(target) = target {
             cmd.args(&["--target", target.as_str()]);
diff --git a/crates/ra_project_model/src/sysroot.rs b/crates/ra_project_model/src/sysroot.rs
index 55ff5ad80..11c26ad89 100644
--- a/crates/ra_project_model/src/sysroot.rs
+++ b/crates/ra_project_model/src/sysroot.rs
@@ -8,6 +8,7 @@ use std::{
 };
 
 use ra_arena::{Arena, Idx};
+use ra_env::get_path_for_executable;
 
 #[derive(Default, Debug, Clone)]
 pub struct Sysroot {
@@ -88,9 +89,14 @@ fn create_command_text(program: &str, args: &[&str]) -> String {
     format!("{} {}", program, args.join(" "))
 }
 
-fn run_command_in_cargo_dir(cargo_toml: &Path, program: &str, args: &[&str]) -> Result<Output> {
+fn run_command_in_cargo_dir(
+    cargo_toml: impl AsRef<Path>,
+    program: impl AsRef<Path>,
+    args: &[&str],
+) -> Result<Output> {
+    let program = program.as_ref().as_os_str().to_str().expect("Invalid Unicode in path");
     let output = Command::new(program)
-        .current_dir(cargo_toml.parent().unwrap())
+        .current_dir(cargo_toml.as_ref().parent().unwrap())
         .args(args)
         .output()
         .context(format!("{} failed", create_command_text(program, args)))?;
@@ -114,13 +120,15 @@ fn get_or_install_rust_src(cargo_toml: &Path) -> Result<PathBuf> {
     if let Ok(path) = env::var("RUST_SRC_PATH") {
         return Ok(path.into());
     }
-    let rustc_output = run_command_in_cargo_dir(cargo_toml, "rustc", &["--print", "sysroot"])?;
+    let rustc = get_path_for_executable("rustc")?;
+    let rustc_output = run_command_in_cargo_dir(cargo_toml, &rustc, &["--print", "sysroot"])?;
     let stdout = String::from_utf8(rustc_output.stdout)?;
     let sysroot_path = Path::new(stdout.trim());
     let src_path = sysroot_path.join("lib/rustlib/src/rust/src");
 
     if !src_path.exists() {
-        run_command_in_cargo_dir(cargo_toml, "rustup", &["component", "add", "rust-src"])?;
+        let rustup = get_path_for_executable("rustup")?;
+        run_command_in_cargo_dir(cargo_toml, &rustup, &["component", "add", "rust-src"])?;
     }
     if !src_path.exists() {
         bail!(
diff --git a/editors/code/src/cargo.ts b/editors/code/src/cargo.ts
index a328ba9bd..2a2c2e0e1 100644
--- a/editors/code/src/cargo.ts
+++ b/editors/code/src/cargo.ts
@@ -1,6 +1,9 @@
 import * as cp from 'child_process';
+import * as os from 'os';
+import * as path from 'path';
 import * as readline from 'readline';
 import { OutputChannel } from 'vscode';
+import { isValidExecutable } from './util';
 
 interface CompilationArtifact {
     fileName: string;
@@ -10,17 +13,9 @@ interface CompilationArtifact {
 }
 
 export class Cargo {
-    rootFolder: string;
-    env?: Record<string, string>;
-    output: OutputChannel;
-
-    public constructor(cargoTomlFolder: string, output: OutputChannel, env: Record<string, string> | undefined = undefined) {
-        this.rootFolder = cargoTomlFolder;
-        this.output = output;
-        this.env = env;
-    }
+    constructor(readonly rootFolder: string, readonly output: OutputChannel) { }
 
-    public async artifactsFromArgs(cargoArgs: string[]): Promise<CompilationArtifact[]> {
+    private async artifactsFromArgs(cargoArgs: string[]): Promise<CompilationArtifact[]> {
         const artifacts: CompilationArtifact[] = [];
 
         try {
@@ -37,17 +32,13 @@ export class Cargo {
                                 isTest: message.profile.test
                             });
                         }
-                    }
-                    else if (message.reason === 'compiler-message') {
+                    } else if (message.reason === 'compiler-message') {
                         this.output.append(message.message.rendered);
                     }
                 },
-                stderr => {
-                    this.output.append(stderr);
-                }
+                stderr => this.output.append(stderr),
             );
-        }
-        catch (err) {
+        } catch (err) {
             this.output.show(true);
             throw new Error(`Cargo invocation has failed: ${err}`);
         }
@@ -55,9 +46,8 @@ export class Cargo {
         return artifacts;
     }
 
-    public async executableFromArgs(args: string[]): Promise<string> {
-        const cargoArgs = [...args]; // to remain  args unchanged
-        cargoArgs.push("--message-format=json");
+    async executableFromArgs(args: readonly string[]): Promise<string> {
+        const cargoArgs = [...args, "--message-format=json"];
 
         const artifacts = await this.artifactsFromArgs(cargoArgs);
 
@@ -70,24 +60,27 @@ export class Cargo {
         return artifacts[0].fileName;
     }
 
-    runCargo(
+    private runCargo(
         cargoArgs: string[],
         onStdoutJson: (obj: any) => void,
         onStderrString: (data: string) => void
     ): Promise<number> {
-        return new Promise<number>((resolve, reject) => {
-            const cargo = cp.spawn('cargo', cargoArgs, {
+        return new Promise((resolve, reject) => {
+            let cargoPath;
+            try {
+                cargoPath = getCargoPathOrFail();
+            } catch (err) {
+                return reject(err);
+            }
+
+            const cargo = cp.spawn(cargoPath, cargoArgs, {
                 stdio: ['ignore', 'pipe', 'pipe'],
-                cwd: this.rootFolder,
-                env: this.env,
+                cwd: this.rootFolder
             });
 
-            cargo.on('error', err => {
-                reject(new Error(`could not launch cargo: ${err}`));
-            });
-            cargo.stderr.on('data', chunk => {
-                onStderrString(chunk.toString());
-            });
+            cargo.on('error', err => reject(new Error(`could not launch cargo: ${err}`)));
+
+            cargo.stderr.on('data', chunk => onStderrString(chunk.toString()));
 
             const rl = readline.createInterface({ input: cargo.stdout });
             rl.on('line', line => {
@@ -103,4 +96,28 @@ export class Cargo {
             });
         });
     }
-}
\ No newline at end of file
+}
+
+// Mirrors `ra_env::get_path_for_executable` implementation
+function getCargoPathOrFail(): string {
+    const envVar = process.env.CARGO;
+    const executableName = "cargo";
+
+    if (envVar) {
+        if (isValidExecutable(envVar)) return envVar;
+
+        throw new Error(`\`${envVar}\` environment variable points to something that's not a valid executable`);
+    }
+
+    if (isValidExecutable(executableName)) return executableName;
+
+    const standardLocation = path.join(os.homedir(), '.cargo', 'bin', executableName);
+
+    if (isValidExecutable(standardLocation)) return standardLocation;
+
+    throw new Error(
+        `Failed to find \`${executableName}\` executable. ` +
+        `Make sure \`${executableName}\` is in \`$PATH\`, ` +
+        `or set \`${envVar}\` to point to a valid executable.`
+    );
+}
diff --git a/editors/code/src/commands/runnables.ts b/editors/code/src/commands/runnables.ts
index d77e8188c..2ed150e25 100644
--- a/editors/code/src/commands/runnables.ts
+++ b/editors/code/src/commands/runnables.ts
@@ -119,8 +119,11 @@ export function debugSingle(ctx: Ctx): Cmd {
         }
 
         if (!debugEngine) {
-            vscode.window.showErrorMessage(`Install [CodeLLDB](https://marketplace.visualstudio.com/items?itemName=${lldbId})`
-                + ` or [MS C++ tools](https://marketplace.visualstudio.com/items?itemName=${cpptoolsId}) extension for debugging.`);
+            vscode.window.showErrorMessage(
+                `Install [CodeLLDB](https://marketplace.visualstudio.com/items?itemName=${lldbId}) ` +
+                `or [MS C++ tools](https://marketplace.visualstudio.com/items?itemName=${cpptoolsId}) ` +
+                `extension for debugging.`
+            );
             return;
         }
 
diff --git a/editors/code/src/main.ts b/editors/code/src/main.ts
index efd56a84b..9b020d001 100644
--- a/editors/code/src/main.ts
+++ b/editors/code/src/main.ts
@@ -8,10 +8,9 @@ import { activateInlayHints } from './inlay_hints';
 import { activateStatusDisplay } from './status_display';
 import { Ctx } from './ctx';
 import { Config, NIGHTLY_TAG } from './config';
-import { log, assert } from './util';
+import { log, assert, isValidExecutable } from './util';
 import { PersistentState } from './persistent_state';
 import { fetchRelease, download } from './net';
-import { spawnSync } from 'child_process';
 import { activateTaskProvider } from './tasks';
 
 let ctx: Ctx | undefined;
@@ -179,10 +178,7 @@ async function bootstrapServer(config: Config, state: PersistentState): Promise<
 
     log.debug("Using server binary at", path);
 
-    const res = spawnSync(path, ["--version"], { encoding: 'utf8' });
-    log.debug("Checked binary availability via --version", res);
-    log.debug(res, "--version output:", res.output);
-    if (res.status !== 0) {
+    if (!isValidExecutable(path)) {
         throw new Error(`Failed to execute ${path} --version`);
     }
 
diff --git a/editors/code/src/util.ts b/editors/code/src/util.ts
index 6f91f81d6..127a9e911 100644
--- a/editors/code/src/util.ts
+++ b/editors/code/src/util.ts
@@ -1,6 +1,7 @@
 import * as lc from "vscode-languageclient";
 import * as vscode from "vscode";
 import { strict as nativeAssert } from "assert";
+import { spawnSync } from "child_process";
 
 export function assert(condition: boolean, explanation: string): asserts condition {
     try {
@@ -82,3 +83,13 @@ export function isRustDocument(document: vscode.TextDocument): document is RustD
 export function isRustEditor(editor: vscode.TextEditor): editor is RustEditor {
     return isRustDocument(editor.document);
 }
+
+export function isValidExecutable(path: string): boolean {
+    log.debug("Checking availability of a binary at", path);
+
+    const res = spawnSync(path, ["--version"], { encoding: 'utf8' });
+
+    log.debug(res, "--version output:", res.output);
+
+    return res.status === 0;
+}